On Fri, 4 Mar 2022 14:39:50 GMT, Jaikiran Pai <j...@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 > > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > line 423: > >> 421: String algorithm = params.getAlgorithm (); >> 422: boolean userhash = params.getUserhash (); >> 423: params.incrementNC (); > > I am not familiar with the code and the request/response flow involved in > this code, so what I mention may not be relevant. However, do you think the > algorithm validation that we added in this PR should happen before this > `incrementNC()` is called and the `ncString` construction is done? I see > that this incremented `ncCount` then gets used in the `checkResponse` part. > In the case where the algorithm validation fails and we return `null` from > this method (which effectively means not setting the authorization header), > do you think the subsequent `checkResponse` would run into issues due to the > `incrementNC()` being done here? I think there probably wouldn't be a problem as the value would always be increasing and the server is only checking for duplicates, or replays rather than gaps. But, nevertheless, it seems like better practice to do the check before incrementing the counter. ------------- PR: https://git.openjdk.java.net/jdk/pull/7688
