[9] RFR: 8138990: Implementation of HTTP Digest authentication may be more flexible

[Michael McMahon]

Artem,

The change looks fine.

Thanks,
Michael

On 04/01/16 18:39, Artem Smotrakov wrote:
> Hi Michael,
>
> On 01/04/2016 02:28 AM, Michael McMahon wrote:
>> On 30/12/15 03:22, Artem Smotrakov wrote:
>>> Hi Michael,
>>>
>>> Thanks for review, it looks like BNF notation uses only a comma as a 
>>> separator
>>>
>>> http://www.w3.org/Notation.html
>>>
>>> ...
>>> <l>#<m>element
>>>
>>> indicating at least l and at most m elements, each separated by one 
>>> or more commas (",").
>>> ...
>>>
>>
>> Hi Artem,
>>
>> The BNF definitions used in RFC 2617 come from section 2.1 of RFC 2616
>> which allows linear white space between tokens.
>>
>> #rule
>>        A construct "#" is defined, similar to "*", for defining lists of
>>        elements. The full form is "<n>#<m>element" indicating at least
>>        <n> and at most <m> elements, each separated by one or more commas
>>        (",") and OPTIONAL linear white space (LWS).
> If I read it correctly, it means that a comma is used as a separator, 
> but there also may be a linear whitespace after a comma. So for 
> example "auth,auth-int" and "auth, auth-int" are equal, and should 
> result to a list of two elements. Current version of webrev seems to 
> follow it (first, it splits a string by a comma, and then ignores 
> white spaces):
>
> http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.01/
>
> Am I missing something?
>
> Artem
>
>>
>> So, I think we should be conservative and check for white-space.
>>
>> - Michael.
>> Michael
>>> And here is "qop" definition from https://tools.ietf.org/html/rfc2617
>>>
>>> ...
>>>       qop-options       = "qop" "=" <"> 1#qop-value <">
>>>       qop-value         = "auth" | "auth-int" | token
>>> ...
>>>
>>> Please take a look at updated webrev:
>>>
>>> http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.01/
>>>
>>> Artem
>>>
>>> On 12/22/2015 05:59 AM, Michael McMahon wrote:
>>>> Hi Artem,
>>>>
>>>>
>>>> On 04/12/15 11:41, Artem Smotrakov wrote:
>>>>> Hello,
>>>>>
>>>>> Please review this small fix for DigestAuthentication class.
>>>>>
>>>>> 1. Added a check in DigestAuthentication.setNonce(String) that 
>>>>> nonce is not null. NPE may happen if a buggy HTTP server returns 
>>>>> "WWW-Authenticate" header which doesn't contain a "nonce" field. 
>>>>> According to RFCs 2069 [1] and 2617 [2], this is not expected 
>>>>> behaviour, but it would be better if an HTTP client threw a 
>>>>> checked IOException instead of NPE.
>>>>>
>>>>
>>>> That's fine.
>>>>
>>>>> 2. Updated DigestAuthentication.setQop(String) method to accept 
>>>>> both a whitespace and a comma as a delimiter. RFC 2617 [2] says 
>>>>> that "qop" may contain more than one token, but it doesn't specify 
>>>>> a delimiter for "qop" field in "WWW-Authenticate" header. There is 
>>>>> an example of "WWW-Authenticate" header in RFC 2617 [2] where a 
>>>>> comma is used as a delimiter of value in "qop" field.
>>>>>
>>>>
>>>> It looks like the BNF specification mandates a comma and optional 
>>>> linear white space.
>>>> So, the old code was buggy, but we didn't see the problem because 
>>>> there is typically
>>>> only at most ever one value used for the qop field. But, to be 
>>>> strictly correct, we would
>>>> have to check for TABs also. So, I think the correct behavior is to 
>>>> delimit using comma
>>>> and remove any white space
>>>>
>>>> - Michael.
>>>>
>>>>> 3. Added a test for Digest authentication.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8138990
>>>>> Webrev: 
>>>>> http://cr.openjdk.java.net/~asmotrak/http_auth_digest/webrev.00/
>>>>>
>>>>> [1] https://tools.ietf.org/html/rfc2069
>>>>> [2] https://tools.ietf.org/html/rfc2617
>>>>>
>>>>> Artem
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160121/2f29de8c/attachment-0001.html>