Re: RFR: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297 [v2]
On Mon, 18 Apr 2022 20:43:06 GMT, Smita Kamath wrote: >> When input length provided to the intrinsic is 8192, only 7680 bytes are >> processed as the intrinsic operates on multiples of 768 bytes. >> In implGCMCrypt(ByteBuffer src, ByteBuffer dst) method, >> dst.put(bout, 0, PARALLEL_LEN) statement caused the ciphertext mismatch as >> PARALLEL_LEN was set to 8192. >> Since the intrinsic only processed 7680 bytes, the rest output was incorrect. > > Smita Kamath has updated the pull request incrementally with one additional > commit since the last revision: > > Updated copyright year I'm not sure if the last commit removed the approval or not.. so i'll approve again - Marked as reviewed by ascarpino (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8280
Re: RFR: 8285398: Cache the results of constraint checks
On Thu, 21 Apr 2022 19:58:39 GMT, Daniel Jeliński wrote: > Profiling the TLS handshakes using SSLHandshake benchmark shows that a large > portion of time is spent in HandshakeContext initialization, specifically in > DisabledAlgorithmConstraints class. > > There are only a few instances of that class, and they are immutable. Caching > the results should be a low-risk operation. > > The cache is implemented as a softly reachable ConcurrentHashMap; this way it > can be removed from memory after a period of inactivity. Under normal > circumstances the cache holds no more than 100 algorithms. src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java line 969: > 967: result = checkAlgorithm(disabledAlgorithms, algorithm, > decomposer); > 968: cache.put(algorithm, result); > 969: return result; Would it be worth using `cache.computeIfAbsent` or do you want to avoid lambda allocation overhead and potentially blocking concurrent handshakes on writer thread? Suggestion: return cache.computeIfAbsent(algorithm, algo -> checkAlgorithm(disabledAlgorithms, algo, decomposer)); - PR: https://git.openjdk.java.net/jdk/pull/8349
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Thu, 21 Apr 2022 23:15:10 GMT, Valerie Peng wrote:
>>> For (1), how about something like below:
>>>
>>> > ```
>>> > * The returned parameters may be the same that were used to initialize
>>> > * this cipher, or may contain additional default or random parameter
>>> > * values used by the underlying cipher implementation.
>>
>> I like this first sentence.
>>
>>> > If no parameters
>>> > * is supplied and this cipher successfully generated the required
>>> > * parameter values, it will be returned.
>>
>> What does "successfully" mean? If it wasn't successful, what happens? Maybe
>> we should avoid that word. How about:
>>
>> "If parameters were not supplied and this cipher requires parameters, the
>> returned parameters will contain the parameter values generated by the
>> underlying cipher implementation."
>>
>>> > Otherwise, {@code null} is
>>> > * returned.
>>> > ```
>
>> > Hmm, I tried the suggested approach in (1), the result looks very lengthy.
>> > Actually, the Cipher.init(..) methods already has a few paragraphs
>> > describing the behavior for parameter generation, perhaps we should not
>> > repeat stating the "If this cipher was initialized" vs "was not
>> > initialized" since lengthy description may confuse users and have higher
>> > risks of inconsistency. What do you think?
>>
>> That's a good point, the `init` methods do go into a lot of detail about
>> that.
>>
>> > As for (2), currently only RSASSA-PSS signature impl uses parameters. When
>> > specified using PSSParameterSpec, it's possible that some of the
>> > parameters are not set, so it's possible for providers to use
>> > provider-specific default values for PSS case. So, yes, Signature class
>> > may have to updated in the same way to cover this particular scenario.
>>
>> Ok, I think we should try to make the spec consistent across `Cipher` and
>> `Signature` once we settle on the right wording. I think it would be better
>> to do it as part of this issue, but I would be ok doing it later as long as
>> it is also fixed in 19.
>
> I can do the Signature update through
> https://bugs.openjdk.java.net/browse/JDK-8253176 which I have targeted to fix
> in 19 also.
As for the 2nd sentence, it boils down whether we requires provider to generate
default parameters and return it when parameter is required. Existing javadoc
states that null is returned when parameter is not required. Given Cipher
covers many algorithms, if a provider does not want to generate a default
parameter when parameter is required, it can't return null when getParameters()
is called.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Wed, 20 Apr 2022 16:40:34 GMT, Sean Mullan wrote:
>>> Hmm, I tried the suggested approach in (1), the result looks very lengthy.
>>> Actually, the Cipher.init(..) methods already has a few paragraphs
>>> describing the behavior for parameter generation, perhaps we should not
>>> repeat stating the "If this cipher was initialized" vs "was not
>>> initialized" since lengthy description may confuse users and have higher
>>> risks of inconsistency. What do you think?
>>
>> That's a good point, the `init` methods do go into a lot of detail about
>> that.
>>
>>> As for (2), currently only RSASSA-PSS signature impl uses parameters. When
>>> specified using PSSParameterSpec, it's possible that some of the parameters
>>> are not set, so it's possible for providers to use provider-specific
>>> default values for PSS case. So, yes, Signature class may have to updated
>>> in the same way to cover this particular scenario.
>>
>> Ok, I think we should try to make the spec consistent across `Cipher` and
>> `Signature` once we settle on the right wording. I think it would be better
>> to do it as part of this issue, but I would be ok doing it later as long as
>> it is also fixed in 19.
>
>> For (1), how about something like below:
>>
>> > ```
>> > * The returned parameters may be the same that were used to initialize
>> > * this cipher, or may contain additional default or random parameter
>> > * values used by the underlying cipher implementation.
>
> I like this first sentence.
>
>> > If no parameters
>> > * is supplied and this cipher successfully generated the required
>> > * parameter values, it will be returned.
>
> What does "successfully" mean? If it wasn't successful, what happens? Maybe
> we should avoid that word. How about:
>
> "If parameters were not supplied and this cipher requires parameters, the
> returned parameters will contain the parameter values generated by the
> underlying cipher implementation."
>
>> > Otherwise, {@code null} is
>> > * returned.
>> > ```
> > Hmm, I tried the suggested approach in (1), the result looks very lengthy.
> > Actually, the Cipher.init(..) methods already has a few paragraphs
> > describing the behavior for parameter generation, perhaps we should not
> > repeat stating the "If this cipher was initialized" vs "was not
> > initialized" since lengthy description may confuse users and have higher
> > risks of inconsistency. What do you think?
>
> That's a good point, the `init` methods do go into a lot of detail about that.
>
> > As for (2), currently only RSASSA-PSS signature impl uses parameters. When
> > specified using PSSParameterSpec, it's possible that some of the parameters
> > are not set, so it's possible for providers to use provider-specific
> > default values for PSS case. So, yes, Signature class may have to updated
> > in the same way to cover this particular scenario.
>
> Ok, I think we should try to make the spec consistent across `Cipher` and
> `Signature` once we settle on the right wording. I think it would be better
> to do it as part of this issue, but I would be ok doing it later as long as
> it is also fixed in 19.
I can do the Signature update through
https://bugs.openjdk.java.net/browse/JDK-8253176 which I have targeted to fix
in 19 also.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8285398: Cache the results of constraint checks
Nice. On 22/04/2022 6:47 am, Daniel Jeliński wrote: On Thu, 21 Apr 2022 19:58:39 GMT, Daniel Jeliński wrote: Profiling the TLS handshakes using SSLHandshake benchmark shows that a large portion of time is spent in HandshakeContext initialization, specifically in DisabledAlgorithmConstraints class. There are only a few instances of that class, and they are immutable. Caching the results should be a low-risk operation. The cache is implemented as a softly reachable ConcurrentHashMap; this way it can be removed from memory after a period of inactivity. Under normal circumstances the cache holds no more than 100 algorithms. before: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake true TLSv1.2 thrpt5 2165.081 ± 440.204 ops/s SSLHandshake.doHandshake true TLS thrpt5 534.681 ± 146.931 ops/s SSLHandshake.doHandshake false TLSv1.2 thrpt5 369.104 ± 11.143 ops/s SSLHandshake.doHandshake false TLS thrpt5 253.903 ± 58.056 ops/s after: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake true TLSv1.2 thrpt5 10440.501 ± 478.177 ops/s SSLHandshake.doHandshake true TLS thrpt5762.995 ± 33.842 ops/s SSLHandshake.doHandshake false TLSv1.2 thrpt5440.471 ± 52.867 ops/s SSLHandshake.doHandshake false TLS thrpt5305.928 ± 57.847 ops/s After this patch the HandshakeContext initialization practically disappears from the CPU profile; it only takes ~5% in TLS1.2 resumption, and much less in the remaining scenarios. - PR: https://git.openjdk.java.net/jdk/pull/8349 -- Regards, Peter Firmstone 0498 286 363 Zeus Project Services Pty Ltd.
RFR: 8285398: Cache the results of constraint checks
Profiling the TLS handshakes using SSLHandshake benchmark shows that a large portion of time is spent in HandshakeContext initialization, specifically in DisabledAlgorithmConstraints class. There are only a few instances of that class, and they are immutable. Caching the results should be a low-risk operation. The cache is implemented as a softly reachable ConcurrentHashMap; this way it can be removed from memory after a period of inactivity. Under normal circumstances the cache holds no more than 100 algorithms. - Commit messages: - Implement constraint cache Changes: https://git.openjdk.java.net/jdk/pull/8349/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8349=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285398 Stats: 26 lines in 1 file changed: 23 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8349.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8349/head:pull/8349 PR: https://git.openjdk.java.net/jdk/pull/8349
Re: RFR: 8285398: Cache the results of constraint checks
On Thu, 21 Apr 2022 19:58:39 GMT, Daniel Jeliński wrote: > Profiling the TLS handshakes using SSLHandshake benchmark shows that a large > portion of time is spent in HandshakeContext initialization, specifically in > DisabledAlgorithmConstraints class. > > There are only a few instances of that class, and they are immutable. Caching > the results should be a low-risk operation. > > The cache is implemented as a softly reachable ConcurrentHashMap; this way it > can be removed from memory after a period of inactivity. Under normal > circumstances the cache holds no more than 100 algorithms. before: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake true TLSv1.2 thrpt5 2165.081 ± 440.204 ops/s SSLHandshake.doHandshake true TLS thrpt5 534.681 ± 146.931 ops/s SSLHandshake.doHandshake false TLSv1.2 thrpt5 369.104 ± 11.143 ops/s SSLHandshake.doHandshake false TLS thrpt5 253.903 ± 58.056 ops/s after: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake true TLSv1.2 thrpt5 10440.501 ± 478.177 ops/s SSLHandshake.doHandshake true TLS thrpt5762.995 ± 33.842 ops/s SSLHandshake.doHandshake false TLSv1.2 thrpt5440.471 ± 52.867 ops/s SSLHandshake.doHandshake false TLS thrpt5305.928 ± 57.847 ops/s After this patch the HandshakeContext initialization practically disappears from the CPU profile; it only takes ~5% in TLS1.2 resumption, and much less in the remaining scenarios. - PR: https://git.openjdk.java.net/jdk/pull/8349
Re: RFR: 8285380: Fix typos in security [v2]
> I ran `codespell` on modules owned by the security team (`java.security.jgss > java.security.sasl java.smartcardio java.xml.crypto jdk.crypto.cryptoki > jdk.crypto.ec jdk.crypto.mscapi jdk.security.auth jdk.security.jgss`), and > accepted those changes where it indeed discovered real typos. > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Revert changes to Apache code - Changes: - all: https://git.openjdk.java.net/jdk/pull/8340/files - new: https://git.openjdk.java.net/jdk/pull/8340/files/74a062cc..760e45c6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8340=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8340=00-01 Stats: 19 lines in 11 files changed: 0 ins; 0 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/8340.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8340/head:pull/8340 PR: https://git.openjdk.java.net/jdk/pull/8340
Re: RFR: 8285380: Fix typos in security
On Thu, 21 Apr 2022 14:11:08 GMT, Alan Bateman wrote: >> I ran `codespell` on modules owned by the security team (`java.security.jgss >> java.security.sasl java.smartcardio java.xml.crypto jdk.crypto.cryptoki >> jdk.crypto.ec jdk.crypto.mscapi jdk.security.auth jdk.security.jgss`), and >> accepted those changes where it indeed discovered real typos. >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > The folks on security-dev will know for sure but I assume that the changes to > the imported Apache Santuario code should be dropped as it will make upgrades > more complicated. > @AlanBateman So there is even more 3rd party code in there? :-( I tried to > ignore fixes for all files that I could identify as 3rd party. It's actually > a bit annoying that we have imported source code thrown around like this in > the source tree, so there is no clear boundary between code we own and code > we import from someone else... security-dev can say for sure but the only 3rd party code I see in this change is in the src/java.xml.crypto/share/classes/com/sun/org/apache tree (the package name gives a hint has it was it was re-packaged). - PR: https://git.openjdk.java.net/jdk/pull/8340
Re: RFR: 8285380: Fix typos in security
On Thu, 21 Apr 2022 14:11:08 GMT, Alan Bateman wrote: >> I ran `codespell` on modules owned by the security team (`java.security.jgss >> java.security.sasl java.smartcardio java.xml.crypto jdk.crypto.cryptoki >> jdk.crypto.ec jdk.crypto.mscapi jdk.security.auth jdk.security.jgss`), and >> accepted those changes where it indeed discovered real typos. >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > The folks on security-dev will know for sure but I assume that the changes to > the imported Apache Santuario code should be dropped as it will make upgrades > more complicated. @AlanBateman So there is even more 3rd party code in there? :-( I tried to ignore fixes for all files that I could identify as 3rd party. It's actually a bit annoying that we have imported source code thrown around like this in the source tree, so there is no clear boundary between code we own and code we import from someone else... - PR: https://git.openjdk.java.net/jdk/pull/8340
Re: RFR: 8285380: Fix typos in security
On Thu, 21 Apr 2022 13:51:27 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the security team (`java.security.jgss > java.security.sasl java.smartcardio java.xml.crypto jdk.crypto.cryptoki > jdk.crypto.ec jdk.crypto.mscapi jdk.security.auth jdk.security.jgss`), and > accepted those changes where it indeed discovered real typos. > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. The folks on security-dev will know for sure but I assume that the changes to the imported Apache Santuario code should be dropped as it will make upgrades more complicated. - PR: https://git.openjdk.java.net/jdk/pull/8340
RFR: 8285380: Fix typos in security
I ran `codespell` on modules owned by the security team (`java.security.jgss java.security.sasl java.smartcardio java.xml.crypto jdk.crypto.cryptoki jdk.crypto.ec jdk.crypto.mscapi jdk.security.auth jdk.security.jgss`), and accepted those changes where it indeed discovered real typos. I will update copyright years using a script before pushing (otherwise like every second change would be a copyright update, making reviewing much harder). The long term goal here is to make tooling support for running `codespell`. The trouble with automating this is of course all false positives. But before even trying to solve that issue, all true positives must be fixed. Hence this PR. - Commit messages: - Pass #2 - Pass #1 Changes: https://git.openjdk.java.net/jdk/pull/8340/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8340=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285380 Stats: 100 lines in 63 files changed: 0 ins; 0 del; 100 mod Patch: https://git.openjdk.java.net/jdk/pull/8340.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8340/head:pull/8340 PR: https://git.openjdk.java.net/jdk/pull/8340
Re: RFR: 8284910: Buffer clean in PasswordCallback [v2]
On Thu, 21 Apr 2022 06:50:58 GMT, Xue-Lei Andrew Fan wrote: >> I'd recommend setting `cleanable` to null after it's been cleaned to make >> the state machine easier to reason about. The invariant would be: if >> `cleanable` is non-null, then we have something dirty that needs to be >> cleaned. If we don't clear it to null after cleaning, it potentially results >> in confusing states. For example, suppose the app calls >> `setPassword(nonNull)` and later calls `setPassword(null)`. The second call >> will set `inputPassword` to null but leave a stale reference in `cleanable`. >> This isn't necessarily harmful, but it's confusing. > >> The code in `clearPassword` can be simplified and only test `cleanable != >> null`; it will be null unless there is an inputPassword to clean. > > Yes. The testing of `cleanable != null` is sufficient. > I'd recommend setting `cleanable` to null after it's been cleaned to make the > state machine easier to reason about. I like this idea. Updated. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v2]
On Wed, 20 Apr 2022 23:38:59 GMT, Stuart Marks wrote: >> Calling `Cleanable.clean()` calls the `Runnable` action at-most-once. >> Each `Cleanable` inserted in a list when it is created and is removed when >> `clear()` or `clean()` is invoked. >> The action is called only if it is still in the list. So there is no extra >> overhead. >> >> There is no harm in clearing the cleanable field; but it does not save much. >> >> The code in `clearPassword` can be simplified and only test `cleanable != >> null`; it will be null unless there is an inputPassword to clean. > > I'd recommend setting `cleanable` to null after it's been cleaned to make the > state machine easier to reason about. The invariant would be: if `cleanable` > is non-null, then we have something dirty that needs to be cleaned. If we > don't clear it to null after cleaning, it potentially results in confusing > states. For example, suppose the app calls `setPassword(nonNull)` and later > calls `setPassword(null)`. The second call will set `inputPassword` to null > but leave a stale reference in `cleanable`. This isn't necessarily harmful, > but it's confusing. > The code in `clearPassword` can be simplified and only test `cleanable != > null`; it will be null unless there is an inputPassword to clean. Yes. The testing of `cleanable != null` is sufficient. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v4]
> Please review this password cleanup enhancement in the PasswordCallback > implementation. This is one of the effort to clean up the buffered passwords. > > The PasswordCallback.setPassword() clones the password, but is not registered > for cleanup. An application could call clearPassword() for the purpose, but > it would be nice to cleanup the buffer as well if clearPassword() was not > called in an application. And, if the setPassword() get called multiple > times, the clearPassword() should also be called the same times if not > relying on finalization. It could be fragile in practice. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: Code clean up per feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/8272/files - new: https://git.openjdk.java.net/jdk/pull/8272/files/324f89fc..01542bb6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8272=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8272=02-03 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8272.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8272/head:pull/8272 PR: https://git.openjdk.java.net/jdk/pull/8272
