Re: A possible JEP to replace SecurityManager after JEP 411

[David Lloyd]

On Fri, Apr 22, 2022 at 10:04 PM Martin Balao  wrote:
> In my view, authorization decisions at higher layer generally
> have better context, are more clear and less riskier. At a lower layer
> there is more complexity and chances of both subtle combinations or
> unseen paths that may lead to check bypasses. I lean towards not
> splitting authorization responsibility through different layers, which
> might create confusion or even a false sense of security in some cases.
> To illustrate with a trivial example, if a subject is not supposed to
> access to some information, it's the application that has enough context
> to decide and block right away. Letting the attempt go though the call
> stack down to the network or the file system might be the recipe for
> missing a channel.

The point of the proposal is to allow checks at system-level
operations _in addition to_ application-level operations. It sounds
like you are saying that adding the extra authorization check is
somehow less secure? I'm not sure I agree with that reasoning.

> I won't enter the untrusted code case -which has been extensively
> discussed already- but want to briefly mention something about the
> "trusted code performing risky operations" case. My first point is that
> vulnerabilities at the JVM level (i.e.: memory safety compromises) are
> serious enough to potentially bypass a SecurityManager. My second point
> is that the SecurityManager is really unable to deal with tainted data
> flowing from low to high integrity domains, and sanitation must be
> performed by the application or the library anyways because there are
> infinite ways in which it can be harmful. Even when the data is obtained
> at a low integrity domain, there will be a flow towards a high integrity
> domain to perform a legitimate action (i.e.: SQL query, OS command
> execution, etc). The OS and the DB engine, in this example, have the
> knowledge, granularity and power to be the next level of enforcement
> after data sanitation. Again, I wouldn't split this responsibility or
> pass it to the JDK for the same reasons than before.

Nothing in the proposal causes splitting or delegation of
responsibilities. It is _only_ about preserving security checkpoints
in the JDK, which *add* a layer of checks to what the container might
already provide. Nothing is being subtracted, and thus I find it hard
to accept that preserving these checks somehow reduces security
overall. In absolute terms, using a security manager (even with all of
its problems) is *more* secure than not using it on the same code base
and deployment. I'm not sure how this can be rationally refuted. The
proposal is about retaining this incremental increase in security,
while both adjusting the user's expectations via documentation *and*
significantly reducing the burden of CVEs and maintenance on the JDK
itself (and putting it on to the third party authorization framework),
which in my estimation is the *real* stakes here.

Nothing in the proposal is intended to solve the issue of tainted
data; with or without this proposal, this is an unsolved problem.
-- 
- DML • he/him

Re: RFR: 8285516: clearPassword should be called in a finally try block

[Sean Mullan]

On Sun, 24 Apr 2022 05:13:36 GMT, Xue-Lei Andrew Fan  wrote:

> Hi,
> 
> Could I have the simple update reviewed?
> 
> In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() should 
> be called in a finally try block.  Otherwise, the password cleanup could be 
> interrupted by exceptions.
> 
> Thanks,
> Xuelei

Marked as reviewed by mullan (Reviewer).

-

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

Re: RFR: 8285398: Cache the results of constraint checks

[Daniel Fuchs]

On Mon, 25 Apr 2022 06:46:20 GMT, Daniel Jeliński  wrote:

>> src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
>>  line 105:
>> 
>>> 103: private final Set disabledAlgorithms;
>>> 104: private final Constraints algorithmConstraints;
>>> 105: private volatile SoftReference> cacheRef =
>> 
>> It looks like a one-clear-for-all mechanism.  Once the clearing happens, the 
>> full map will be collected.  For SoftReferences, it clears them fairly 
>> eagerly.  Maybe, the performance could be further improved in practice by 
>> using soft reference for each map entry (See sun.security.util.Cache code 
>> for an example). 
>> 
>> I will have another look next week.
>
> Right, soft references are likely to be cleaned if they are not used in an 
> entire GC cycle.
> Using a soft reference for each map entry would not help here; note that all 
> keys and all values in this map are GC roots (keys are enum names, values are 
> `TRUE` and `FALSE`), so in practice they would never be collected.
> Even if we made the entries collectable, it would not help with the TLS case; 
> SSLEngine & SSLSocket only check the constraints once at the beginning of the 
> handshake, so they would all expire at the same time anyway. What's even 
> worse, we would then have to clear the expired entries from the map.
> 
> I also considered limiting the size of the map. That's a tricky subject; we 
> would need to get the size limit exactly right. Given that the algorithms are 
> always checked in the same order, if the cache were too small, we would get 
> zero hits.
> With all the above in mind I decided not to use `sun.security.util.Cache` 
> here.

FWIW: I wouldn't expect `SoftReference` (as opposed to `WeakReference`) to be 
eagerly cleaned.

-

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

Re: RFR: 8285516: clearPassword should be called in a finally try block

[Weijun Wang]

On Sun, 24 Apr 2022 05:13:36 GMT, Xue-Lei Andrew Fan  wrote:

> Hi,
> 
> Could I have the simple update reviewed?
> 
> In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() should 
> be called in a finally try block.  Otherwise, the password cleanup could be 
> interrupted by exceptions.
> 
> Thanks,
> Xuelei

Change looks fine. One tiny enhancement to make: The "throw" line seems only 3 
spaces indented.

-

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

Re: RFR: 8285398: Cache the results of constraint checks

[Daniel Jeliński]

On Mon, 25 Apr 2022 13:22:34 GMT, Daniel Fuchs  wrote:

>> Right, soft references are likely to be cleaned if they are not used in an 
>> entire GC cycle.
>> Using a soft reference for each map entry would not help here; note that all 
>> keys and all values in this map are GC roots (keys are enum names, values 
>> are `TRUE` and `FALSE`), so in practice they would never be collected.
>> Even if we made the entries collectable, it would not help with the TLS 
>> case; SSLEngine & SSLSocket only check the constraints once at the beginning 
>> of the handshake, so they would all expire at the same time anyway. What's 
>> even worse, we would then have to clear the expired entries from the map.
>> 
>> I also considered limiting the size of the map. That's a tricky subject; we 
>> would need to get the size limit exactly right. Given that the algorithms 
>> are always checked in the same order, if the cache were too small, we would 
>> get zero hits.
>> With all the above in mind I decided not to use `sun.security.util.Cache` 
>> here.
>
> FWIW: I wouldn't expect `SoftReference` (as opposed to `WeakReference`) to be 
> eagerly cleaned.

`SoftReference`s are guaranteed to survive one GC after use; beyond that their 
lifespan is determined by `SoftRefLRUPolicyMSPerMB` and the amount of memory 
available.

-

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

Re: RFR: 8285516: clearPassword should be called in a finally try block [v2]

[Xue-Lei Andrew Fan]

> Hi,
> 
> Could I have the simple update reviewed?
> 
> In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() should 
> be called in a finally try block.  Otherwise, the password cleanup could be 
> interrupted by exceptions.
> 
> Thanks,
> Xuelei

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  an extra whitespace added

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8377/files
  - new: https://git.openjdk.java.net/jdk/pull/8377/files/123374d1..99079d30

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8377&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8377&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8377.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8377/head:pull/8377

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

Re: RFR: 8285516: clearPassword should be called in a finally try block

[Xue-Lei Andrew Fan]

On Mon, 25 Apr 2022 13:23:53 GMT, Weijun Wang  wrote:

> Change looks fine. One tiny enhancement to make: The "throw" line seems only 
> 3 spaces indented.

Nice catch.  Thank you!

-

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

Re: RFR: 8285398: Cache the results of constraint checks

[Xue-Lei Andrew Fan]

On Mon, 25 Apr 2022 13:59:44 GMT, Daniel Jeliński  wrote:

>> FWIW: I wouldn't expect `SoftReference` (as opposed to `WeakReference`) to 
>> be eagerly cleaned.
>
> `SoftReference`s are guaranteed to survive one GC after use; beyond that 
> their lifespan is determined by `SoftRefLRUPolicyMSPerMB` and the amount of 
> memory available.

> With all the above in mind I decided not to use `sun.security.util.Cache` here

I was not meant to use Cache and timeout for this update.

SoftReference and  this patch should work good in larger memory boxes.  
Performance improvement sometimes is a trade-off game between memory and CPU. 
Did you have a chance to check the performance in the limited memory 
circumstance?  like '-mx64M".

-

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

Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v7]

[Xue-Lei Andrew Fan]

On Fri, 22 Apr 2022 13:27:12 GMT, Daniel Fuchs  wrote:

> Please get another reviewer for the security-libs related and native changes.

@wangweij Did you have cycle and have another look at the update?

-

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

Re: Java Cryptographic Extension Survey

[Anthony Scarpino]

Reminder this survey ends Friday.  Please fill it out if you have not.

Thanks

On 4/12/22 8:10 AM, Anthony Scarpino wrote:

Hello,

Java Cryptographic Extension (JCE) has been in Java SE for a long time 
and has made incremental changes over the years.  Looking forward, we 
would like to know more about how projects are using JCE and what 
changes, features, and API enhancements would be helpful for your projects.


Below is a survey that we hope you can spend 5 minutes to fill out.  If 
you have written or maintain code that uses the JCE API, then we would 
appreciate if you would complete this survey:


https://www.questionpro.com/t/AUzP7ZrFWv

The survey will be open through April 29.

Thank you,
Anthony Scarpino
OpenJDK Security Group
https://openjdk.java.net/groups/security/

Integrated: 8285389: EdDSA trimming zeros

[Anthony Scarpino]

On Fri, 22 Apr 2022 21:04:58 GMT, Anthony Scarpino  
wrote:

> Hi,
> 
> I'd like a code review of this change to EdDSA. ed25519 and ed448 internally 
> was trimming extra zeros off the end of the signature before processing. This 
> can result in some verify testing failures which are strict about the 
> signature length passed into the operation. 
> 
> thanks
> 
> Tony

This pull request has now been integrated.

Changeset: 414918d9
Author:Anthony Scarpino 
URL:   
https://git.openjdk.java.net/jdk/commit/414918d9113b447c9ae774cdfd087f1636b8e5a0
Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod

8285389: EdDSA trimming zeros

Reviewed-by: xuelei

-

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

Re: RFR: 8285398: Cache the results of constraint checks

[Anthony Scarpino]

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.

I'm good with this idea.  It's not caching the whole list like I struggled 
with, but it's a good solution for the algorithm name based constraints.  It 
also shows that more caching probably would help further.
I'll let Xuelei finish his review comments

-

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

Re: RFR: 8285389: EdDSA trimming zeros

[Sean Mullan]

On Sun, 24 Apr 2022 15:54:15 GMT, Xue-Lei Andrew Fan  wrote:

> I like this explanation more. I have no more comment about the patch. Please 
> add a noreg label in JBS.

`noreg-sqe` seems appropriate.

-

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

Re: RFR: 8285398: Cache the results of constraint checks

[Sean Coffey]

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.

Another nice performance boost in this area. Looks good to me.

-

Marked as reviewed by coffeys (Reviewer).

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

Re: RFR: 8285398: Cache the results of constraint checks

[Daniel Jeliński]

On Mon, 25 Apr 2022 14:33:13 GMT, Xue-Lei Andrew Fan  wrote:

>> `SoftReference`s are guaranteed to survive one GC after use; beyond that 
>> their lifespan is determined by `SoftRefLRUPolicyMSPerMB` and the amount of 
>> memory available.
>
>> With all the above in mind I decided not to use `sun.security.util.Cache` 
>> here
> 
> I was not meant to use Cache and timeout for this update.
> 
> SoftReference and  this patch should work good in larger memory boxes.  
> Performance improvement sometimes is a trade-off game between memory and CPU. 
> Did you have a chance to check the performance in the limited memory 
> circumstance?  like '-mx64M".

I run a few tests:
`-Xmx16m`, with this patch, still better than the original version:

Benchmark (resume)  (tlsVersion)   Mode  Cnt Score 
Error  Units
SSLHandshake.doHandshake  true   TLSv1.2  thrpt5  4477.444 ± 
375.975  ops/s
SSLHandshake.doHandshake  true   TLS  thrpt5   681.471 ±  
72.531  ops/s
SSLHandshake.doHandshake false   TLSv1.2  thrpt5   335.366 ±  
89.056  ops/s
SSLHandshake.doHandshake false   TLS  thrpt5   308.711 ±  
90.134  ops/s


`-Xmx8m`, before patch:

Benchmark (resume)  (tlsVersion)   Mode  CntScoreError  
Units
SSLHandshake.doHandshake  true   TLSv1.2  thrpt5  153.760 ± 12.025  
ops/s
SSLHandshake.doHandshake  true   TLS  thrpt5   70.700 ±  7.506  
ops/s
SSLHandshake.doHandshake false   TLSv1.2  thrpt5   67.459 ±  4.325  
ops/s
SSLHandshake.doHandshake false   TLS  thrpt5   64.695 ±  1.647  
ops/s

after:

Benchmark (resume)  (tlsVersion)   Mode  CntScoreError  
Units
SSLHandshake.doHandshake  true   TLSv1.2  thrpt5  557.324 ± 50.269  
ops/s
SSLHandshake.doHandshake  true   TLS  thrpt5  155.258 ± 13.866  
ops/s
SSLHandshake.doHandshake false   TLSv1.2  thrpt5  181.755 ± 29.319  
ops/s
SSLHandshake.doHandshake false   TLS  thrpt5  120.627 ± 25.832  
ops/s

Much slower, but still faster with the patch.
With `-Xmx4m` the test failed with OOM.

-

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

Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]

[Sean Mullan]

On Thu, 21 Apr 2022 23:31:37 GMT, Valerie Peng  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.
>> 
>> 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.

> I can do the Signature update through 
> https://bugs.openjdk.java.net/browse/JDK-8253176 which I have targeted to fix 
> in 19 also.

Ok.

-

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

RFR: JDK-8285504 Minor cleanup could be done in javax.net

[Mark Powers]

https://bugs.openjdk.java.net/browse/JDK-8285504

JDK-8273046 is the umbrella bug for this bug. The changes were too large for a 
single code review, so it was decided to split into smaller chunks. This is one 
such chunk: 

open/src/java.base/share/classes/java/net

-

Commit messages:
 - second iteration
 - Merge
 - Merge
 - first iteration

Changes: https://git.openjdk.java.net/jdk/pull/8384/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8384&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8285504
  Stats: 128 lines in 20 files changed: 1 ins; 21 del; 106 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8384.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8384/head:pull/8384

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

Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net

[Alan Bateman]

On Mon, 25 Apr 2022 17:40:13 GMT, Mark Powers  wrote:

> https://bugs.openjdk.java.net/browse/JDK-8285504
> 
> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
> a single code review, so it was decided to split into smaller chunks. This is 
> one such chunk: 
> 
> open/src/java.base/share/classes/java/net

src/java.base/share/classes/javax/net/ssl/SSLSessionContext.java line 110:

> 108:  */
> 109: void setSessionTimeout(int seconds)
> 110:  throws IllegalArgumentException;

IllegalArgumentException is a runtime exception, it's unusual to have "throws 
IllegalArgumentException". It would not impact compatibility to drop it. The 
important thing is that the javadoc has the `@throws` describing when it is 
thrown.

-

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

Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net

[Mark Powers]

On Mon, 25 Apr 2022 18:48:31 GMT, Alan Bateman  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8285504
>> 
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
>> a single code review, so it was decided to split into smaller chunks. This 
>> is one such chunk: 
>> 
>> open/src/java.base/share/classes/java/net
>
> src/java.base/share/classes/javax/net/ssl/SSLSessionContext.java line 110:
> 
>> 108:  */
>> 109: void setSessionTimeout(int seconds)
>> 110:  throws IllegalArgumentException;
> 
> IllegalArgumentException is a runtime exception, it's unusual to have "throws 
> IllegalArgumentException". It would not impact compatibility to drop it. The 
> important thing is that the javadoc has the `@throws` describing when it is 
> thrown.

I'll drop it then. Thanks for noticing this.

-

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

Reproducer for JDK-8221218

[Flavia Rainone]

Hi everyone,

I work with the XNIO ( https://github.com/xnio/xnio/ ) project, led by
David Lloyd in CC.

I'm not sure if this is the best way to get in touch, but I could not find
out how to create an account for OpenJDK Jira to add a comment there.

We have a reproducer for JDK-8221218, and it appears it is not solved yet
(tested with jdk 11.0.12). It is not intermittent, and it happens when we
try to do an SSLEngine.unwrap with a zero remaining() ByteBuffer (in older
versions, the engine would just return a BUFFER_UNDERFLOW result).

You can see more information about this here:
https://issues.redhat.com/browse/XNIO-406

To reproduce the issue, just follow the steps in "How to reproduce" using
JDK11 and add a breakpoint at this line:
https://github.com/xnio/xnio/pull/282/commits/2ae0c1b368c6e0886f9c03030ec443c346cb0a71#diff-9f162fd704e4d5eaff8abb932a923ae868adf3f642d85bc6f5bcfd5f7fe1763bR695

Or you can simply open the test output file to view the exception:
xnio/nio-impl/target/surefire-reports/org.xnio.nio.test.NioSslTcpChannelTestCase-output.txt
(but then you have to comment out the XNIO-406 workaround above)

Notice that there are other failures with XNIO testsuite related to JDK11
and newer, as tracked by another Jira (XNIO-347), but they seem to be
unrelated to this issue, hence why the mvn clean install -DskipTests in the
reproduce steps.

Branch of choice should be 3.8

Since I won't be around for the next few days, Richard Opalka in CC will
follow up with you.


Best regards,

-- 

Flavia Rainone

Principal Software Engineer

Red Hat 

frain...@redhat.com

Timeframe for JEP-411 completely removing SecurityManager APIs

[Scott Stark]

Hello,

I'm Scott Stark of Red Hat, and a member of the Jakarta EE platform dev
group (EEPD). I'm currently coordinating the Jakarta EE 10 release that is
targeting June of this year (2022). The removal of the SecurityManager as
described in JEP-411 has been a topic for the EEPD on may calls this year.
Recent discussions make it clear that any SecurityManager alternative would
need to be taken up by the EEPD, and such an effort is going to be a
non-trivial undertaking, and may not be addressed at all.

A general concern among vendors in the EEPD is the timeframe for the code
that bridges between the JVM running with and without a SecurityManager
instance needing to be updated. Such code is the subject of this JEP-411
paragraph:

"In feature releases after Java 18, we will degrade other Security Manager
APIs so that they remain in place but with limited or no functionality. For
example, we may revise AccessController::doPrivileged simply to run the
given action, or revise System::getSecurityManager always to return null.
This will allow libraries that support the Security Manager and were
compiled against previous Java releases to continue to work without change
or even recompilation. We expect to remove the APIs once the compatibility
risk of doing so declines to an acceptable level."

Of particular interest is the timeframe for "remove the APIs once the
compatibility risk of doing so declines to an acceptable level".

Vendors in EEPD would like to see Java SE 21 ship with a migration feature
along the lines of the proposed "AccessController::doPrivileged simply to
run the given action, or revise System::getSecurityManager always to return
null" behaviors.

Is there some metric for tracking "when the compatibility risk of doing so
declines to an acceptable level."? I believe the EEPD vendors would like
readiness of their projects and upstream dependencies to somehow be
included in any such tracking.

Thanks,
Scott Stark

Re: RFR: 8284910: Buffer clean in PasswordCallback [v2]

[Sean Mullan]

On Mon, 25 Apr 2022 05:48:04 GMT, Xue-Lei Andrew Fan  wrote:

> > However, I think that we need to carefully check the interactions between 
> > cleaners and methods that explicitly allow the contents to be cleared so 
> > that there are not unexpected results.
> 
> I think @RogerRiggs explained the behavior of Cleanable.clean(), and 
> @stuart-marks' suggestion was added in the last update. I will file another 
> RFE for the specification clarification. Did I miss something? Did you have 
> more comments?

I am ok with the fix now but I had a few comments on the test.

-

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

Re: RFR: 8284910: Buffer clean in PasswordCallback [v4]

[Sean Mullan]

On Thu, 21 Apr 2022 06:55:22 GMT, Xue-Lei Andrew Fan  wrote:

>> 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

test/jdk/javax/security/auth/callback/PasswordCleanup.java line 58:

> 56: }
> 57: 
> 58: private static void clearWithMethod() throws Exception {

This looks like the exact same test as `clearAtCollection`.

test/jdk/javax/security/auth/callback/PasswordCleanup.java line 74:

> 72: }
> 73: 
> 74: private static void checkClearing() throws Exception {

How is this test testing that the password is cleared?

test/jdk/javax/security/auth/callback/PasswordCleanup.java line 83:

> 81: // Check if the object has been collected.
> 82: if (weakHashMap.size() > 0) {
> 83: throw new RuntimeException("GSSName object is not released");

Did you mean to say "PasswordCallback object is not released"?

-

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

Integrated: 8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with -Xcomp after 8273297

[Smita Kamath]

On Mon, 18 Apr 2022 05:06:26 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.

This pull request has now been integrated.

Changeset: 3416bfa2
Author:Smita Kamath 
Committer: Anthony Scarpino 
URL:   
https://git.openjdk.java.net/jdk/commit/3416bfa2560e240b5e602f10e98e8a06c96852df
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod

8283022: com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java failing with 
-Xcomp after 8273297

Reviewed-by: ascarpino

-

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

Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

[Mark Powers]

> https://bugs.openjdk.java.net/browse/JDK-8285504
> 
> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
> a single code review, so it was decided to split into smaller chunks. This is 
> one such chunk: 
> 
> open/src/java.base/share/classes/java/net

Mark Powers has updated the pull request incrementally with one additional 
commit since the last revision:

  Alan Bateman comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8384/files
  - new: https://git.openjdk.java.net/jdk/pull/8384/files/5ac7f1ec..d964c05a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8384&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8384&range=00-01

  Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8384.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8384/head:pull/8384

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

Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v8]

[Weijun Wang]

On Mon, 25 Apr 2022 06:07:00 GMT, Xue-Lei Andrew Fan  wrote:

>> Please review the update to remove finalizer method in the 
>> java.security.jgss module. It is one of the efforts to clean up the use of 
>> finalizer method in JDK.
>
> Xue-Lei Andrew Fan 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 and resovle merge conflict
>  - change the calling order in dispose()
>  - More code cleanup
>  - re-org the code per feedback
>  - Update to set context method
>  - add test cases
>  - Merge
>  - Update copyright year
>  - the object reference issue for Cleaner
>  - 8284490: Remove finalizer method in java.security.jgss

The latest source change is OK, but I'll look more into it tomorrow.

The test needs some enhancement. Or I can consider contributing one. Sorry I'll 
be a little busy this week.

src/java.security.jgss/share/classes/sun/security/jgss/wrapper/GSSNameElement.java
 line 54:

> 52: private final Cleaner.Cleanable cleanable;
> 53: 
> 54: long pName = 0; // Pointer to the gss_name_t structure

Can this be final?

test/jdk/sun/security/jgss/GssContextCleanup.java line 61:

> 59: if (whm.size() > 0) {
> 60: throw new RuntimeException("GSSContext object is not 
> released");
> 61: }

I think it's necessary to check the debug message to make sure 
"[GSSLibStub_deleteContext]" is seen. This test should have already succeeded 
before your latest update to `NativeGSSContext`.

-

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

Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v8]

[Weijun Wang]

On Tue, 26 Apr 2022 02:01:10 GMT, Weijun Wang  wrote:

>> Xue-Lei Andrew Fan 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 and resovle merge conflict
>>  - change the calling order in dispose()
>>  - More code cleanup
>>  - re-org the code per feedback
>>  - Update to set context method
>>  - add test cases
>>  - Merge
>>  - Update copyright year
>>  - the object reference issue for Cleaner
>>  - 8284490: Remove finalizer method in java.security.jgss
>
> test/jdk/sun/security/jgss/GssContextCleanup.java line 61:
> 
>> 59: if (whm.size() > 0) {
>> 60: throw new RuntimeException("GSSContext object is not 
>> released");
>> 61: }
> 
> I think it's necessary to check the debug message to make sure 
> "[GSSLibStub_deleteContext]" is seen. This test should have already succeeded 
> before your latest update to `NativeGSSContext`.

The same for `GSSName`. Where is a test for `GSSCredential`?

-

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

RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException

[Valerie Peng]

This is to update the method javadoc of java.security.Signature.getParameters() 
with the missing `@throws UnsupportedOperationException`. In addition, the 
wording on the returned parameters are updated to match those in Cipher and 
CipherSpi classes. 

CSR will be filed later.

Thanks,
Valerie

-

Commit messages:
 - 8253176: Signature.getParameters should specify that it can throw 
UnsupportedOperationException

Changes: https://git.openjdk.java.net/jdk/pull/8396/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8396&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8253176
  Stats: 28 lines in 2 files changed: 4 ins; 3 del; 21 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8396.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8396/head:pull/8396

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

Withdrawn: 8273042: TLS Certificate Compression

[duke]

On Wed, 23 Feb 2022 20:15:24 GMT, Xue-Lei Andrew Fan  wrote:

> Hi,
> 
> Please review the implementation of RFC 8879, TLS Certificate Compression, in 
> JDK.  The TLS Certificate Compression standard is an essential part for QUIC 
> connections and performance improvement for TLS connections.  More details, 
> please refer to the [JEP](https://bugs.openjdk.java.net/browse/JDK-8281710) 
> proposal.
> 
> The JEP was submitted, and it may take time for the final approval.  But let 
> me know you ideas and concerns about the proposal and implementation.
> 
> JEP: https://bugs.openjdk.java.net/browse/JDK-8281710

This pull request has been closed without being integrated.

-

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

Re: RFR: 8285398: Cache the results of constraint checks

[Xue-Lei Andrew Fan]

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.

Marked as reviewed by xuelei (Reviewer).

-

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

Re: RFR: 8285398: Cache the results of constraint checks

[Xue-Lei Andrew Fan]

On Mon, 25 Apr 2022 16:04:22 GMT, Anthony Scarpino  
wrote:

> It also shows that more caching probably would help further.

+1.

-

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

Re: RFR: 8285398: Cache the results of constraint checks

[Xue-Lei Andrew Fan]

On Mon, 25 Apr 2022 17:22:57 GMT, Daniel Jeliński  wrote:

>>> With all the above in mind I decided not to use `sun.security.util.Cache` 
>>> here
>> 
>> I was not meant to use Cache and timeout for this update.
>> 
>> SoftReference and  this patch should work good in larger memory boxes.  
>> Performance improvement sometimes is a trade-off game between memory and 
>> CPU. Did you have a chance to check the performance in the limited memory 
>> circumstance?  like '-mx64M".
>
> I run a few tests:
> `-Xmx16m`, with this patch, still better than the original version:
> 
> Benchmark (resume)  (tlsVersion)   Mode  Cnt Score 
> Error  Units
> SSLHandshake.doHandshake  true   TLSv1.2  thrpt5  4477.444 ± 
> 375.975  ops/s
> SSLHandshake.doHandshake  true   TLS  thrpt5   681.471 ±  
> 72.531  ops/s
> SSLHandshake.doHandshake false   TLSv1.2  thrpt5   335.366 ±  
> 89.056  ops/s
> SSLHandshake.doHandshake false   TLS  thrpt5   308.711 ±  
> 90.134  ops/s
> 
> 
> `-Xmx8m`, before patch:
> 
> Benchmark (resume)  (tlsVersion)   Mode  CntScore
> Error  Units
> SSLHandshake.doHandshake  true   TLSv1.2  thrpt5  153.760 ± 
> 12.025  ops/s
> SSLHandshake.doHandshake  true   TLS  thrpt5   70.700 ±  
> 7.506  ops/s
> SSLHandshake.doHandshake false   TLSv1.2  thrpt5   67.459 ±  
> 4.325  ops/s
> SSLHandshake.doHandshake false   TLS  thrpt5   64.695 ±  
> 1.647  ops/s
> 
> after:
> 
> Benchmark (resume)  (tlsVersion)   Mode  CntScore
> Error  Units
> SSLHandshake.doHandshake  true   TLSv1.2  thrpt5  557.324 ± 
> 50.269  ops/s
> SSLHandshake.doHandshake  true   TLS  thrpt5  155.258 ± 
> 13.866  ops/s
> SSLHandshake.doHandshake false   TLSv1.2  thrpt5  181.755 ± 
> 29.319  ops/s
> SSLHandshake.doHandshake false   TLS  thrpt5  120.627 ± 
> 25.832  ops/s
> 
> Much slower, but still faster with the patch.
> With `-Xmx4m` the test failed with OOM.

Thanks for the additional testing.  The improvement is impressive.

-

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

Re: RFR: 8284910: Buffer clean in PasswordCallback [v4]

[Xue-Lei Andrew Fan]

On Mon, 25 Apr 2022 20:37:38 GMT, Sean Mullan  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code clean up per feedback
>
> test/jdk/javax/security/auth/callback/PasswordCleanup.java line 83:
> 
>> 81: // Check if the object has been collected.
>> 82: if (weakHashMap.size() > 0) {
>> 83: throw new RuntimeException("GSSName object is not released");
> 
> Did you mean to say "PasswordCallback object is not released"?

Ooops, bad copy and past of mine.

-

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

Re: RFR: 8284910: Buffer clean in PasswordCallback [v4]

[Xue-Lei Andrew Fan]

On Mon, 25 Apr 2022 20:41:47 GMT, Sean Mullan  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code clean up per feedback
>
> test/jdk/javax/security/auth/callback/PasswordCleanup.java line 58:
> 
>> 56: }
>> 57: 
>> 58: private static void clearWithMethod() throws Exception {
> 
> This looks like the exact same test as `clearAtCollection`.

Nice catch.  The passwordCallback.clearPassword() should not be called in 
clearAtCollection.

> test/jdk/javax/security/auth/callback/PasswordCleanup.java line 74:
> 
>> 72: }
>> 73: 
>> 74: private static void checkClearing() throws Exception {
> 
> How is this test testing that the password is cleared?

The test case is used to check that the Cleaner used is not bind to 'this' 
object, and the cleaner during finalization could work.  Unfortunately, as the 
cleaner behavior is not visible, I don't find a way to automated test that the 
password is really cleared during finalization.

-

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

Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

[Jaikiran Pai]

On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8285504
>> 
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
>> a single code review, so it was decided to split into smaller chunks. This 
>> is one such chunk: 
>> 
>> open/src/java.base/share/classes/java/net
>
> Mark Powers has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Alan Bateman comments

src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line 
71:

> 69: }
> 70: 
> 71: this.parameters = List.copyOf(parameters);

Hello Mark, this would actually be a change in behaviour. The `List.copyOf` 
says:

> The given Collection must not be null and it must not contain any null 
> elements.

The current implementation of the public constructor on the public 
`KeyStoreBuilderParameters` mandates no such requirement. So if there's some 
code which currently passes a list with a null element in it, then this change 
will now end up throwing a `NullPointerException` as per the contract of 
`List.copyOf`.

-

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

Re: RFR: 8284910: Buffer clean in PasswordCallback [v5]

[Xue-Lei Andrew Fan]

> 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:

  correct test typo and test clearPassword()

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8272/files
  - new: https://git.openjdk.java.net/jdk/pull/8272/files/01542bb6..aaedee46

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

  Stats: 12 lines in 1 file changed: 10 ins; 0 del; 2 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

Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v8]

[Xue-Lei Andrew Fan]

On Tue, 26 Apr 2022 02:02:09 GMT, Weijun Wang  wrote:

> Where is a test for `GSSCredential`?

I did not find a way to create a GSSCredential object successfully, exception 
thrown.  Is there an example I can refer to?

-

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

Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v8]

[Xue-Lei Andrew Fan]

On Tue, 26 Apr 2022 01:53:43 GMT, Weijun Wang  wrote:

>> Xue-Lei Andrew Fan 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 and resovle merge conflict
>>  - change the calling order in dispose()
>>  - More code cleanup
>>  - re-org the code per feedback
>>  - Update to set context method
>>  - add test cases
>>  - Merge
>>  - Update copyright year
>>  - the object reference issue for Cleaner
>>  - 8284490: Remove finalizer method in java.security.jgss
>
> src/java.security.jgss/share/classes/sun/security/jgss/wrapper/GSSNameElement.java
>  line 54:
> 
>> 52: private final Cleaner.Cleanable cleanable;
>> 53: 
>> 54: long pName = 0; // Pointer to the gss_name_t structure
> 
> Can this be final?

Did you mean pName? The dispose() method will reset it to zero.  'pName" is 
used a lot in native implementation.  It may be doable to make it final, but it 
may be more complicated than I could expect.  I would like to leave it as it is 
in this PR.

-

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

Re: Reproducer for JDK-8221218

[Alan Bateman]

On 25/04/2022 15:45, Flavia Rainone wrote:

Hi everyone,

I work with the XNIO ( https://github.com/xnio/xnio/ ) project, led by 
David Lloyd in CC.


I'm not sure if this is the best way to get in touch, but I could not 
find out how to create an account for OpenJDK Jira to add a comment there.


We have a reproducer for JDK-8221218, and it appears it is not solved 
yet (tested with jdk 11.0.12). It is not intermittent, and it happens 
when we try to do an SSLEngine.unwrap with a zero remaining() 
ByteBuffer (in older versions, the engine would just return a 
BUFFER_UNDERFLOW result).




JDK-8221218 says it has been fixed in JDK 17. Have you tested with JDK 
17 or JDK 18 ?


-Alan

Re: RFR: 8285398: Cache the results of constraint checks

[Daniel Jeliński]

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.

Thanks for reviewing!

-

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

Integrated: 8285398: Cache the results of constraint checks

[Daniel Jeliński]

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.

This pull request has now been integrated.

Changeset: 00e9c96d
Author:Daniel Jeliński 
URL:   
https://git.openjdk.java.net/jdk/commit/00e9c96d51bec53d4ae8a07c9c98af2c62f3d290
Stats: 26 lines in 1 file changed: 23 ins; 1 del; 2 mod

8285398: Cache the results of constraint checks

Reviewed-by: coffeys, xuelei

-

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