Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v3]
On Tue, 15 Mar 2022 10:24:43 GMT, Michael McMahon wrote: >> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java >> line 102: >> >>> 100: propPrefix + "reEnabledAlgorithms"; >>> 101: >>> 102: private static final Set disabledAlgorithms = new >>> HashSet<>(); >> >> It would be much better if this was an immutable set to make it MT-safe. You >> could set the value in the static block below using Set.copyOf(). > > The Set is private to the class and is not modified after the static > initializer completes. It's not clear to me how using Set.copyOf provides > stronger MT-safe guarantees than this. Better safe than sorry. An alternative could be to use ConcurrentHashMap.newKeySet(); But since it's supposed to be both immutable and MT-safe then Set.copyOf() would probably be a lighter and better choice. - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v3]
On Fri, 11 Mar 2022 18:12:27 GMT, Daniel Fuchs wrote: >> Michael McMahon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> update after second review round > > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > line 102: > >> 100: propPrefix + "reEnabledAlgorithms"; >> 101: >> 102: private static final Set disabledAlgorithms = new >> HashSet<>(); > > It would be much better if this was an immutable set to make it MT-safe. You > could set the value in the static block below using Set.copyOf(). The Set is private to the class and is not modified after the static initializer completes. It's not clear to me how using Set.copyOf provides stronger MT-safe guarantees than this. - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v3]
On Fri, 11 Mar 2022 17:37:44 GMT, Michael McMahon 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 one > additional commit since the last revision: > > update after second review round src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java line 102: > 100: propPrefix + "reEnabledAlgorithms"; > 101: > 102: private static final Set disabledAlgorithms = new > HashSet<>(); It would be much better if this was an immutable set to make it MT-safe. You could set the value in the static block below using Set.copyOf(). - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v3]
On Fri, 11 Mar 2022 17:37:44 GMT, Michael McMahon 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 one > additional commit since the last revision: > > update after second review round Please remove `src/java.base/share/classes/java/util/.Random.java.swp`. src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java line 430: > 428: algorithm = "MD5"; // The default, accoriding to rfc2069 > 429: } > 430: var oid = > KnownOIDs.findMatch(algorithm.toUpperCase(Locale.ROOT)); No need to call `toUpperCase`. `findMatch` already called that. On the other hand, we don't support the name `SHA-512-256`, and if you translate it to stdName here there is no need to do it again in `computeUserhash`. - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v3]
On Fri, 11 Mar 2022 17:37:44 GMT, Michael McMahon 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 one > additional commit since the last revision: > > update after second review round I still plan to add a couple more tests for UTF-8 encoding and if possible for username hashing - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v3]
> 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 one additional commit since the last revision: update after second review round - Changes: - all: https://git.openjdk.java.net/jdk/pull/7688/files - new: https://git.openjdk.java.net/jdk/pull/7688/files/b765d412..3d5ef143 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7688=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7688=01-02 Stats: 160 lines in 12 files changed: 59 ins; 25 del; 76 mod Patch: https://git.openjdk.java.net/jdk/pull/7688.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7688/head:pull/7688 PR: https://git.openjdk.java.net/jdk/pull/7688
