Qpid
  1. Qpid
  2. QPID-3415

CRAM-MD5-HASHED not supported by 0-10 protocol (+ no suppport for custom SASL mechanisms).

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.10
    • Fix Version/s: 0.13
    • Component/s: Java Client
    • Labels:
      None

      Description

      If the Java broker is configured to use the Base64MD5Password password database the Java client is unable to connect even if they use the sasl_mechs broker option in the connection URL (sasl_mechs='CRAM-MD5-HASHED').

      Instead the user sees:

      org.apache.qpid.AMQException: Cannot connect to broker: Callback handler with support for AuthorizeCallback required
      

      The user can work around the problem by passing the -Dqpid.amqp.version system property to the client, and selecting a protocol < 0-10.

      The problem is happening because on the 0-10 code path on the client, the SASL CallbackHandler in use is hardcoded to UsernamePasswordCallbackhandler (ClientDelegate), rather than using the facilities of CallbackHandlerRegistry (as does the 0-8 and 0-9* code paths). CRAM-MD5-HASHED requires the use of a different Callbackhandler.

      This also inhibits the use of custom SASL methods by the client.

        Issue Links

          Activity

          Hide
          Keith Wall added a comment -

          I'll submit a patch to change the 0-10 code path to use CallbackHandlerRegistry, whilst retaining support for the sasl_mechs property and the EXTERNAL/GSSAPI SASL mechanisms.

          Show
          Keith Wall added a comment - I'll submit a patch to change the 0-10 code path to use CallbackHandlerRegistry, whilst retaining support for the sasl_mechs property and the EXTERNAL/GSSAPI SASL mechanisms.
          Hide
          Keith Wall added a comment -

          Hi Rajith

          Would you mind reviewing my patch for this Jira? It is on review board:

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

          cheers Keith.

          Show
          Keith Wall added a comment - Hi Rajith Would you mind reviewing my patch for this Jira? It is on review board: https://reviews.apache.org/r/1608/ cheers Keith.
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Review request for qpid and rajith attapattu.

          Summary
          -------

          This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too.

          If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use.

          If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server.

          Removed dead code and strengthen unit tests.

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

          Diffs


          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1160136
          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1160136
          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1160136
          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1160136
          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1160136
          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1160136
          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1160136
          /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1160136
          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1160136
          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1160136

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

          Testing
          -------

          Improved unit testing. Run java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally.

          Thanks,

          Keith

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/ ----------------------------------------------------------- Review request for qpid and rajith attapattu. Summary ------- This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too. If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use. If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server. Removed dead code and strengthen unit tests. This addresses bug QPID-3415 . https://issues.apache.org/jira/browse/QPID-3415 Diffs /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1160136 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1160136 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1160136 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1160136 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1160136 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1160136 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1160136 /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1160136 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1160136 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1160136 Diff: https://reviews.apache.org/r/1608/diff Testing ------- Improved unit testing. Run java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally. Thanks, Keith
          Hide
          Rajith Attapattu added a comment -

          Keith, I just skimmed through the patch and I think I need to look at this patch very closely before I comment on it.
          I would also want to test the GSSAPI mechanism to ensure it works as before.

          I am tied up this week, could you please give me time till next week?

          Show
          Rajith Attapattu added a comment - Keith, I just skimmed through the patch and I think I need to look at this patch very closely before I comment on it. I would also want to test the GSSAPI mechanism to ensure it works as before. I am tied up this week, could you please give me time till next week?
          Hide
          Robbie Gemmell added a comment -

          Changes look reasonable to me, I think we should get it committed before code rot sets in.

          Rajith have you had a chance to look at this yet?

          Show
          Robbie Gemmell added a comment - Changes look reasonable to me, I think we should get it committed before code rot sets in. Rajith have you had a chance to look at this yet?
          Hide
          Robbie Gemmell added a comment -

          Linked to related discussion on QPID-3246

          Show
          Robbie Gemmell added a comment - Linked to related discussion on QPID-3246
          Hide
          Rajith Attapattu added a comment -

          Robbie,

          Quick comment to say that I am now looking at the changes in detail.

          Rajith

          Show
          Rajith Attapattu added a comment - Robbie, Quick comment to say that I am now looking at the changes in detail. Rajith
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java
          <https://reviews.apache.org/r/1608/#comment4018>

          I believe you forgot to add ClientConnectionDelegate ?
          I can't seem to find this in the current source tree.

          Perhaps some of the missing code from ClientDelegate is moved to this class?

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java
          <https://reviews.apache.org/r/1608/#comment4012>

          Why is this code (related to GSSAPI) is removed ?

          I don't see this code moved elsewhere either? This will break existing functionality

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java
          <https://reviews.apache.org/r/1608/#comment4013>

          Removing this without a suitable replacement will break existing functionality.

          Is this check performed else where? Could you pls help me located it?

          This was in place to ensure the client to throw an exception if it's not configured to support any of the mechanisms supported by the broker. It used to be that we just ignored SASL all together if no matching mechs were found.

          I'd argue that this is an important check. Could you please explain your reasons behind the removal (if the same check is not performed elsewhere)?

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java
          <https://reviews.apache.org/r/1608/#comment4014>

          Lines 138 to 141 were required for SASL encryption support. Why is this removed ?

          Is this taken care of elsewhere? If so my apologies (but could you point me to the relevant code?).

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java
          <https://reviews.apache.org/r/1608/#comment4015>

          The user identity when using GSSAPI and External are crucial for ACL support.

          We have existing customers relying on this feature.

          Why was this removed? (Again is there a replacement for this some where?)

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java
          <https://reviews.apache.org/r/1608/#comment4016>

          Again this method was used to retrieved the kerberos identity of the user.

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java
          <https://reviews.apache.org/r/1608/#comment4017>

          I would think it's better to default to PLAIN as that would be the one that will be universally supported.
          However I believe you now retrieve the default from the sasl config file ?

          • rajith

          On 2011-08-22 08:58:27, Keith Wall wrote:

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

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

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

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

          (Updated 2011-08-22 08:58:27)

          Review request for qpid and rajith attapattu.

          Summary

          -------

          This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too.

          If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use.

          If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server.

          Removed dead code and strengthen unit tests.

          This addresses bug QPID-3415.

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

          Diffs

          -----

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1160136

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1160136

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1160136

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1160136

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1160136

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1160136

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1160136

          /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1160136

          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1160136

          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1160136

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

          Testing

          -------

          Improved unit testing. Run java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally.

          Thanks,

          Keith

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/#review1767 ----------------------------------------------------------- /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java < https://reviews.apache.org/r/1608/#comment4018 > I believe you forgot to add ClientConnectionDelegate ? I can't seem to find this in the current source tree. Perhaps some of the missing code from ClientDelegate is moved to this class? /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java < https://reviews.apache.org/r/1608/#comment4012 > Why is this code (related to GSSAPI) is removed ? I don't see this code moved elsewhere either? This will break existing functionality /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java < https://reviews.apache.org/r/1608/#comment4013 > Removing this without a suitable replacement will break existing functionality. Is this check performed else where? Could you pls help me located it? This was in place to ensure the client to throw an exception if it's not configured to support any of the mechanisms supported by the broker. It used to be that we just ignored SASL all together if no matching mechs were found. I'd argue that this is an important check. Could you please explain your reasons behind the removal (if the same check is not performed elsewhere)? /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java < https://reviews.apache.org/r/1608/#comment4014 > Lines 138 to 141 were required for SASL encryption support. Why is this removed ? Is this taken care of elsewhere? If so my apologies (but could you point me to the relevant code?). /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java < https://reviews.apache.org/r/1608/#comment4015 > The user identity when using GSSAPI and External are crucial for ACL support. We have existing customers relying on this feature. Why was this removed? (Again is there a replacement for this some where?) /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java < https://reviews.apache.org/r/1608/#comment4016 > Again this method was used to retrieved the kerberos identity of the user. /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java < https://reviews.apache.org/r/1608/#comment4017 > I would think it's better to default to PLAIN as that would be the one that will be universally supported. However I believe you now retrieve the default from the sasl config file ? rajith On 2011-08-22 08:58:27, Keith Wall wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/ ----------------------------------------------------------- (Updated 2011-08-22 08:58:27) Review request for qpid and rajith attapattu. Summary ------- This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too. If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use. If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server. Removed dead code and strengthen unit tests. This addresses bug QPID-3415 . https://issues.apache.org/jira/browse/QPID-3415 Diffs ----- /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1160136 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1160136 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1160136 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1160136 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1160136 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1160136 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1160136 /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1160136 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1160136 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1160136 Diff: https://reviews.apache.org/r/1608/diff Testing ------- Improved unit testing. Run java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally. Thanks, Keith
          Hide
          Rajith Attapattu added a comment -

          I would also want to test this patch with GSSAPI and EXTERNAL to ensure everything works as expected.
          We do have existing customers that rely on those two mechanisms. I'll try to get the testing going asap.

          Show
          Rajith Attapattu added a comment - I would also want to test this patch with GSSAPI and EXTERNAL to ensure everything works as expected. We do have existing customers that rely on those two mechanisms. I'll try to get the testing going asap.
          Hide
          Rajith Attapattu added a comment -

          I forgot to mention that this patch seems to be missing a class - ClientConnectionDelegate.
          I will wait until Keith gets a chance to review my comments and add that class before proceeding with testing.

          Show
          Rajith Attapattu added a comment - I forgot to mention that this patch seems to be missing a class - ClientConnectionDelegate. I will wait until Keith gets a chance to review my comments and add that class before proceeding with testing.
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-09-06 18:12:46, rajith attapattu wrote:

          > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java, line 38

          > <https://reviews.apache.org/r/1608/diff/1/?file=34085#file34085line38>

          >

          > I believe you forgot to add ClientConnectionDelegate ?

          > I can't seem to find this in the current source tree.

          >

          > Perhaps some of the missing code from ClientDelegate is moved to this class?

          Well damn, thatll teach me to say something looks fine without looking at the diff to make sure its actually what I looked it ;(

          Pretty much all of your comments stem from this one issue as the missing code will indeed be in the ClientConnectionDelegate, which is used in place of ClientDelegate for actual client use now as can be seen further down the class. ClientDelegate is really mainly used in ConnectionTest after this (the same way ServerDelegate was really only used in ConnectionTest, but the real broker uses the ServerConnectionDelegate subclass). The diff posted to ReviewBoard is basically incomplete for some reason. I dont have a copy of the original patch that I actually looked over a couple weeks ago and Keith is on holiday just now, but I'm sure he will post the corrected version on his return next week...until then, no need to test what wont evne compile... :S

          On 2011-09-06 18:12:46, rajith attapattu wrote:

          > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java, line 61

          > <https://reviews.apache.org/r/1608/diff/1/?file=34091#file34091line61>

          >

          > I would think it's better to default to PLAIN as that would be the one that will be universally supported.

          > However I believe you now retrieve the default from the sasl config file ?

          It doesnt default to a particular mechanism now, and instead chooses from all of the supported mechanisms of the client like the 0-8/9 codebase did (though the changes also improves that to be deterministic). The null default here indicates the user hasnt asked for a specific set of mechs to be the supported ones, whereas a non-null result will be used to override the clients supported list and specify exactly what should be used as the supported mechs, as it did previously.

          • Robbie

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

          On 2011-08-22 08:58:27, Keith Wall wrote:

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

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

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

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

          (Updated 2011-08-22 08:58:27)

          Review request for qpid and rajith attapattu.

          Summary

          -------

          This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too.

          If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use.

          If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server.

          Removed dead code and strengthen unit tests.

          This addresses bug QPID-3415.

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

          Diffs

          -----

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1160136

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1160136

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1160136

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1160136

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1160136

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1160136

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1160136

          /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1160136

          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1160136

          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1160136

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

          Testing

          -------

          Improved unit testing. Run java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally.

          Thanks,

          Keith

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-09-06 18:12:46, rajith attapattu wrote: > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java, line 38 > < https://reviews.apache.org/r/1608/diff/1/?file=34085#file34085line38 > > > I believe you forgot to add ClientConnectionDelegate ? > I can't seem to find this in the current source tree. > > Perhaps some of the missing code from ClientDelegate is moved to this class? Well damn, thatll teach me to say something looks fine without looking at the diff to make sure its actually what I looked it ;( Pretty much all of your comments stem from this one issue as the missing code will indeed be in the ClientConnectionDelegate, which is used in place of ClientDelegate for actual client use now as can be seen further down the class. ClientDelegate is really mainly used in ConnectionTest after this (the same way ServerDelegate was really only used in ConnectionTest, but the real broker uses the ServerConnectionDelegate subclass). The diff posted to ReviewBoard is basically incomplete for some reason. I dont have a copy of the original patch that I actually looked over a couple weeks ago and Keith is on holiday just now, but I'm sure he will post the corrected version on his return next week...until then, no need to test what wont evne compile... :S On 2011-09-06 18:12:46, rajith attapattu wrote: > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java, line 61 > < https://reviews.apache.org/r/1608/diff/1/?file=34091#file34091line61 > > > I would think it's better to default to PLAIN as that would be the one that will be universally supported. > However I believe you now retrieve the default from the sasl config file ? It doesnt default to a particular mechanism now, and instead chooses from all of the supported mechanisms of the client like the 0-8/9 codebase did (though the changes also improves that to be deterministic). The null default here indicates the user hasnt asked for a specific set of mechs to be the supported ones, whereas a non-null result will be used to override the clients supported list and specify exactly what should be used as the supported mechs, as it did previously. Robbie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/#review1767 ----------------------------------------------------------- On 2011-08-22 08:58:27, Keith Wall wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/ ----------------------------------------------------------- (Updated 2011-08-22 08:58:27) Review request for qpid and rajith attapattu. Summary ------- This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too. If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use. If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server. Removed dead code and strengthen unit tests. This addresses bug QPID-3415 . https://issues.apache.org/jira/browse/QPID-3415 Diffs ----- /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1160136 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1160136 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1160136 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1160136 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1160136 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1160136 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1160136 /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1160136 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1160136 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1160136 Diff: https://reviews.apache.org/r/1608/diff Testing ------- Improved unit testing. Run java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally. Thanks, Keith
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          (Updated 2011-09-12 12:05:21.892862)

          Review request for qpid and rajith attapattu.

          Changes
          -------

          Attached complete udiff. Apologies for the bother.

          Summary
          -------

          This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too.

          If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use.

          If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server.

          Removed dead code and strengthen unit tests.

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

          Diffs (updated)


          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1169685
          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1169685
          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1169685
          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1169685
          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/transport/ClientConnectionDelegate.java PRE-CREATION
          /trunk/qpid/java/client/src/test/java/org/apache/qpid/client/security/CallbackHandlerRegistryTest.java PRE-CREATION
          /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/AMQPCallbackHandler.java 1169685
          /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/UsernamePasswordCallbackHandler.java 1169685
          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1169685
          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1169685
          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1169685
          /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1169685
          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1169685
          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1169685

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

          Testing (updated)
          -------

          Improved unit testing. Ran java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally.

          Thanks,

          Keith

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/ ----------------------------------------------------------- (Updated 2011-09-12 12:05:21.892862) Review request for qpid and rajith attapattu. Changes ------- Attached complete udiff. Apologies for the bother. Summary ------- This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too. If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use. If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server. Removed dead code and strengthen unit tests. This addresses bug QPID-3415 . https://issues.apache.org/jira/browse/QPID-3415 Diffs (updated) /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/transport/ClientConnectionDelegate.java PRE-CREATION /trunk/qpid/java/client/src/test/java/org/apache/qpid/client/security/CallbackHandlerRegistryTest.java PRE-CREATION /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/AMQPCallbackHandler.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/UsernamePasswordCallbackHandler.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1169685 /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1169685 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1169685 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1169685 Diff: https://reviews.apache.org/r/1608/diff Testing (updated) ------- Improved unit testing. Ran java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally. Thanks, Keith
          Hide
          jiraposter@reviews.apache.org added a comment -

          On 2011-09-06 18:12:46, rajith attapattu wrote:

          > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java, line 38

          > <https://reviews.apache.org/r/1608/diff/1/?file=34085#file34085line38>

          >

          > I believe you forgot to add ClientConnectionDelegate ?

          > I can't seem to find this in the current source tree.

          >

          > Perhaps some of the missing code from ClientDelegate is moved to this class?

          Robbie Gemmell wrote:

          Well damn, thatll teach me to say something looks fine without looking at the diff to make sure its actually what I looked it ;(

          Pretty much all of your comments stem from this one issue as the missing code will indeed be in the ClientConnectionDelegate, which is used in place of ClientDelegate for actual client use now as can be seen further down the class. ClientDelegate is really mainly used in ConnectionTest after this (the same way ServerDelegate was really only used in ConnectionTest, but the real broker uses the ServerConnectionDelegate subclass). The diff posted to ReviewBoard is basically incomplete for some reason. I dont have a copy of the original patch that I actually looked over a couple weeks ago and Keith is on holiday just now, but I'm sure he will post the corrected version on his return next week...until then, no need to test what wont evne compile... :S

          You are both quite correct, I accidentally omitted the new class and the removal of now redundant files from the udiff. Apologies. I have now updated the udiff and believe this should answer Rajith's questions.

          On 2011-09-06 18:12:46, rajith attapattu wrote:

          > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 60

          > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line60>

          >

          > Why is this code (related to GSSAPI) is removed ?

          >

          > I don't see this code moved elsewhere either? This will break existing functionality

          Moved to ClientConnectionDelegate

          On 2011-09-06 18:12:46, rajith attapattu wrote:

          > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 127

          > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line127>

          >

          > Removing this without a suitable replacement will break existing functionality.

          >

          > Is this check performed else where? Could you pls help me located it?

          >

          > This was in place to ensure the client to throw an exception if it's not configured to support any of the mechanisms supported by the broker. It used to be that we just ignored SASL all together if no matching mechs were found.

          >

          > I'd argue that this is an important check. Could you please explain your reasons behind the removal (if the same check is not performed elsewhere)?

          Check now performed with ClientConnectionDelegate.

          On 2011-09-06 18:12:46, rajith attapattu wrote:

          > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 143

          > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line143>

          >

          > Lines 138 to 141 were required for SASL encryption support. Why is this removed ?

          >

          > Is this taken care of elsewhere? If so my apologies (but could you point me to the relevant code?).

          Moved to ClientConnectionDelegate

          On 2011-09-06 18:12:46, rajith attapattu wrote:

          > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 215

          > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line215>

          >

          > The user identity when using GSSAPI and External are crucial for ACL support.

          >

          > We have existing customers relying on this feature.

          >

          > Why was this removed? (Again is there a replacement for this some where?)

          Moved to ClientConnectionDelegate

          On 2011-09-06 18:12:46, rajith attapattu wrote:

          > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 304

          > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line304>

          >

          > Again this method was used to retrieved the kerberos identity of the user.

          Moved to ClientConnectionDelegate

          • Keith

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

          On 2011-09-12 12:05:21, Keith Wall wrote:

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

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

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

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

          (Updated 2011-09-12 12:05:21)

          Review request for qpid and rajith attapattu.

          Summary

          -------

          This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too.

          If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use.

          If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server.

          Removed dead code and strengthen unit tests.

          This addresses bug QPID-3415.

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

          Diffs

          -----

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1169685

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1169685

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1169685

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1169685

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/transport/ClientConnectionDelegate.java PRE-CREATION

          /trunk/qpid/java/client/src/test/java/org/apache/qpid/client/security/CallbackHandlerRegistryTest.java PRE-CREATION

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/AMQPCallbackHandler.java 1169685

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/UsernamePasswordCallbackHandler.java 1169685

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1169685

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1169685

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1169685

          /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1169685

          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1169685

          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1169685

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

          Testing

          -------

          Improved unit testing. Ran java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally.

          Thanks,

          Keith

          Show
          jiraposter@reviews.apache.org added a comment - On 2011-09-06 18:12:46, rajith attapattu wrote: > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java, line 38 > < https://reviews.apache.org/r/1608/diff/1/?file=34085#file34085line38 > > > I believe you forgot to add ClientConnectionDelegate ? > I can't seem to find this in the current source tree. > > Perhaps some of the missing code from ClientDelegate is moved to this class? Robbie Gemmell wrote: Well damn, thatll teach me to say something looks fine without looking at the diff to make sure its actually what I looked it ;( Pretty much all of your comments stem from this one issue as the missing code will indeed be in the ClientConnectionDelegate, which is used in place of ClientDelegate for actual client use now as can be seen further down the class. ClientDelegate is really mainly used in ConnectionTest after this (the same way ServerDelegate was really only used in ConnectionTest, but the real broker uses the ServerConnectionDelegate subclass). The diff posted to ReviewBoard is basically incomplete for some reason. I dont have a copy of the original patch that I actually looked over a couple weeks ago and Keith is on holiday just now, but I'm sure he will post the corrected version on his return next week...until then, no need to test what wont evne compile... :S You are both quite correct, I accidentally omitted the new class and the removal of now redundant files from the udiff. Apologies. I have now updated the udiff and believe this should answer Rajith's questions. On 2011-09-06 18:12:46, rajith attapattu wrote: > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 60 > < https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line60 > > > Why is this code (related to GSSAPI) is removed ? > > I don't see this code moved elsewhere either? This will break existing functionality Moved to ClientConnectionDelegate On 2011-09-06 18:12:46, rajith attapattu wrote: > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 127 > < https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line127 > > > Removing this without a suitable replacement will break existing functionality. > > Is this check performed else where? Could you pls help me located it? > > This was in place to ensure the client to throw an exception if it's not configured to support any of the mechanisms supported by the broker. It used to be that we just ignored SASL all together if no matching mechs were found. > > I'd argue that this is an important check. Could you please explain your reasons behind the removal (if the same check is not performed elsewhere)? Check now performed with ClientConnectionDelegate. On 2011-09-06 18:12:46, rajith attapattu wrote: > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 143 > < https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line143 > > > Lines 138 to 141 were required for SASL encryption support. Why is this removed ? > > Is this taken care of elsewhere? If so my apologies (but could you point me to the relevant code?). Moved to ClientConnectionDelegate On 2011-09-06 18:12:46, rajith attapattu wrote: > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 215 > < https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line215 > > > The user identity when using GSSAPI and External are crucial for ACL support. > > We have existing customers relying on this feature. > > Why was this removed? (Again is there a replacement for this some where?) Moved to ClientConnectionDelegate On 2011-09-06 18:12:46, rajith attapattu wrote: > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, line 304 > < https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line304 > > > Again this method was used to retrieved the kerberos identity of the user. Moved to ClientConnectionDelegate Keith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/#review1767 ----------------------------------------------------------- On 2011-09-12 12:05:21, Keith Wall wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/ ----------------------------------------------------------- (Updated 2011-09-12 12:05:21) Review request for qpid and rajith attapattu. Summary ------- This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too. If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use. If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server. Removed dead code and strengthen unit tests. This addresses bug QPID-3415 . https://issues.apache.org/jira/browse/QPID-3415 Diffs ----- /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/transport/ClientConnectionDelegate.java PRE-CREATION /trunk/qpid/java/client/src/test/java/org/apache/qpid/client/security/CallbackHandlerRegistryTest.java PRE-CREATION /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/AMQPCallbackHandler.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/UsernamePasswordCallbackHandler.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1169685 /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1169685 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1169685 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1169685 Diff: https://reviews.apache.org/r/1608/diff Testing ------- Improved unit testing. Ran java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally. Thanks, Keith
          Hide
          jiraposter@reviews.apache.org added a comment -

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

          Ship it!

          I tested the patch with GSSAPI (Kerberos) and it works fine.
          I was unable to test EXTERNAL as it doesn't work. However that is not due to this patch.
          I will be investigating this issue on monday and see if I could fix and then test this patch as well.

          However I have no objections in committing this patch as it hasn't caused the issue.

          • rajith

          On 2011-09-12 12:05:21, Keith Wall wrote:

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

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

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

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

          (Updated 2011-09-12 12:05:21)

          Review request for qpid and rajith attapattu.

          Summary

          -------

          This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too.

          If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use.

          If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server.

          Removed dead code and strengthen unit tests.

          This addresses bug QPID-3415.

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

          Diffs

          -----

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1169685

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1169685

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1169685

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1169685

          /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/transport/ClientConnectionDelegate.java PRE-CREATION

          /trunk/qpid/java/client/src/test/java/org/apache/qpid/client/security/CallbackHandlerRegistryTest.java PRE-CREATION

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/AMQPCallbackHandler.java 1169685

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/UsernamePasswordCallbackHandler.java 1169685

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1169685

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1169685

          /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1169685

          /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1169685

          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1169685

          /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1169685

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

          Testing

          -------

          Improved unit testing. Ran java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally.

          Thanks,

          Keith

          Show
          jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/#review1949 ----------------------------------------------------------- Ship it! I tested the patch with GSSAPI (Kerberos) and it works fine. I was unable to test EXTERNAL as it doesn't work. However that is not due to this patch. I will be investigating this issue on monday and see if I could fix and then test this patch as well. However I have no objections in committing this patch as it hasn't caused the issue. rajith On 2011-09-12 12:05:21, Keith Wall wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/ ----------------------------------------------------------- (Updated 2011-09-12 12:05:21) Review request for qpid and rajith attapattu. Summary ------- This patch changes the 0-10 code path to create the SASL callback handler using the CallbackHandlerRegistry. This allows the 0-10 code path to support SASL mechanisms requiring other callback handlers, such as CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has been retained and now applies to the 0-8..0-9-1 code paths too. If the user specifies a sasl_mechs client connection option the behaviour of the code is unchanged from the previous version: it restricts the list of SASL mechanisms in use. If the user does not specify a sasl_mechs client connection option, the old code used a hardcoded PLAIN default. This is no longer the case. Now the client will use the first SASL mechanism from the list CallbackHandlerRegistry.properties that is also available on the server. Removed dead code and strengthen unit tests. This addresses bug QPID-3415 . https://issues.apache.org/jira/browse/QPID-3415 Diffs ----- /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties 1169685 /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/transport/ClientConnectionDelegate.java PRE-CREATION /trunk/qpid/java/client/src/test/java/org/apache/qpid/client/security/CallbackHandlerRegistryTest.java PRE-CREATION /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/AMQPCallbackHandler.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/UsernamePasswordCallbackHandler.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java 1169685 /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java 1169685 /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java 1169685 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java 1169685 /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java 1169685 Diff: https://reviews.apache.org/r/1608/diff Testing ------- Improved unit testing. Ran java, cpp and cpp.ssl profiles. I am not able to test GSSAPI locally. Thanks, Keith
          Hide
          Keith Wall added a comment -

          Thanks to Rajith for the review.
          Patch applied.

          Show
          Keith Wall added a comment - Thanks to Rajith for the review. Patch applied.

            People

            • Assignee:
              Keith Wall
              Reporter:
              Keith Wall
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development