Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v20]
On Sun, 15 May 2022 06:43:27 GMT, Jaikiran Pai wrote: >> This change proposes to implement the enhancement noted in >> https://bugs.openjdk.java.net/browse/JDK-8209137. >> >> The change introduces a new API to allow applications to build a >> `java.net.http.HTTPClient` configured with a specific local address that >> will be used while creating `Socket`(s) for connections. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use the byte[] address form for testing the client address instead of > relying on hostname which > doesn't always return localhost for 127.0.0.1 on Windows Marked as reviewed by michaelm (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8286194: ExecutorShutdown test fails intermittently [v3]
On Sat, 7 May 2022 11:46:37 GMT, Daniel Fuchs wrote: >> Hi, please find here a patch that solves a rare intermittent test failure >> observed in the test `java/net/httpclient/ExecutorShutdown.java` >> >> A race condition coupled with some too eager synchronization was causing a >> deadlock between an Http2Connection close, a thread trying to shutdown the >> HttpClient due to a RejectedTaskException, and the SelectorManager thread >> trying to exit. >> >> The fix comprises several cleanup - in particular: >> >> - `Http2Connection`: no need to try to send a `GOAWAY` frame if the >> underlying TCP connection is already closed >> - `SSLFlowDelegate`/`SubscriberWrapper`: no need to trigger code that will >> request more data from upstream if the sequential scheduler that is supposed >> to handle that data once it arrives is already closed >> - `Http1Exchange`/`Http1Request`: proper cancellation of subscription if an >> exception is raised before `onSubscribe()` has been called >> - `HttpClientImpl`: avoid calling callbacks from within synchronized blocks >> when not necessary >> - `ReferenceTracker`: better thread dumps in case where the selector is >> still alive at the end of the test (remove the limit that limited the stack >> traces to 8 element max by no longer relying on `ThreadInfo::toString`) > > Daniel Fuchs 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 three additional > commits since the last revision: > > - Merge branch 'master' into ExecutorShutdown-intermittent-8286194 > - Added a comment to ReferenceTracker.java as suggested in the review > - 8286194: ExecutorShutdown test fails intermittently Change looks fine to me. We can revisit the ThreadInfo code later. - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 8286194: ExecutorShutdown test fails intermittently [v3]
On Sat, 7 May 2022 11:46:37 GMT, Daniel Fuchs wrote: >> Hi, please find here a patch that solves a rare intermittent test failure >> observed in the test `java/net/httpclient/ExecutorShutdown.java` >> >> A race condition coupled with some too eager synchronization was causing a >> deadlock between an Http2Connection close, a thread trying to shutdown the >> HttpClient due to a RejectedTaskException, and the SelectorManager thread >> trying to exit. >> >> The fix comprises several cleanup - in particular: >> >> - `Http2Connection`: no need to try to send a `GOAWAY` frame if the >> underlying TCP connection is already closed >> - `SSLFlowDelegate`/`SubscriberWrapper`: no need to trigger code that will >> request more data from upstream if the sequential scheduler that is supposed >> to handle that data once it arrives is already closed >> - `Http1Exchange`/`Http1Request`: proper cancellation of subscription if an >> exception is raised before `onSubscribe()` has been called >> - `HttpClientImpl`: avoid calling callbacks from within synchronized blocks >> when not necessary >> - `ReferenceTracker`: better thread dumps in case where the selector is >> still alive at the end of the test (remove the limit that limited the stack >> traces to 8 element max by no longer relying on `ThreadInfo::toString`) > > Daniel Fuchs 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 three additional > commits since the last revision: > > - Merge branch 'master' into ExecutorShutdown-intermittent-8286194 > - Added a comment to ReferenceTracker.java as suggested in the review > - 8286194: ExecutorShutdown test fails intermittently Good to see the dead code removed from HttpClientImpl (cancel). Question about the code copied in from ThreadInfo. Is there any way we could request a change to that class to adjust the number of stack frames printed? Okay, I see the same issue has been raised already. I agree we should request that enhancement. - PR: https://git.openjdk.java.net/jdk/pull/8562
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v6]
On Thu, 12 May 2022 10:51:04 GMT, Daniel Fuchs wrote: >> In relation to >> [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find >> here a patch that addresses possibly lossy conversions in java.net.http > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > lengthOfCode() returns int; Removed excessive comments Marked as reviewed by michaelm (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http [v5]
On Wed, 11 May 2022 17:10:45 GMT, Daniel Fuchs wrote: >> src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java >> line 739: >> >>> 737: buffer |= (codeValueOf(c) >>> bufferLen); // >>> append >>> 738: bufferLen += (int) len; >>> 739: pos++; >> >> Would it be cleaner to do the cast in the codeLengthOf method, so it returns >> an int? > > I believe the method returns an "unsigned int" - having the method return an > int would then potentially cause `bufferLen + len <= 64` to yield true when > it shouldn't. Hopefully @pavelrappo will comment. codeLengthOf() returns long. It could be changed to return int with a cast internally and then you could avoid the two new casts. - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8286386: Address possibly lossy conversions in java.net.http
On Wed, 11 May 2022 15:42:25 GMT, Daniel Fuchs wrote: > In relation to > [JDK-8244681](https://bugs.openjdk.java.net/browse/JDK-8244681), please find > here a patch that addresses possibly lossy conversions in java.net.http src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java line 739: > 737: buffer |= (codeValueOf(c) >>> bufferLen); // > append > 738: bufferLen += (int) len; > 739: pos++; Would it be cleaner to do the cast in the codeLengthOf method, so it returns an int? src/java.net.http/share/classes/jdk/internal/net/http/websocket/Frame.java line 222: > 220: // compiler will emit a warning if not cast > 221: firstChar &= (char) ~0b1000_; > 222: } Am I right in believing that there was an implicit cast to char here before and the only change is to make it explicit? ie. there is no actual behavior change? - PR: https://git.openjdk.java.net/jdk/pull/8656
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
On Wed, 11 May 2022 08:54:48 GMT, Jaikiran Pai wrote:
>> src/java.net.http/share/classes/java/net/http/HttpClient.java line 378:
>>
>>> 376: *
>>> 377: * @implSpec The default implementation of this method throws
>>> 378: * {@code UnsupportedOperationException}. {@code Builder}s
>>> obtained
>>
>> I think the implSpec here is correct, but will be confusing for most users.
>> I'm not sure what value it adds. Do we really need it?
>
> Hello Michael,
> Most users will be using the `HttpClient.newBuilder()` to create the builder,
> so this note about `UnsupportedOperationException` can be confusing. However,
> for implementations (libraries?) which provide their own implementation of
> the `java.net.http.HttpClient.Builder`, I think, this note would be relevant.
> This approach is similar to what we already have on
> `java.net.http.HttpClient.newWebSocketBuilder()` method.
Sure, I just think when most developers read "The default implementation of
this method throws UOE" they will think they can't use it without implementing
something themselves. Library developers will figure it out anyway.
-
PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8209137: Add ability to bind to specific local address to HTTP client [v19]
On Tue, 10 May 2022 13:51:37 GMT, Jaikiran Pai wrote:
>> This change proposes to implement the enhancement noted in
>> https://bugs.openjdk.java.net/browse/JDK-8209137.
>>
>> The change introduces a new API to allow applications to build a
>> `java.net.http.HTTPClient` configured with a specific local address that
>> will be used while creating `Socket`(s) for connections.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fix javadoc link in test
src/java.net.http/share/classes/java/net/http/HttpClient.java line 378:
> 376: *
> 377: * @implSpec The default implementation of this method throws
> 378: * {@code UnsupportedOperationException}. {@code Builder}s
> obtained
I think the implSpec here is correct, but will be confusing for most users. I'm
not sure what value it adds. Do we really need it?
-
PR: https://git.openjdk.java.net/jdk/pull/6690
Re: RFR: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources [v3]
On Mon, 9 May 2022 12:37:59 GMT, Daniel Fuchs wrote: >> Hi, >> >> Please find here a simple test fix that re-architecture ShortResponseBody >> for better resource usage. >> The test is split to test GET and POST separately and avoids testing GET >> twice. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed @build LGTM - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8569
Re: RFR: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources [v2]
On Sat, 7 May 2022 11:46:32 GMT, Daniel Fuchs wrote: >> Hi, >> >> Please find here a simple test fix that re-architecture ShortResponseBody >> for better resource usage. >> The test is split to test GET and POST separately and avoids testing GET >> twice. > > Daniel Fuchs 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 two additional > commits since the last revision: > > - Merge branch 'master' into ShortResponseBody > - 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use > less resources test/jdk/java/net/httpclient/ShortResponseBodyPost.java line 31: > 29: * @library /test/lib > 30: * @build jdk.test.lib.net.SimpleSSLContext ShortResponseBody > ShortResponseBodyGet > 31: * @run testng/othervm Seems like the build directive should refer to ShortResponseBodyPost instead of ShortResponseBodyGet - PR: https://git.openjdk.java.net/jdk/pull/8569
Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v6]
On Sat, 30 Apr 2022 09:55:16 GMT, Michael Felt wrote:
>> with IP "0.0.0.0"
>>
>> - it either does nothing and ping fails, or, in some virtual environments
>> is treated as the default route address.
>> - IPv6 support for ::1 is available since 1977; however, ::0 is not accepted
>> as a vaild psuedo IPv6 address. '::1' must be used instead.
>>
>> ping: bind: The socket name is not available on this system.
>> ping: bind: The socket name is not available on this system.
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.037 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.045 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>> round-trip min/avg/max = 0/0/0 ms
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.052 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.047 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>>
>>
>> A long commit message.
>>
>> This came to light because some systems failed with IPv4 (those that passed
>> replaced 0.0.0.0 with the default router. but most just fail - not
>> substituting
>> 0.0.0.0 with 127.0.0.1. However, InetAddress.getByName("") returns 127.0.0.1
>> which compares well with other platform's behavior.
>
> Michael Felt has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use @requires jtreg tag rather than check in code
Changes requested by michaelm (Reviewer).
test/jdk/java/net/Inet4Address/PingThis.java line 32:
> 30: * @requires os.family != "windows" && os.family != "aix"
> 31: * @library /test/lib
> 32: * @summary InetAddress.isReachable is returning false
'&&' double ampersand syntax does not work here. Should be a single ampersand
as suggested by Aleksei.
-
PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: JDK-8280498: jdk/java/net/Inet4Address/PingThis.java fails on AIX [v4]
On Wed, 27 Apr 2022 15:27:45 GMT, Michael Felt wrote:
>> with IP "0.0.0.0"
>>
>> - it either does nothing and ping fails, or, in some virtual environments
>> is treated as the default route address.
>> - IPv6 support for ::1 is available since 1977; however, ::0 is not accepted
>> as a vaild psuedo IPv6 address. '::1' must be used instead.
>>
>> ping: bind: The socket name is not available on this system.
>> ping: bind: The socket name is not available on this system.
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.037 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.045 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>> round-trip min/avg/max = 0/0/0 ms
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.052 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.047 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>>
>>
>> A long commit message.
>>
>> This came to light because some systems failed with IPv4 (those that passed
>> replaced 0.0.0.0 with the default router. but most just fail - not
>> substituting
>> 0.0.0.0 with 127.0.0.1. However, InetAddress.getByName("") returns 127.0.0.1
>> which compares well with other platform's behavior.
>
> Michael Felt has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adjusted and moved comments per review
Changes requested by michaelm (Reviewer).
test/jdk/java/net/Inet4Address/PingThis.java line 49:
> 47: public static void main(String args[]) throws Exception {
> 48: osname = System.getProperty("os.name")
> 49: /*
Doesn't look like the line above will compile.
-
PR: https://git.openjdk.java.net/jdk/pull/7013
Integrated: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing
On Wed, 27 Apr 2022 09:43:52 GMT, Michael McMahon wrote: > Hi, > > Can I get the following fix reviewed please? JDK-8284890 was pushed yesterday > and is causing failures on older versions of macOS that do not support the > option. The fix here is to check at initialization time whether it is > supported before adding it to the list of supported options. The error causes > the new test and two existing ones to fail. I will remove the two tests from > the problem list separately. > > Thanks, > Michael. This pull request has now been integrated. Changeset: ef27081f Author:Michael McMahon URL: https://git.openjdk.java.net/jdk/commit/ef27081fe7e00c8ec8e21d3ee31d7194b5339da2 Stats: 96 lines in 7 files changed: 91 ins; 3 del; 2 mod 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing [v2]
On Wed, 27 Apr 2022 14:12:40 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> updated test
>
> test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 44:
>
>> 42:
>> 43: public static void main(String[] args) throws IOException {
>> 44: isMacos = System.getProperty("os.name").equals("Mac OS X");
>
> I believe there's a test library class that does that. I never remember what
> the os.name is supposed to look like.
Okay. It's `Platform.isOSX()`
-
PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing [v3]
> Hi, > > Can I get the following fix reviewed please? JDK-8284890 was pushed yesterday > and is causing failures on older versions of macOS that do not support the > option. The fix here is to check at initialization time whether it is > supported before adding it to the list of supported options. The error causes > the new test and two existing ones to fail. I will remove the two tests from > the problem list separately. > > Thanks, > Michael. Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: test update - Changes: - all: https://git.openjdk.java.net/jdk/pull/8419/files - new: https://git.openjdk.java.net/jdk/pull/8419/files/bb5c4db5..c83a9daa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8419=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8419=01-02 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8419/head:pull/8419 PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing [v2]
On Wed, 27 Apr 2022 14:10:55 GMT, Daniel Fuchs wrote: >> Michael McMahon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> updated test > > src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c line 209: > >> 207: return JNI_FALSE; >> 208: } >> 209: fd = socket(AF_INET6, SOCK_DGRAM, 0); > > So if IPv6 is not supported on the machine, won't that result on reporting > that IP don't fragment is unsupported? Same question for line 201, but for > IPv6 only machines? I'm not sure you can disable IPv6 completely on mac OS. You can disable it on a per-interface basis. Even then it leaves a link local address available and that code succeeds. I was originally concerned that running with -Djava.net.preferIPv4Stack=true might cause problems, but even in that case, the check succeeds because it ignores that settting. - PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing [v2]
> Hi, > > Can I get the following fix reviewed please? JDK-8284890 was pushed yesterday > and is causing failures on older versions of macOS that do not support the > option. The fix here is to check at initialization time whether it is > supported before adding it to the list of supported options. The error causes > the new test and two existing ones to fail. I will remove the two tests from > the problem list separately. > > Thanks, > Michael. Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: updated test - Changes: - all: https://git.openjdk.java.net/jdk/pull/8419/files - new: https://git.openjdk.java.net/jdk/pull/8419/files/a0fd8e8c..bb5c4db5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8419=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8419=00-01 Stats: 28 lines in 1 file changed: 25 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8419/head:pull/8419 PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing
On Wed, 27 Apr 2022 11:02:08 GMT, Daniel Fuchs wrote:
>> Hi,
>>
>> Can I get the following fix reviewed please? JDK-8284890 was pushed
>> yesterday and is causing failures on older versions of macOS that do not
>> support the option. The fix here is to check at initialization time whether
>> it is supported before adding it to the list of supported options. The error
>> causes the new test and two existing ones to fail. I will remove the two
>> tests from the problem list separately.
>>
>> Thanks,
>> Michael.
>
> test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 62:
>
>> 60:
>> 61: if (!c1.supportedOptions().contains(IP_DONTFRAGMENT)) {
>> 62: return;
>
> Should we assert that this should only happens on mac here? Same remark for
> the two other places below...
I think that is reasonable.
-
PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing
On Wed, 27 Apr 2022 09:43:52 GMT, Michael McMahon wrote: > Hi, > > Can I get the following fix reviewed please? JDK-8284890 was pushed yesterday > and is causing failures on older versions of macOS that do not support the > option. The fix here is to check at initialization time whether it is > supported before adding it to the list of supported options. The error causes > the new test and two existing ones to fail. I will remove the two tests from > the problem list separately. > > Thanks, > Michael. Just spotted an issue with the native code check, if IPv6 is disabled, it will fail. The implementation should be able to work in that scenario. - PR: https://git.openjdk.java.net/jdk/pull/8419
Re: RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing
On Wed, 27 Apr 2022 09:43:52 GMT, Michael McMahon wrote: > Hi, > > Can I get the following fix reviewed please? JDK-8284890 was pushed yesterday > and is causing failures on older versions of macOS that do not support the > option. The fix here is to check at initialization time whether it is > supported before adding it to the list of supported options. The error causes > the new test and two existing ones to fail. I will remove the two tests from > the problem list separately. > > Thanks, > Michael. So, I had to remove the two problem listed tests, to start the review. - PR: https://git.openjdk.java.net/jdk/pull/8419
RFR: 8285671: java/nio/channels/etc/PrintSupportedOptions.java and java/nio/channels/DatagramChannel/AfterDisconnect.java are failing
Hi, Can I get the following fix reviewed please? JDK-8284890 was pushed yesterday and is causing failures on older versions of macOS that do not support the option. The fix here is to check at initialization time whether it is supported before adding it to the list of supported options. The error causes the new test and two existing ones to fail. I will remove the two tests from the problem list separately. Thanks, Michael. - Commit messages: - Seem to have to remove problem listed tests - Merge branch 'master' into mtu_macos - initial fix Changes: https://git.openjdk.java.net/jdk/pull/8419/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8419=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285671 Stats: 67 lines in 7 files changed: 63 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8419/head:pull/8419 PR: https://git.openjdk.java.net/jdk/pull/8419
Withdrawn: 8285646: Add test that checks IP_DONTFRAGMENT is a supported option
On Tue, 26 Apr 2022 16:41:08 GMT, Michael McMahon wrote: > Hi, > > Could I get the following small test update reviewed please? > It includes a test scenario that should have been included with JDK-8284890 > > Thanks, > Michael This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/8405
Re: RFR: 8285646: Add test that checks IP_DONTFRAGMENT is a supported option
On Tue, 26 Apr 2022 16:41:08 GMT, Michael McMahon wrote: > Hi, > > Could I get the following small test update reviewed please? > It includes a test scenario that should have been included with JDK-8284890 > > Thanks, > Michael I'm closing this PR as the related bugfix has caused some regressions and this change will be included with the fix for the regression. - PR: https://git.openjdk.java.net/jdk/pull/8405
RFR: 8285646: Add test that checks IP_DONTFRAGMENT is a supported option
Hi, Could I get the following small test update reviewed please? It includes a test scenario that should have been included with JDK-8284890 Thanks, Michael - Commit messages: - test update Changes: https://git.openjdk.java.net/jdk/pull/8405/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8405=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285646 Stats: 9 lines in 1 file changed: 9 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8405.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8405/head:pull/8405 PR: https://git.openjdk.java.net/jdk/pull/8405
Integrated: 8284890: Support for Do not fragment IP socket options
On Thu, 14 Apr 2022 16:04:22 GMT, Michael McMahon wrote: > Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael This pull request has now been integrated. Changeset: 67755edd Author:Michael McMahon URL: https://git.openjdk.java.net/jdk/commit/67755edd6ff2e2eeafafec207d0459bca53f882b Stats: 498 lines in 11 files changed: 495 ins; 0 del; 3 mod 8284890: Support for Do not fragment IP socket options Reviewed-by: erikj, ihse, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v8]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: test update - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/428a9801..e6b12eb8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=06-07 Stats: 20 lines in 1 file changed: 2 ins; 17 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v9]
On Sat, 16 Apr 2022 11:06:21 GMT, Daniel Fuchs wrote: >> These changes make sure that pending requests are terminated if the selector >> manager thread exits due to exceptions. >> This includes: >>1. completing CompletableFutures that were returned to the caller code >>2. cancelling requests that are in flight >>3. calling onError on BodySubscribers that may not have been completed >> Note that step 3 is necessary as certain CompletableFutures, such as those >> returned by `BodySubscribers.ofInputStream`, complete immediately, the >> operation being eventually completed when the last bite of the response is >> read. Completing a completable future that is already completed has no >> effect, this case is handled by completing the BodySubscriber too. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed accepted exception logging in tests So, I assume the main point of the change is to "remember" the original cause of the selector manager to close and always throw that exception cause going forward. Makes sense, and there is some useful refactoring in the tests. LGTM - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7196
RFR: release note for HTTPS Channel binding feature
Hi, https://bugs.openjdk.java.net/browse/JDK-8285240 contains the release note for the TLS Channel binding enhancement in JDK 19 added in: https://bugs.openjdk.java.net/browse/JDK-8279842 (PR: https://github.com/openjdk/jdk/pull/7065) The release note uses the explanatory text from the CSR and links to the net-properties file in the docs for the details. I suspect the same RN can be used for the backports also. Any comments on this release note? Please reply to this. Thanks Michael.
Re: RFR: 8284890: Support for Do not fragment IP socket options [v7]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: test update - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/1e08ee9a..428a9801 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=05-06 Stats: 18 lines in 1 file changed: 18 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v5]
On Tue, 19 Apr 2022 16:07:29 GMT, Daniel Fuchs wrote: >> I'm not sure there is value in testing all of these permutations. >> Distinguishing DatagramChannel and DatagramSocket probably made sense, but >> it's all the same implementation under the hood. > > 1. `DatagramChannel.open()` => opens a dual socket unless > `-Djava.net.preferIPv4Stack=true`, in which case it should be equivalent to > `DatagramChannel.open(StandardProtocolFamily.INET)` > 2. `DatagramChannel.open(StandardProtocolFamily.INET)` => opens an IPv4 socket > 3. `DatagramChannel.open(StandardProtocolFamily.INET6)` => opens an IPv6 > socket > > So I believe it makes sense to test the no-arg constructor since that's the > only way to open a dual socket. I don't mind adding it. Though, the no-arg constructor is the same as cases 2. or 3. depending on the value of the preferIPv4Stack property. - PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v6]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: typo in windows native code - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/509c3f81..1e08ee9a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=04-05 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v5]
On Tue, 19 Apr 2022 14:50:57 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fix whitespace
>
> src/jdk.net/windows/native/libextnet/WindowsSocketOptions.c line 112:
>
>> 110: return optval;
>> 111: }
>> 112: handleError(env, rv, "get option IP_DONTFRAGMENT failed");
>
> Is there some indentation issue here?
Yes, there is.
> test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 44:
>
>> 42: StandardProtocolFamily fam = args[0].equals("ipv4") ? INET :
>> INET6;
>> 43: System.out.println("Family = " + fam);
>> 44: testDatagramChannel(args, fam);
>
> Shouldn't there be a testcase for when DatagramChannel is opened using the no
> arg factory method `DatagramChannel.open()`?
I'm not sure there is value in testing all of these permutations.
Distinguishing DatagramChannel and DatagramSocket probably made sense, but it's
all the same implementation under the hood.
-
PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v5]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: fix whitespace - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/5f1d87eb..509c3f81 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v4]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: updates - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/e90aa7c3..5f1d87eb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=02-03 Stats: 42 lines in 2 files changed: 13 ins; 10 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v3]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon 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 25 additional commits since the last revision: - windows update - test update - Merge branch 'master' into mtu - builds in github action now - fix whitespace errors - minor spec update - windows 2016 issue - windows issue - windows update - test update - ... and 15 more: https://git.openjdk.java.net/jdk/compare/3b3bec01...e90aa7c3 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/14c776b1..e90aa7c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=01-02 Stats: 148852 lines in 1314 files changed: 108219 ins; 7158 del; 33475 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v2]
On Fri, 15 Apr 2022 10:04:48 GMT, Daniel Jeliński wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> builds in github action now
>
> src/jdk.net/windows/native/libextnet/WindowsSocketOptions.c line 73:
>
>> 71: if (family == AF_INET) {
>> 72: opt = optval;
>> 73: rv = setsockopt(fd, IPPROTO_IP, IP_DONTFRAGMENT, (char *),
>> sizeof(int));
>
> Why do we only use `IPV6_MTU_DISCOVER` but not `IP_MTU_DISCOVER`? As far as I
> can tell, `IP_DONTFRAGMENT` alone doesn't guarantee that the DF bit will be
> set.
I did (manually) check that the DF bit is set, though unfortunately, there's no
straightforward way to test that in the regression test. We could have the same
construction for AF_INET as AF_INET6 and try IP_MTU_DISCOVER first which won't
work pre Windows 10/2019. So, I'll make that change.
-
PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v2]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: builds in github action now - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/446dd6cf..14c776b1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=00-01 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options
On Fri, 15 Apr 2022 09:19:48 GMT, Daniel Fuchs wrote:
>> Hi,
>>
>> Could I get the following PR review please? It adds a new JDK specific
>> extended socket option
>> called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4
>> and IPv6
>> UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF
>> (Dont Fragment) bit
>> in the IP header. There is no equivalent in the IPv6 packet header as
>> fragmentation is implemented
>> exclusively by the sending and receiving nodes.
>>
>> Thanks,
>> Michael
>
> test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 40:
>
>> 38:
>> 39: public class DontFragmentTest {
>> 40:
>
> Should we have a similar test for DatagramSocket / MulticastSocket /
> DatagramChannel.socket() ?
Good idea
-
PR: https://git.openjdk.java.net/jdk/pull/8245
RFR: 8284890: Support for Do not fragment IP socket options
Hi, Could I get the following PR review please? It adds a new JDK specific extended socket option called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 and IPv6 UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF (Dont Fragment) bit in the IP header. There is no equivalent in the IPv6 packet header as fragmentation is implemented exclusively by the sending and receiving nodes. Thanks, Michael - Commit messages: - fix whitespace errors - minor spec update - windows 2016 issue - windows issue - windows update - test update - updates - simplified test. Loosened spec - Merge branch 'master' into mtu - fixed test - ... and 11 more: https://git.openjdk.java.net/jdk/compare/40ddb755...446dd6cf Changes: https://git.openjdk.java.net/jdk/pull/8245/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8245=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284890 Stats: 437 lines in 11 files changed: 434 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284892: java/net/httpclient/http2/TLSConnection.java fails intermittently [v2]
On Thu, 14 Apr 2022 18:45:10 GMT, Daniel Fuchs wrote: >> java/net/httpclient/http2/TLSConnection.java has been observed failing (even >> though rarely) in test jobs. >> >> The issue is that the handler used on the the server sides maintains a >> volatile `sslSession` field which it sets when receiving a request, so that >> the client can check which SSLParameters were negotiated after receiving the >> response. >> However that field is set a little too late: after writing the request body >> bytes. This means that sometimes the client is able to receive the last byte >> before the server has updated the field, and can observe the previous value, >> which was set by the previous request. Checking of SSL parameters on the >> client side then usually fails, as it is looking at the wrong session. >> >> The proposed fix is very simple: just update the `sslSession` field a little >> earlier - before writing the response. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Copyright update LGTM - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8249
Re: RFR: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task [v3]
On Thu, 10 Mar 2022 11:17:26 GMT, Daniel Fuchs wrote: >> These changes make sure that pending requests are terminated if the selector >> manager thread exits due to exceptions. >> This includes: >>1. completing CompletableFutures that were returned to the caller code >>2. cancelling requests that are in flight >>3. calling onError on BodySubscribers that may not have been completed >> Note that step 3 is necessary as certain CompletableFutures, such as those >> returned by `BodySubscribers.ofInputStream`, complete immediately, the >> operation being eventually completed when the last bite of the response is >> read. Completing a completable future that is already completed has no >> effect, this case is handled by completing the BodySubscriber too. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > asserts that acquired == true Thorny problem. Nice work. - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7196
Re: RFR: 8283544: HttpClient GET method adds Content-Length: 0 header [v3]
On Tue, 29 Mar 2022 19:43:50 GMT, Daniel Fuchs wrote:
>> src/java.net.http/share/classes/jdk/internal/net/http/Http1Request.java line
>> 302:
>>
>>> 300:
>>> 301: // GET with no body should not set the Content-Length header
>>> 302: if (requestPublisher != null ||
>>> !"GET".equals(request.method())) {
>>
>> Can we remove the check for "GET"? This way we will let the users decide if
>> they want to send content-length or not, regardless of the chosen request
>> method.
>
> Practically that would mean not sending Content-Length: 0 by default for GET,
> DELETE, and HEAD. All other requests methods would have either a
> Content-Length or Transfer-Encoding. I suspect that HEAD should probably be
> handled the same way than GET. But should DELETE not have a body? I know that
> some servers will balk if DELETE has a body. But would they expect
> Content-Length: 0 not to be included? @Michael-Mc-Mahon what do you think?
RFC 7230 section 3.3.2 says:
"A user agent SHOULD NOT send a
Content-Length header field when the request message does not contain
a payload body and the method semantics do not anticipate such a
body."
I don't think the DELETE method anticipates a a request body. So, it shouldn't
have a Content-Length header either IMO. Same for HEAD also.
-
PR: https://git.openjdk.java.net/jdk/pull/8017
Integrated: 8281561: Disable http DIGEST mechanism with MD5 and SHA-1 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 This pull request has now been integrated. Changeset: 7f2a3ca2 Author:Michael McMahon URL: https://git.openjdk.java.net/jdk/commit/7f2a3ca289ae14bec1af24d0a51e98ba697ce9c1 Stats: 585 lines in 14 files changed: 490 ins; 14 del; 81 mod 8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default Reviewed-by: weijun, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default [v9]
> 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 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 25 additional commits since the last revision: - merge branch 'master' into md5 - Merge branch 'master' into md5 - forgot update to DigestAuth test - latest update - Merge branch 'master' into md5 - delete .swp file - incomplete test update - made disabledAlgorithms immutable - Merge branch 'master' into md5 - update after third review round - ... and 15 more: https://git.openjdk.java.net/jdk/compare/a77604cd...946142f3 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7688/files - new: https://git.openjdk.java.net/jdk/pull/7688/files/b2095e02..946142f3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7688=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7688=07-08 Stats: 92145 lines in 1153 files changed: 75778 ins; 14325 del; 2042 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: 8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default [v8]
On Mon, 28 Mar 2022 09:29:58 GMT, Daniel Fuchs wrote: >> No, the digest field refers to the actual message digest algorithm (as known >> to the security libraries). The algorithm field holds the algorithm name as >> it is defined in RFC7616. > > I am confused here - because you converted `algorithm` to upper case, so it > should never end with `-sess`? Look at line 478: The `algorithm` field is reset here to be the upper case of the digest name plus the -sess suffix in lower case. - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default [v8]
On Fri, 25 Mar 2022 17:21:11 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> forgot update to DigestAuth test
>
> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
> line 524:
>
>> 522: }
>> 523:
>> 524: boolean session = algorithm.endsWith ("-sess");
>
> should that be `digest.endsWith("-sess");` ?
No, the digest field refers to the actual message digest algorithm (as known to
the security libraries). The algorithm field holds the algorithm name as it is
defined in RFC71616.
-
PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default [v8]
On Fri, 25 Mar 2022 15:07:40 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: > > forgot update to DigestAuth test webrevs 06 & 07 have the latest changes, if anyone is just looking at the incremental changes - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default [v8]
> 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: forgot update to DigestAuth test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7688/files - new: https://git.openjdk.java.net/jdk/pull/7688/files/2ef5636d..b2095e02 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7688=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7688=06-07 Stats: 25 lines in 1 file changed: 14 ins; 11 del; 0 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: 8281561: Disable http DIGEST mechanism with MD5 and SHA-1 by default [v7]
> 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 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 22 additional commits since the last revision: - latest update - Merge branch 'master' into md5 - delete .swp file - incomplete test update - made disabledAlgorithms immutable - Merge branch 'master' into md5 - update after third review round - removed swp file - update after second review round - update - ... and 12 more: https://git.openjdk.java.net/jdk/compare/7047a23e...2ef5636d - Changes: - all: https://git.openjdk.java.net/jdk/pull/7688/files - new: https://git.openjdk.java.net/jdk/pull/7688/files/66c320ce..2ef5636d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7688=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7688=05-06 Stats: 7185 lines in 421 files changed: 3921 ins; 1034 del; 2230 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: 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=7688=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7688=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: 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 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
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v5]
> 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 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 17 additional commits since the last revision: - Merge branch 'master' into md5 - update after third review round - removed swp file - update after second review round - update - update after first review round - fix whitespace - update property name. add documentation - fixed one more test - fixed up existing tests using digest auth - ... and 7 more: https://git.openjdk.java.net/jdk/compare/13dc4330...c55fdd94 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7688/files - new: https://git.openjdk.java.net/jdk/pull/7688/files/65d549ff..c55fdd94 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7688=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7688=03-04 Stats: 15463 lines in 460 files changed: 9745 ins; 3242 del; 2476 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: 8281561: Disable http DIGEST mechanism with MD5 by default [v4]
> Hi, > > Could I get the following change reviewed please, which is to disable the MD5 > message digest algorithm by default in the HTTP Digest authentication > mechanism? The algorithm can be opted into by setting a new system property > "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also > updates the Digest authentication implementation to use some of the more > secure features defined in RFC7616, such as username hashing and additional > digest algorithms like SHA256 and SHA512-256. > > - Michael Michael McMahon has updated the pull request incrementally with two additional commits since the last revision: - update after third review round - removed swp file - Changes: - all: https://git.openjdk.java.net/jdk/pull/7688/files - new: https://git.openjdk.java.net/jdk/pull/7688/files/3d5ef143..65d549ff Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7688=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7688=02-03 Stats: 10 lines in 3 files changed: 2 ins; 6 del; 2 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: 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
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?
-
PR: https://git.openjdk.java.net/jdk/pull/7776
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v3]
On Fri, 11 Mar 2022 17:37:44 GMT, Michael McMahon wrote: >> Hi, >> >> Could I get the following change reviewed please, which is to disable the >> MD5 message digest algorithm by default in the HTTP Digest authentication >> mechanism? The algorithm can be opted into by setting a new system property >> "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also >> updates the Digest authentication implementation to use some of the more >> secure features defined in RFC7616, such as username hashing and additional >> digest algorithms like SHA256 and SHA512-256. >> >> - Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > update after second review round I still plan to add a couple more tests for UTF-8 encoding and if possible for username hashing - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v3]
> Hi, > > Could I get the following change reviewed please, which is to disable the MD5 > message digest algorithm by default in the HTTP Digest authentication > mechanism? The algorithm can be opted into by setting a new system property > "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also > updates the Digest authentication implementation to use some of the more > secure features defined in RFC7616, such as username hashing and additional > digest algorithms like SHA256 and SHA512-256. > > - Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: update after second review round - Changes: - all: https://git.openjdk.java.net/jdk/pull/7688/files - new: https://git.openjdk.java.net/jdk/pull/7688/files/b765d412..3d5ef143 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7688=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7688=01-02 Stats: 160 lines in 12 files changed: 59 ins; 25 del; 76 mod Patch: https://git.openjdk.java.net/jdk/pull/7688.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7688/head:pull/7688 PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
On Mon, 7 Mar 2022 14:41:47 GMT, Weijun Wang wrote:
>> 2nd test of https://datatracker.ietf.org/doc/html/rfc7616#section-3.9 is on
>> this algorithm, but it requires UTF-8 charset support and a way to provide a
>> predefined cnonce. If it's not worth modifying our implementation to create
>> a regression test, I think at least we can temporarily hack our own JDK and
>> try on it. And I think it's most likely true that this algorithm is using a
>> different initialization vector as Bernd pointed out.
>
> As https://www.rfc-editor.org/errata_search.php?rfc=7616_status=0 shows,
> that 2nd test in rfc7616 was wrong in the initial version as it used a
> truncated version of SHA-512. The real SHA-512/256 algorithm should be used.
>
> $ jshell
> jshell> import java.security.MessageDigest
>
> jshell>
> HexFormat.of().formatHex(MessageDigest.getInstance("SHA-512").digest("J\u00e4s\u00f8n
> Doe:a...@example.org".getBytes("UTF-8")))
> $2 ==>
> "488869477bf257147b804c45308cd62ac4e25eb717b12b298c79e62dcea254ec5211a6631b181289b4dd8c14890f38f04bff8a388106dabb900c6984ba592b6a"
>
> jshell>
> HexFormat.of().formatHex(MessageDigest.getInstance("SHA-512/256").digest("J\u00e4s\u00f8n
> Doe:a...@example.org".getBytes("UTF-8")))
> $3 ==> "793263caabb707a56211940d90411ea4a575adeccb7e360aeb624ed06ece9b0b"
We could put a property in there to seed the random number generator, but I'd
prefer not to do that. I will check with a modified JDK that it works with this
data though.
-
PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
On Thu, 10 Mar 2022 15:02:17 GMT, Weijun Wang wrote: >> Michael McMahon has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - update >> - update after first review round > > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > line 701: > >> 699: } >> 700: byte[] digest = md.digest(); >> 701: StringBuilder res = new StringBuilder(digest.length * 2); > > Can we use `HexFormat` to encode the bytes? The fix will probably be back ported, so I'd prefer not to use HexFormat. - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
On Thu, 10 Mar 2022 14:26:28 GMT, Weijun Wang wrote:
>> Michael McMahon has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - update
>> - update after first review round
>
> src/java.base/share/classes/java/net/doc-files/net-properties.html line 234:
>
>> 232: in the {@code java.security} properties file and currently
>> comprises {@code MD5} and
>> 233: {@code SHA-1}. If it is still required to use one of these
>> algorithms, then they can be
>> 234: re-enabled by setting this property to a comma separated list
>> of the algorithm names.
>
> Can we use "re-enabled" in the property name? To me, the name "enabled"
> sounds like all enabled algorithms are listed here.
Okay, I'm suggesting "http.auth.digest.reEnabledAlgorithms" now.
Hopefully we can stick with that.
> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
> line 78:
>
>> 76: private static final Set defDisabledAlgs = getDefaultAlgs();
>> 77:
>> 78: private static Set getDefaultAlgs() {
>
> How about rename the method to include "disabled"?
That code is reworked so the method no longer exists
-
PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
On Thu, 10 Mar 2022 14:21:41 GMT, Weijun Wang wrote:
>> Michael McMahon has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - update
>> - update after first review round
>
> src/java.base/share/conf/security/java.security line 711:
>
>> 709: # separated list of algorithms to be allowed.
>> 710: #
>> 711: jdk.httpdigest.defaultDisabledAlgorithms = MD5, MD-5, SHA1, SHA-1
>
> I haven't seen people using "MD-5". It's also not an alias of "MD5" in our
> own security providers. On the other hand, we support both "SHA1" and "SHA"
> (and its OID) as aliases of "SHA-1". So, either we list all these aliases
> here, or we only put the standard names here and "canonicalize" the name when
> we see one. `sun.security.util.KnownOIDs.findMatch("SHA-1").stdName()` can be
> used.
The aliases are a PITA. It's a shame there isn't support in the public API to
canonicalize the names. But, what I will do is canonicalize the incoming
strings from the HTTP server and then we only have to list canonical names in
the properties.
-
PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
On Wed, 9 Mar 2022 15:41:08 GMT, Weijun Wang wrote: >> Michael McMahon has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - update >> - update after first review round > > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > line 426: > >> 424: algorithm = "MD5"; // The default, accoriding to rfc2069 >> 425: } >> 426: algorithm = algorithm.toUpperCase(); > > Please use `toUpperCase(Locale.ROOT)` or `toUpperCase(Locale.ENGLISH)`. > Should we have a test case for "SHA-512-256" too? If there's hardcoded example in the RFC, I will include it - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
On Wed, 9 Mar 2022 15:18:43 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - update
>> - update after first review round
>
> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
> line 82:
>
>> 80: @SuppressWarnings("removal")
>> 81: String secprops = AccessController.doPrivileged(
>> 82: new PrivilegedAction<>() {
>
> could use a lambda instead of an anonymous class?
Had tried it first and compiler didn't know whether lambda is a
PrivilegedAction or a PrivilegedExceptionAction, but it seems a cast works.
Will change it.
> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
> line 127:
>
>> 125: String s = NetProperties.get(enabledAlgPropName);
>> 126: return s == null
>> 127: ? "" : s.replaceAll("\\s", "").toUpperCase();
>
> Should probably use Local.ROOT to convert to upper case.
> It seems to me that the code that takes a String as argument, check for null
> and returns an empty set, remove spaces, convert it to upper case, splits the
> string at commas, and create an immutable set from that, could be moved to an
> auxillary function and called for parsing both the Security property and the
> System property - since their syntax is identical.
good idea
-
PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
On Wed, 9 Mar 2022 14:23:24 GMT, Weijun Wang wrote: >> Michael McMahon has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - update >> - update after first review round > > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > line 99: > >> 97: // A net property which overrides the disabled set above. >> 98: private static final String enabledAlgPropName = >> 99: "http.auth.digest.enabledAlgorithms"; > > I'm not familiar with the practice of overriding a security property with a > net property. Just FYI, in security libs, we often override a security > property with a system property and we have a dedicated method for this at > https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.base/share/classes/sun/security/util/SecurityProperties.java#L46. A net property can be a system property. But, it can also be specified in the net.properties file. We're using different names for the security and net property as the security property specifies algortithms to be disabled and the net property ones to be (re)enabled. > src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java > line 232: > >> 230: ? StandardCharsets.UTF_8 >> 231: : StandardCharsets.ISO_8859_1; >> 232: } > > Do you want to reject other values? According to the RFC, `utf-8` is the only > valid one. You mean reject the whole response as a protocol error? I guess we probably should do that. - PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
On Wed, 9 Mar 2022 15:53:02 GMT, Daniel Fuchs wrote:
>> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
>> line 85:
>>
>>> 83: public String run() {
>>> 84: return Security.getProperty(secPropName)
>>> 85:.replaceAll("\\s", "")
>>
>> `Security.getProperty` may return `null` so replacement should only be made
>> after checking that it is non null.
>
> Maybe `String.trim()` should be called on each element after splitting
> instead: you want to remove spaces before and after commas, not necessarily
> spaces within a name. "MD 5, SHA-256" probably shouldn't be parsed as
> "MD5,SHA-256".
Okay.
-
PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
On Mon, 7 Mar 2022 20:35:13 GMT, Sean Mullan wrote:
>> Michael McMahon has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - update
>> - update after first review round
>
> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
> line 514:
>
>> 512: if (getAuthType() == AuthCacheValue.Type.Server &&
>> 513: getProtocolScheme().equals("https")) {
>> 514: // HTTPS server authentication can use any algorithm
>
> A more security conscious user may want to disable MD5 digest authentication,
> even when used over HTTPS, even though the risks are far less than when used
> over HTTP. Is there a way to do that?
There isn't a way to do that currently. I'd prefer not to further complicate
the configuration to allow it. It would probably be a lot simpler to just
disable MD5 across the board for proxy and server instead? We're including
SHA-1 in that list now as well by the way.
-
PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]
> Hi, > > Could I get the following change reviewed please, which is to disable the MD5 > message digest algorithm by default in the HTTP Digest authentication > mechanism? The algorithm can be opted into by setting a new system property > "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also > updates the Digest authentication implementation to use some of the more > secure features defined in RFC7616, such as username hashing and additional > digest algorithms like SHA256 and SHA512-256. > > - Michael Michael McMahon has updated the pull request incrementally with two additional commits since the last revision: - update - update after first review round - Changes: - all: https://git.openjdk.java.net/jdk/pull/7688/files - new: https://git.openjdk.java.net/jdk/pull/7688/files/f0fb72de..b765d412 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7688=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7688=00-01 Stats: 168 lines in 12 files changed: 73 ins; 27 del; 68 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: 8281561: Disable http DIGEST mechanism with MD5 by default
On Sat, 5 Mar 2022 15:07:15 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 514:
>
>> 512: if (getAuthType() == AuthCacheValue.Type.Server &&
>> 513: getProtocolScheme().equals("https")) {
>> 514: // HTTPS server authentication can use any algorithm
>
> Hello Michael,
> Should it be noted somewhere in the text of the linked CSR that the property
> values play no role if the request URL's scheme is HTTPS (and proxy isn't
> involved)?
>
> As a related question - should the implementation even bother about this HTTP
> vs HTTPS protocol check? As far as I understand, the reason why we are
> proposing to disable support for MD5 is because of its cryptographic
> weakness/vulnerabilities. So irrespective of whether or not the computed MD5
> (or any other disabled algorithm) digest value gets transferred over HTTP or
> HTTPS, the value would still be cryptographically vulnerable isn't it?
The distinction may be worth mentioning in the CSR and the docs. The
distinction is useful because like Basic authentication which is completely
insecure with plaintext HTTP, it is secure and a potentially useful web
authentication scheme controlled by the browser when run over HTTPS (except
when authenticating with HTTPS to a proxy).
-
PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default
I'm wrong. It is implemented in the security libs. So, that means we can
support it also
Michael
On 07/03/2022 12:24, Michael McMahon wrote:
Bernd,
In that case we should defer to the security libraries to implement
SHA-512-256, which does not seem to be supported currently. We already
support SHA-512 so that should be sufficient at this point.
Thanks
Michael.
On 07/03/2022 11:27, Bernd Eckenfels wrote:
Hello,
SHA-512/256 is normally not a simple truncation (because similiar
hashes are not a robust crypto practice, instead it is using
different initialisation vectors).
Haven’t checked the example vectors in rfc 7616, but I would asume
they refer to FIPS 180-4 truncation variants.
Gruss
Bernd
--
http://bernd.eckenfels.net
Von: net-dev im Auftrag von Michael
McMahon
Gesendet: Monday, March 7, 2022 12:04:02 PM
An:net-dev@openjdk.java.net
Betreff: Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by
default
On Fri, 4 Mar 2022 14:59:48 GMT, Weijun Wang 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 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
Right, probably better to allow whitespace, which seems to be
commonly used in the existing security properties
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?
I'll try and see if it fits the normal line width
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.
Okay, I'll double check that. I haven't found any server
implementations of this feature to test with yet,
-
PR:https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default
Bernd,
In that case we should defer to the security libraries to implement
SHA-512-256, which does not seem to be supported currently. We already
support SHA-512 so that should be sufficient at this point.
Thanks
Michael.
On 07/03/2022 11:27, Bernd Eckenfels wrote:
Hello,
SHA-512/256 is normally not a simple truncation (because similiar hashes are
not a robust crypto practice, instead it is using different initialisation
vectors).
Haven’t checked the example vectors in rfc 7616, but I would asume they refer
to FIPS 180-4 truncation variants.
Gruss
Bernd
--
http://bernd.eckenfels.net
Von: net-dev im Auftrag von Michael
McMahon
Gesendet: Monday, March 7, 2022 12:04:02 PM
An:net-dev@openjdk.java.net
Betreff: Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default
On Fri, 4 Mar 2022 14:59:48 GMT, Weijun Wang 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 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
Right, probably better to allow whitespace, which seems to be commonly used in
the existing security properties
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?
I'll try and see if it fits the normal line width
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.
Okay, I'll double check that. I haven't found any server implementations of
this feature to test with yet,
-
PR:https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default
On Fri, 4 Mar 2022 16:26:52 GMT, Weijun Wang 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 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".
I'll look into supporting that.
-
PR: https://git.openjdk.java.net/jdk/pull/7688
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default
On Fri, 4 Mar 2022 14:59:48 GMT, Weijun Wang 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 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
Right, probably better to allow whitespace, which seems to be commonly used in
the existing security properties
> 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?
I'll try and see if it fits the normal line width
> 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.
Okay, I'll double check that. I haven't found any server implementations of
this feature to test with yet,
-
PR: https://git.openjdk.java.net/jdk/pull/7688
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 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
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
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
Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default
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
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
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: 8281561: Disable http DIGEST mechanism with MD5 by default
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: 8282617: sun.net.www.protocol.https.HttpsClient#putInKeepAliveCache() doesn't use a lock while dealing with inCache field
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
RFR: 8281561: Disable http DIGEST mechanism with MD5 by default
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=7688=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: 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.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
Re: Integrated: 8282020: ProblemList sun/net/www/protocol/https/HttpsURLConnection/B6216082.java until JDK-8282017 is fixed
On Wed, 16 Feb 2022 21:27:40 GMT, Daniel Fuchs wrote: > Please find here a changeset to ProblemList > sun/net/www/protocol/https/HttpsURLConnection/B6216082.java until JDK-8282017 > is fixed LGTM - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7505
Integrated: 8278067: Make HttpURLConnection default keep alive timeout configurable
On Fri, 4 Feb 2022 13:11:02 GMT, Michael McMahon wrote: > Hi, > > Could I get the following patch reviewed please? (A CSR is also required > which I will submit when the docs are agreed) > > It adds a pair of new system properties to make the keep alive timer in > java.net.HttpURLConnection configurable. > The proposed property names are: > > "http.keepAlive.time.server" and "http.keepAlive.time.proxy" > > Thanks, > Michael This pull request has now been integrated. Changeset: d8f44aa3 Author:Michael McMahon URL: https://git.openjdk.java.net/jdk/commit/d8f44aa39e921594505864e6270f42b745265293 Stats: 262 lines in 4 files changed: 249 ins; 1 del; 12 mod 8278067: Make HttpURLConnection default keep alive timeout configurable Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v6]
> Hi, > > Could I get the following patch reviewed please? (A CSR is also required > which I will submit when the docs are agreed) > > It adds a pair of new system properties to make the keep alive timer in > java.net.HttpURLConnection configurable. > The proposed property names are: > > "http.keepAlive.time.server" and "http.keepAlive.time.proxy" > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: remove extraneous .swp file - Changes: - all: https://git.openjdk.java.net/jdk/pull/7349/files - new: https://git.openjdk.java.net/jdk/pull/7349/files/9a7a4ced..d047808e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7349=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7349=04-05 Stats: 0 lines in 1 file changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7349/head:pull/7349 PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v5]
> Hi, > > Could I get the following patch reviewed please? (A CSR is also required > which I will submit when the docs are agreed) > > It adds a pair of new system properties to make the keep alive timer in > java.net.HttpURLConnection configurable. > The proposed property names are: > > "http.keepAlive.time.server" and "http.keepAlive.time.proxy" > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: tighten up spec wording and move props beside existing keep alive ones - Changes: - all: https://git.openjdk.java.net/jdk/pull/7349/files - new: https://git.openjdk.java.net/jdk/pull/7349/files/79f64a76..9a7a4ced Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7349=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7349=03-04 Stats: 18 lines in 2 files changed: 10 ins; 8 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7349/head:pull/7349 PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v4]
> Hi, > > Could I get the following patch reviewed please? (A CSR is also required > which I will submit when the docs are agreed) > > It adds a pair of new system properties to make the keep alive timer in > java.net.HttpURLConnection configurable. > The proposed property names are: > > "http.keepAlive.time.server" and "http.keepAlive.time.proxy" > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: Fixed test case problem and update from Daniel's review - Changes: - all: https://git.openjdk.java.net/jdk/pull/7349/files - new: https://git.openjdk.java.net/jdk/pull/7349/files/b0b7673c..79f64a76 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7349=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7349=02-03 Stats: 22 lines in 1 file changed: 5 ins; 6 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/7349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7349/head:pull/7349 PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v3]
On Mon, 14 Feb 2022 13:38:06 GMT, Daniel Fuchs wrote:
>> Michael McMahon 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 12 additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into keepalive
>> - update after Daniel's first review
>> - Merge branch 'master' into keepalive
>> - added docs
>> - reverted change to LIFETIME constant. Were not necessary
>> - updates
>> - Merge branch 'master' into keepalive
>> - Merge branch 'master' into keepalive
>> - Merge branch 'master' into keepalive
>> - Merge branch 'master' into keepalive
>> - ... and 2 more:
>> https://git.openjdk.java.net/jdk/compare/2fc8366a...b0b7673c
>
> test/jdk/sun/net/www/http/KeepAliveCache/KeepAliveProperty.java line 111:
>
>> 109: out.print(BODY);
>> 110: out.flush();
>> 111: pass = true;
>
> should that be: pass = ! expectClose?
Good catch. That was masking another problem in the test where it wasn't
detecting the socket close properly.
> test/jdk/sun/net/www/http/KeepAliveCache/KeepAliveProperty.java line 125:
>
>> 123:
>> 124: static String fetch(URL url) throws Exception {
>> 125: InputStream in = url.openConnection(NO_PROXY).getInputStream();
>
> could use try-with-resource here
Ok
-
PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v3]
> Hi, > > Could I get the following patch reviewed please? (A CSR is also required > which I will submit when the docs are agreed) > > It adds a pair of new system properties to make the keep alive timer in > java.net.HttpURLConnection configurable. > The proposed property names are: > > "http.keepAlive.time.server" and "http.keepAlive.time.proxy" > > Thanks, > Michael Michael McMahon 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 12 additional commits since the last revision: - Merge branch 'master' into keepalive - update after Daniel's first review - Merge branch 'master' into keepalive - added docs - reverted change to LIFETIME constant. Were not necessary - updates - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - ... and 2 more: https://git.openjdk.java.net/jdk/compare/7e38b27f...b0b7673c - Changes: - all: https://git.openjdk.java.net/jdk/pull/7349/files - new: https://git.openjdk.java.net/jdk/pull/7349/files/edcbb13f..b0b7673c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7349=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7349=01-02 Stats: 3129 lines in 84 files changed: 2334 ins; 392 del; 403 mod Patch: https://git.openjdk.java.net/jdk/pull/7349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7349/head:pull/7349 PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable [v2]
> Hi, > > Could I get the following patch reviewed please? (A CSR is also required > which I will submit when the docs are agreed) > > It adds a pair of new system properties to make the keep alive timer in > java.net.HttpURLConnection configurable. > The proposed property names are: > > "http.keepAlive.time.server" and "http.keepAlive.time.proxy" > > Thanks, > Michael Michael McMahon 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: - update after Daniel's first review - Merge branch 'master' into keepalive - added docs - reverted change to LIFETIME constant. Were not necessary - updates - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - ... and 1 more: https://git.openjdk.java.net/jdk/compare/98c4ce4c...edcbb13f - Changes: - all: https://git.openjdk.java.net/jdk/pull/7349/files - new: https://git.openjdk.java.net/jdk/pull/7349/files/85d5662b..edcbb13f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7349=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7349=00-01 Stats: 12764 lines in 314 files changed: 9568 ins; 1658 del; 1538 mod Patch: https://git.openjdk.java.net/jdk/pull/7349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7349/head:pull/7349 PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable
On Fri, 4 Feb 2022 14:11:49 GMT, Daniel Fuchs wrote:
>> Hi,
>>
>> Could I get the following patch reviewed please? (A CSR is also required
>> which I will submit when the docs are agreed)
>>
>> It adds a pair of new system properties to make the keep alive timer in
>> java.net.HttpURLConnection configurable.
>> The proposed property names are:
>>
>> "http.keepAlive.time.server" and "http.keepAlive.time.proxy"
>>
>> Thanks,
>> Michael
>
> src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java line 159:
>
>> 157: int keepAliveTimeout = http.getKeepAliveTimeout();
>> 158: if (keepAliveTimeout == 0) {
>> 159: keepAliveTimeout =
>> getUserKeepAlive(http.getUsingProxy());
>
> This could be 0 if -Dhttp.keepAlive.time.xxx=0 was specified which would
> result in an assert below.
> Also I am not sure I understand the logic of having same 5s timeout for
> server & proxy if nothing was specified anywhere, but having a different
> value for proxy & server if the server specified keep-alive without providing
> a value.
> Where does that come from?
Ah, I meant to catch that case as well in the method where the property is
read. Probably should be documented as well.
The other behavior is just maintaining the present behavior, purely for
compatibility reasons.
-
PR: https://git.openjdk.java.net/jdk/pull/7349
RFR: 8278067: Make HttpURLConnection default keep alive timeout configurable
Hi, Could I get the following patch reviewed please? (A CSR is also required which I will submit when the docs are agreed) It adds a pair of new system properties to make the keep alive timer in java.net.HttpURLConnection configurable. The proposed property names are: "http.keepAlive.time.server" and "http.keepAlive.time.proxy" Thanks, Michael - Commit messages: - added docs - reverted change to LIFETIME constant. Were not necessary - updates - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - Merge branch 'master' into keepalive - initial work Changes: https://git.openjdk.java.net/jdk/pull/7349/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7349=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278067 Stats: 252 lines in 4 files changed: 242 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/7349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7349/head:pull/7349 PR: https://git.openjdk.java.net/jdk/pull/7349
Re: RFR: 8280965: Tests com/sun/net/httpserver/simpleserver fail with FileSystemException on Windows 11
On Thu, 3 Feb 2022 09:24:46 GMT, Julia Boes wrote:
> This change updates the tests in question to handle the case where sym link
> creation is not supported, as observed on Windows 11 where additional
> privileges are required to create sym links.
>
> Testing: tier 1-3 across platforms, plus tests in question on Windows 11
> specifically
test/jdk/com/sun/net/httpserver/simpleserver/CustomFileSystemTest.java line 444:
> 442: }
> 443:
> 444: private boolean createSymLink(Path symlink, Path target) {
Seems like createSymLink should actually return true or false, or if it throws
an exception in one case, then it should be void, in which case you wouldn't
need the if statement in the calling code
-
PR: https://git.openjdk.java.net/jdk/pull/7335
Re: RFR: 8280868: LineBodyHandlerTest.java creates and discards too many clients
On Fri, 28 Jan 2022 09:45:02 GMT, Daniel Fuchs wrote: > Please find enclosed a simple test fix for: > 8280868: LineBodyHandlerTest.java creates and discards too many clients > > The LineBodyHandlerTest.java creates and discards many clients (64). > The test has been observed failing intermittently on some systems (Windows > 10) while trying to open the client's selector, apparently due to some > connection limit. > > It seems that using a single client reduces the occurrences in which the test > fails. LGTM - Marked as reviewed by michaelm (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7263
Integrated: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Thu, 13 Jan 2022 12:10:11 GMT, Michael McMahon wrote: > Hi, > > This change adds Channel Binding Token (CBT) support to HTTPS > (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) > authentication scheme. When enabled, the implementation preemptively includes > a CBT with authentication requests over Kerberos. The feature is enabled as > follows: > > A system property "jdk.spnego.cbt" is defined which can have the values > "never" (default), which means the feature is disabled, "always", which means > the CBT is included for all https Negotiate authentications, or it can take > the form "domain:a,b.c,*.d.com" which is a comma separated list of > domains/hosts where the feature is enabled, and disabled everywhere else. In > the given example, the CBT would be included in authentication requests for > hosts "a", "b.c" and all hosts under the domain "d.com" and all of its > sub-domains. > > A test will be added separately to the implementation. > > Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842 > > Thanks, > Michael This pull request has now been integrated. Changeset: de3113b9 Author:Michael McMahon URL: https://git.openjdk.java.net/jdk/commit/de3113b998550021bb502cd6f766036fb8351e7d Stats: 858 lines in 12 files changed: 696 ins; 146 del; 16 mod 8279842: HTTPS Channel Binding support for Java GSS/Kerberos Co-authored-by: Weijun Wang Reviewed-by: dfuchs, weijun, darcy - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v13]
> Hi, > > This change adds Channel Binding Token (CBT) support to HTTPS > (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, Kerberos) > authentication scheme. When enabled, the implementation preemptively includes > a CBT with authentication requests over Kerberos. The feature is enabled as > follows: > > A system property "jdk.spnego.cbt" is defined which can have the values > "never" (default), which means the feature is disabled, "always", which means > the CBT is included for all https Negotiate authentications, or it can take > the form "domain:a,b.c,*.d.com" which is a comma separated list of > domains/hosts where the feature is enabled, and disabled everywhere else. In > the given example, the CBT would be included in authentication requests for > hosts "a", "b.c" and all hosts under the domain "d.com" and all of its > sub-domains. > > A test will be added separately to the implementation. > > Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842 > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: spec update from CSR - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/59f703da..468e5345 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=11-12 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: JDK-8280498: [aix]: jdk/java/net/Inet4Address/PingThis.java fails
On Fri, 28 Jan 2022 14:46:32 GMT, Michael Felt wrote: >> If it defeats the purpose, then it needs to be skipped. >> >> * When I was trying to understand the test, it seemed to be that it >> _assumed_ that "0.0.0.0" was 'converted' to 127.0.0.1. >> * If there is an international standard (ISO, POSIX) that states 0.0.0.0 >> needs to be treated as 127.0.0.1 - AIX clearly has a bug. Same for `::)` >> mapping to `::1` - AIX doesn't do that IPv4 (now), and, afaik, never has for >> IPv6. >> * While testing I also saw that AIX reacts differently (error message iirc) >> when passed NULL, whereas it behaves "as expected" when given a null-string >> `""` >> * FYI: Adoptium has put this test on the excluded list, but if a fix using >> `""` rather than "0.0.0.0" and/or "::1" rather than "::0", the test should >> perhaps state the exclusion as a comment in the code, and return >> immediately, as it does for Windows. > >> Or is it possible to change the implementation on AIX so the test passes >> without change? > > * Digging into the java guts to map "0.0.0.0" to "127.0.0.1" seems too far to > me - as I believe interfaces are not suppossed to be "translating" as well. > * If the behavior "0.0.0.0" becomes "127.0.0.1" is an official standard - a > bug needs to be filed with IBM (which I cannot do) - but we need to also be > aware that any fix to AIX is not likely to ever be applied on AIX 7.1 TL4 > (maybe TL5) which are systems used to build distributions. > * Again - being a noob (or fresh face; fresh meat) the test title is merely > "PingThis" - where is "This" defined as "0.0.0.0" (if it had been named > _PingHERE_ I could understand as the alias, iirc, for 0.0.0.0 is "Here". Might be better to skip the test then by adding to the header: * * @requires os.family != "aix" * rather than the check in the test source - PR: https://git.openjdk.java.net/jdk/pull/7013
Re: RFR: JDK-8280498: [aix]: jdk/java/net/Inet4Address/PingThis.java fails
On Fri, 28 Jan 2022 14:28:43 GMT, Alan Bateman wrote:
>> with IP "0.0.0.0"
>>
>> - it either does nothing and ping fails, or, in some virtual environments
>> is treated as the default route address.
>> - IPv6 support for ::1 is available since 1977; however, ::0 is not accepted
>> as a vaild psuedo IPv6 address. '::1' must be used instead.
>>
>> ping: bind: The socket name is not available on this system.
>> ping: bind: The socket name is not available on this system.
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.037 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.045 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>> round-trip min/avg/max = 0/0/0 ms
>> PING ::1: (::1): 56 data bytes
>> 64 bytes from ::1: icmp_seq=0 ttl=255 time=0.052 ms
>> 64 bytes from ::1: icmp_seq=1 ttl=255 time=0.047 ms
>>
>> --- ::1 ping statistics ---
>> 2 packets transmitted, 2 packets received, 0% packet loss
>>
>>
>> A long commit message.
>>
>> This came to light because some systems failed with IPv4 (those that passed
>> replaced 0.0.0.0 with the default router. but most just fail - not
>> substituting
>> 0.0.0.0 with 127.0.0.1. However, InetAddress.getByName("") returns 127.0.0.1
>> which compares well with other platform's behavior.
>
> test/jdk/java/net/Inet4Address/PingThis.java line 62:
>
>> 60: else {
>> 61: addrs.add("0.0.0.0");
>> 62: }
>
> This conflicts with the purpose of the test. Maybe this test needs to be
> skipped on AIX?
Or is it possible to change the implementation on AIX so the test passes
without change?
-
PR: https://git.openjdk.java.net/jdk/pull/7013
