qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Keith Wall" <keith.w...@gmail.com>
Subject Re: Review Request: QPID-3415 CRAM-MD5-HASHED not supported by 0-10 protocol (+ no suppport for custom SASL mechanisms).
Date Thu, 15 Sep 2011 07:13:45 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?
> 
> 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
> 
>


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