You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Ortwin Glück <or...@nose.ch> on 2003/09/11 13:55:30 UTC
[PATCH] Reworked digest auth
While reviewing a Patch to include MD5-sess into the Digest
Authentication Scheme I came across a few flaws in that class. I suggest
the following changes (see attached patch):
- The qop Parameter must be parsed correctly and not just be ignored
- The fact that it is legal to have a missing qop must not be ignored
- The class should be prepared to handle the auth-int qop option
(even though an implementation is not possible with the current design)
- The public interface of this class is narrowed (as it is not needed by
the tests any more)
- The test cases should check the actual result rather than checking for
equality after another run through the same logic. Note: This is not
simple for requests that require the client to generate a cnonce.
The patch is against HEAD. The 2.0 branch would be unaffected by these
changes.
Odi
Re: [PATCH] Reworked digest auth
Posted by Ortwin Glück <or...@nose.ch>.
Can I get feedback about this patch please, fellows?
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
Re: [PATCH] Reworked digest auth
Posted by Ortwin Glück <or...@nose.ch>.
Oleg Kalnichevski wrote:
> Odi, the patch looks fine to me. I just dislike this part:
>
> try {
> cnonce = createCnonce();
> } catch(AuthenticationException e) {
> throw new RuntimeException(e.toString());
> }
>
>
> At the very least I would rethrow the exception as
> MalformedChallengeException, or, preferably, change createCnonce to
> throw MalformedChallengeException instead of AuthenticationException in
> the very first place.
Agreed. It's not nice.
The original exception that is generated is because MD5 is not
available. I think this should generally trigger an unchecked exception
since it is a basic feature needed for HttpClient to work (and should
never happen anyway). I think createCnonce should just throw a
RuntimeException instead of AuthenticationException. The
AuthenticationException is sematically wrong here anyway.
> As to the stateful authentication framework, it sounds like a reasonable
> improvement, but I suggest that we hold it off until the comprehensive
> API redesign that should follow the 2.1 release. Feel free to file a bug
> report meanwhile.
Okay.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
Re: [PATCH] Reworked digest auth
Posted by Oleg Kalnichevski <ol...@apache.org>.
Odi, the patch looks fine to me. I just dislike this part:
try {
cnonce = createCnonce();
} catch(AuthenticationException e) {
throw new RuntimeException(e.toString());
}
At the very least I would rethrow the exception as
MalformedChallengeException, or, preferably, change createCnonce to
throw MalformedChallengeException instead of AuthenticationException in
the very first place.
As to the stateful authentication framework, it sounds like a reasonable
improvement, but I suggest that we hold it off until the comprehensive
API redesign that should follow the 2.1 release. Feel free to file a bug
report meanwhile.
Cheers
Oleg
On Wed, 2003-09-17 at 10:13, Ortwin Glück wrote:
> Take 2:
>
> - DigestScheme requires a nonce in every challange now
> (according to RFC 2617) test cases changed accordingly
> - included Olegs feedback
>
> After reading RFC 2617 I came to the conclusion, that we need to rework
> out authentication mechanism to support stateful authentication retries.
> Digest Authentication requires you to keep track of the number of times
> authentication has been retries (NC) and to calculate some values only
> on the first time etc. This can be achieved by making the DigestScheme
> more stateful (a1) and by changing authenticator to keep the
> DigestScheme instance across retries and pass in the challenge each time.
>
> Odi
>
> Oleg Kalnichevski wrote:
>
> > Odi,
> > The patch looks fine to me. There is just a few minor points that I
> > would like to be considered before the patch is committed:
> >
> > - DigestScheme#DigestScheme( String ) constructor should probably log a
> > warning message or even throw an exception if it encounters an
> > unrecognised 'qop' element. Currently they are just silently ignored.
> > - Use StringBuffer to concatenate strings in DigestScheme#createDigest(
> > String, String ).
> > - A test case for unsupported qop in HTTP Digest authentication would be
> > nice.
> > - The patch makes a few public methods private (quite appropriately in
> > my opinion). Nobody is going to miss them, I think, however, the fact of
> > 2.0 API breakage should be reflected in API_CHANGES_2_1.txt
> >
> > Cheers
> >
> > Oleg
>
> ______________________________________________________________________
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
Re: [PATCH] Reworked digest auth
Posted by Ortwin Glück <or...@nose.ch>.
Take 2:
- DigestScheme requires a nonce in every challange now
(according to RFC 2617) test cases changed accordingly
- included Olegs feedback
After reading RFC 2617 I came to the conclusion, that we need to rework
out authentication mechanism to support stateful authentication retries.
Digest Authentication requires you to keep track of the number of times
authentication has been retries (NC) and to calculate some values only
on the first time etc. This can be achieved by making the DigestScheme
more stateful (a1) and by changing authenticator to keep the
DigestScheme instance across retries and pass in the challenge each time.
Odi
Oleg Kalnichevski wrote:
> Odi,
> The patch looks fine to me. There is just a few minor points that I
> would like to be considered before the patch is committed:
>
> - DigestScheme#DigestScheme( String ) constructor should probably log a
> warning message or even throw an exception if it encounters an
> unrecognised 'qop' element. Currently they are just silently ignored.
> - Use StringBuffer to concatenate strings in DigestScheme#createDigest(
> String, String ).
> - A test case for unsupported qop in HTTP Digest authentication would be
> nice.
> - The patch makes a few public methods private (quite appropriately in
> my opinion). Nobody is going to miss them, I think, however, the fact of
> 2.0 API breakage should be reflected in API_CHANGES_2_1.txt
>
> Cheers
>
> Oleg
Re: [PATCH] Reworked digest auth
Posted by Oleg Kalnichevski <ol...@apache.org>.
Odi,
The patch looks fine to me. There is just a few minor points that I
would like to be considered before the patch is committed:
- DigestScheme#DigestScheme( String ) constructor should probably log a
warning message or even throw an exception if it encounters an
unrecognised 'qop' element. Currently they are just silently ignored.
- Use StringBuffer to concatenate strings in DigestScheme#createDigest(
String, String ).
- A test case for unsupported qop in HTTP Digest authentication would be
nice.
- The patch makes a few public methods private (quite appropriately in
my opinion). Nobody is going to miss them, I think, however, the fact of
2.0 API breakage should be reflected in API_CHANGES_2_1.txt
Cheers
Oleg
On Thu, 2003-09-11 at 11:55, Ortwin Glück wrote:
> While reviewing a Patch to include MD5-sess into the Digest
> Authentication Scheme I came across a few flaws in that class. I suggest
> the following changes (see attached patch):
>
> - The qop Parameter must be parsed correctly and not just be ignored
> - The fact that it is legal to have a missing qop must not be ignored
> - The class should be prepared to handle the auth-int qop option
> (even though an implementation is not possible with the current design)
> - The public interface of this class is narrowed (as it is not needed by
> the tests any more)
> - The test cases should check the actual result rather than checking for
> equality after another run through the same logic. Note: This is not
> simple for requests that require the client to generate a cnonce.
>
> The patch is against HEAD. The 2.0 branch would be unaffected by these
> changes.
>
> Odi
>
> ______________________________________________________________________
> --
> To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org