Re: RFR: JDK-8282354 : Remove dependancy of TestHttpServer, HttpTransaction, HttpCallback from open/test/jdk/ tests [v4]

[Mahendra Chhipa]

> Updated following remaining tests to remove depenedies of TestHttpServer, 
> HttpTransaction, HttpCallback
> open/test/jdk/java/net/ProxySelector/LoopbackAddresses.java
> open/test/jdk/java/net/ProxySelector/ProxyTest.java
> open/test/jdk/java/net/URL/PerConnectionProxy.java
> open/test/jdk/java/net/URLConnection/B5052093.java
> open/test/jdk/sun/net/www/AuthHeaderTest.java
> open/test/jdk/sun/net/www/http/KeepAliveCache/B5045306.java

Mahendra Chhipa has updated the pull request incrementally with one additional 
commit since the last revision:

  Implemented the review comments.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7616/files
  - new: https://git.openjdk.java.net/jdk/pull/7616/files/d62ba21c..f2218025

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7616&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7616&range=02-03

  Stats: 27 lines in 6 files changed: 15 ins; 0 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7616.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7616/head:pull/7616

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

Re: RFR: JDK-8282354 : Remove dependancy of TestHttpServer, HttpTransaction, HttpCallback from open/test/jdk/ tests [v3]

[Mahendra Chhipa]

On Wed, 2 Mar 2022 12:35:47 GMT, Daniel Fuchs  wrote:

>> Mahendra Chhipa has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Removed extra whitespace
>
> test/jdk/java/net/URLConnection/B5052093.java line 113:
> 
>> 111: exchange.close();
>> 112: } catch (IOException e) {
>> 113: e.printStackTrace();
> 
> Are you sure that this results in the same response headers than before?
> If I'm not mistaken here we will send both Content-Length and 
> Transfer-Encoding: chunked. Was that what the previous server did, and what 
> the test wants to test?

Yes, previously also, setting the content-length Integer.MAX_VALUE)) + 2, and 
was not sending any content. Here wanted to test, that 
URLConnection.getContentLength() does not throw NumberFormatException and 
return -1 if content-length is long value.
In case of HttpExchange.setResponseHeader(). If responseLength is -1, then 
content-length value is overridden to 0, if already set explicitly. Same is the 
case when responseLength is > 0. Only in the case when responseLength == 0, 
content-length value is not overriden if already set explicitly., thats why I 
am using chunked encoding and not writing any data.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

On Fri, 4 Mar 2022 09:37:21 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.enabledDigestAlgs" 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

I have still to add the doc update describing the system property, which will 
come shortly. I may suggest a change of name to the system property to better 
reflect its exact meaning/purpose

-

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

RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

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.enabledDigestAlgs" 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

-

Commit messages:
 - fix whitespace
 - update property name. add documentation
 - fixed one more test
 - fixed up existing tests using digest auth
 - Merge branch 'master' into md5
 - added userhash support plus test
 - fixed problem in copyright header
 - added test
 - Merge branch 'master' into md5
 - update
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/b6c35ae4...f0fb72de

Changes: https://git.openjdk.java.net/jdk/pull/7688/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7688&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8281561
  Stats: 302 lines in 11 files changed: 247 ins; 3 del; 52 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

Re: RFR: JDK-8282354 : Remove dependancy of TestHttpServer, HttpTransaction, HttpCallback from open/test/jdk/ tests [v3]

[Mahendra Chhipa]

On Wed, 2 Mar 2022 12:42:11 GMT, Daniel Fuchs  wrote:

>> Mahendra Chhipa has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Removed extra whitespace
>
> test/jdk/sun/net/www/http/KeepAliveCache/B5045306.java line 206:
> 
>> 204: // override the Content-length header to be greater 
>> than the actual response body
>> 205: trans.getResponseHeaders().set("Content-length", 
>> Integer.toString(responseBody.length+1));
>> 206: trans.sendResponseHeaders(200, 0);
> 
> Here again we will be mixing Content-Length and chunked

In case of HttpExchange.setResponseHeader(). If responseLength is -1, then 
content-length value is overridden to 0, if already set explicitly. Same is the 
case when responseLength is > 0. Only in the case when responseLength == 0, 
content-length value is not overriden if already set explicitly., that's why I 
am using chunked encoding and writing the data less than the content length.

-

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

Re: RFR: JDK-8282354 : Remove dependancy of TestHttpServer, HttpTransaction, HttpCallback from open/test/jdk/ tests [v3]

[Daniel Fuchs]

On Fri, 4 Mar 2022 11:10:40 GMT, Mahendra Chhipa  wrote:

>> test/jdk/sun/net/www/http/KeepAliveCache/B5045306.java line 206:
>> 
>>> 204: // override the Content-length header to be greater 
>>> than the actual response body
>>> 205: trans.getResponseHeaders().set("Content-length", 
>>> Integer.toString(responseBody.length+1));
>>> 206: trans.sendResponseHeaders(200, 0);
>> 
>> Here again we will be mixing Content-Length and chunked
>
> In case of HttpExchange.setResponseHeader(). If responseLength is -1, then 
> content-length value is overridden to 0, if already set explicitly. Same is 
> the case when responseLength is > 0. Only in the case when responseLength == 
> 0, content-length value is not overriden if already set explicitly., that's 
> why I am using chunked encoding and writing the data less than the content 
> length.

I understand why you do it - but the client will react differently if both 
Content-Length *and* chunk are specified, as opposed to when only 
Content-Length is specified. So I just want to make sure that we are testing 
the same thing than before. If we are not testing the same thing, then you 
might have to use a ServerSocket directly - rather than an HttpServer, to make 
sure we're sending back the same things than before.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Daniel Fuchs]

On Fri, 4 Mar 2022 09:37:21 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

Should we instead have a property to disable algorithms, whose default value 
would contain "MD5" by default?

-

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

Re: RFR: 8282617: sun.net.www.protocol.https.HttpsClient#putInKeepAliveCache() doesn't use a lock while dealing with inCache field

[Michael McMahon]

On Thu, 3 Mar 2022 16:13:37 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this change which proposes to fix the issue 
> noted in https://bugs.openjdk.java.net/browse/JDK-8282617? 
> 
> The `HttpClient` class uses a `inCache` (internal) field to keep track of 
> whether a connection is in the keep-alive cache. This is a mutable field and 
> when dealing with this field the `HttpClient` class uses a lock.
> 
> The `HttpsClient` class extends this `HttpClient` class. It additionally 
> overrides the `putInKeepAliveCache()` method to use a Https specific way of 
> populating the keep alive cache. While doing so it also reads and updates the 
> `inCache` `protected` field from its super `HttpClient` class, but doesn't 
> use a lock to do so. This can thus cause a race condition when the `inCache` 
> field gets accessed concurrently, for example a separate thread calling the 
> `HttpClient#isInKeepAliveCache()` method.
> 
> To fix this issue, the commit here uses the same lock/unlock construct used 
> in the `HttpClient` super class.

LGTM

-

Marked as reviewed by michaelm (Reviewer).

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

On Fri, 4 Mar 2022 11:25:38 GMT, Daniel Fuchs  wrote:

> Should we instead have a property to disable algorithms, whose default value 
> would contain "MD5" by default?

I considered that and implemented it that way at the start, but what you would 
end up with then is users running their code with something like: 
-DdisabledAlgNames="" 

I find that style leads to a much less explicit "opting in" than by making the 
user explicitly identify the deprecated algorithm by name.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Daniel Fuchs]

On Fri, 4 Mar 2022 12:03:44 GMT, Michael McMahon  wrote:

> I considered that and implemented it that way at the start, but what you 
> would end up with then is users running their code with something like: 
> -DdisabledAlgNames=""
> 
> I find that style leads to a much less explicit "opting in" than by making 
> the user explicitly identify the deprecated algorithm by name.

Right - but it would also allow users to opt-in to disable more algorithms by 
listing them in the property

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

On Fri, 4 Mar 2022 12:12:25 GMT, Daniel Fuchs  wrote:

> > I considered that and implemented it that way at the start, but what you 
> > would end up with then is users running their code with something like: 
> > -DdisabledAlgNames=""
> > I find that style leads to a much less explicit "opting in" than by making 
> > the user explicitly identify the deprecated algorithm by name.
> 
> Right - but it would also allow users to opt-in to disable more algorithms by 
> listing them in the property

In practical terms, the only other likely candidate there is SHA-1. If that 
weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the default disabled 
algorithms and in net.properties we identify MD5 only for now. Users could add 
to that list if they want or even specify it on the command line. I think it's 
potentially confusing, but maybe there is a case for adding to the disabled 
list. I need to think about a way to do this without subvertng the point about 
making the user explicitly opt in.

-

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

Re: RFR: JDK-8282354 : Remove dependancy of TestHttpServer, HttpTransaction, HttpCallback from open/test/jdk/ tests [v5]

[Mahendra Chhipa]

> Updated following remaining tests to remove depenedies of TestHttpServer, 
> HttpTransaction, HttpCallback
> open/test/jdk/java/net/ProxySelector/LoopbackAddresses.java
> open/test/jdk/java/net/ProxySelector/ProxyTest.java
> open/test/jdk/java/net/URL/PerConnectionProxy.java
> open/test/jdk/java/net/URLConnection/B5052093.java
> open/test/jdk/sun/net/www/AuthHeaderTest.java
> open/test/jdk/sun/net/www/http/KeepAliveCache/B5045306.java

Mahendra Chhipa has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains 11 additional commits 
since the last revision:

 - Removed extra space.
 - Removed Transfer encoding chunked from B5045306
 - Merge branch 'master' into JDK-8282354
 - Implemented the review comments.
 - Removed extra whitespace
 - Implemented the review comments.
 - JDK-8282354 : Remove dependancy of TestHttpServer, HttpTransaction, 
HttpCallback from open/test/jdk/ tests
 - Merge branch 'master' into JDK-8282354
 - WIP
 - Merge branch 'master' into httptrans
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/b46ce84b...7f947718

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7616/files
  - new: https://git.openjdk.java.net/jdk/pull/7616/files/f2218025..7f947718

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7616&range=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7616&range=03-04

  Stats: 18347 lines in 450 files changed: 12637 ins; 3117 del; 2593 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7616.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7616/head:pull/7616

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

On Fri, 4 Mar 2022 09:37:21 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

> > > I considered that and implemented it that way at the start, but what you 
> > > would end up with then is users running their code with something like: 
> > > -DdisabledAlgNames=""
> > > I find that style leads to a much less explicit "opting in" than by 
> > > making the user explicitly identify the deprecated algorithm by name.
> > 
> > 
> > Right - but it would also allow users to opt-in to disable more algorithms 
> > by listing them in the property
> 
> In practical terms, the only other likely candidate there is SHA-1. If that 
> weren't the case, I'd disagree with your point.
> 
> So, maybe, we could have a 2nd net property with the default disabled 
> algorithms and in net.properties we identify MD5 only for now. Users could 
> add to that list if they want or even specify it on the command line. I think 
> it's potentially confusing, but maybe there is a case for adding to the 
> disabled list. I need to think about a way to do this without subvertng the 
> point about making the user explicitly opt in.



> > > I considered that and implemented it that way at the start, but what you 
> > > would end up with then is users running their code with something like: 
> > > -DdisabledAlgNames=""
> > > I find that style leads to a much less explicit "opting in" than by 
> > > making the user explicitly identify the deprecated algorithm by name.
> > 
> > 
> > Right - but it would also allow users to opt-in to disable more algorithms 
> > by listing them in the property
> 
> In practical terms, the only other likely candidate there is SHA-1. If that 
> weren't the case, I'd disagree with your point.
> 
> So, maybe, we could have a 2nd net property with the default disabled 
> algorithms and in net.properties we identify MD5 only for now. Users could 
> add to that list if they want or even specify it on the command line. I think 
> it's potentially confusing, but maybe there is a case for adding to the 
> disabled list. I need to think about a way to do this without subvertng the 
> point about making the user explicitly opt in.

Thinking about it again, I wonder if we should just deprecate SHA-1 at the same 
time. I think there will be less compatibility impact than with MD5, and it's 
basically broken as well. I don't see a reason to opt out of other algorithms 
at this time.

-

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

Re: RFR: JDK-8282354 : Remove dependancy of TestHttpServer, HttpTransaction, HttpCallback from open/test/jdk/ tests [v3]

[Mahendra Chhipa]

On Fri, 4 Mar 2022 11:21:47 GMT, Daniel Fuchs  wrote:

>> In case of HttpExchange.setResponseHeader(). If responseLength is -1, then 
>> content-length value is overridden to 0, if already set explicitly. Same is 
>> the case when responseLength is > 0. Only in the case when responseLength == 
>> 0, content-length value is not overriden if already set explicitly., that's 
>> why I am using chunked encoding and writing the data less than the content 
>> length.
>
> I understand why you do it - but the client will react differently if both 
> Content-Length *and* chunk are specified, as opposed to when only 
> Content-Length is specified. So I just want to make sure that we are testing 
> the same thing than before. If we are not testing the same thing, then you 
> might have to use a ServerSocket directly - rather than an HttpServer, to 
> make sure we're sending back the same things than before.

Now not using Transfer-Encoding Chunk.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Daniel Fuchs]

On Fri, 4 Mar 2022 12:29:28 GMT, Michael McMahon  wrote:

> > So, maybe, we could have a 2nd net property with the default disabled 
> > algorithms and in net.properties we identify MD5 only for now. Users could 
> > add to that list if they want or even specify it on the command line. I 
> > think it's potentially confusing, but maybe there is a case for adding to 
> > the disabled list. I need to think about a way to do this without subvertng 
> > the point about making the user explicitly opt in.
> 
> Thinking about it again, I wonder if we should just deprecate SHA-1 at the 
> same time. I think there will be less compatibility impact than with MD5, and 
> it's basically broken as well. I don't see a reason to opt out of other 
> algorithms at this time.

I see - maybe we should have a security property identifying the list of 
algorithm that are disabled, and then a system property to reenable them?

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Daniel Fuchs]

On Fri, 4 Mar 2022 09:37:21 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

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

> 69: // This will probably be expanded to include SHA-1 eventually
> 70: private static final Set defDisabledAlgs =
> 71: Set.of("MD5");

What I'm suggesting is that the content of this set could be seeded with the 
value of a Security Property, defined in java.security - rather than have the 
default value hardcoded here.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

On Fri, 4 Mar 2022 13:13:47 GMT, Daniel Fuchs  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 71:
> 
>> 69: // This will probably be expanded to include SHA-1 eventually
>> 70: private static final Set defDisabledAlgs =
>> 71: Set.of("MD5");
> 
> What I'm suggesting is that the content of this set could be seeded with the 
> value of a Security Property, defined in java.security - rather than have the 
> default value hardcoded here.

Yes, I think that should work

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Jaikiran Pai]

On Fri, 4 Mar 2022 09:37:21 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

src/java.base/share/classes/java/net/doc-files/net-properties.html line 227:

> 225:name.
> 226:  
> 227:  {@systemProperty http.auth.digest.reEnabledAlgs} 
> (default: )

Hello Michael, from what I understand of `doc-files` directory (which is where 
this html file resides) the `javadoc` tool considers it an unprocessed files 
location[1]. So would adding the `systemProperty` javadoc taglet here be 
necessary? I think this won't end up being listed in the system properties 
index generated by the javadoc tool and I think this line here will just get 
rendered literally.

[1] https://docs.oracle.com/javase/8/docs/technotes/tools/windows/javadoc.html

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Bernd Eckenfels]

Hello,

While I like the idea of the user having to explicitely specify the rexenabled 
legacy algorithms (as opposed to removing the defaultsdisabled) it is not the 
style the other algorithm policies in JCE work - so it might be confusing.

But, more critically I would separate the enabling/implementing of new 
algorithms from disabling old ones. Especially since there needs to be changes 
on the server side first. (And I wonder if this can be negotiated anyway?).

So why not start with a “provide new DIGEST Mechanisms” change? Having said 
that, would it need to start out with disabled new mechanisms so the update 
won’t change the behavior? (If there is no negotiation?)

Gruss
Bernd
--
http://bernd.eckenfels.net

Von: net-dev  im Auftrag von Michael McMahon 

Gesendet: Friday, March 4, 2022 1:33:06 PM
An: net-dev@openjdk.java.net 
Betreff: Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

On Fri, 4 Mar 2022 09:37:21 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

> > > I considered that and implemented it that way at the start, but what you 
> > > would end up with then is users running their code with something like: 
> > > -DdisabledAlgNames=""
> > > I find that style leads to a much less explicit "opting in" than by 
> > > making the user explicitly identify the deprecated algorithm by name.
> >
> >
> > Right - but it would also allow users to opt-in to disable more algorithms 
> > by listing them in the property
>
> In practical terms, the only other likely candidate there is SHA-1. If that 
> weren't the case, I'd disagree with your point.
>
> So, maybe, we could have a 2nd net property with the default disabled 
> algorithms and in net.properties we identify MD5 only for now. Users could 
> add to that list if they want or even specify it on the command line. I think 
> it's potentially confusing, but maybe there is a case for adding to the 
> disabled list. I need to think about a way to do this without subvertng the 
> point about making the user explicitly opt in.



> > > I considered that and implemented it that way at the start, but what you 
> > > would end up with then is users running their code with something like: 
> > > -DdisabledAlgNames=""
> > > I find that style leads to a much less explicit "opting in" than by 
> > > making the user explicitly identify the deprecated algorithm by name.
> >
> >
> > Right - but it would also allow users to opt-in to disable more algorithms 
> > by listing them in the property
>
> In practical terms, the only other likely candidate there is SHA-1. If that 
> weren't the case, I'd disagree with your point.
>
> So, maybe, we could have a 2nd net property with the default disabled 
> algorithms and in net.properties we identify MD5 only for now. Users could 
> add to that list if they want or even specify it on the command line. I think 
> it's potentially confusing, but maybe there is a case for adding to the 
> disabled list. I need to think about a way to do this without subvertng the 
> point about making the user explicitly opt in.

Thinking about it again, I wonder if we should just deprecate SHA-1 at the same 
time. I think there will be less compatibility impact than with MD5, and it's 
basically broken as well. I don't see a reason to opt out of other algorithms 
at this time.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Jaikiran Pai]

On Fri, 4 Mar 2022 09:37:21 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

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

> 522: logger.info(msg + " This constraint may be relaxed by 
> setting " +
> 523:  "the \"http.auth.digest.enabledDigestAlgs\" system 
> property.");
> 524: }

I suspect this is a typo and perhaps should have been 
`http.auth.digest.reEnabledAlgs`?

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Daniel Fuchs]

On Fri, 4 Mar 2022 13:50:37 GMT, Jaikiran Pai  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/java/net/doc-files/net-properties.html line 227:
> 
>> 225:   name.
>> 226: 
>> 227: {@systemProperty http.auth.digest.reEnabledAlgs} 
>> (default: )
> 
> Hello Michael, from what I understand of `doc-files` directory (which is 
> where this html file resides) the `javadoc` tool considers it an unprocessed 
> files location[1]. So would adding the `systemProperty` javadoc taglet here 
> be necessary? I think this won't end up being listed in the system properties 
> index generated by the javadoc tool and I think this line here will just get 
> rendered literally.
> 
> [1] https://docs.oracle.com/javase/8/docs/technotes/tools/windows/javadoc.html

This actually seems to work. If you build the docs for JDK mainline, you can 
search for instance, for `http.keepAlive.time.proxy` in the javadoc search box 
and it will lead you `net-properties.html`.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Jaikiran Pai]

On Fri, 4 Mar 2022 09:37:21 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

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

> 441: } catch (IOException e) {
> 442: // should not happen since the algorithm has already been
> 443: // validated

Hello Michael, was this comment meant for something else? The comment feels a 
bit odd since it says "has already been validated" which isn't the case since 
the `validateAlgorithm` itself has failed here.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Jaikiran Pai]

On Fri, 4 Mar 2022 09:37:21 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

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

> 582: boolean truncate256 = false;
> 583: 
> 584: if (algorithm.equals("SHA-512-256")) {

It appears that the incoming param `algorithm`  can be of any case. In some 
other places like the `validateAlgorithm`, this `algorithm` value has been 
first converted to an uppercase value and then used for additional checks. 
Should a similar upper case conversion be done here before this equality check? 
Perhaps, we should convert this to upper case once and then pass it around to 
these `validateAlgorithm` and `computeUserhash` methods?

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Jaikiran Pai]

On Fri, 4 Mar 2022 09:37:21 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

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?

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

On Fri, 4 Mar 2022 14:11:00 GMT, Jaikiran Pai  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 443:
> 
>> 441: } catch (IOException e) {
>> 442: // should not happen since the algorithm has already been
>> 443: // validated
> 
> Hello Michael, was this comment meant for something else? The comment feels a 
> bit odd since it says "has already been validated" which isn't the case since 
> the `validateAlgorithm` itself has failed here.

Yes, copy and paste error. Thanks

> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
>  line 524:
> 
>> 522: logger.info(msg + " This constraint may be relaxed by 
>> setting " +
>> 523:  "the \"http.auth.digest.enabledDigestAlgs\" system 
>> property.");
>> 524: }
> 
> I suspect this is a typo and perhaps should have been 
> `http.auth.digest.reEnabledAlgs`?

Thanks. I'm going to change the name once more and will update it then.

> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
>  line 584:
> 
>> 582: boolean truncate256 = false;
>> 583: 
>> 584: if (algorithm.equals("SHA-512-256")) {
> 
> It appears that the incoming param `algorithm`  can be of any case. In some 
> other places like the `validateAlgorithm`, this `algorithm` value has been 
> first converted to an uppercase value and then used for additional checks. 
> Should a similar upper case conversion be done here before this equality 
> check? Perhaps, we should convert this to upper case once and then pass it 
> around to these `validateAlgorithm` and `computeUserhash` methods?

Right. I'll check all that for the next round.

-

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

RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large

[Conor Cleary]

**Problem**
When a Continuation Frame is received by the httpclient using HTTP/2 after a 
Push Promise frame (can happen if the amount of headers to be sent in a single 
Push Promise frame exceeds the maximum frame size, so a Continuation frame is 
required), the following exception occurs:


java.io.IOException: no statuscode in response
at 
java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565)
at 
java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119)
...

This exception occurs because there is no existing flow in 
`jdk/internal/net/http/Http2Connection.java` which accounts for the case where 
a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. When this 
occurs, the only acceptable frame/s (as multiple continuations are also 
acceptable) that can be received by the client on the same stream is a 
continuation frame.

**Fix**
To ensure correct behavior, the following changes were made to 
`jdk/internal/net/http/Http2Connection.java`.

- The existing method `handlePushPromise()` was modified so that if the 
END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track 
the state of the Push Promise containing a shared `HeaderDecoder` and the 
received `PushPromiseFrame` is initialised.
- When the subsequent `ContinuationFrame` is received in `processFrame()`, the 
method `handlePushContinuation()` is called instead of the default flow 
resulting in `stream.incoming(frame)` being called (the source of the incorrect 
behaviour originally).
- In `handlePushContinuation()`, the shared decoder is used to decode the 
received `ContinuationFrame` headers and if the `END_HEADERS` flag is set 
(flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a whole 
is constructed which serves to combine the headers from both the 
`PushPromiseFrame` and the `ContinuationFrame`.

-

Commit messages:
 - 8263031: Created new flow for Push Promises followed by Continuations
 - 8263031: Setting END_HEADERS flag where appropriate
 - 8263031: Update test name and cleanup
 - 8263031: HttpClient throws Exception if it receives a Push Promise that is 
too large

Changes: https://git.openjdk.java.net/jdk/pull/7696/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263031
  Stats: 423 lines in 4 files changed: 336 ins; 67 del; 20 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7696.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7696/head:pull/7696

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Jaikiran Pai]

On Fri, 4 Mar 2022 14:06:14 GMT, Daniel Fuchs  wrote:

>> src/java.base/share/classes/java/net/doc-files/net-properties.html line 227:
>> 
>>> 225:  name.
>>> 226:
>>> 227:{@systemProperty http.auth.digest.reEnabledAlgs} 
>>> (default: )
>> 
>> Hello Michael, from what I understand of `doc-files` directory (which is 
>> where this html file resides) the `javadoc` tool considers it an unprocessed 
>> files location[1]. So would adding the `systemProperty` javadoc taglet here 
>> be necessary? I think this won't end up being listed in the system 
>> properties index generated by the javadoc tool and I think this line here 
>> will just get rendered literally.
>> 
>> [1] 
>> https://docs.oracle.com/javase/8/docs/technotes/tools/windows/javadoc.html
>
> This actually seems to work. If you build the docs for JDK mainline, you can 
> search for instance, for `http.keepAlive.time.proxy` in the javadoc search 
> box and it will lead you `net-properties.html`.

You are right indeed. I just checked the generated system properties index and 
it does list these properties and even links back to this document. In fact, I 
even found a mail which clearly states that this processing of 
`@systemProperties` is supported for `doc-files` 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056653.html

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

On Fri, 4 Mar 2022 14:39:50 GMT, Jaikiran Pai  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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Michael McMahon]

Bernd,

If I understand you correctly, there is no negotiation at this level. 
Once Digest is selected, it's up to the server to choose the algorithm 
and then the client can either accept it or else reject it and the whole 
request fails then.


It's true that with this change, if the server proposes MD5 by default, 
then the request will fail. But, that's the point of the change, and the 
compatibility impact is considered in the related CSR. In that case, 
either the server needs to change or else the system property to 
re-enable MD5 must be set.


I'd like to try out (for the next review round) the idea of establishing 
the default "insecure" protocols as a fixed security property (including 
MD5 and SHA-1) and then provide a settable system or networking property 
to override that.


Thanks

Michael

On 04/03/2022 14:03, Bernd Eckenfels wrote:

Hello,

While I like the idea of the user having to explicitely specify the rexenabled 
legacy algorithms (as opposed to removing the defaultsdisabled) it is not the 
style the other algorithm policies in JCE work - so it might be confusing.

But, more critically I would separate the enabling/implementing of new 
algorithms from disabling old ones. Especially since there needs to be changes 
on the server side first. (And I wonder if this can be negotiated anyway?).

So why not start with a “provide new DIGEST Mechanisms” change? Having said 
that, would it need to start out with disabled new mechanisms so the update 
won’t change the behavior? (If there is no negotiation?)

Gruss
Bernd
--
http://bernd.eckenfels.net

Von: net-dev  im Auftrag von Michael 
McMahon
Gesendet: Friday, March 4, 2022 1:33:06 PM
An:net-dev@openjdk.java.net  
Betreff: Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

On Fri, 4 Mar 2022 09:37:21 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

I considered that and implemented it that way at the start, but what you would end up 
with then is users running their code with something like: -DdisabledAlgNames=""
I find that style leads to a much less explicit "opting in" than by making the 
user explicitly identify the deprecated algorithm by name.


Right - but it would also allow users to opt-in to disable more algorithms by 
listing them in the property

In practical terms, the only other likely candidate there is SHA-1. If that 
weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the default disabled 
algorithms and in net.properties we identify MD5 only for now. Users could add 
to that list if they want or even specify it on the command line. I think it's 
potentially confusing, but maybe there is a case for adding to the disabled 
list. I need to think about a way to do this without subvertng the point about 
making the user explicitly opt in.




I considered that and implemented it that way at the start, but what you would end up 
with then is users running their code with something like: -DdisabledAlgNames=""
I find that style leads to a much less explicit "opting in" than by making the 
user explicitly identify the deprecated algorithm by name.


Right - but it would also allow users to opt-in to disable more algorithms by 
listing them in the property

In practical terms, the only other likely candidate there is SHA-1. If that 
weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the default disabled 
algorithms and in net.properties we identify MD5 only for now. Users could add 
to that list if they want or even specify it on the command line. I think it's 
potentially confusing, but maybe there is a case for adding to the disabled 
list. I need to think about a way to do this without subvertng the point about 
making the user explicitly opt in.

Thinking about it again, I wonder if we should just deprecate SHA-1 at the same 
time. I think there will be less compatibility impact than with MD5, and it's 
basically broken as well. I don't see a reason to opt out of other algorithms 
at this time.

-

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

Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large

[Conor Cleary]

On Fri, 4 Mar 2022 14:42:40 GMT, Conor Cleary  wrote:

> **Problem**
> When a Continuation Frame is received by the httpclient using HTTP/2 after a 
> Push Promise frame (can happen if the amount of headers to be sent in a 
> single Push Promise frame exceeds the maximum frame size, so a Continuation 
> frame is required), the following exception occurs:
> 
> 
> java.io.IOException: no statuscode in response
> at 
> java.net.http/jdk.internal.net.http.HttpClientImpl.send(HttpClientImpl.java:565)
> at 
> java.net.http/jdk.internal.net.http.HttpClientFacade.send(HttpClientFacade.java:119)
> ...
> 
> This exception occurs because there is no existing flow in 
> `jdk/internal/net/http/Http2Connection.java` which accounts for the case 
> where a PushPromiseFrame is received with the END_HEADERS flag set to 0x0. 
> When this occurs, the only acceptable frame/s (as multiple continuations are 
> also acceptable) that can be received by the client on the same stream is a 
> continuation frame.
> 
> **Fix**
> To ensure correct behavior, the following changes were made to 
> `jdk/internal/net/http/Http2Connection.java`.
> 
> - The existing method `handlePushPromise()` was modified so that if the 
> END_HEADERS flag is _unset_ (flags equal to 0x0), then a record used to track 
> the state of the Push Promise containing a shared `HeaderDecoder` and the 
> received `PushPromiseFrame` is initialised.
> - When the subsequent `ContinuationFrame` is received in `processFrame()`, 
> the method `handlePushContinuation()` is called instead of the default flow 
> resulting in `stream.incoming(frame)` being called (the source of the 
> incorrect behaviour originally).
> - In `handlePushContinuation()`, the shared decoder is used to decode the 
> received `ContinuationFrame` headers and if the `END_HEADERS` flag is set 
> (flags equal to 0x4), the `HttpHeaders` object for the Push Promise as a 
> whole is constructed which serves to combine the headers from both the 
> `PushPromiseFrame` and the `ContinuationFrame`.
> 
> A regression test was included which verifies that the exception is not 
> thrown and that the headers arrive correctly.

I see that a couple of imports got changed by my IDE, will address that shortly

-

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

Re: RFR: 8280494: (D)TLS signature schemes [v18]

[Sean Mullan]

On Thu, 17 Feb 2022 18:57:02 GMT, Xue-Lei Andrew Fan  wrote:

>> This update is to support signature schemes customization for individual 
>> (D)TLS connection.  Please review the CSR as well:
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8280495
>> RFE: https://bugs.openjdk.java.net/browse/JDK-8280494
>> Release-note: https://bugs.openjdk.java.net/browse/JDK-8281290
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   more spec update per CSR feedback

Since this new API is also intended to be supported for DTLS, have you added 
implementation support for that, and if so have you added a test for it?

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Weijun Wang]

On Fri, 4 Mar 2022 09:37:21 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

Several comments added. Also, I am reading rfc7616 and it will be really nice 
if we can include the 2 examples at 
https://datatracker.ietf.org/doc/html/rfc7616#section-3.9 as a known answer 
test. Of course this means you need a way to provide a hardcoded `cnonce` and 
it's probably not easy.

src/java.base/share/classes/java/net/doc-files/net-properties.html line 232:

> 230: includes {@code MD5} but other algorithms may be added in 
> future. If it is still
> 231: required to use one of these algorithms, then they can be 
> re-enabled by setting
> 232: this property to a comma separated list of the algorithm 
> names.

Is it necessary to emphasize that no whitespace is allowed around the comma in 
the property value? Or is it better to modify the implementation below to allow 
whitespaces? I notice that whitespace is allowed in some of the other 
properties. For example: 
https://github.com/openjdk/jdk/blob/de3113b998550021bb502cd6f766036fb8351e7d/src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java#L228

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

> 73: // A net property which overrides the disabled set above.
> 74: private static final String enabledAlgPropName = "http.auth.digest." +
> 75: "reEnabledAlgs";

Why not put the string on one line?

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

> 656: // truncate256 means only use the last 256 bits of the digest (32 
> bytes)
> 657: private String encode(String src, char[] passwd, MessageDigest md, 
> boolean truncate256) {
> 658: md.update(src.getBytes(ISO_8859_1.INSTANCE));

Maybe we can support the "charset" parameter as well. The only allowed value is 
"UTF-8".

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

> 668: if (truncate256) {
> 669: assert digest.length >= 32;
> 670: start = digest.length - 32;

Does this mean the left half is truncated? My understanding is that the right 
half should be.

-

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

RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Matteo Baccan]

Hi

I have reviewed the code for removing double semicolons at the end of lines

all the best
matteo

-

Commit messages:
 - Removed double semicolon at the end of lines

Changes: https://git.openjdk.java.net/jdk/pull/7268/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7268&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8282657
  Stats: 93 lines in 82 files changed: 0 ins; 0 del; 93 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7268.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7268/head:pull/7268

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Matteo Baccan]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Hi

I have pushed this PR about 1 month ago. Only 3 days ago OCA was accepted.
Now: what is the next step?

This is a cleanup PR that removes some double semicolons at the end of some 
lines inside the JDK code.

ciao
matteo

-

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Julian Waters]

On Fri, 25 Feb 2022 15:40:09 GMT, Matteo Baccan  wrote:

>> Hi
>> 
>> I have reviewed the code for removing double semicolons at the end of lines
>> 
>> all the best
>> matteo
>
> Hi
> 
> I have pushed this PR about 1 month ago. Only 3 days ago OCA was accepted.
> Now: what is the next step?
> 
> This is a cleanup PR that removes some double semicolons at the end of some 
> lines inside the JDK code.
> 
> ciao
> matteo

Hi @matteobaccan

The next step would be to create a relevant issue on the tracker and set it to 
track this PR. Since you don't have the ability to create new issues on the JBS 
yet I'll help you create one, please rename your PR title to 8282657 and the 
system should take care of the rest and automatically mark your PR as ready for 
review after setting it to track the corresponding JBS entry.

Cheers,
Julian

-

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Lance Andersen]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

The changes look OK.  The copyright year probably should be updated as part of 
this PR

-

Marked as reviewed by lancea (Reviewer).

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Matteo Baccan]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Hi Lance

I can make a second commit updating the copyright year

Tell me if this is necessary

ciao
matteo

-

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Roger Riggs]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

We usually request that these be be broken up by area to attract the 
appropriate reviewers and avoid eye-strain.  The client modules are usually 
separated out, as are hotspot.
And corresponding tests.
This kind of change is pretty low value for the code base and requires the 
involvement of quite a few reviewers.
If you had ask ahead of time, I would have suggested finding something with 
more value.

-

Changes requested by rriggs (Reviewer).

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Magnus Ihse Bursie]

On Fri, 4 Mar 2022 17:17:12 GMT, Roger Riggs  wrote:

>> Hi
>> 
>> I have reviewed the code for removing double semicolons at the end of lines
>> 
>> all the best
>> matteo
>
> We usually request that these be be broken up by area to attract the 
> appropriate reviewers and avoid eye-strain.  The client modules are usually 
> separated out, as are hotspot.
> And corresponding tests.
> This kind of change is pretty low value for the code base and requires the 
> involvement of quite a few reviewers.
> If you had ask ahead of time, I would have suggested finding something with 
> more value.

@RogerRiggs Otoh, this change is really trivial. I have verified that all 
changes are replacing trailing ";;" in Java code. These are all clearly typos. 
I think it's nice that we try to strive for a high quality, and while you are 
correct this is maybe not the most pressing issue, I think it's nice to get a 
cleanup like this done. 

I'd argue that this is a trivial change. If you are worried, let's get a couple 
of more reviewers. I can't see the need to split this up per area.

-

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Phil Race]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Marked as reviewed by prr (Reviewer).

Looks like  there's only one client source code file touched
Most of the client changes are only in jtreg tests - and this is very trivial, 
so I'm OK with them being included here.

-

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Roger Riggs]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Marked as reviewed by rriggs (Reviewer).

-

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Iris Clark]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Nice tidy of the code.

Is there anything that can be done to prevent re-introduction of this trivial 
problem?  Perhaps a new Skara bot jcheck option similar to what is already in 
place for trailing whitespace?

-

Marked as reviewed by iris (Reviewer).

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Bradford Wetmore]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

LGTM also.  

Similar suggestion for updating copyrights.

-

Marked as reviewed by wetmore (Reviewer).

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Bernd Eckenfels]

Thanks for the clarification Michael,

I was actually wrongly assuming that the client has to propose the method. If 
the server can do that, then it is relative risk free to support additional 
algorithms on the client side (and ignore a challenge if it requests disabled 
algorithm (this would include md5 default for absent attribute)).

Did not know the WWW-Authenticate header does that.

Still I would Seperate the md5 deprecation from the implementation of 
alternative algorithms to make a larger cut-over window (maybe even schedule it 
in the Oracle crypto roadmap)

Gruss
Bernd


--
http://bernd.eckenfels.net

Von: Michael McMahon 
Gesendet: Friday, March 4, 2022 4:07:49 PM
An: Bernd Eckenfels ; net-dev@openjdk.java.net 

Betreff: Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default


Bernd,

If I understand you correctly, there is no negotiation at this level. Once 
Digest is selected, it's up to the server to choose the algorithm and then the 
client can either accept it or else reject it and the whole request fails then.

It's true that with this change, if the server proposes MD5 by default, then 
the request will fail. But, that's the point of the change, and the 
compatibility impact is considered in the related CSR. In that case, either the 
server needs to change or else the system property to re-enable MD5 must be set.

I'd like to try out (for the next review round) the idea of establishing the 
default "insecure" protocols as a fixed security property (including MD5 and 
SHA-1) and then provide a settable system or networking property to override 
that.

Thanks

Michael

On 04/03/2022 14:03, Bernd Eckenfels wrote:

Hello,

While I like the idea of the user having to explicitely specify the rexenabled 
legacy algorithms (as opposed to removing the defaultsdisabled) it is not the 
style the other algorithm policies in JCE work - so it might be confusing.

But, more critically I would separate the enabling/implementing of new 
algorithms from disabling old ones. Especially since there needs to be changes 
on the server side first. (And I wonder if this can be negotiated anyway?).

So why not start with a “provide new DIGEST Mechanisms” change? Having said 
that, would it need to start out with disabled new mechanisms so the update 
won’t change the behavior? (If there is no negotiation?)

Gruss
Bernd
--
http://bernd.eckenfels.net

Von: net-dev 
 im 
Auftrag von Michael McMahon 

Gesendet: Friday, March 4, 2022 1:33:06 PM
An: net-dev@openjdk.java.net 

Betreff: Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

On Fri, 4 Mar 2022 09:37:21 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





I considered that and implemented it that way at the start, but what you would 
end up with then is users running their code with something like: 
-DdisabledAlgNames=""
I find that style leads to a much less explicit "opting in" than by making the 
user explicitly identify the deprecated algorithm by name.




Right - but it would also allow users to opt-in to disable more algorithms by 
listing them in the property



In practical terms, the only other likely candidate there is SHA-1. If that 
weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the default disabled 
algorithms and in net.properties we identify MD5 only for now. Users could add 
to that list if they want or even specify it on the command line. I think it's 
potentially confusing, but maybe there is a case for adding to the disabled 
list. I need to think about a way to do this without subvertng the point about 
making the user explicitly opt in.







I considered that and implemented it that way at the start, but what you would 
end up with then is users running their code with something like: 
-DdisabledAlgNames=""
I find that style leads to a much less explicit "opting in" than by making the 
user explicitly identify the deprecated algorithm by name.




Right - but it would also allow users to opt-in to disable more algorithms by 
listing them in the property



In practical terms, the only other likely candidate there is SHA-1. If that 
weren't the case, I'd disagree with your point.

So, maybe, we could have a 2nd net property with the defa

RFR: JDK-8282686: Add constructors taking a cause to SocketException

[Joe Darcy]

Please review this small API enhancement to add the usual constructors taking a 
cause to SocketException and then update uses of initiCause on creating 
SocketException to instead pass the cause via the constructor.

Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282688

-

Commit messages:
 - JDK-8282686: Add constructors take a cause to SocketException

Changes: https://git.openjdk.java.net/jdk/pull/7705/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7705&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8282686
  Stats: 48 lines in 6 files changed: 23 ins; 12 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7705.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7705/head:pull/7705

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Joe Darcy]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Marked as reviewed by darcy (Reviewer).

-

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

Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v2]

[Bradford Wetmore]

On Thu, 3 Mar 2022 12:36:33 GMT, zzambers  wrote:

>> Fixed API Note in javadoc for javax.net.ssl.SSLSocket class. API Note was 
>> introduced by JDK-8208526 [1]. At that point both Socket.shutdownInput() / 
>> Socket.shutdownOutput() and InputStream.close() / OutputStream.close() 
>> performed half-close of TLS-1.3 connection. However this behaviour has 
>> changed as result of JDK-8216326 [2]. InputStream.close() / 
>> OutputStream.close() no longer perform half-close but full socket close, but 
>> API Note was never updated.
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8208526
>> [2] https://bugs.openjdk.java.net/browse/JDK-8216326
>
> zzambers has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updated copyright for SSLSocket.java

There are more changes needed, and should probably be done as part of this 
issue since we're already in this code anyway.  The current code only talks 
about shutdownInput/shutdownOutput, but makes no mention of the duplex close(), 
which is the other way to shutdown the SSLSocket.  It only needs a few words.

https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029167.html

These two changes will require a CSR to update the @apiNote.

-

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[David Holmes]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

I eyeballed the diff file and all seems okay.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Julian Waters]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

Nice, good work matteo

Should I change the JBS issue title to match the PR title, or is it preferred 
for the PR title to change?

-

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

Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines

[Jan Schlößin]

On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan  wrote:

> Hi
> 
> I have reviewed the code for removing double semicolons at the end of lines
> 
> all the best
> matteo

This PR changes a comment in javax/swing/RepaintManager.java. This commented 
out code should be removed altogether in another PR? Because its an 
System.out.println and because its commented out code.

-

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