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/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
