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

[Weijun Wang]

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/java/net/doc-files/net-properties.html line 234:

> 232:         in the {@code java.security} properties file and currently comprises {@code MD5} and
> 233:         {@code SHA-1}. If it is still required to use one of these algorithms, then they can be
> 234:         re-enabled by setting this property to a comma separated list of the algorithm names.</P>

Can we use "re-enabled" in the property name? To me, the name "enabled" sounds like all enabled algorithms are listed here.

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

> 66: 
> 67:     private static final String secPropName =
> 68:         "jdk.httpdigest.defaultDisabledAlgorithms";

How about a dot between "http" and "digest"?

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

> 74:     // used for proxy connections, or plain text http server connections
> 75: 
> 76:     private static final Set<String> defDisabledAlgs = getDefaultAlgs();

Can we move this field into a local variable in the static block near line 120? After all it's the `disabledAlgs` field finally gets used and the `defDisabledAlgs` is only used to calculate it?

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

> 76:     private static final Set<String> defDisabledAlgs = getDefaultAlgs();
> 77: 
> 78:     private static Set<String> getDefaultAlgs() {

How about rename the method to include "disabled"?

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

> 691:         }
> 692:         md.update(src.getBytes(charset));
> 693:         if (passwd != null) {

[RFC7616 ](https://datatracker.ietf.org/doc/html/rfc7616#section-4) says the password should also support unicode. It's OK we use the old code for compatibility reason.

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

> 699:         }
> 700:         byte[] digest = md.digest();
> 701:         StringBuilder res = new StringBuilder(digest.length * 2);

Can we use `HexFormat` to encode the bytes?

src/java.base/share/conf/security/java.security line 711:

> 709: # separated list of algorithms to be allowed.
> 710: #
> 711: jdk.httpdigest.defaultDisabledAlgorithms = MD5, MD-5, SHA1, SHA-1

I haven't seen people using "MD-5". It's also not an alias of "MD5" in our own security providers. On the other hand, we support both "SHA1" and "SHA" (and its OID) as aliases of "SHA-1". So, either we list all these aliases here, or we only put the standard names here and "canonicalize" the name when we see one. `sun.security.util.KnownOIDs.findMatch("SHA-1").stdName()` can be used.

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

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