RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

On Fri, 4 Mar 2022 14:11:00 GMT, Jaikiran Pai <jpai at openjdk.org> 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