RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]

[Daniel Fuchs]

On Wed, 9 Mar 2022 14:23:38 GMT, Michael McMahon <michaelm 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
>
> Michael McMahon has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - update
>  - update after first review round

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java line 82:

> 80:         @SuppressWarnings("removal")
> 81:         String secprops = AccessController.doPrivileged(
> 82:             new PrivilegedAction<>() {

could use a lambda instead of an anonymous class?

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java line 85:

> 83:                 public String run() {
> 84:                     return Security.getProperty(secPropName)
> 85:                                    .replaceAll("\\s", "")

`Security.getProperty` may return `null` so replacement should only be made after checking that it is non null.

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java line 94:

> 92:             disabledAlgs.add(algorithm);
> 93:         }
> 94:         return Collections.unmodifiableSet(disabledAlgs);

Could use `Set.copyOf()` ? Or is there anything that might call contains(null)? Also should strings be converted to upper case like below?

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java line 127:

> 125:                     String s = NetProperties.get(enabledAlgPropName);
> 126:                     return s == null
> 127:                         ? "" : s.replaceAll("\\s", "").toUpperCase();

Should probably use Local.ROOT to convert to upper case.
It seems to me that the code that takes a String as argument, check for null and returns an empty set, remove spaces, convert it to upper case, splits the string at commas, and create an immutable set from that, could be moved to an auxillary function and called for parsing both the Security property and the System property - since their syntax is identical.

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java line 397:

> 395:         params.setQop (p.findValue("qop"));
> 396:         params.setUserhash (Boolean.valueOf(p.findValue("userhash")));
> 397:         params.setCharset(p.findValue("charset", "ISO_8859_1").toUpperCase());

Should probably use Locale.ROOT when converting to upper case

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java line 426:

> 424:             algorithm = "MD5";  // The default, accoriding to rfc2069
> 425:         }
> 426:         algorithm = algorithm.toUpperCase();

same here

-------------

PR: https://git.openjdk.java.net/jdk/pull/7688