qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robbie Gemmell" <robbie.gemm...@gmail.com>
Subject Re: Review Request: QPID-3415 CRAM-MD5-HASHED not supported by 0-10 protocol (+ no suppport for custom SASL mechanisms).
Date Tue, 06 Sep 2011 19:05:02 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message