On Fri, 4 Mar 2022 14:11:00 GMT, Jaikiran Pai <[email protected]> wrote:
>> Hi,
>>
>> Could I get the following change reviewed please, which is to disable the
>> MD5 message digest algorithm by default in the HTTP Digest authentication
>> mechanism? The algorithm can be opted into by setting a new system property
>> "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also
>> updates the Digest authentication implementation to use some of the more
>> secure features defined in RFC7616, such as username hashing and additional
>> digest algorithms like SHA256 and SHA512-256.
>>
>> - Michael
>
> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
> line 443:
>
>> 441: } catch (IOException e) {
>> 442: // should not happen since the algorithm has already been
>> 443: // validated
>
> Hello Michael, was this comment meant for something else? The comment feels a
> bit odd since it says "has already been validated" which isn't the case since
> the `validateAlgorithm` itself has failed here.
Yes, copy and paste error. Thanks
> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
> line 524:
>
>> 522: logger.info(msg + " This constraint may be relaxed by
>> setting " +
>> 523: "the \"http.auth.digest.enabledDigestAlgs\" system
>> property.");
>> 524: }
>
> I suspect this is a typo and perhaps should have been
> `http.auth.digest.reEnabledAlgs`?
Thanks. I'm going to change the name once more and will update it then.
> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
> line 584:
>
>> 582: boolean truncate256 = false;
>> 583:
>> 584: if (algorithm.equals("SHA-512-256")) {
>
> It appears that the incoming param `algorithm` can be of any case. In some
> other places like the `validateAlgorithm`, this `algorithm` value has been
> first converted to an uppercase value and then used for additional checks.
> Should a similar upper case conversion be done here before this equality
> check? Perhaps, we should convert this to upper case once and then pass it
> around to these `validateAlgorithm` and `computeUserhash` methods?
Right. I'll check all that for the next round.
-------------
PR: https://git.openjdk.java.net/jdk/pull/7688
