On Wed, 9 Mar 2022 14:23:38 GMT, Michael McMahon <micha...@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
