Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v6]
On Tue, 15 Mar 2022 16:00:41 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: > > made disabledAlgorithms immutable Suggest changing the bug title to reflect that SHA-1 is now also disabled by default, i.e. "Disable http DIGEST mechanisms with MD5 or SHA-1 by default". - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8282293: Domain value for system property jdk.https.negotiate.cbt should be case-insensitive [v2]
On Thu, 10 Mar 2022 05:59:14 GMT, Sibabrata Sahoo wrote: >> Domain value for system property jdk.https.negotiate.cbt is >> case-insensitive now. Included Test has been updated to address the change. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > Update HttpsCB.java src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java line 338: > 336: return true; > 337: } > 338: String afterWildCard = domain.substring(1); `domain` could be an empty string if the property value is "domain:a,,b". I know it's invalid but at least let's try our best to avoid a runtime exception. In fact, why is this variable necessary? It looks like `regionMatches` allows you to compare ...er... regions. - PR: https://git.openjdk.java.net/jdk/pull/7759
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v6]
On Tue, 15 Mar 2022 16:00:41 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: > > made disabledAlgorithms immutable Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v6]
> 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: made disabledAlgorithms immutable - Changes: - all: https://git.openjdk.java.net/jdk/pull/7688/files - new: https://git.openjdk.java.net/jdk/pull/7688/files/c55fdd94..66c320ce Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7688&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7688&range=04-05 Stats: 7 lines in 2 files changed: 4 ins; 0 del; 3 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: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v4]
> **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. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8263031: Correct ordering of imports - Changes: - all: https://git.openjdk.java.net/jdk/pull/7696/files - new: https://git.openjdk.java.net/jdk/pull/7696/files/c14f599e..c2ae1e56 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=02-03 Stats: 55 lines in 1 file changed: 27 ins; 28 del; 0 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: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Mon, 7 Mar 2022 12:08:49 GMT, Daniel Fuchs wrote: >> Conor Cleary has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8263031: Tidied up import statements > > src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java > line 82: > >> 80: import java.util.function.Function; >> 81: import java.util.function.Supplier; >> 82: > > Could we keep the original alphabetical ordering and have the java.* imports > come before the jdk.* imports? Done, this was just an IDE hiccup - PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v3]
On Tue, 15 Mar 2022 14:54:35 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.
>
> Conor Cleary has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - 8263031: Cleanup of changes in Http2Connection
> - 8263031: Added test for multiple Continuation Frames
src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line
814:
> 812: // TODO: Maybe say what kind of frame was received
> instead
> 813: pushContinuationState = null;
> 814: protocolError(ErrorFrame.PROTOCOL_ERROR, "Expected a
> Continuation frame but received " + frame);
In all other places in this method we have `return;` just after a call to
`protocolError`, except in the two places where your changes added one. For
consistency you should probably add this `return;` statement, even if it's not
strictly needed. It would avoid having to have to analyze the whole structure
of the nested `if - then - else` to figure out that it's actually not needed.
src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line
880:
> 878: private void handlePushContinuation(Stream parent,
> ContinuationFrame cf)
> 879: throws IOException {
> 880: decodeHeaders(cf, pushContinuationState.pushContDecoder);
I suggest declaring a local variable here to avoid reading
pushContinuationState more than once.
Something like:
var pcs = pushContinuationState;
then use `pcs` wherever needed in that method.
test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 75:
> 73: static HttpHeaders testHeaders;
> 74: static HttpHeadersBuilder testHeadersBuilder;
> 75: static int continuationCount;
Since these three static variables are set by one thread and read by another -
they should all be volatile.
-
PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v3]
> **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. Conor Cleary has updated the pull request incrementally with two additional commits since the last revision: - 8263031: Cleanup of changes in Http2Connection - 8263031: Added test for multiple Continuation Frames - Changes: - all: https://git.openjdk.java.net/jdk/pull/7696/files - new: https://git.openjdk.java.net/jdk/pull/7696/files/664a1105..c14f599e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7696&range=01-02 Stats: 117 lines in 2 files changed: 74 ins; 11 del; 32 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: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Mon, 7 Mar 2022 12:31:41 GMT, Daniel Fuchs wrote:
>> Conor Cleary has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8263031: Tidied up import statements
>
> src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java
> line 855:
>
>> 853:
>> 854: private record PushContinuationState(HeaderDecoder pushContDecoder,
>> PushPromiseFrame pushContFrame) {}
>> 855: private volatile PushContinuationState pcs;
>
> I'd prefer to have a longer name than `pcs` - `pushContinuationState` would
> make it less mysterious at the place where it's used. Also it could be good
> to move these declarations (linew 854 and 855) higher in the file, where
> other instance variables are declared.
Changed to pushContinuationState, its a long name but definitely clearer
-
PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Tue, 15 Mar 2022 11:26:55 GMT, Conor Cleary wrote: >> Good idea yes, to check that the repeat continuation still behaves as >> expected. Should hopefully be straight forward to create another test case. > > On this issue, there is a case where a faulty server might send an indefinite > number of Continuations (maybe the server never sets an END_HEADERS flag). > Should a safe guard for the Push Promise with Continuation/s case be put in > place to prevent the faulty scenario? It would be impossible to detect, but if the server is faulty and "forget" to set the END_HEADERS flag, then we will detect that because the next frame we receive won't be the ContinuationFrame we expect. - PR: https://git.openjdk.java.net/jdk/pull/7696
Re: RFR: 8282536: java.net.InetAddress should be a sealed class [v2]
On Mon, 14 Mar 2022 16:45:24 GMT, Aleksei Efimov wrote: >> The following fix seals the `java.net.InetAddress` class and permits only >> two implementations - `java.net.Inet4Address` and `java.net.Inet6Address`. >> >> No issues have been detected by regression and JCK tests. >> >> Links: [JBS](https://bugs.openjdk.java.net/browse/JDK-8282536) >> [CSR](https://bugs.openjdk.java.net/browse/JDK-8282880) > > Aleksei Efimov has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup unnessecary class loader checks, remove readObjectNoData Marked as reviewed by michaelm (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7789
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
Integrated: 8254786: java/net/httpclient/CancelRequestTest.java failing intermittently
On Thu, 10 Mar 2022 16:44:53 GMT, Daniel Fuchs wrote: > Please find enclosed a patch that solves an intermittent issue detected by > the CancelRequestTest.java > > If during an HTTP upgrade from HTTP/1.1 to HTTP/2, the request is cancelled > after the Http2Connection has been created, and the handshake has proceeded, > and the response headers to the upgrade have been received, but before the > HTTP/2 connection is offered to the HTTP/2 connection pool, the underlying > TCP connection might get closed at a time where it won't be noticed > immediately, resulting in putting a "dead" HTTP/2 connection in the pool. The > next request to the same server will then fail with "ClosedChannelException". > > The fix is to check the state of the underlying TCP connection before > offering the HTTP/2 connection to the pool, and when retrieving it from the > pool, and disabling the "connectionAborter" (which is there to abort the > connection in case of connect timeout, or cancellation before connect is > done) before offering the connection to the pool as well. This pull request has now been integrated. Changeset: 710653ce Author:Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/710653ce1856d13161ae1786d7c5f71997536e78 Stats: 62 lines in 4 files changed: 43 ins; 6 del; 13 mod 8254786: java/net/httpclient/CancelRequestTest.java failing intermittently Reviewed-by: jpai, michaelm - PR: https://git.openjdk.java.net/jdk/pull/7776
Re: RFR: 8263031: HttpClient throws Exception if it receives a Push Promise that is too large [v2]
On Mon, 14 Mar 2022 11:19:36 GMT, Conor Cleary wrote:
>> test/jdk/java/net/httpclient/http2/PushPromiseContinuation.java line 176:
>>
>>> 174: ContinuationFrame cf = new ContinuationFrame(streamid,
>>> HeaderFrame.END_HEADERS, encodedHeaders);
>>> 175:
>>> 176: try {
>>
>> It would be good to have a test-case that creates 2 continuation frames too.
>
> Good idea yes, to check that the repeat continuation still behaves as
> expected. Should hopefully be straight forward to create another test case.
On this issue, there is a case where a faulty server might send an indefinite
number of Continuations (maybe the server never sets an END_HEADERS flag).
Should a safe guard for the Push Promise with Continuation/s case be put in
place to prevent the faulty scenario?
-
PR: https://git.openjdk.java.net/jdk/pull/7696
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: 8254786: java/net/httpclient/CancelRequestTest.java failing intermittently [v3]
On Mon, 14 Mar 2022 13:21:21 GMT, Daniel Fuchs wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/Http2ClientImpl.java
>> line 155:
>>
>>> 153: boolean offerConnection(Http2Connection c) {
>>> 154: if (debug.on()) debug.log("offering to the connection pool:
>>> %s", c);
>>> 155: if (!c.isOpen() || c.finalStream()) {
>>
>> Is this check for isOpen() not redundant given the same check added inside
>> the synchronized block?
>
> Possibly - but it allows to break out fast without having to enter the
> synchronized block.
Okay, that's fine.
-
PR: https://git.openjdk.java.net/jdk/pull/7776
Re: RFR: 8254786: java/net/httpclient/CancelRequestTest.java failing intermittently [v3]
On Thu, 10 Mar 2022 17:05:32 GMT, Daniel Fuchs wrote: >> Please find enclosed a patch that solves an intermittent issue detected by >> the CancelRequestTest.java >> >> If during an HTTP upgrade from HTTP/1.1 to HTTP/2, the request is cancelled >> after the Http2Connection has been created, and the handshake has proceeded, >> and the response headers to the upgrade have been received, but before the >> HTTP/2 connection is offered to the HTTP/2 connection pool, the underlying >> TCP connection might get closed at a time where it won't be noticed >> immediately, resulting in putting a "dead" HTTP/2 connection in the pool. >> The next request to the same server will then fail with >> "ClosedChannelException". >> >> The fix is to check the state of the underlying TCP connection before >> offering the HTTP/2 connection to the pool, and when retrieving it from the >> pool, and disabling the "connectionAborter" (which is there to abort the >> connection in case of connect timeout, or cancellation before connect is >> done) before offering the connection to the pool as well. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Copyright years Marked as reviewed by michaelm (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7776
