Qpid
  1. Qpid
  2. QPID-3522

SASL EXTERNAL mechanism no longer works

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.13
    • Fix Version/s: 0.13
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      Seems to be as a result of QPID-3393 (i.e. regression since 0.12) which was a fix for CRAM-MD5. From a simplistic point of view it seems like the CRAM-MD5 mechanism requires an empty response string to be treated as null, whereas for the EXTERNAL mechanism an empty response should be treated as a zero length string. It may be though that there is more to this than that.

        Activity

        Hide
        Gordon Sim added a comment -

        The issue is indeed related to a distinction between null and a zero-length string. In the cyrus sasl lib, when calling sasl_client_start, the response returned is sometimes null (e.g. for CRAM-MD5) and sometimes a zero-length string (e.g. for EXTERNAL). The corresponding call to sasl_server_start makes a similar distinction. While AMQP 0-10 allows this distinction to be made on the wire, the stubs and skeletons for the c++ client and broker don't distinguish between the two cases.

        Show
        Gordon Sim added a comment - The issue is indeed related to a distinction between null and a zero-length string. In the cyrus sasl lib, when calling sasl_client_start, the response returned is sometimes null (e.g. for CRAM-MD5) and sometimes a zero-length string (e.g. for EXTERNAL). The corresponding call to sasl_server_start makes a similar distinction. While AMQP 0-10 allows this distinction to be made on the wire, the stubs and skeletons for the c++ client and broker don't distinguish between the two cases.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2271/
        -----------------------------------------------------------

        Review request for Alan Conway, michael goulish and Chug Rolke.

        Summary
        -------

        The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration.

        Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made.

        I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response.

        This addresses bug QPID-3522.
        https://issues.apache.org/jira/browse/QPID-3522

        Diffs


        /trunk/qpid/cpp/src/qpid/Sasl.h 1179157
        /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157
        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157
        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157
        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157
        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157
        /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157
        /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157
        /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157
        /trunk/qpid/cpp/src/tests/ssl_test 1179157

        Diff: https://reviews.apache.org/r/2271/diff

        Testing
        -------

        Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/ ----------------------------------------------------------- Review request for Alan Conway, michael goulish and Chug Rolke. Summary ------- The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration. Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made. I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response. This addresses bug QPID-3522 . https://issues.apache.org/jira/browse/QPID-3522 Diffs /trunk/qpid/cpp/src/qpid/Sasl.h 1179157 /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/tests/ssl_test 1179157 Diff: https://reviews.apache.org/r/2271/diff Testing ------- Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2271/#review2439
        -----------------------------------------------------------

        Getting through the compile phase:

        /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp
        <https://reviews.apache.org/r/2271/#comment5544>

        This doesn't compile in windows. Change '*response[0]' to '(*response).c_str()[0]'

        /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp
        <https://reviews.apache.org/r/2271/#comment5543>

        This doesn't compile in windows. The 'response' and 'externalSettings' args are out of order.

        • Chug

        On 2011-10-07 15:42:32, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2271/

        -----------------------------------------------------------

        (Updated 2011-10-07 15:42:32)

        Review request for Alan Conway, michael goulish and Chug Rolke.

        Summary

        -------

        The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration.

        Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made.

        I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response.

        This addresses bug QPID-3522.

        https://issues.apache.org/jira/browse/QPID-3522

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/Sasl.h 1179157

        /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/tests/ssl_test 1179157

        Diff: https://reviews.apache.org/r/2271/diff

        Testing

        -------

        Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/#review2439 ----------------------------------------------------------- Getting through the compile phase: /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp < https://reviews.apache.org/r/2271/#comment5544 > This doesn't compile in windows. Change '*response [0] ' to '(*response).c_str() [0] ' /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp < https://reviews.apache.org/r/2271/#comment5543 > This doesn't compile in windows. The 'response' and 'externalSettings' args are out of order. Chug On 2011-10-07 15:42:32, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/ ----------------------------------------------------------- (Updated 2011-10-07 15:42:32) Review request for Alan Conway, michael goulish and Chug Rolke. Summary ------- The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration. Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made. I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response. This addresses bug QPID-3522 . https://issues.apache.org/jira/browse/QPID-3522 Diffs ----- /trunk/qpid/cpp/src/qpid/Sasl.h 1179157 /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/tests/ssl_test 1179157 Diff: https://reviews.apache.org/r/2271/diff Testing ------- Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2271/#review2443
        -----------------------------------------------------------

        Ship it!

        • Alan

        On 2011-10-07 15:42:32, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2271/

        -----------------------------------------------------------

        (Updated 2011-10-07 15:42:32)

        Review request for Alan Conway, michael goulish and Chug Rolke.

        Summary

        -------

        The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration.

        Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made.

        I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response.

        This addresses bug QPID-3522.

        https://issues.apache.org/jira/browse/QPID-3522

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/Sasl.h 1179157

        /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/tests/ssl_test 1179157

        Diff: https://reviews.apache.org/r/2271/diff

        Testing

        -------

        Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/#review2443 ----------------------------------------------------------- Ship it! Alan On 2011-10-07 15:42:32, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/ ----------------------------------------------------------- (Updated 2011-10-07 15:42:32) Review request for Alan Conway, michael goulish and Chug Rolke. Summary ------- The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration. Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made. I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response. This addresses bug QPID-3522 . https://issues.apache.org/jira/browse/QPID-3522 Diffs ----- /trunk/qpid/cpp/src/qpid/Sasl.h 1179157 /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/tests/ssl_test 1179157 Diff: https://reviews.apache.org/r/2271/diff Testing ------- Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2271/
        -----------------------------------------------------------

        (Updated 2011-10-10 06:32:28.713798)

        Review request for Alan Conway, michael goulish and Chug Rolke.

        Changes
        -------

        Made changes pointed out by Chuck. Thanks, Chuck!

        Summary
        -------

        The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration.

        Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made.

        I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response.

        This addresses bug QPID-3522.
        https://issues.apache.org/jira/browse/QPID-3522

        Diffs (updated)


        /trunk/qpid/cpp/src/qpid/Sasl.h 1179157
        /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157
        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157
        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157
        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157
        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157
        /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157
        /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157
        /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157
        /trunk/qpid/cpp/src/tests/ssl_test 1179157

        Diff: https://reviews.apache.org/r/2271/diff

        Testing
        -------

        Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/ ----------------------------------------------------------- (Updated 2011-10-10 06:32:28.713798) Review request for Alan Conway, michael goulish and Chug Rolke. Changes ------- Made changes pointed out by Chuck. Thanks, Chuck! Summary ------- The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration. Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made. I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response. This addresses bug QPID-3522 . https://issues.apache.org/jira/browse/QPID-3522 Diffs (updated) /trunk/qpid/cpp/src/qpid/Sasl.h 1179157 /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/tests/ssl_test 1179157 Diff: https://reviews.apache.org/r/2271/diff Testing ------- Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2271/#review2480
        -----------------------------------------------------------

        I think this all makes sense – I just have one question inspired by a comment in SaslAuthenticator.cpp

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp
        <https://reviews.apache.org/r/2271/#comment5616>

        What do you think about the question in this comment? Should we indeed throw an error here if this is not a valid PLAIN response?

        • mick

        On 2011-10-10 06:32:28, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2271/

        -----------------------------------------------------------

        (Updated 2011-10-10 06:32:28)

        Review request for Alan Conway, michael goulish and Chug Rolke.

        Summary

        -------

        The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration.

        Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made.

        I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response.

        This addresses bug QPID-3522.

        https://issues.apache.org/jira/browse/QPID-3522

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/Sasl.h 1179157

        /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/tests/ssl_test 1179157

        Diff: https://reviews.apache.org/r/2271/diff

        Testing

        -------

        Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/#review2480 ----------------------------------------------------------- I think this all makes sense – I just have one question inspired by a comment in SaslAuthenticator.cpp /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp < https://reviews.apache.org/r/2271/#comment5616 > What do you think about the question in this comment? Should we indeed throw an error here if this is not a valid PLAIN response? mick On 2011-10-10 06:32:28, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/ ----------------------------------------------------------- (Updated 2011-10-10 06:32:28) Review request for Alan Conway, michael goulish and Chug Rolke. Summary ------- The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration. Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made. I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response. This addresses bug QPID-3522 . https://issues.apache.org/jira/browse/QPID-3522 Diffs ----- /trunk/qpid/cpp/src/qpid/Sasl.h 1179157 /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/tests/ssl_test 1179157 Diff: https://reviews.apache.org/r/2271/diff Testing ------- Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-10-10 14:56:29, mick goulish wrote:

        > /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp, line 193

        > <https://reviews.apache.org/r/2271/diff/2/?file=49037#file49037line193>

        >

        > What do you think about the question in this comment? Should we indeed throw an error here if this is not a valid PLAIN response?

        Could do. I would do it as a separate change from this. It's not a particularly critical point as this is only for the case where authentication has been turned off, the client has chosen PLAIN, but doesn't then format the response correctly.

        • Gordon

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2271/#review2480
        -----------------------------------------------------------

        On 2011-10-10 06:32:28, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2271/

        -----------------------------------------------------------

        (Updated 2011-10-10 06:32:28)

        Review request for Alan Conway, michael goulish and Chug Rolke.

        Summary

        -------

        The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration.

        Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made.

        I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response.

        This addresses bug QPID-3522.

        https://issues.apache.org/jira/browse/QPID-3522

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/Sasl.h 1179157

        /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/tests/ssl_test 1179157

        Diff: https://reviews.apache.org/r/2271/diff

        Testing

        -------

        Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-10-10 14:56:29, mick goulish wrote: > /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp, line 193 > < https://reviews.apache.org/r/2271/diff/2/?file=49037#file49037line193 > > > What do you think about the question in this comment? Should we indeed throw an error here if this is not a valid PLAIN response? Could do. I would do it as a separate change from this. It's not a particularly critical point as this is only for the case where authentication has been turned off, the client has chosen PLAIN, but doesn't then format the response correctly. Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/#review2480 ----------------------------------------------------------- On 2011-10-10 06:32:28, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/ ----------------------------------------------------------- (Updated 2011-10-10 06:32:28) Review request for Alan Conway, michael goulish and Chug Rolke. Summary ------- The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration. Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made. I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response. This addresses bug QPID-3522 . https://issues.apache.org/jira/browse/QPID-3522 Diffs ----- /trunk/qpid/cpp/src/qpid/Sasl.h 1179157 /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/tests/ssl_test 1179157 Diff: https://reviews.apache.org/r/2271/diff Testing ------- Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2271/#review2483
        -----------------------------------------------------------

        Ship it!

        • mick

        On 2011-10-10 06:32:28, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2271/

        -----------------------------------------------------------

        (Updated 2011-10-10 06:32:28)

        Review request for Alan Conway, michael goulish and Chug Rolke.

        Summary

        -------

        The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration.

        Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made.

        I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response.

        This addresses bug QPID-3522.

        https://issues.apache.org/jira/browse/QPID-3522

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/Sasl.h 1179157

        /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/tests/ssl_test 1179157

        Diff: https://reviews.apache.org/r/2271/diff

        Testing

        -------

        Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/#review2483 ----------------------------------------------------------- Ship it! mick On 2011-10-10 06:32:28, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/ ----------------------------------------------------------- (Updated 2011-10-10 06:32:28) Review request for Alan Conway, michael goulish and Chug Rolke. Summary ------- The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration. Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made. I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response. This addresses bug QPID-3522 . https://issues.apache.org/jira/browse/QPID-3522 Diffs ----- /trunk/qpid/cpp/src/qpid/Sasl.h 1179157 /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/tests/ssl_test 1179157 Diff: https://reviews.apache.org/r/2271/diff Testing ------- Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however. Thanks, Gordon
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2271/#review2519
        -----------------------------------------------------------

        Ship it!

        bulds on windows

        • Chug

        On 2011-10-10 06:32:28, Gordon Sim wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2271/

        -----------------------------------------------------------

        (Updated 2011-10-10 06:32:28)

        Review request for Alan Conway, michael goulish and Chug Rolke.

        Summary

        -------

        The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration.

        Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made.

        I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response.

        This addresses bug QPID-3522.

        https://issues.apache.org/jira/browse/QPID-3522

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/Sasl.h 1179157

        /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157

        /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157

        /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157

        /trunk/qpid/cpp/src/tests/ssl_test 1179157

        Diff: https://reviews.apache.org/r/2271/diff

        Testing

        -------

        Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however.

        Thanks,

        Gordon

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/#review2519 ----------------------------------------------------------- Ship it! bulds on windows Chug On 2011-10-10 06:32:28, Gordon Sim wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2271/ ----------------------------------------------------------- (Updated 2011-10-10 06:32:28) Review request for Alan Conway, michael goulish and Chug Rolke. Summary ------- The stubs and skeletons used to invoke and handle AMQP 0-10 commands/controls at present does not distinguish between null and an empty string. It turns out this distinction is critical for Cyrus SASL integration. Rather than altering the general approach - which I fear would mean a large patch and the potential for lots of irritating bugs to creep in - I've restricted the change to the specific control of relevance here. By operating on the command body directly rather than using the parameter list to- and from- which it is converted, the appropriate check can be made. I have also had to alter the SASL interfaces to make the same distinction with regard to the initial response. This addresses bug QPID-3522 . https://issues.apache.org/jira/browse/QPID-3522 Diffs ----- /trunk/qpid/cpp/src/qpid/Sasl.h 1179157 /trunk/qpid/cpp/src/qpid/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.h 1179157 /trunk/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.h 1179157 /trunk/qpid/cpp/src/qpid/broker/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/broker/windows/SaslAuthenticator.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/ConnectionHandler.cpp 1179157 /trunk/qpid/cpp/src/qpid/client/windows/SaslFactory.cpp 1179157 /trunk/qpid/cpp/src/tests/ssl_test 1179157 Diff: https://reviews.apache.org/r/2271/diff Testing ------- Tested both CRAM-MD5 and EXTERNAL work. Added automated test for EXTERNAL which was previously missing. I haven't tested at all on windows however. Thanks, Gordon
        Hide
        Gordon Sim added a comment -

        Thanks to all reviewers, especially Chuck for testing compilation on windows!

        Show
        Gordon Sim added a comment - Thanks to all reviewers, especially Chuck for testing compilation on windows!

          People

          • Assignee:
            Gordon Sim
            Reporter:
            Gordon Sim
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development