Re: RFR: 8338395: Add test coverage for instantiating NativePRNG with SecureRandomParameters
On Thu, 15 Aug 2024 09:29:00 GMT, Fernando Guallini wrote: > In order to improve performance when instantiating NativePRNG, a dummy > constructor was added in the PR: https://github.com/openjdk/jdk/pull/17560 > which takes and ignores a `java.security.SecureRandomParameters`, throwing an > exception if any parameter is passed. > > This PR adds test coverage for those scenarios where the constructor is > called passing a not null parameter. This looks good to me. The behavior is slightly different on older JVMs (17), but only in the underlying cause of the NSAE. Shouldn't matter going forward. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20592#pullrequestreview-2291212545
Integrated: 8337826: Improve logging in OCSPTimeout and SimpleOCSPResponder to help diagnose JDK-8309754
On Mon, 5 Aug 2024 19:01:30 GMT, Jamil Nimeh wrote: > This proposed enhancement adds logging to the OCSPTimeout test, which is > intermittently failing and difficult to reproduce. The hope is that with > extra logging enabled that additional clues as to the cause of these rare > failures will become apparent. > > JBS: https://bugs.openjdk.org/browse/JDK-8337826 This pull request has now been integrated. Changeset: 9b11bd7f Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/9b11bd7f4a511ddadf9f02e82aab6ba78beb6763 Stats: 105 lines in 2 files changed: 46 ins; 24 del; 35 mod 8337826: Improve logging in OCSPTimeout and SimpleOCSPResponder to help diagnose JDK-8309754 Reviewed-by: mullan - PR: https://git.openjdk.org/jdk/pull/20471
Re: RFR: 8337826: Improve logging in OCSPTimeout and SimpleOCSPResponder to help diagnose JDK-8309754 [v2]
> This proposed enhancement adds logging to the OCSPTimeout test, which is > intermittently failing and difficult to reproduce. The hope is that with > extra logging enabled that additional clues as to the cause of these rare > failures will become apparent. > > JBS: https://bugs.openjdk.org/browse/JDK-8337826 Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision: Removed JBS entry from test comment block - Changes: - all: https://git.openjdk.org/jdk/pull/20471/files - new: https://git.openjdk.org/jdk/pull/20471/files/077c8cbd..558626be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20471&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20471&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20471.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20471/head:pull/20471 PR: https://git.openjdk.org/jdk/pull/20471
RFR: 8337826: Improve logging in OCSPTimeout and SimpleOCSPResponder to help diagnose JDK-8309754
This proposed enhancement adds logging to the OCSPTimeout test, which is intermittently failing and difficult to reproduce. The hope is that with extra logging enabled that additional clues as to the cause of these rare failures will become apparent. JBS: https://bugs.openjdk.org/browse/JDK-8337826 - Commit messages: - 8337826: Improve logging in OCSPTimeout and SimpleOCSPResponder to help diagnose JDK-8309754 Changes: https://git.openjdk.org/jdk/pull/20471/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20471&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8337826 Stats: 106 lines in 2 files changed: 46 ins; 24 del; 36 mod Patch: https://git.openjdk.org/jdk/pull/20471.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20471/head:pull/20471 PR: https://git.openjdk.org/jdk/pull/20471
Re: RFR: 8328608: Multiple NewSessionTicket support for TLS
On Wed, 29 May 2024 18:53:55 GMT, Anthony Scarpino wrote: > Hi > > This change is to improve TLS 1.3 session resumption by allowing a TLS server > to send more than one resumption ticket per connection and clients to store > more. Resumption is a quick way to use an existing TLS session to establish > another session by avoiding the long TLS full handshake process. In TLS 1.2 > and below, clients can repeatedly resume a session by using the session ID > from an established connection. In TLS 1.3, a one-time "resumption ticket" > is sent by the server after the TLS connection has been established. The > server may send multiple resumption tickets to help clients that rapidly > resume connections. If the client does not have another resumption ticket, > it must go through the full TLS handshake again. The current implementation > in JDK 23 and below, only sends and store one resumption ticket. > > The number of resumption tickets a server can send should be configurable by > the application developer or administrator. [RFC > 8446](https://www.rfc-editor.org/rfc/rfc8446) does not specify a default > value. A system property called `jdk.tls.server.newSessionTicketCount` > allows the user to change the number of resumption tickets sent by the > server. If this property is not set or given an invalid value, the default > value of 3 is used. Further details are in the CSR. > > A large portion of the changeset is on the client side by changing the > caching system used by TLS. It creates a new `CacheEntry<>` type called > `QueueCacheEntry<>` that will store multiple values for a Map entry. src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java line 127: > 125: static final int serverNewSessionTicketCount; > 126: // Default for NST > 127: static final int SERVER_NST_DEFAULT = 3; I suggest you make the default 1 or 2 rather than 3. NSTs can get pretty large and we may want to preserve bandwidth a bit, especially for servers at scale. I think for many applications the default we're currently doing of 1 NST per connection might be enough, and the property allows folks to ratchet up the number of NSTs to fit their needs. But defaulting to 2 is probably fine also. - PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1639095203
Re: RFR: 8328608: Multiple NewSessionTicket support for TLS
On Wed, 29 May 2024 18:53:55 GMT, Anthony Scarpino
wrote:
> Hi
>
> This change is to improve TLS 1.3 session resumption by allowing a TLS server
> to send more than one resumption ticket per connection and clients to store
> more. Resumption is a quick way to use an existing TLS session to establish
> another session by avoiding the long TLS full handshake process. In TLS 1.2
> and below, clients can repeatedly resume a session by using the session ID
> from an established connection. In TLS 1.3, a one-time "resumption ticket"
> is sent by the server after the TLS connection has been established. The
> server may send multiple resumption tickets to help clients that rapidly
> resume connections. If the client does not have another resumption ticket,
> it must go through the full TLS handshake again. The current implementation
> in JDK 23 and below, only sends and store one resumption ticket.
>
> The number of resumption tickets a server can send should be configurable by
> the application developer or administrator. [RFC
> 8446](https://www.rfc-editor.org/rfc/rfc8446) does not specify a default
> value. A system property called `jdk.tls.server.newSessionTicketCount`
> allows the user to change the number of resumption tickets sent by the
> server. If this property is not set or given an invalid value, the default
> value of 3 is used. Further details are in the CSR.
>
> A large portion of the changeset is on the client side by changing the
> caching system used by TLS. It creates a new `CacheEntry<>` type called
> `QueueCacheEntry<>` that will store multiple values for a Map entry.
src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java line 202:
> 200: if (nstServerCount == null || nstServerCount < 1 ||
> 201: nstServerCount > 10) {
> 202: serverNewSessionTicketCount = 3;
I'm always on the fence about this, but do you think it is worthwhile to put a
debug log message here so folks can tell why their value is getting altered to
a default 3? If we don't do it for other props then it's probably not worth
it, but I thought I'd ask.
src/java.base/share/classes/sun/security/ssl/SSLSessionImpl.java line 503:
> 501: this.preSharedKey = new SecretKeySpec(b, alg);
> 502: // Get identity len
> 503: i = buf.get();
Given that this is just a small change to a large function it may not be
important to change, but a lot of these X-byte lengths followed by arrays might
be simplified with Record.getBytes8 or the other similar routines that handle
longer length fields. But you wouldn't end up with a null byte[] when the
length is zero, so that's a change, not necessarily a down-side.
src/java.base/share/classes/sun/security/util/Cache.java line 683:
> 681:
> 682: // Limit the number of queue entries.
> 683: private static final int MAXQUEUESIZE = 10;
What do you think about making this tunable through the constructor, perhaps
with a default value of 10?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1626171765
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1626184113
PR Review Comment: https://git.openjdk.org/jdk/pull/19465#discussion_r1628114149
Re: RFR: 8325513: Export method for Cipher [v3]
On Fri, 10 May 2024 14:00:55 GMT, Weijun Wang wrote: >> Add `Cipher::export` API. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > change new method to non final That seems like a good approach. If Cipher can address all the use cases for HPKE then is seems more prudent to proceed with #2. As for the KDF impact, do you see a potential Cipher-based HPKE needing to know the details of the KDF API? Or would it just need to know a standard algorithm name for a given KDF? If the latter then you may not need to wait for KDF in order to proceed given where it is in its review cycle. - PR Comment: https://git.openjdk.org/jdk/pull/18409#issuecomment-2113005112
Re: RFR: 8325513: Export method for Cipher [v3]
On Fri, 10 May 2024 14:00:55 GMT, Weijun Wang wrote: >> Add `Cipher::export` API. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > change new method to non final I see that it could work that way, but have we firmly established that HPKE will be implemented in JCE as a Cipher? I see no CSR for this and I would think we'd need one. If we have not firmly established that HPKE is being done as a Cipher, then changing Cipher in advance seems like putting the cart before the horse. I just thought I'd see CSRs in place for JDK-8325513 and/or JDK-8325548 and some kind of consensus on the direction. Maybe I missed an agreement on this approach in the past. - PR Comment: https://git.openjdk.org/jdk/pull/18409#issuecomment-2112942149
Re: RFR: 8331008: KDF Implementation
On Tue, 23 Apr 2024 20:42:51 GMT, Kevin Driver wrote: > Introduce an API for Key Derivation Functions (KDFs), which are cryptographic > algorithms for deriving additional keys from a secret key and other data. See > [JEP 478](https://openjdk.org/jeps/478). src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java line 370: > 368: } > 369: int rounds = (outLen + hmacLen - 1) / hmacLen; > 370: kdfOutput = new byte[rounds * hmacLen]; Are we missing a check to ensure that the `outLen` parameter is less than or equal to 255 * HashLen? See RFC 5869 sec. 2.3. - PR Review Comment: https://git.openjdk.org/jdk/pull/18924#discussion_r1594617681
Re: RFR: 8325024: java/security/cert/CertPathValidator/OCSP/OCSPTimeout.java incorrect comment information
On Wed, 31 Jan 2024 08:19:55 GMT, SendaoYan wrote: > 8325024: java/security/cert/CertPathValidator/OCSP/OCSPTimeout.java incorrect > comment information Looks good, but please label the JBS bug with noreg-trivial. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17646#pullrequestreview-1854456004
Re: RFR: 8325022: Incorrect error message on TLS 1.2 client authentication
On Wed, 31 Jan 2024 07:42:32 GMT, John Jiang wrote: > If the server doesn't receive the client certificate for required client > authentication, it should raise error `Empty client certificate chain`. Looks good. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17645#pullrequestreview-1854321874
Re: [jdk22] RFR: 8322100: Fix GCMIncrementByte4 & GCMIncrementDirect4, and increase overlap testing
On Thu, 18 Jan 2024 19:11:56 GMT, Anthony Scarpino wrote: > This is the straight backport to 22 of > https://github.com/openjdk/jdk/pull/17362 LGTM - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk22/pull/93#pullrequestreview-1830622552
Re: RFR: JDK-8322100: Fix GCMIncrementByte4 & GCMIncrementDirect4, and increase overlap testing
On Thu, 11 Jan 2024 03:26:03 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of a few simple test changes. This fixes a failure with two > manually run AES/GCM tests that depended on another test that changed with > [JDK-8318756](https://bugs.openjdk.org/browse/JDK-8318756). It also adds > testing for overlapping buffers that AES/GCM has, but CC20P1305 needs after > JDK-8318756 was included. > > After the change, manual testing of GCMIncrementByte4 and GCMIncrementDirect4 > were successfully and automated tests passed as well. > > thanks > > Tony LGTM - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17362#pullrequestreview-1826243669
Integrated: 8321542: C2: Missing ChaCha20 stub for x86_32 leads to crashes
On Tue, 12 Dec 2023 01:02:59 GMT, Jamil Nimeh wrote: > This fix corrects an oversight in the ChaCha20 intrinsics delivered by > JDK-8247645. An ifdef guard is now part of the x86 ChaCha20 intrinsics code > which disables them by default on 32-bit platforms, as this architecture was > not part of the feature delivery. This pull request has now been integrated. Changeset: 5718039a Author: Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/5718039a46ae51fa9b7042fe7163e3637e981b05 Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod 8321542: C2: Missing ChaCha20 stub for x86_32 leads to crashes Reviewed-by: chagedorn, shade - PR: https://git.openjdk.org/jdk/pull/17072
Re: RFR: 8321542: C2: Missing ChaCha20 stub for x86_32 leads to crashes [v2]
> This fix corrects an oversight in the ChaCha20 intrinsics delivered by > JDK-8247645. An ifdef guard is now part of the x86 ChaCha20 intrinsics code > which disables them by default on 32-bit platforms, as this architecture was > not part of the feature delivery. Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision: Modify warning message on 32-bit - Changes: - all: https://git.openjdk.org/jdk/pull/17072/files - new: https://git.openjdk.org/jdk/pull/17072/files/fe0d4461..ff55249e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17072&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17072&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17072.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17072/head:pull/17072 PR: https://git.openjdk.org/jdk/pull/17072
Re: RFR: 8321542: C2: Missing ChaCha20 stub for x86_32 leads to crashes
On Tue, 12 Dec 2023 09:24:48 GMT, Aleksey Shipilev wrote:
>> src/hotspot/cpu/x86/vm_version_x86.cpp line 1152:
>>
>>> 1150: // No support currently for ChaCha20 intrinsics on 32-bit platforms
>>> 1151: if (UseChaCha20Intrinsics) {
>>> 1152: warning("Support for ChaCha20 intrinsics not available on this
>>> CPU.");
>>
>> Maybe we can adapt the message to follow the convention of the other warning
>> messages for intrinsics like AES
>> https://github.com/openjdk/jdk/blob/2611a49ea13ee7a07f22692c3a4b32856ec5898f/src/hotspot/cpu/x86/vm_version_x86.cpp#L1062-L1065
>> or CRC32C
>> https://github.com/openjdk/jdk/blob/2611a49ea13ee7a07f22692c3a4b32856ec5898f/src/hotspot/cpu/x86/vm_version_x86.cpp#L1116-L1118
>>
>> Suggestion:
>>
>> warning("ChaCha20 intrinsics are not available on this CPU.");
>>
>>
>> Though I see that not all existing warning messages are consistent.
>>
>> Anyway, the bailout looks good to me.
>
> +1 on changing the message to "ChaCha20 intrinsics are not available on this
> CPU."
Makes sense to me. Will fix.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17072#discussion_r1424063190
Re: RFR: 8314199: Initial size PBEKeyFactory#validTypes is not up-to-date [v3]
On Mon, 9 Oct 2023 20:44:50 GMT, Kevin Driver wrote: >> fixes [JDK-8314199](https://bugs.openjdk.org/browse/JDK-8314199) by >> initializing the HashSet with a more accurate number > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > convert Set back to HashSet LGTM. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16103#pullrequestreview-1665251892
Re: RFR: 8314199: Initial size PBEKeyFactory#validTypes is not up-to-date
On Mon, 9 Oct 2023 19:19:49 GMT, Kevin Driver wrote: >> I wondered about an approach like this. I'll push another commit with these >> changes. > > Do you think we'll lose performance in a meaningful way? One of the > guarantees of HashSet is constant-time operations. > > There is no such guarantee for Set. The number of members is probably small > enough, relatively speaking, to not affect things much at creation time (and > we only do this once); however, lookup time (in `engineGetKeySpec` and > `engineTranslateKey`) might take a small hit if these methods are called > repeatedly. > > Let me know whether you think this is a valid concern. Thanks! I think there could be sight timing variations between `HashSet` and whatever Set implementation is returned by `Set.of()` but creation only happens once as you stated, and iterations over 20-ish elements just seems like the variance would be negligible in any real-world use cases. - PR Review Comment: https://git.openjdk.org/jdk/pull/16103#discussion_r1350752518
Re: RFR: 8314199: Initial size PBEKeyFactory#validTypes is not up-to-date
On Mon, 9 Oct 2023 16:36:06 GMT, Kevin Driver wrote:
> fixes [JDK-8314199](https://bugs.openjdk.org/browse/JDK-8314199) by
> initializing the HashSet with a more accurate number
src/java.base/share/classes/com/sun/crypto/provider/PBEKeyFactory.java line 59:
> 57:
> 58: static {
> 59: validTypes = HashSet.newHashSet(21);
Rather than having to change the initial size and keep it synchronized with all
the subsequent adds, could we just change the type of `validTypes` to
`Set` and initialize it using `Set.of(E...)`? It seems like that would
allow you to set all the values directly in the declaration up on line 49,
would remove the need for the static block and I think you could even declare
validTypes final, couldn't you? And any new additions to the set could simply
be added and not worry about setting an accurate count in the constructor.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16103#discussion_r1350563277
Re: RFR: 8293176: SSLEngine handshaker does not send an alert after a bad parameters [v2]
On Fri, 11 Aug 2023 21:38:04 GMT, Daniel Jeliński wrote: >> Please review this patch that ensures that all exceptions thrown by >> SSLEngine delegated tasks are translated to alerts. >> >> All exceptions should already be translated to SSLExceptions and alerts by >> the time we exit from context.dispatch; these exceptions are rethrown by >> `conContext.fatal` without modification. With this patch the remaining >> exceptions are translated to `internal_error` alerts. >> >> SSLSocket implements similar handling in SSLSocketImpl#startHandshake. >> SSLSocket rethrows `SocketException`s without modification, and translates >> other `IOException`s to `handshake_failure` alerts. SSLEngine does not need >> to handle `SocketException`s, and IMO `internal_error` is a better choice >> here. >> >> Tier1-3 tests pass. > > Daniel Jeliński has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix exception handling > - Fix indentation LGTM - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15148#pullrequestreview-1644692042
Re: RFR: 8313229: DHEKeySizing.java should be modified to use TLS versions TLSv1, TLSv1.1, TLSv1.2 [v2]
On Thu, 21 Sep 2023 13:29:10 GMT, Sean Mullan wrote: >> Please review this change to ensure this test is tested on different TLS >> protocols (1.0, 1.1, 1.2) >> >> I added a protocol parameter to the test arguments so that different >> protocols are tested. I also removed the boolean exportable argument as it >> wasn't doing anything. > > Sean Mullan has updated the pull request incrementally with one additional > commit since the last revision: > > Only adjust sever hello size for TLS_DHE_RSA_WITH_AES_128_CBC_SHA with > TLSv1.2. > Fix some typos. LGTM - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15846#pullrequestreview-1638329315
Re: RFR: 8313229: DHEKeySizing.java should be modified to use TLS versions TLSv1, TLSv1.1, TLSv1.2 [v2]
On Thu, 21 Sep 2023 13:30:07 GMT, Sean Mullan wrote: >> test/jdk/sun/security/ssl/DHKeyExchange/DHEKeySizing.java line 35: >> >>> 33: * @library /javax/net/ssl/templates >>> 34: * @run main/othervm -Djdk.tls.client.enableSessionTicketExtension=false >>> 35: * DHEKeySizing TLS_DHE_RSA_WITH_AES_128_CBC_SHA 1645 267 TLSv1 >> >> Just curious why the server key exchange length went up in size by a couple >> bytes. Was 1643 incorrect before this change? > > Good question. Part of this is a cut-and-paste error. The only change to 1645 > bytes should be for line 64. The previous version of this test used TLS 1.0 > for all the tests. When testing this on different protocols, I noticed the > server hello for this cipher suite takes 2 extra bytes on TLSv1.2, and this > was enough to cause the test to fail even with the 6 extra bytes for > KEY_LEN_BIAS. - I don't know the exact reason why it takes a few extra bytes > though. > > I fixed this in the latest commit - only line 64 should be different now for > the server hello length. An extra two bytes for a server hello could be due to the assertion of a SH extension that was not asserted in earlier versions of the protocol or something along those lines. Since that 1645 bytes relates to "Server Hello Series" (I assume that means the entire SH flight of messages) there could be a two-byte variance in a number of places. The fix looks good to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/15846#discussion_r165888
Re: RFR: 8313229: DHEKeySizing.java should be modified to use TLS versions TLSv1, TLSv1.1, TLSv1.2
On Wed, 20 Sep 2023 19:51:28 GMT, Sean Mullan wrote: > Please review this change to ensure this test is tested on different TLS > protocols (1.0, 1.1, 1.2) > > I added a protocol parameter to the test arguments so that different > protocols are tested. I also removed the boolean exportable argument as it > wasn't doing anything. test/jdk/sun/security/ssl/DHKeyExchange/DHEKeySizing.java line 35: > 33: * @library /javax/net/ssl/templates > 34: * @run main/othervm -Djdk.tls.client.enableSessionTicketExtension=false > 35: * DHEKeySizing TLS_DHE_RSA_WITH_AES_128_CBC_SHA 1645 267 TLSv1 Just curious why the server key exchange length went up in size by a couple bytes. Was 1643 incorrect before this change? - PR Review Comment: https://git.openjdk.org/jdk/pull/15846#discussion_r1332124810
Re: RFR: 8311596: Add separate system properties for TLS server and client for maximum chain length [v2]
On Wed, 6 Sep 2023 20:02:10 GMT, Hai-May Chao wrote: >> Please review the enhancement for JDK-8311596 and its CSR JDK-8313236. Thank >> you. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Set to default if a negative value is set This looks good to me. Thanks for making that negative value update. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15163#pullrequestreview-1614147348
Re: RFR: JDK-8315422: getSoTimeout() would be in try block in SSLSocketImpl
On Thu, 31 Aug 2023 02:34:58 GMT, John Jiang wrote:
> The method `SSLSocketImpl::closeSocket` has the below code snippet,
>
>
> if (appInput.readLock.tryLock()) {
> int soTimeout = getSoTimeout();
> try {
> // deplete could hang on the skip operation
> // in case of infinite socket read timeout.
> // Change read timeout to avoid deadlock.
> // This workaround could be replaced later
> // with the right synchronization
> if (soTimeout == 0)
> setSoTimeout(DEFAULT_SKIP_TIMEOUT);
> inputRecord.deplete(false);
> } catch (java.net.SocketTimeoutException stEx) {
> // skip timeout exception during deplete
> } finally {
> if (soTimeout == 0)
> setSoTimeout(soTimeout);
> appInput.readLock.unlock();
> }
> }
>
>
> If `getSoTimeout()` throws an exception, say `SocketException`,
> `appInput.readLock.unlock()` cannot be called.
This looks good to me.
-
Marked as reviewed by jnimeh (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15503#pullrequestreview-1603786718
Re: RFR: 8309214: sun/security/pkcs11/KeyStore/CertChainRemoval.java fails after 8301154
On Thu, 3 Aug 2023 20:51:33 GMT, Valerie Peng wrote: > This change addresses the scenario where a certificate is first stored as > part of a certificate chain and then stored again as a certificate > corresponding to a PrivateKey entry. Newer version of NSS errors out with > CKR_GENERAL_ERROR with the 2nd store, i.e. C_CreateObject() call. > > Proposed fix is to check for match before calling C_CreateObject(), if a > match is found, set its alias instead. This looks good. Does the bug need a noreg label since this is addressing an existing test failure? - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15146#pullrequestreview-1589954528
Re: RFR: 8311596: Add separate system properties for TLS server and client for maximum chain length
On Fri, 4 Aug 2023 17:30:06 GMT, Hai-May Chao wrote: > Please review the enhancement for JDK-8311596 and its CSR JDK-8313236. Thank > you. src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java line 159: > 157: maxServerCertificateChainLength = (serverLen != null) ? > 158: serverLen : maxCertificateChainLength; > 159: } I wonder if we should take the opportunity here with these new properties as well as `jdk.tls.maxCertificateChainLength` to also equate negative numbers (and maybe zero) to be the default. Right now only property values that fail the internal parseInt conversion will evaluate to `null` and would be assigned the default I think. But a negative value I think would be taken as-is from the property. Should a negative max cert chain length get set to the default? If so, it might also make sense to give a warning about the offending value and note that it is being set to the default (similar to what `GetPropertyAction.privilegedGetTimeoutProp()` does). If you think this is worthwhile, the CSR should probably be updated to reflect that also. - PR Review Comment: https://git.openjdk.org/jdk/pull/15163#discussion_r1286185634
Re: RFR: 8312259: StatusResponseManager unused code clean up [v4]
On Mon, 31 Jul 2023 19:09:53 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have the code cleanup update reviewed? With this update, the unused >> code in StatusResponseManager.java will be removed. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > remove unintended blank line The removal of these methods is fine. Many of these were in there to support a different planned approach to populating the SRM's cache, but we went with a simpler approach when it was implemented in JDK 9. If we ever decide to implement a scheduler-based refresh we can always add the methods that we need back. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14924#pullrequestreview-1565627903
Re: RFR: 8312259: StatusResponseManager unused code clean up [v3]
On Wed, 2 Aug 2023 00:45:36 GMT, Xue-Lei Andrew Fan wrote: >> I think @jnimeh should review this, as I think these methods were added when >> implementing OCSP Stapling, and it would be good for him to make sure they >> are no longer needed.. > >> I think @jnimeh should review this, as I think these methods were added when >> implementing OCSP Stapling, and it would be good for him to make sure they >> are no longer needed.. > > @jnimeh Did you have a chance to have a look at this update? @XueleiFan I will have time to review this today. - PR Comment: https://git.openjdk.org/jdk/pull/14924#issuecomment-1662578602
Re: RFR: 8313226: Redundant condition test in X509CRLImpl
On Thu, 27 Jul 2023 04:00:21 GMT, John Jiang wrote:
> if ((nextByte == DerValue.tag_SequenceOf)
> && (! ((nextByte & 0x0c0) == 0x080))) {
> ...
> ...
> }
>
> If `nextByte` is `DerValue.tag_SequenceOf`, exactly `0x30`, then the test
> after `&&` should always be true.
The change itself looks fine to me since bits 8 and 7 will always be zero when
`nextByte` is 0x30. It looks like the second check was to see if the tag was a
context-specific tag, but I don't know why since RFC 5280 doesn't indicate that
it can be context specific. I wonder if it is a remnant from an earlier
version of the code.
Regardless, the second clause isn't doing anything so the removal looks good to
me.
-
Marked as reviewed by jnimeh (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15051#pullrequestreview-1557271462
Re: RFR: 8310629: java/security/cert/CertPathValidator/OCSP/OCSPTimeout.java fails with RuntimeException Server not ready
On Mon, 17 Jul 2023 17:45:56 GMT, Matthew Donovan wrote: > In this PR, i raised the client timeout from 5 to 60 seconds. I considered > refactoring the SimpleOSCPServer class a little but ultimately, the client > needs to just wait until the server is ready or a time-out is reached. > Alternately, we can wait indefinitely and let the test time out but that > results in holding onto resources for too long. > > Thanks I'm good with this. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14905#pullrequestreview-1533717455
Re: RFR: 8310070: Test: javax/net/ssl/DTLS/DTLSWontNegotiateV10.java timed out
On Thu, 13 Jul 2023 17:36:47 GMT, Matthew Donovan wrote:
>> test/jdk/javax/net/ssl/DTLS/DTLSWontNegotiateV10.java line 51:
>>
>>> 49: private static final String DTLSV_1_2 = "DTLSv1.2";
>>> 50:
>>> 51: private static final int READ_TIMEOUT_SECS =
>>> Integer.getInteger("readtimeout", 30);
>>
>> Nit on line 37 (since I can't flag that directly): Maybe add 8310070 to the
>> @bug line.
>
> On previous PRs, I've been told that test-only updates don't need the bug id.
Works for me. Then as far as I'm concerned you're good to go.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14658#discussion_r1262886217
Re: RFR: 8310070: Test: javax/net/ssl/DTLS/DTLSWontNegotiateV10.java timed out
On Mon, 26 Jun 2023 17:38:04 GMT, Matthew Donovan wrote:
> In this PR, I updated the test to use read time-outs. The test is restarted
> if the read operations time-out within (default) 30 seconds. The test makes 5
> attempts before giving up.
Aside from the nit, looks good to me.
test/jdk/javax/net/ssl/DTLS/DTLSWontNegotiateV10.java line 51:
> 49: private static final String DTLSV_1_2 = "DTLSv1.2";
> 50:
> 51: private static final int READ_TIMEOUT_SECS =
> Integer.getInteger("readtimeout", 30);
Nit on line 37 (since I can't flag that directly): Maybe add 8310070 to the
@bug line.
-
Marked as reviewed by jnimeh (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14658#pullrequestreview-1528864782
PR Review Comment: https://git.openjdk.org/jdk/pull/14658#discussion_r1262818408
[jdk21] Integrated: 8309740: Expand timeout windows for tests in JDK-8179502
On Fri, 23 Jun 2023 14:55:45 GMT, Jamil Nimeh wrote: > This is a backport of the test fixes comprising JDK-8309740. This pull request has now been integrated. Changeset: 17b6f7b9 Author: Jamil Nimeh URL: https://git.openjdk.org/jdk21/commit/17b6f7b9a5a14a869d3f1efd0ab51fea4fa40c83 Stats: 21 lines in 3 files changed: 5 ins; 0 del; 16 mod 8309740: Expand timeout windows for tests in JDK-8179502 Reviewed-by: xuelei, hchao Backport-of: 5ca4cdd2caceba9dad8025e5a8851740a3961921 - PR: https://git.openjdk.org/jdk21/pull/58
Re: RFR: 8279254: PKCS9Attribute SigningTime always encoded in UTFTime [v4]
On Fri, 23 Jun 2023 15:20:24 GMT, Ben Perez wrote: >> Added single-argument `putTime` method to `DerOutputStream` that selects the >> correct encoding based on the `Date` value. Similarly, a `getTime` method >> was added to `DerValue` to automatically call the correct decoding function >> based on the date type specified by the `tag`. Furthermore, the `encode` >> method in `PKCS9Attribute` was changed to utilize the new `putTime` method. > > Ben Perez has updated the pull request incrementally with one additional > commit since the last revision: > > Moved UTC date range to constants Looks good to me. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14617#pullrequestreview-1495360483
[jdk21] RFR: 8309740: Expand timeout windows for tests in JDK-8179502
This is a backport of the test fixes comprising JDK-8309740. - Commit messages: - Backport 5ca4cdd2caceba9dad8025e5a8851740a3961921 Changes: https://git.openjdk.org/jdk21/pull/58/files Webrev: https://webrevs.openjdk.org/?repo=jdk21&pr=58&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309740 Stats: 21 lines in 3 files changed: 5 ins; 0 del; 16 mod Patch: https://git.openjdk.org/jdk21/pull/58.diff Fetch: git fetch https://git.openjdk.org/jdk21.git pull/58/head:pull/58 PR: https://git.openjdk.org/jdk21/pull/58
Re: RFR: 8309740: Expand timeout windows for tests in JDK-8179502
On Fri, 16 Jun 2023 18:42:32 GMT, Xue-Lei Andrew Fan wrote: >> This PR is for tests that were modified/added in JDK-8179502. The timeout >> windows for those tests were a little too short on some test systems, >> especially when the system is under heavy load. After a few iterations >> trying out various longer time windows I have a set that should not run into >> issues. > > Marked as reviewed by xuelei (Reviewer). @XueleiFan and/or @MBaesken, would you be able to check out the JDK21 backport (https://github.com/openjdk/jdk21/pull/58)? - PR Comment: https://git.openjdk.org/jdk/pull/14526#issuecomment-1604405990
Integrated: 8309740: Expand timeout windows for tests in JDK-8179502
On Fri, 16 Jun 2023 18:19:45 GMT, Jamil Nimeh wrote: > This PR is for tests that were modified/added in JDK-8179502. The timeout > windows for those tests were a little too short on some test systems, > especially when the system is under heavy load. After a few iterations > trying out various longer time windows I have a set that should not run into > issues. This pull request has now been integrated. Changeset: 5ca4cdd2 Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/5ca4cdd2caceba9dad8025e5a8851740a3961921 Stats: 21 lines in 3 files changed: 5 ins; 0 del; 16 mod 8309740: Expand timeout windows for tests in JDK-8179502 Reviewed-by: xuelei, mbaesken - PR: https://git.openjdk.org/jdk/pull/14526
Re: RFR: 8279254: PKCS9Attribute SigningTime always encoded in UTFTime [v3]
On Thu, 22 Jun 2023 23:22:14 GMT, Ben Perez wrote:
>> Added single-argument `putTime` method to `DerOutputStream` that selects the
>> correct encoding based on the `Date` value. Similarly, a `getTime` method
>> was added to `DerValue` to automatically call the correct decoding function
>> based on the date type specified by the `tag`. Furthermore, the `encode`
>> method in `PKCS9Attribute` was changed to utilize the new `putTime` method.
>
> Ben Perez has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Fixed epoch literals to be in milliseconds
> - Updated copyright in DerInputStream
src/java.base/share/classes/sun/security/util/DerOutputStream.java line 490:
> 488: public DerOutputStream putTime(Date d) {
> 489: Date low = new Date(-63115200L); // Dates before 1/1/1950
> 490: Date high = new Date(2524607999000L); // Dates after 12/31/2049
Could these be `private static final` since they are constants? It would avoid
the extra constructor calls each time this method is called.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14617#discussion_r1239478167
Re: RFR: 8279254: PKCS9Attribute SigningTime always encoded in UTFTime [v2]
On Thu, 22 Jun 2023 19:50:10 GMT, Jamil Nimeh wrote: >> Ben Perez has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Replaced depreciated ctor in putTime. Added getTime method to >> DerInputStream > > test/jdk/sun/security/util/DerOutputStream/DerTimeEncoding.java line 40: > >> 38: //Check that dates after 2050 use GeneralizedTime >> 39: DerOutputStream out = new DerOutputStream(); >> 40: Date generalizedTimeDate = new Date(2055,3,17); > > Same comment as above for both this line and line 50. Don't forget to change these to use the long-based `Date` constructor, also in milliseconds. - PR Review Comment: https://git.openjdk.org/jdk/pull/14617#discussion_r1239108332
Re: RFR: 8279254: PKCS9Attribute SigningTime always encoded in UTFTime [v2]
On Thu, 22 Jun 2023 21:21:14 GMT, Ben Perez wrote:
>> Added single-argument `putTime` method to `DerOutputStream` that selects the
>> correct encoding based on the `Date` value. Similarly, a `getTime` method
>> was added to `DerValue` to automatically call the correct decoding function
>> based on the date type specified by the `tag`. Furthermore, the `encode`
>> method in `PKCS9Attribute` was changed to utilize the new `putTime` method.
>
> Ben Perez has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Replaced depreciated ctor in putTime. Added getTime method to DerInputStream
src/java.base/share/classes/sun/security/util/DerOutputStream.java line 490:
> 488: public DerOutputStream putTime(Date d) {
> 489: Date low = new Date(-631152000L); // Dates before 1/1/1950
> 490: Date high = new Date(2524607999L); // Dates after 12/31/2049
These two values should be in milliseconds.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14617#discussion_r1239107752
Re: RFR: 8279254: PKCS9Attribute SigningTime always encoded in UTFTime
On Thu, 22 Jun 2023 18:45:14 GMT, Ben Perez wrote:
> Added single-argument `putTime` method to `DerOutputStream` that selects the
> correct encoding based on the `Date` value. Similarly, a `getTime` method was
> added to `DerValue` to automatically call the correct decoding function based
> on the date type specified by the `tag`. Furthermore, the `encode` method in
> `PKCS9Attribute` was changed to utilize the new `putTime` method.
src/java.base/share/classes/sun/security/util/DerOutputStream.java line 490:
> 488: public DerOutputStream putTime(Date d) {
> 489: @SuppressWarnings("deprecation")
> 490: Date low = new Date(1950,1,1);
Isn't this form of the `Date` constructor deprecated? Alternately you could
convert 1/1/1950 into epoch time as a long and use `new Date(long)` which is
not deprecated. Same for line 492.
test/jdk/sun/security/util/DerOutputStream/DerTimeEncoding.java line 40:
> 38: //Check that dates after 2050 use GeneralizedTime
> 39: DerOutputStream out = new DerOutputStream();
> 40: Date generalizedTimeDate = new Date(2055,3,17);
Same comment as above for both this line and line 50.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14617#discussion_r1238963194
PR Review Comment: https://git.openjdk.org/jdk/pull/14617#discussion_r1238964501
RFR: 8309740: Expand timeout windows for tests in JDK-8179502
This PR is for tests that were modified/added in JDK-8179502. The timeout windows for those tests were a little too short on some test systems, especially when the system is under heavy load. After a few iterations trying out various longer time windows I have a set that should not run into issues. - Commit messages: - 8309740: Expand timeout windows for tests in JDK-8179502 Changes: https://git.openjdk.org/jdk/pull/14526/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14526&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309740 Stats: 21 lines in 3 files changed: 5 ins; 0 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/14526.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14526/head:pull/14526 PR: https://git.openjdk.org/jdk/pull/14526
Re: RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v4]
On Fri, 16 Jun 2023 14:52:44 GMT, Xue-Lei Andrew Fan wrote: >> There is a difference, though. The close() method in SSLSocketImpl is not >> synchronized and there is the chance of a NullPointerException in >> duplexCloseOutput() because `conContext.handshakeContext` is being set to >> null by the TransportContext class in a different thread. I think that can >> affect any method in SSLSocketImpl that accesses that field. > > Yes, socket close is a headache problem for me. There's a bit of a history with SSLSocket closures since the new handshaker was brought into JDK11. Some of it dealt with synchronization, others with properly handling full vs. half-duplex closes. You may want to look up some of the bug history on changes surrounding the SSLSocket closures just to make sure we don't undo any fixes that were previously integrated. As Xuelei's comment indicates, this is a delicate part of the handshaker. - PR Review Comment: https://git.openjdk.org/jdk/pull/13742#discussion_r1232367902
Re: RFR: 8290005: com/sun/jndi/ldap/LdapCBPropertiesTest.java failling with NullPointerException [v3]
On Wed, 3 May 2023 11:26:32 GMT, Matthew Donovan wrote: >> In this PR, I added methods to the TransportContext class to synchronize >> access to the handshakeContext field. I also updated locations in the code >> that rely on the handshakeContext field to not be null to use the >> synchronized methods. >> >> Thanks > > Matthew Donovan has updated the pull request incrementally with one > additional commit since the last revision: > > made handshake context lock final This looks good to me. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13742#pullrequestreview-1482759220
Re: RFR: 8307144: namedParams in XECParameters and EdDSAParameters can be private final
On Thu, 25 May 2023 21:17:40 GMT, Ben Perez wrote: > Changed `namedParams` in XECParameters and EdDSAParameters to be `private > final` Looks good to me. Just a minor nit in each file: The copyrights on line 2 in each file could be updated to 2023. - Marked as reviewed by jnimeh (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14162#pullrequestreview-1466560004
Integrated: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts
On Tue, 2 May 2023 21:12:31 GMT, Jamil Nimeh wrote: > This set of enhancements extends the allowed syntax for the > `com.sun.security.ocsp.timeout`, `com.sun.security.crl.timeout` and > `com.sun.security.crl.readtimeout` System properties. These properties > retain their current behavior where a purely numeric value is interpreted in > seconds, but now the numeric value may also be appended with "ms" > (case-insensitive) to be interpreted as milliseconds. > > This enhancement also adds two new System properties: > `com.sun.security.cert.timeout` and `com.sun.security.cert.readtimeout` which > follow the same new allowed syntax. These timeouts only come into play when > an AIA extension on a certificate is followed for pulling the issuing > authority certificate and only when the `com.sun.security.enableAIAcaIssuers` > property is true (default false). > > JBS: https://bugs.openjdk.org/browse/JDK-8179502 > CSR: https://bugs.openjdk.org/browse/JDK-8300722 This pull request has now been integrated. Changeset: 2836c34b Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/2836c34b64e4626e25c86a53e5bef2bf32f95d2e Stats: 913 lines in 7 files changed: 795 ins; 25 del; 93 mod 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts Reviewed-by: mullan - PR: https://git.openjdk.org/jdk/pull/13762
Integrated: 8305091: Change ChaCha20 cipher init behavior to match AES-GCM
On Tue, 11 Apr 2023 17:26:25 GMT, Jamil Nimeh wrote: > This fixes an issue where the key/nonce reuse policy for SunJCE ChaCha20 and > ChaCha20-Poly1305 was overly strict in enforcing no-reuse when the Cipher was > in DECRYPT_MODE. For decryption, this should be allowed and be consistent > with the AES-GCM decryption initialization behavior. > > - Issue: https://bugs.openjdk.org/browse/JDK-8305091 > - CSR: https://bugs.openjdk.org/browse/JDK-8305822 This pull request has now been integrated. Changeset: bb0ff48a Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/bb0ff48aa94c4648a2f929226dd8d252431bcd03 Stats: 77 lines in 2 files changed: 31 ins; 14 del; 32 mod 8305091: Change ChaCha20 cipher init behavior to match AES-GCM Reviewed-by: djelinski, ascarpino - PR: https://git.openjdk.org/jdk/pull/13428
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v4]
On Mon, 22 May 2023 17:39:59 GMT, Jamil Nimeh wrote: >> src/java.base/share/classes/sun/security/provider/certpath/URICertStore.java >> line 131: >> >>> 129: private static final int DEFAULT_CRL_READ_TIMEOUT = 15000; >>> 130: >>> 131: // Default connect and read timeouts for CA certificate fetching >>> (15 sec) >> >> Does 15 seconds make sense as the default timeout, especially for certs? >> CRLs are generally larger than certs, so a longer read timeout makes sense. >> >> I'm ok with keeping these default values the same for consistency, but I >> think we should re-evaluate each of these default timeouts and compare them >> to other products/technologies to see if some adjustments may be needed - >> can you file a follow-on RFE for that? > > Yes, I can make a follow on for that. Filed https://bugs.openjdk.org/browse/JDK-8308601 - PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1201306717
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v5]
> This set of enhancements extends the allowed syntax for the > `com.sun.security.ocsp.timeout`, `com.sun.security.crl.timeout` and > `com.sun.security.crl.readtimeout` System properties. These properties > retain their current behavior where a purely numeric value is interpreted in > seconds, but now the numeric value may also be appended with "ms" > (case-insensitive) to be interpreted as milliseconds. > > This enhancement also adds two new System properties: > `com.sun.security.cert.timeout` and `com.sun.security.cert.readtimeout` which > follow the same new allowed syntax. These timeouts only come into play when > an AIA extension on a certificate is followed for pulling the issuing > authority certificate and only when the `com.sun.security.enableAIAcaIssuers` > property is true (default false). > > JBS: https://bugs.openjdk.org/browse/JDK-8179502 > CSR: https://bugs.openjdk.org/browse/JDK-8300722 Jamil Nimeh 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 eight additional commits since the last revision: - Add additional debug message in timeout property parser - Merge with main - Use privilegedGetProperty, catch NFE following string match - Add OCSP readtimeout property - Add 's' suffix to allowed syntax - Fix more whitespace errors - Fix whitespace errors - 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts - Changes: - all: https://git.openjdk.org/jdk/pull/13762/files - new: https://git.openjdk.org/jdk/pull/13762/files/e73818ef..659da859 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13762&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13762&range=03-04 Stats: 183453 lines in 3181 files changed: 133144 ins; 26618 del; 23691 mod Patch: https://git.openjdk.org/jdk/pull/13762.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13762/head:pull/13762 PR: https://git.openjdk.org/jdk/pull/13762
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v4]
On Mon, 22 May 2023 17:17:26 GMT, Sean Mullan wrote: >> Jamil Nimeh has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Use privilegedGetProperty, catch NFE following string match > > src/java.base/share/classes/sun/security/action/GetPropertyAction.java line > 211: > >> 209: " for timeout value " + propVal + >> 210: ". Using default value of " + def + " >> msec."); >> 211: } > > This would also be useful debug for the else case on line 214-216 if the > value is not an integer. Sounds good, I can add that. > src/java.base/share/classes/sun/security/provider/certpath/URICertStore.java > line 131: > >> 129: private static final int DEFAULT_CRL_READ_TIMEOUT = 15000; >> 130: >> 131: // Default connect and read timeouts for CA certificate fetching >> (15 sec) > > Does 15 seconds make sense as the default timeout, especially for certs? CRLs > are generally larger than certs, so a longer read timeout makes sense. > > I'm ok with keeping these default values the same for consistency, but I > think we should re-evaluate each of these default timeouts and compare them > to other products/technologies to see if some adjustments may be needed - can > you file a follow-on RFE for that? Yes, I can make a follow on for that. - PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1200831410 PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1200832770
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v2]
On Tue, 9 May 2023 14:59:36 GMT, Sean Mullan wrote: >> Jamil Nimeh has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add 's' suffix to allowed syntax > > I think you should also apply the cert and CRL timeouts to the > `LDAPCertStore` implementation, using the JNDI properties: > `com.sun.jndi.ldap.connect.timeout` and `com.sun.jndi.ldap.read.timeout`. @seanjmullan if you have a chance would you mind taking a quick look at the release note for this change? (https://bugs.openjdk.org/browse/JDK-8308582) It is pretty close to the CSR solution text, but I tried to trim it down a little. It's still pretty long, but there's a lot to convey. - PR Comment: https://git.openjdk.org/jdk/pull/13762#issuecomment-1557618613
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v4]
> This set of enhancements extends the allowed syntax for the > `com.sun.security.ocsp.timeout`, `com.sun.security.crl.timeout` and > `com.sun.security.crl.readtimeout` System properties. These properties > retain their current behavior where a purely numeric value is interpreted in > seconds, but now the numeric value may also be appended with "ms" > (case-insensitive) to be interpreted as milliseconds. > > This enhancement also adds two new System properties: > `com.sun.security.cert.timeout` and `com.sun.security.cert.readtimeout` which > follow the same new allowed syntax. These timeouts only come into play when > an AIA extension on a certificate is followed for pulling the issuing > authority certificate and only when the `com.sun.security.enableAIAcaIssuers` > property is true (default false). > > JBS: https://bugs.openjdk.org/browse/JDK-8179502 > CSR: https://bugs.openjdk.org/browse/JDK-8300722 Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision: Use privilegedGetProperty, catch NFE following string match - Changes: - all: https://git.openjdk.org/jdk/pull/13762/files - new: https://git.openjdk.org/jdk/pull/13762/files/81c7cb35..e73818ef Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13762&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13762&range=02-03 Stats: 20 lines in 3 files changed: 12 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/13762.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13762/head:pull/13762 PR: https://git.openjdk.org/jdk/pull/13762
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v3]
On Mon, 22 May 2023 15:58:14 GMT, Sean Mullan wrote:
>> Jamil Nimeh has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add OCSP readtimeout property
>
> src/java.base/share/classes/sun/security/action/GetPropertyAction.java line
> 186:
>
>> 184: }
>> 185:
>> 186: String propVal = System.getProperty(prop, "").trim();
>
> You should call `privilegedGetProperty` here instead of `System.getProperty`
> so the call is wrapped in `doPrivileged` when an SM is active.
Good catch. Will fix.
> src/java.base/share/classes/sun/security/action/GetPropertyAction.java line
> 202:
>
>> 200: // Next check to make sure the string is built only from digits
>> 201: if (propVal.matches("^\\d+$")) {
>> 202: int timeout = Integer.parseInt(propVal);
>
> Is this guaranteed never to throw `NumberFormatException`? It might be safer
> to catch it just in case.
I'll change this to catch NFE, but I'm pretty sure the pattern will only ever
return on true if the string is comprised solely of digits from start to end -
I could never get a string that would pass when it shouldn't. But point taken,
better safe than sorry.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1200729759
PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1200728908
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v2]
On Tue, 9 May 2023 15:56:02 GMT, Jamil Nimeh wrote: >> Yes, I noticed that too. I wasn't sure if we needed to make a change there. >> I opted to leave well-enough alone since nobody was asking for it and it's >> one less property to keep track of. All of these property sets end up with >> a max latency of connect-timeout + read-timeout, and by default they are set >> to the same values. So in practice much of the time they are all 2x. >> >> It's easy enough I think to make a separate property for >> `com.sun.security.ocsp.readtimeout` and then the existing `.timeout` >> property would be for connect timeouts (keeping in line with the other >> props). I don't think it will introduce significant risk but I will >> highlight that in the CSR. > >> I think you should also apply the cert and CRL timeouts to the >> `LDAPCertStore` implementation, using the JNDI properties: >> `com.sun.jndi.ldap.connect.timeout` and `com.sun.jndi.ldap.read.timeout`. > > I will look into this. I've added the OCSP readtimeout property, seems to be working well. As discussed offline we'll hold off on the LDAP changes for now. - PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1199323604
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v3]
> This set of enhancements extends the allowed syntax for the > `com.sun.security.ocsp.timeout`, `com.sun.security.crl.timeout` and > `com.sun.security.crl.readtimeout` System properties. These properties > retain their current behavior where a purely numeric value is interpreted in > seconds, but now the numeric value may also be appended with "ms" > (case-insensitive) to be interpreted as milliseconds. > > This enhancement also adds two new System properties: > `com.sun.security.cert.timeout` and `com.sun.security.cert.readtimeout` which > follow the same new allowed syntax. These timeouts only come into play when > an AIA extension on a certificate is followed for pulling the issuing > authority certificate and only when the `com.sun.security.enableAIAcaIssuers` > property is true (default false). > > JBS: https://bugs.openjdk.org/browse/JDK-8179502 > CSR: https://bugs.openjdk.org/browse/JDK-8300722 Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision: Add OCSP readtimeout property - Changes: - all: https://git.openjdk.org/jdk/pull/13762/files - new: https://git.openjdk.org/jdk/pull/13762/files/3c524e17..81c7cb35 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13762&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13762&range=01-02 Stats: 26 lines in 2 files changed: 14 ins; 3 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/13762.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13762/head:pull/13762 PR: https://git.openjdk.org/jdk/pull/13762
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v2]
On Tue, 9 May 2023 15:55:24 GMT, Jamil Nimeh wrote: >> src/java.base/share/classes/sun/security/provider/certpath/OCSP.java line 1: >> >>> 1: /* >> >> I see there is no way to individually control the OCSP read and connect >> timeouts like there is for certs and CRLs. Perhaps this isn't as big an >> issue, but when you set the OCSP timeout, it really means 2x what you set. > > Yes, I noticed that too. I wasn't sure if we needed to make a change there. > I opted to leave well-enough alone since nobody was asking for it and it's > one less property to keep track of. All of these property sets end up with a > max latency of connect-timeout + read-timeout, and by default they are set to > the same values. So in practice much of the time they are all 2x. > > It's easy enough I think to make a separate property for > `com.sun.security.ocsp.readtimeout` and then the existing `.timeout` property > would be for connect timeouts (keeping in line with the other props). I > don't think it will introduce significant risk but I will highlight that in > the CSR. > I think you should also apply the cert and CRL timeouts to the > `LDAPCertStore` implementation, using the JNDI properties: > `com.sun.jndi.ldap.connect.timeout` and `com.sun.jndi.ldap.read.timeout`. I will look into this. - PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1188817169
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v2]
On Tue, 9 May 2023 15:01:29 GMT, Sean Mullan wrote: >> Jamil Nimeh has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add 's' suffix to allowed syntax > > src/java.base/share/classes/sun/security/provider/certpath/OCSP.java line 1: > >> 1: /* > > I see there is no way to individually control the OCSP read and connect > timeouts like there is for certs and CRLs. Perhaps this isn't as big an > issue, but when you set the OCSP timeout, it really means 2x what you set. Yes, I noticed that too. I wasn't sure if we needed to make a change there. I opted to leave well-enough alone since nobody was asking for it and it's one less property to keep track of. All of these property sets end up with a max latency of connect-timeout + read-timeout, and by default they are set to the same values. So in practice much of the time they are all 2x. It's easy enough I think to make a separate property for `com.sun.security.ocsp.readtimeout` and then the existing `.timeout` property would be for connect timeouts (keeping in line with the other props). I don't think it will introduce significant risk but I will highlight that in the CSR. - PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1188816429
Re: RFR: 8305169: java/security/cert/CertPathValidator/OCSP/GetAndPostTests.java -- test server didn't start in timely manner
On Fri, 5 May 2023 11:27:48 GMT, Matthew Donovan wrote: > Could someone please review this PR? It is a small change to increase the > time that the client waits for the server thread to start. > > Thanks! Marked as reviewed by jnimeh (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13829#pullrequestreview-1414857598
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts [v2]
> This set of enhancements extends the allowed syntax for the > `com.sun.security.ocsp.timeout`, `com.sun.security.crl.timeout` and > `com.sun.security.crl.readtimeout` System properties. These properties > retain their current behavior where a purely numeric value is interpreted in > seconds, but now the numeric value may also be appended with "ms" > (case-insensitive) to be interpreted as milliseconds. > > This enhancement also adds two new System properties: > `com.sun.security.cert.timeout` and `com.sun.security.cert.readtimeout` which > follow the same new allowed syntax. These timeouts only come into play when > an AIA extension on a certificate is followed for pulling the issuing > authority certificate and only when the `com.sun.security.enableAIAcaIssuers` > property is true (default false). > > JBS: https://bugs.openjdk.org/browse/JDK-8179502 > CSR: https://bugs.openjdk.org/browse/JDK-8300722 Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision: Add 's' suffix to allowed syntax - Changes: - all: https://git.openjdk.org/jdk/pull/13762/files - new: https://git.openjdk.org/jdk/pull/13762/files/3e0f5822..3c524e17 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13762&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13762&range=00-01 Stats: 17 lines in 4 files changed: 7 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/13762.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13762/head:pull/13762 PR: https://git.openjdk.org/jdk/pull/13762
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts
On Wed, 3 May 2023 00:27:55 GMT, Weijun Wang wrote: >> Well, all the existing documentation already states that they are in >> seconds. That was why I didn't add any additional suffixes. The goal was >> to make it so folks don't need to make any changes if the existing >> seconds-level granularity is sufficient for them. > > I don't mean not to support bare numbers. It's just a little unfair that > millisecond has a suffix but second does not. We can support all of "1", > "1s", and "1000ms". Okay, we can make that change. - PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1183212032
Re: RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts
On Tue, 2 May 2023 22:33:47 GMT, Weijun Wang wrote:
>> This set of enhancements extends the allowed syntax for the
>> `com.sun.security.ocsp.timeout`, `com.sun.security.crl.timeout` and
>> `com.sun.security.crl.readtimeout` System properties. These properties
>> retain their current behavior where a purely numeric value is interpreted in
>> seconds, but now the numeric value may also be appended with "ms"
>> (case-insensitive) to be interpreted as milliseconds.
>>
>> This enhancement also adds two new System properties:
>> `com.sun.security.cert.timeout` and `com.sun.security.cert.readtimeout`
>> which follow the same new allowed syntax. These timeouts only come into
>> play when an AIA extension on a certificate is followed for pulling the
>> issuing authority certificate and only when the
>> `com.sun.security.enableAIAcaIssuers` property is true (default false).
>>
>> JBS: https://bugs.openjdk.org/browse/JDK-8179502
>> CSR: https://bugs.openjdk.org/browse/JDK-8300722
>
> src/java.base/share/classes/sun/security/action/GetPropertyAction.java line
> 192:
>
>> 190:
>> 191: // Determine if "ms" is on the end of the string
>> 192: boolean isMillis = propVal.toLowerCase().endsWith("ms");
>
> Shall we allow the `s` suffix as well? This makes it clear that a value is in
> seconds.
Well, all the existing documentation already states that they are in seconds.
That was why I didn't add any additional suffixes. The goal was to make it so
folks don't need to make any changes if the existing seconds-level granularity
is sufficient for them.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13762#discussion_r1183129362
RFR: 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts
This set of enhancements extends the allowed syntax for the `com.sun.security.ocsp.timeout`, `com.sun.security.crl.timeout` and `com.sun.security.crl.readtimeout` System properties. These properties retain their current behavior where a purely numeric value is interpreted in seconds, but now the numeric value may also be appended with "ms" (case-insensitive) to be interpreted as milliseconds. This enhancement also adds two new System properties: `com.sun.security.cert.timeout` and `com.sun.security.cert.readtimeout` which follow the same new allowed syntax. These timeouts only come into play when an AIA extension on a certificate is followed for pulling the issuing authority certificate and only when the `com.sun.security.enableAIAcaIssuers` property is true (default false). JBS: https://bugs.openjdk.org/browse/JDK-8179502 CSR: https://bugs.openjdk.org/browse/JDK-8300722 - Commit messages: - Fix more whitespace errors - Fix whitespace errors - 8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts Changes: https://git.openjdk.org/jdk/pull/13762/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13762&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8179502 Stats: 873 lines in 7 files changed: 759 ins; 27 del; 87 mod Patch: https://git.openjdk.org/jdk/pull/13762.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13762/head:pull/13762 PR: https://git.openjdk.org/jdk/pull/13762
Re: RFR: 8305091: Change ChaCha20 cipher init behavior to match AES-GCM
On Tue, 11 Apr 2023 18:15:22 GMT, Xue-Lei Andrew Fan wrote: >> This fixes an issue where the key/nonce reuse policy for SunJCE ChaCha20 and >> ChaCha20-Poly1305 was overly strict in enforcing no-reuse when the Cipher >> was in DECRYPT_MODE. For decryption, this should be allowed and be >> consistent with the AES-GCM decryption initialization behavior. >> >> - Issue: https://bugs.openjdk.org/browse/JDK-8305091 >> - CSR: https://bugs.openjdk.org/browse/JDK-8305822 > > In the decryption side, does it sound like good to detect and reject > key/nonce reuse for security reason(i.e., if key/nonce is reused, the > decryption side will not accept the encryption)? Did you known real problems > in practice for the key/nonce reuse for decryption? @XueleiFan I think the key/nonce reuse protections are there to prevent sending ciphertexts out that are created from XORing against the same value twice. But that is an encryption use case. Decryption is a different story, since you're consuming the ciphertext and not emitting data out on the wire. The decrypting entity might use that data, but that doesn't leak information to a passive observer. There are cases where people using the ChaCha20 and ChaCha20-Poly1305 ciphers have run into behavioral issues because they operate differently than AES-GCM does. It seems sensible that the ciphers should be unified in their behavior. - PR Comment: https://git.openjdk.org/jdk/pull/13428#issuecomment-1503878118
RFR: 8305091: Change ChaCha20 cipher init behavior to match AES-GCM
This fixes an issue where the key/nonce reuse policy for SunJCE ChaCha20 and ChaCha20-Poly1305 was overly strict in enforcing no-reuse when the Cipher was in DECRYPT_MODE. For decryption, this should be allowed and be consistent with the AES-GCM decryption initialization behavior. - Issue: https://bugs.openjdk.org/browse/JDK-8305091 - CSR: https://bugs.openjdk.org/browse/JDK-8305822 - Commit messages: - 8305091: Change ChaCha20 cipher init behavior to match AES-GCM Changes: https://git.openjdk.org/jdk/pull/13428/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13428&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8305091 Stats: 77 lines in 2 files changed: 31 ins; 14 del; 32 mod Patch: https://git.openjdk.org/jdk/pull/13428.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13428/head:pull/13428 PR: https://git.openjdk.org/jdk/pull/13428
Re: RFR: 8182621: JSSE should reject empty TLS plaintexts [v2]
On Mon, 10 Apr 2023 21:12:38 GMT, Xue-Lei Andrew Fan wrote: >> Matthew Donovan has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - added comment referring to relevant RFC >> - clarified if-statements; fixed exception message wording > > @mpdonova Did you have a chance to pass Mach5 testing? If the testing is > good, I would like to sponsor the committing. @XueleiFan Yes, @mpdonova did have successful mach5 runs. I had a similar question I asked him directly and I've reviewed the results. jdk_security test groups are clean. - PR Comment: https://git.openjdk.org/jdk/pull/13306#issuecomment-1502467293
Integrated: 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors
On Wed, 1 Feb 2023 18:10:41 GMT, Jamil Nimeh wrote: > Hello all, > > This addresses a test bug where the SimpleOCSPServer would reset the > connections made by a client CertPathValidator. I've made some minor changes > to how the network data is read and sent from OCSP HTTP GET URLs and on > responses, respectively. This will take the test off the problem list as > well. > > This has been taken through hundreds of test runs and does not see the > failure any longer where there used to be intermittent failures. Also > multiple tier2 runs have been executed with no failures. > > - JBS: https://bugs.openjdk.org/browse/JDK-8300939 This pull request has now been integrated. Changeset: da044dd5 Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/da044dd5698d14eccd2a30a24cc691e30fa00cbd Stats: 55 lines in 3 files changed: 39 ins; 2 del; 14 mod 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors Reviewed-by: djelinski, weijun - PR: https://git.openjdk.org/jdk/pull/12370
Re: RFR: 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors [v3]
On Tue, 14 Mar 2023 15:01:07 GMT, Weijun Wang wrote:
>> Jamil Nimeh has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 11 commits:
>>
>> - merge with main
>> - merge with main
>> - 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java
>> fails due to network errors
>> - Merge with main
>> - Restore policy Root.java lost during merge
>> - Merge with main
>> - 8300946: Add sun/security/provider/certpath/OCSP/OCSPNoContentLength to
>> ProblemList
>> - Remove dead commented code
>> - Throw exception directly from non 200 HTTP response codes
>> - Moved SimpleOCSPServer to use CountdownLatch for ready state, updated
>> tests
>> - ... and 1 more: https://git.openjdk.org/jdk/compare/55aa1224...b2d25b7e
>
> test/jdk/java/security/testlibrary/SimpleOCSPServer.java line 340:
>
>> 338: */
>> 339: private static String dumpHexBytes(byte[] data, int dataLen,
>> 340: int itemsPerLine, String lineDelim, String itemDelim) {
>
> You always call with `dataLen = data.length`. Is it still necessary to add
> this argument?
Yes, I would prefer to keep this. I made the change in order to help me debug
draining the input stream and I felt it prudent to leave it in place, along
with the main test's debug flag just in case.
-
PR: https://git.openjdk.org/jdk/pull/12370
Re: RFR: 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors [v3]
> Hello all, > > This addresses a test bug where the SimpleOCSPServer would reset the > connections made by a client CertPathValidator. I've made some minor changes > to how the network data is read and sent from OCSP HTTP GET URLs and on > responses, respectively. This will take the test off the problem list as > well. > > This has been taken through hundreds of test runs and does not see the > failure any longer where there used to be intermittent failures. Also > multiple tier2 runs have been executed with no failures. > > - JBS: https://bugs.openjdk.org/browse/JDK-8300939 Jamil Nimeh has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - merge with main - merge with main - 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors - Merge with main - Restore policy Root.java lost during merge - Merge with main - 8300946: Add sun/security/provider/certpath/OCSP/OCSPNoContentLength to ProblemList - Remove dead commented code - Throw exception directly from non 200 HTTP response codes - Moved SimpleOCSPServer to use CountdownLatch for ready state, updated tests - ... and 1 more: https://git.openjdk.org/jdk/compare/55aa1224...b2d25b7e - Changes: https://git.openjdk.org/jdk/pull/12370/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12370&range=02 Stats: 55 lines in 3 files changed: 39 ins; 2 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/12370.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12370/head:pull/12370 PR: https://git.openjdk.org/jdk/pull/12370
Re: RFR: 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors [v2]
On Fri, 3 Feb 2023 17:14:41 GMT, Mark Powers wrote: > It's not clear to me what is causing this test failure, but your fix is to > drain the input stream and flush the output? Yes, that's the gist of it. I didn't realize originally that I was leaving unread data in the input stream, so draining that and making sure everything was flushed definitely improved things. The other changes were just minor extras (additional logging, etc.) that made sense while I was in this class. > test/jdk/sun/security/provider/certpath/OCSP/OCSPNoContentLength.java line 59: > >> 57: >> 58: // Enable debugging for additional output >> 59: static final boolean debug = true; > > Do you intend to leave this `true`? Yes, I would like to leave this in the off chance this does rear its ugly head again. I've done hundreds upon hundreds of iterations of this test both by itself and as part of tier2 runs and no failures occur. If it did fail though, I'd like to have the extra logging. It doesn't add very much to the output, but the info is useful. - PR: https://git.openjdk.org/jdk/pull/12370
Re: RFR: 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors [v2]
> Hello all, > > This addresses a test bug where the SimpleOCSPServer would reset the > connections made by a client CertPathValidator. I've made some minor changes > to how the network data is read and sent from OCSP HTTP GET URLs and on > responses, respectively. This will take the test off the problem list as > well. > > This has been taken through hundreds of test runs and does not see the > failure any longer where there used to be intermittent failures. Also > multiple tier2 runs have been executed with no failures. > > - JBS: https://bugs.openjdk.org/browse/JDK-8300939 Jamil Nimeh has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits: - merge with main - 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors - Merge with main - Restore policy Root.java lost during merge - Merge with main - 8300946: Add sun/security/provider/certpath/OCSP/OCSPNoContentLength to ProblemList - Remove dead commented code - Throw exception directly from non 200 HTTP response codes - Moved SimpleOCSPServer to use CountdownLatch for ready state, updated tests - 8296343: CPVE thrown on missing content-length in OCSP response - Changes: https://git.openjdk.org/jdk/pull/12370/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12370&range=01 Stats: 55 lines in 3 files changed: 39 ins; 2 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/12370.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12370/head:pull/12370 PR: https://git.openjdk.org/jdk/pull/12370
Re: RFR: 8301299: Wrong class spec on sun.security.util.Pem
On Wed, 1 Feb 2023 19:25:54 GMT, Weijun Wang wrote: > A trivial doc-only change. Marked as reviewed by jnimeh (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12373
RFR: 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors
Hello all, This addresses a test bug where the SimpleOCSPServer would reset the connections made by a client CertPathValidator. I've made some minor changes to how the network data is read and sent from OCSP HTTP GET URLs and on responses, respectively. This will take the test off the problem list as well. This has been taken through hundreds of test runs and does not see the failure any longer where there used to be intermittent failures. Also multiple tier2 runs have been executed with no failures. - JBS: https://bugs.openjdk.org/browse/JDK-8300939 - Commit messages: - 8300939: sun/security/provider/certpath/OCSP/OCSPNoContentLength.java fails due to network errors - Merge with main - Restore policy Root.java lost during merge - Merge with main - 8300946: Add sun/security/provider/certpath/OCSP/OCSPNoContentLength to ProblemList - Remove dead commented code - Throw exception directly from non 200 HTTP response codes - Moved SimpleOCSPServer to use CountdownLatch for ready state, updated tests - 8296343: CPVE thrown on missing content-length in OCSP response Changes: https://git.openjdk.org/jdk/pull/12370/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12370&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300939 Stats: 55 lines in 3 files changed: 39 ins; 2 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/12370.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12370/head:pull/12370 PR: https://git.openjdk.org/jdk/pull/12370
Integrated: 8300946: Add sun/security/provider/certpath/OCSP/OCSPNoContentLength to ProblemList
On Tue, 24 Jan 2023 01:50:52 GMT, Jamil Nimeh wrote: > This adds the OCSPNoContentLength test recently added as part of JDK-8296343. > The test exposes what looks to be an intermittent socket closure issue in > SimpleOCSPServer. In order to take the time to solve it properly, I'd like > to add this test to the problem list. > > - JBS: https://bugs.openjdk.org/browse/JDK-8300946 > - See also: https://bugs.openjdk.org/browse/JDK-8300939 This pull request has now been integrated. Changeset: 2da2e5a0 Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/2da2e5a0e80e6c54c848cf39bee534fa9b7086b1 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8300946: Add sun/security/provider/certpath/OCSP/OCSPNoContentLength to ProblemList Reviewed-by: jpai - PR: https://git.openjdk.org/jdk/pull/12156
RFR: 8300946: Add sun/security/provider/certpath/OCSP/OCSPNoContentLength to ProblemList
This adds the OCSPNoContentLength test recently added as part of JDK-8296343. The test exposes what looks to be an intermittent socket closure issue in SimpleOCSPServer. In order to take the time to solve it properly, I'd like to add this test to the problem list. - JBS: https://bugs.openjdk.org/browse/JDK-8300946 - See also: https://bugs.openjdk.org/browse/JDK-8300939 - Commit messages: - Restore policy Root.java lost during merge - Merge with main - 8300946: Add sun/security/provider/certpath/OCSP/OCSPNoContentLength to ProblemList - Remove dead commented code - Throw exception directly from non 200 HTTP response codes - Moved SimpleOCSPServer to use CountdownLatch for ready state, updated tests - 8296343: CPVE thrown on missing content-length in OCSP response Changes: https://git.openjdk.org/jdk/pull/12156/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12156&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300946 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12156.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12156/head:pull/12156 PR: https://git.openjdk.org/jdk/pull/12156
Integrated: 8296343: CPVE thrown on missing content-length in OCSP response
On Tue, 10 Jan 2023 06:02:29 GMT, Jamil Nimeh wrote: > This fixes an issue where HTTP responses that do not have an explicit > Content-Length are causing an EOFException which unravels into a > CertPathValidatorException during validations that involve OCSP checks. > > - JBS: https://bugs.openjdk.org/browse/JDK-8296343 This pull request has now been integrated. Changeset: 1a3cb8c5 Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/1a3cb8c5018bc016c2ad6b078e4abe13b39d151c Stats: 399 lines in 9 files changed: 283 ins; 37 del; 79 mod 8296343: CPVE thrown on missing content-length in OCSP response Reviewed-by: mullan, rhalade - PR: https://git.openjdk.org/jdk/pull/11917
Re: RFR: 8297972: Poly1305 Endianness on ByteBuffer not enforced [v2]
On Thu, 19 Jan 2023 22:13:07 GMT, Volodymyr Paprotski wrote: >> Looks good to me. > > @jnimeh would you mind running this through your tests? The build failures > reported above seem unrelated.. @vpaprotsk regression tests completed successfully on my end. - PR: https://git.openjdk.org/jdk/pull/11463
CSR RFR: JDK-8300722: The unit of OCSP timeout should be changed from seconds to milliseconds
Hello all, I put together a draft CSR for enhancing the com.sun.security.ocsp.timeout System property to allow it to be specified either in seconds (as it is currently done, no change to the format) or by appending "ms" (case-insensitive) to allow the numeric value to be interpreted in milliseconds. I could use a review before proposing it. https://bugs.openjdk.org/browse/JDK-8300722 Thanks, --Jamil
Re: RFR: 8297972: Poly1305 Endianness on ByteBuffer not enforced [v2]
On Thu, 19 Jan 2023 22:13:07 GMT, Volodymyr Paprotski wrote: >> Looks good to me. > > @jnimeh would you mind running this through your tests? The build failures > reported above seem unrelated.. @vpaprotsk Yes, happy to take it through a regression run on my side. I'll will run it later tonight. - PR: https://git.openjdk.org/jdk/pull/11463
Re: RFR: 8297972: Poly1305 Endianness on ByteBuffer not enforced [v2]
On Thu, 19 Jan 2023 18:30:04 GMT, Volodymyr Paprotski wrote: >> Per rfc7539 Section 2.5, "Read the block as a little-endian number." >> >> sun.security.util.math.intpoly.IntegerPolynomial1305 enforces this on input >> when input is provided as `[]byte` but not when input is in `ByteBuffer` >> >> Tested with `Poly1305IntrinsicFuzzTest.java` from >> https://github.com/openjdk/jdk/pull/11338 which compares Poly1305 MAC >> between `ByteBuffer` and `[]byte` > > Volodymyr Paprotski 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: > > - remove workaround from test > - Merge remote-tracking branch 'origin/master' into endian-poly1305 > - enforce reading input as little_endian numbers Looks good to me. - Marked as reviewed by jnimeh (Reviewer). PR: https://git.openjdk.org/jdk/pull/11463
Re: RFR: 8296343: CPVE thrown on missing content-length in OCSP response [v2]
On Thu, 12 Jan 2023 15:31:30 GMT, Matthew Donovan wrote:
>> Jamil Nimeh has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Throw exception directly from non 200 HTTP response codes
>> - Moved SimpleOCSPServer to use CountdownLatch for ready state, updated
>> tests
>
> test/jdk/sun/security/provider/certpath/OCSP/OCSPNoContentLength.java line
> 154:
>
>> 152: //if (!rootOcsp.isServerReady()) {
>> 153: //throw new RuntimeException("Server not ready yet");
>> 154: //}
>
> Lines 149-154 can be deleted
I thought I caught all the dead comments, but I guess I missed this one. Good
catch, will fix.
-
PR: https://git.openjdk.org/jdk/pull/11917
Re: RFR: 8296343: CPVE thrown on missing content-length in OCSP response [v3]
> This fixes an issue where HTTP responses that do not have an explicit > Content-Length are causing an EOFException which unravels into a > CertPathValidatorException during validations that involve OCSP checks. > > - JBS: https://bugs.openjdk.org/browse/JDK-8296343 Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision: Remove dead commented code - Changes: - all: https://git.openjdk.org/jdk/pull/11917/files - new: https://git.openjdk.org/jdk/pull/11917/files/36a0911c..ddcd124a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11917&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11917&range=01-02 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/11917.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11917/head:pull/11917 PR: https://git.openjdk.org/jdk/pull/11917
Re: RFR: 8296343: CPVE thrown on missing content-length in OCSP response [v2]
> This fixes an issue where HTTP responses that do not have an explicit > Content-Length are causing an EOFException which unravels into a > CertPathValidatorException during validations that involve OCSP checks. > > - JBS: https://bugs.openjdk.org/browse/JDK-8296343 Jamil Nimeh has updated the pull request incrementally with two additional commits since the last revision: - Throw exception directly from non 200 HTTP response codes - Moved SimpleOCSPServer to use CountdownLatch for ready state, updated tests - Changes: - all: https://git.openjdk.org/jdk/pull/11917/files - new: https://git.openjdk.org/jdk/pull/11917/files/16a60c85..36a0911c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11917&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11917&range=00-01 Stats: 151 lines in 9 files changed: 28 ins; 38 del; 85 mod Patch: https://git.openjdk.org/jdk/pull/11917.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11917/head:pull/11917 PR: https://git.openjdk.org/jdk/pull/11917
Re: RFR: 8296343: CPVE thrown on missing content-length in OCSP response
On Tue, 10 Jan 2023 18:32:08 GMT, Jamil Nimeh wrote: >> It may be more effective/accuracy to stop read OCSP response bytes if >> response code is not OK. > > Logging the error code and returning with no read and not throwing an > exception I believe would still work since the revocation information would > be missing. I'm wondering though if this needs to be a separate issue given > that we're talking about a different use case, and one that involves the > behavior of HttpURLConnection when dealing with different response codes. > I'll also check to see if there are existing tests that make CPV checks > against URIs that have non-200 response codes. Hmmm, I was not quite correct about the HttpURLConnection behavior - it's not the 404 that's causing the issue directly, it is indeed the getContentLength when the 404 happens. So forget a separate issue, I will deal with non-200 codes in this PR. - PR: https://git.openjdk.org/jdk/pull/11917
Re: RFR: 8296343: CPVE thrown on missing content-length in OCSP response
On Tue, 10 Jan 2023 18:26:50 GMT, Xue-Lei Andrew Fan wrote: >> Well, in the case of a 404 what appears to happen is that HttpURLConnection >> would throw a FileNotFoundException. That ultimately would result in a CPVE >> if there were no other sources of revocation information (e.g. CRL) for that >> certificate. > > It may be more effective/accuracy to stop read OCSP response bytes if > response code is not OK. Logging the error code and returning with no read and not throwing an exception I believe would still work since the revocation information would be missing. I'm wondering though if this needs to be a separate issue given that we're talking about a different use case, and one that involves the behavior of HttpURLConnection when dealing with different response codes. I'll also check to see if there are existing tests that make CPV checks against URIs that have non-200 response codes. - PR: https://git.openjdk.org/jdk/pull/11917
Re: RFR: 8296343: CPVE thrown on missing content-length in OCSP response
On Tue, 10 Jan 2023 17:30:08 GMT, Xue-Lei Andrew Fan wrote: >> This fixes an issue where HTTP responses that do not have an explicit >> Content-Length are causing an EOFException which unravels into a >> CertPathValidatorException during validations that involve OCSP checks. >> >> - JBS: https://bugs.openjdk.org/browse/JDK-8296343 > > src/java.base/share/classes/sun/security/provider/certpath/OCSP.java line 217: > >> 215: >> 216: int contentLength = con.getContentLength(); >> 217: return (contentLength == -1) ? >> con.getInputStream().readAllBytes() : > > For the returned OCSP bytes, what if the response code is not OK? Well, in the case of a 404 what appears to happen is that HttpURLConnection would throw a FileNotFoundException. That ultimately would result in a CPVE if there were no other sources of revocation information (e.g. CRL) for that certificate. - PR: https://git.openjdk.org/jdk/pull/11917
Re: RFR: 8296343: CPVE thrown on missing content-length in OCSP response
On Tue, 10 Jan 2023 15:49:26 GMT, Mark Powers wrote: >> This fixes an issue where HTTP responses that do not have an explicit >> Content-Length are causing an EOFException which unravels into a >> CertPathValidatorException during validations that involve OCSP checks. >> >> - JBS: https://bugs.openjdk.org/browse/JDK-8296343 > > test/jdk/sun/security/provider/certpath/OCSP/OCSPNoContentLength.java line 58: > >> 56: >> 57: // Turn on debugging >> 58: static final boolean debug = true; > > Do you really mean to set `debug` to `true`? The overall output is pretty small even with it on, but I'll switch it off. - PR: https://git.openjdk.org/jdk/pull/11917
RFR: 8296343: CPVE thrown on missing content-length in OCSP response
Hello all, This fixes an issue in OCSP where HTTP responses that do not have an explicit Content-Length are causing an EOFException which unravels into a CertPathValidatorException during validations that involve OCSP checks. * JBS: https://bugs.openjdk.org/browse/JDK-8296343 https://github.com/openjdk/jdk/pull/11917 Thanks, --Jamil
Re: RFR: 8298381: Improve handling of session tickets for multiple SSLContexts [v5]
On Tue, 3 Jan 2023 17:43:43 GMT, Volker Simonis wrote: >> Looks good to me. Thanks! > >> Looks good to me. Thanks! > > Thanks @XueleiFan! > > I've updated the copyright year to 2023 and will wait one or two more days > just in case @ascarpino wants to take one more look as well. Hi @simonis, I am sorry for chiming in so late on this issue. I do think it might be worthwhile to make your proof-of-concept code into a jtreg test as you mentioned in your summary. I think it really comes down to how feasible the conversion would be. It's always better to have an automated test if we can, but it depends on if jtreg code can get access to reliable information about the session tickets via the session cache and know that things are behaving as intended. - PR: https://git.openjdk.org/jdk/pull/11590
[jdk20] Integrated: 8298592: Add java man page documentation for ChaCha20 and Poly1305 intrinsics
On Wed, 28 Dec 2022 15:54:49 GMT, Jamil Nimeh wrote: > This adds documentation to the `java(1)` man page for new ChaCha20 and > Poly1305 intrinsics, highlighting the diagnostic flags that were delivered in > those feature enhancements. This is similar to what has already been done > for AES and GHASH diagnostic flags. > > - JBS: https://bugs.openjdk.org/browse/JDK-8298592 > - Flags were delivered in ( openjdk/jdk#7702 for ChaCha20 and > openjdk/jdk#10582 for Poly1305, with a minor change to the Poly1305 flag name > in #49 ) This pull request has now been integrated. Changeset: 37f8b059 Author:Jamil Nimeh URL: https://git.openjdk.org/jdk20/commit/37f8b059c1c9245e7f3af90d6ed47c862fee54a3 Stats: 16 lines in 1 file changed: 16 ins; 0 del; 0 mod 8298592: Add java man page documentation for ChaCha20 and Poly1305 intrinsics Reviewed-by: weijun - PR: https://git.openjdk.org/jdk20/pull/78
Re: [jdk20] RFR: 8298592: Add java man page documentation for ChaCha20 and Poly1305 intrinsics [v2]
> This adds documentation to the `java(1)` man page for new ChaCha20 and > Poly1305 intrinsics, highlighting the diagnostic flags that were delivered in > those feature enhancements. This is similar to what has already been done > for AES and GHASH diagnostic flags. > > - JBS: https://bugs.openjdk.org/browse/JDK-8298592 > - Flags were delivered in ( openjdk/jdk#7702 for ChaCha20 and > openjdk/jdk#10582 for Poly1305, with a minor change to the Poly1305 flag name > in #49 ) Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision: Add highlighting for intrinsic diag switch title - Changes: - all: https://git.openjdk.org/jdk20/pull/78/files - new: https://git.openjdk.org/jdk20/pull/78/files/83b7df59..4b0c4cd6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk20&pr=78&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk20&pr=78&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk20/pull/78.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/78/head:pull/78 PR: https://git.openjdk.org/jdk20/pull/78
[jdk20] RFR: 8298592: Add java man page documentation for ChaCha20 and Poly1305 intrinsics
This adds documentation to the `java(1)` man page for new ChaCha20 and Poly1305 intrinsics, highlighting the diagnostic flags that were delivered in those feature enhancements. This is similar to what has already been done for AES and GHASH diagnostic flags. - JBS: https://bugs.openjdk.org/browse/JDK-8298592 - Flags were delivered in ( openjdk/jdk#7702 for ChaCha20 and openjdk/jdk#10582 for Poly1305, with a minor change to the Poly1305 flag name in #49 ) - Commit messages: - 8298592: Add java man page documentation for ChaCha20 and Poly1305 intrinsics Changes: https://git.openjdk.org/jdk20/pull/78/files Webrev: https://webrevs.openjdk.org/?repo=jdk20&pr=78&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8298592 Stats: 16 lines in 1 file changed: 16 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk20/pull/78.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/78/head:pull/78 PR: https://git.openjdk.org/jdk20/pull/78
Re: RFR: 8297798: Timeout with DTLSOverDatagram test template [v3]
On Wed, 14 Dec 2022 18:41:35 GMT, Matthew Donovan wrote:
>> This fix is intended to address various time-out errors in tests that use
>> DTLSOverDatagram as a test template. Based on test output from those bugs
>> (JDK-8202059, JDK-8249562, JDK-8280185, JDK-8280186, JDK-8269887,
>> JDK-8268899), this fix:
>>
>> * refactors the class to only create one additional thread
>> * adds a CountdownLatch so if the server thread doesn't start for some
>> reason, it is reported quickly
>> * cleans up code to remove a loop condition that never fired: tests always
>> time-out before too many loop iterations
>> * removes CipherSuite.java from ProblemList
>>
>> Ran the following tests 200 times each with no failures.
>> * open/test/jdk/javax/net/ssl/DTLS/ClientAuth.java
>> * open/test/jdk/javax/net/ssl/DTLS/PacketLossRetransmission.java
>> * open/test/jdk/javax/net/ssl/DTLS/RespondToRetransmit.java
>> * open/test/jdk/javax/net/ssl/DTLS/InvalidCookie.java
>> * open/test/jdk/javax/net/ssl/DTLS/CipherSuite.java
>
> Matthew Donovan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> formatting changes
Overall it looks good to me.
test/jdk/javax/net/ssl/DTLS/InvalidRecords.java line 42:
> 40:
> 41: /**
> 42: * Test that if handshake messages are crasged, the handshake would fail
crasged? Was that supposed to be "changed?"
test/jdk/javax/net/ssl/DTLS/InvalidRecords.java line 66:
> 64: if (needInvalidRecords.get() && (ba.length >= 60) &&
> 65: (ba[0x00] == (byte)0x16) && (ba[0x0D] == (byte)0x01) &&
> 66: (ba[0x3B] == (byte)0x00) && (ba[0x3C] > 0)) {
I just want to make sure - this test is only designed to be run for initial
handshakes with cookies, not resumed handshakes, correct? I assume that is the
intent since this test dates back to the initial DTLS release where resumptions
didn't use cookies (that was a recent change to include support for resumption
cookies).
-
Marked as reviewed by jnimeh (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11558
Re: RFR: 8297379: Enable the ByteBuffer path of Poly1305 optimizations [v4]
On Mon, 5 Dec 2022 22:05:51 GMT, Volodymyr Paprotski wrote: >> There is now an intrinsic for Poly1305, which is only enabled on the >> `engineUpdate([]byte)` path. This PR adds intrinsic support >> `engineUpdate(ByteBuffer)` (when the bytebuffer `hasArray`). >> >> Fuzzing test expanded to also include ByteBuffer payloads. >> >> Performance is now matched: >> >> Benchmark (dataSize) (provider) Mode Cnt >> ScoreError Units >> Poly1305DigestBench.digestBuffer 64 thrpt3 >> 3320909.857 ± 787300.545 ops/s >> Poly1305DigestBench.digestBuffer 256 thrpt3 >> 3006447.324 ± 704790.796 ops/s >> Poly1305DigestBench.digestBuffer1024 thrpt3 >> 2645041.509 ± 664904.622 ops/s >> Poly1305DigestBench.digestBuffer 16384 thrpt3 >> 893389.209 ± 6288.743 ops/s >> Poly1305DigestBench.digestBuffer 1048576 thrpt3 >> 14932.680 ±170.238 ops/s >> Poly1305DigestBench.digestBytes 64 thrpt3 >> 3548298.515 ± 859964.530 ops/s >> Poly1305DigestBench.digestBytes 256 thrpt3 >> 3083261.994 ± 141802.417 ops/s >> Poly1305DigestBench.digestBytes 1024 thrpt3 >> 2704357.584 ± 554683.019 ops/s >> Poly1305DigestBench.digestBytes16384 thrpt3 >> 898913.339 ± 99733.295 ops/s >> Poly1305DigestBench.digestBytes 1048576 thrpt3 >> 14961.872 ± 38.003 ops/s >> Finished running test >> 'micro:org.openjdk.bench.javax.crypto.full.Poly1305DigestBench' >> >> Relates to: >> - https://github.com/openjdk/jdk/pull/11463: Found inconsistency between >> processing `[]byte` and `ByteBuffer`. When that one is fixed, >> `Poly1305IntrinsicFuzzTest.java` should not be setting the endianness on the >> `ByteBuffer` >> - Intrinsic introduced by https://github.com/openjdk/jdk/pull/10582. > > Volodymyr Paprotski 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 six additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into avx512-poly-buf > - remove comment > - bench and handle buf.position() > - Merge remote-tracking branch 'origin/master' into avx512-poly-buf > - whitespace > - ByteBuffer support and tests Changes look fine. tier1-5 + hs-precheckin-comp test jobs came back clean with the exception of two jaxp failures only on linux-x64 but they don't appear to be related to your changes. - Marked as reviewed by jnimeh (Reviewer). PR: https://git.openjdk.org/jdk/pull/11338
Re: RFR: 8297379: Enable the ByteBuffer path of Poly1305 optimizations [v3]
On Mon, 5 Dec 2022 18:22:00 GMT, Sandhya Viswanathan wrote: >> Volodymyr Paprotski has updated the pull request incrementally with one >> additional commit since the last revision: >> >> remove comment > > @valeriepeng Could you please take a look at this PR? @sviswa7 I will be looking at this today - PR: https://git.openjdk.org/jdk/pull/11338
Re: RFR: 8296507: GCM using more memory than necessary with in-place operations [v3]
On Thu, 1 Dec 2022 04:19:37 GMT, Anthony Scarpino wrote: >> I would like a review of an update to the GCM code. A recent report showed >> that GCM memory usage for TLS was very large. This was a result of in-place >> buffers, which TLS uses, and how the code handled the combined intrinsic >> method during decryption. A temporary buffer was used because the combined >> intrinsic does gctr before ghash which results in a bad tag. The fix is to >> not use the combined intrinsic during in-place decryption and depend on the >> individual GHASH and CounterMode intrinsics. Direct ByteBuffers are not >> affected as they are not used by the intrinsics directly. >> >> The reduction in the memory usage boosted performance back to where it was >> before despite using slower intrinsics (gctr & ghash individually). The >> extra memory allocation for the temporary buffer out-weighted the faster >> intrinsic. >> >> >> JDK 17: 122913.554 ops/sec >> JDK 19:94885.008 ops/sec >> Post fix: 122735.804 ops/sec >> >> There is no regression test because this is a memory change and test >> coverage already existing. > > Anthony Scarpino has updated the pull request incrementally with one > additional commit since the last revision: > > comment update This looks good, I only have nit comments. src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 580: > 578: * an upper limit on the number of blocks encrypted in the intrinsic. > 579: * > 580: * For decrypting in-place byte[], calling methods must ct must set > to null Typo nit? Should it be "calling methods must set ct to null" src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1642: > 1640: // Clear output data > 1641: dst.reset(); > 1642: // If this is no an in-place array, zero the dst buffer nit: no -> not - Marked as reviewed by jnimeh (Reviewer). PR: https://git.openjdk.org/jdk/pull/11121
Integrated: 8247645: ChaCha20 intrinsics
On Fri, 4 Mar 2022 16:47:54 GMT, Jamil Nimeh wrote: > This PR delivers ChaCha20 intrinsics that accelerate the core block function > that generates key stream from the key, counter and nonce. Intrinsics have > been written for the following platforms and instruction sets: > > - x86_64: AVX, AVX2 and AVX512 > - aarch64: platforms that support the advanced SIMD instructions > > Note: Microbenchmark results moved to a comment in the PR so we don't have to > see it in every email. > > Special thanks to the folks who have made many helpful comments while this PR > was in draft form. This pull request has now been integrated. Changeset: cd6bebbf Author:Jamil Nimeh URL: https://git.openjdk.org/jdk/commit/cd6bebbf34215723fad1d6bfe070a409351920c1 Stats: 1596 lines in 28 files changed: 1558 ins; 6 del; 32 mod 8247645: ChaCha20 intrinsics Reviewed-by: sviswanathan, ngasson, vlivanov, ascarpino - PR: https://git.openjdk.org/jdk/pull/7702
Re: RFR: 8247645: ChaCha20 intrinsics [v5]
On Mon, 28 Nov 2022 22:58:26 GMT, Anthony Scarpino wrote: >> Jamil Nimeh has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 50 commits: >> >> - Merge with main >> - Add AVX assertion guard >> - Pull out common macro code into function parameter pack >> - replace hi/lo word shuffles and left-right shift/or operations for >> vpshufd on byte-aligned rotations >> - Change intrinsic helper method name conform to convention >> - consolidate chacha macroAssembler routines into chacha stubGenerator file >> - More indentation fixes on aarch64 >> - rename chapoly->chacha for macro file >> - rename chacha macro file to be consistent with x86_64 naming >> - Fix indentation issues >> - ... and 40 more: https://git.openjdk.org/jdk/compare/392ac705...bb3f4264 > > src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line > 92: > >> 90: private long counter; >> 91: >> 92: // The 16-int state array and output keystream array: > > I think it would help readability if these comments were separated for each > declaration Agreed. I've split those up for each declaration as suggested. - PR: https://git.openjdk.org/jdk/pull/7702
Re: RFR: 8247645: ChaCha20 intrinsics [v6]
> This PR delivers ChaCha20 intrinsics that accelerate the core block function > that generates key stream from the key, counter and nonce. Intrinsics have > been written for the following platforms and instruction sets: > > - x86_64: AVX, AVX2 and AVX512 > - aarch64: platforms that support the advanced SIMD instructions > > Note: Microbenchmark results moved to a comment in the PR so we don't have to > see it in every email. > > Special thanks to the folks who have made many helpful comments while this PR > was in draft form. Jamil Nimeh has updated the pull request incrementally with one additional commit since the last revision: Split comment paragraph up for readability/clarity - Changes: - all: https://git.openjdk.org/jdk/pull/7702/files - new: https://git.openjdk.org/jdk/pull/7702/files/bb3f4264..b818411b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=7702&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=7702&range=04-05 Stats: 10 lines in 1 file changed: 4 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/7702.diff Fetch: git fetch https://git.openjdk.org/jdk pull/7702/head:pull/7702 PR: https://git.openjdk.org/jdk/pull/7702
Re: RFR: 8247645: ChaCha20 intrinsics [v4]
On Mon, 21 Nov 2022 19:06:49 GMT, Vladimir Ivanov wrote:
>> Jamil Nimeh has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Pull out common macro code into function parameter pack
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_chacha.cpp line 107:
>
>> 105: if (VM_Version::supports_evex()) {
>> 106: StubRoutines::_chacha20Block = generate_chacha20Block_avx512();
>> 107: } else {// Either AVX or AVX2 is supported
>
> Worth to supplement the comment with an assert (either `UseAVX > 0` or
> `VM_Version::supports_avx() == true`).
Good catch. This has been implemented.
-
PR: https://git.openjdk.org/jdk/pull/7702
