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

[Valerie Peng]

On Wed, 6 Apr 2022 19:48:04 GMT, Valerie Peng  wrote:

>> src/java.base/share/classes/javax/crypto/Cipher.java line 1053:
>> 
>>> 1051:  * If this cipher has been previously initialized with 
>>> parameters,
>>> 1052:  * this method returns the same parameters. Otherwise, this 
>>> method may
>>> 1053:  * return a combination of user-supplied, default and randomly 
>>> generated
>> 
>>> ... a combination of user-supplied, default and randomly generated 
>>> parameter values ...
>> 
>> Other than init(), which APIs could impact the user-supplied parameters?  Is 
>> it the getInstance()?
>> 
>> Is it OK to just use the provider-specific default values instead, by 
>> replacing the "a combination of user-supplied, default and randomly 
>> generated ..."?
>
> I am thinking just init() as it takes parameters.
> Let me take a second look at various Cipher algorithm implementations and see 
> if this can be further pared down.

Please see the updated wording in earlier comment above.

-

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

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

[Valerie Peng]

On Wed, 6 Apr 2022 19:41:28 GMT, Xue-Lei Andrew Fan  wrote:

>> Can you be more specific like which block of code that you are referring to 
>> as the filtering and modification?
>> I'd expect the intention is to use the parameter specified. Otherwise an 
>> exception should be thrown if the specified parameter is invalid. Only when 
>> no parameter is specified, the provider would try to generate the parameters 
>> using whatever values it has or got.
>
> I read the following methods in com.sun.crypto.provider.CipherCore:
> 
> void init(int opmode, Key key, AlgorithmParameterSpec params,
> SecureRandom random)
> 
> where the 'params' are converted to IV byte array for further processing. 
> 
> 
> void init(int opmode, Key key, AlgorithmParameters params,
>   SecureRandom random)
> 
> where the spec is retrieved from the 'params', and then pass the call to the 
> init() method above.
> 
> 
> AlgorithmParameters getParameters(String algName) {
> 
> where the returned parameters are re-constructed.
> 
> It may be fine to use a strict spec, but there is a chance to have 
> compatibility impact unless we check the implementation carefully.  It may 
> not worthy the risks as a behavioral update may be not necessary for 
> developers.

How about this then?

 * The returned parameters may be the same that were used to initialize
 * this cipher, or may contain additional default and random parameter
 * values used by the underlying cipher implementation if this
 * cipher can successfully generate the required parameter values when
 * not supplied. Otherwise, {@code null} is returned.

-

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

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

[Valerie Peng]

On Mon, 11 Apr 2022 15:20:22 GMT, Sean Mullan  wrote:

>> Anyone can help review this javadoc update? The main change is the wording 
>> for the method javadoc of 
>> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording 
>> is somewhat restrictive and request is to broaden this to accommodate more 
>> scenarios such as when null can be returned.
>> The rest are minor things like add {@code } to class name and null, and 
>> remove redundant ".". 
>> 
>> Will file CSR after the review is close to being wrapped up.
>> Thanks~
>
> src/java.base/share/classes/javax/crypto/Cipher.java line 488:
> 
>> 486:  * A new {@code Cipher} object encapsulating the
>> 487:  * {@code CipherSpi} implementation from the first
>> 488:  * Provider that supports the specified algorithm is returned.
> 
> Since "Provider" is capitalized, I think the intent was that this was the 
> classname, so it should also probably be in an `@code` tag. Alternatively, 
> you could change this to non-capitalized "provider" (w/o the @code tag) and I 
> think it would still be readable (and my vote would be for this).

Sure, I will use the non-capitalized "provider" whenever suitable. For some 
cases, there are "provider" argument, so it looks that {@code provider} is more 
suitable.

> src/java.base/share/classes/javax/crypto/Cipher.java line 655:
> 
>> 653:  *
>> 654:  *  A new {@code Cipher} object encapsulating the
>> 655:  * {@code CipherSpi} implementation from the specified Provider
> 
> Since `Provider` here is a parameter, it is probably better to put this in an 
> `@code` tag.

Yes, I will use the {@code provider} for the "provider" parameter.

> src/java.base/share/classes/javax/crypto/Cipher.java line 2641:
> 
>> 2639:  *
>> 2640:  * @param transformation the cipher transformation
>> 2641:  * @return the maximum key length in bits or Integer.MAX_VALUE
> 
> Integer.MAX_VALUE should be inside a `@code` tag.

Ok.

-

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

Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec

[Michael StJohns]

On 4/11/2022 9:34 PM, Valerie Peng wrote:

This trivial change is to deprecate the DEFAULT static field of 
OAEPParameterSpec class. Wordings are mostly the same as the previous 
PSSParameterSpec deprecation change. Rest are just minor code re-factoring.

The CSR will be filed once review is somewhat finished.

Thanks,
Valerie

-

Commit messages:
  - 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec

Changes:https://git.openjdk.java.net/jdk/pull/8191/files
  Webrev:https://webrevs.openjdk.java.net/?repo=jdk=8191=00
   Issue:https://bugs.openjdk.java.net/browse/JDK-8284553
   Stats: 42 lines in 1 file changed: 13 ins; 10 del; 19 mod
   Patch:https://git.openjdk.java.net/jdk/pull/8191.diff
   Fetch: git fetchhttps://git.openjdk.java.net/jdk  pull/8191/head:pull/8191

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


Hi Valerie -

I think deprecating DEFAULT  is wrong.  RFC8017 still has this definition:


RSAES-OAEP-params ::= SEQUENCE {
hashAlgorithm  [0] HashAlgorithm DEFAULT sha1,
maskGenAlgorithm   [1] MaskGenAlgorithm  DEFAULT mgf1SHA1,
pSourceAlgorithm   [2] PSourceAlgorithm  DEFAULT pSpecifiedEmpty
}
and DEFAULT is what you should be getting if you see an empty sequence 
in the param field.  It's DEFAULT in ASN1 terms, not DEFAULT in terms of 
what you should use going forward  to create signatures, and the ASN1 
DEFAULT won't change.


In any event, I can't find where RFC8017 says anything about deprecating 
the defaults.  AFAICT, although there's general guidance to go away from 
SHA1,  the math suggests that SHA1 is still sufficient for OAEP,  and 
there's no guidance specific to OAEP's use of SHA1 that I can find other 
than the requirement in NIST SP800-56B rev 2 to use "Approved Hash 
Functions" for OAEP. If there's a section in 8017 that you're looking at 
for this guidance that I missed, you may want to update your comment to 
point to it.


Take care - Mike

RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec

[Valerie Peng]

This trivial change is to deprecate the DEFAULT static field of 
OAEPParameterSpec class. Wordings are mostly the same as the previous 
PSSParameterSpec deprecation change. Rest are just minor code re-factoring.

The CSR will be filed once review is somewhat finished.

Thanks,
Valerie

-

Commit messages:
 - 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec

Changes: https://git.openjdk.java.net/jdk/pull/8191/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8191=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8284553
  Stats: 42 lines in 1 file changed: 13 ins; 10 del; 19 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8191.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8191/head:pull/8191

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

Re: RFR: JDK-8284688 Minor cleanup could be done in java.security.jgss

[Dalibor Topic]

On Tue, 8 Mar 2022 17:21:34 GMT, Mark Powers  wrote:

> https://bugs.openjdk.java.net/browse/JDK-8284688
> 
> [JDK-8273046](https://bugs.openjdk.java.net/browse/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.security.jgss/share/classes/javax/security 
> open/src/java.security.jgss/share/classes/org/ietf 
> open/src/java.security.jgss/share/classes/sun/security

Hi, please send an e-mail to dalibor.to...@oracle.com so that I can verify your 
account.

-

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

RFR: JDK-8284688 Minor cleanup could be done in java.security.jgss

[Mark Powers]

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

[JDK-8273046](https://bugs.openjdk.java.net/browse/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.security.jgss/share/classes/javax/security 
open/src/java.security.jgss/share/classes/org/ietf 
open/src/java.security.jgss/share/classes/sun/security

-

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

Changes: https://git.openjdk.java.net/jdk/pull/7746/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7746=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8284688
  Stats: 1009 lines in 73 files changed: 79 ins; 222 del; 708 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7746.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7746/head:pull/7746

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

Re: Proposal: Extend Windows KeyStore support to include access to the local machine location

[Wei-Jun Wang]

Added a comment and assigned the enhancement to you. Thanks.

--Weijun

> On Apr 11, 2022, at 5:02 PM, Mat Carter  wrote:
> 
> Thanks, Weijun
> 
> Let's move ahead with the two new strings while we consider read-only access. 
>  As the current assignee can you update the JBS issue [1] with what we've 
> agreed here.
> 
> I have an implementation that I've been testing in 11u which can easily move 
> to tip; if you are happy for me to make the changes then please ack here and 
> re-assign the issue to me
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-6782021
> 
> 
> Thanks
> Mat

Re: Proposal: Extend Windows KeyStore support to include access to the local machine location

[Mat Carter]

Thanks, Weijun

Let's move ahead with the two new strings while we consider read-only access.  
As the current assignee can you update the JBS issue [1] with what we've agreed 
here.

I have an implementation that I've been testing in 11u which can easily move to 
tip; if you are happy for me to make the changes then please ack here and 
re-assign the issue to me

[1] https://bugs.openjdk.java.net/browse/JDK-6782021


Thanks
Mat


Sent from Outlook


From: Wei-Jun Wang 
Sent: Monday, April 11, 2022 11:45 AM
To: Mat Carter 
Cc: Bernd Eckenfels ; security-dev@openjdk.java.net 

Subject: Re: Proposal: Extend Windows KeyStore support to include access to the 
local machine location

Sorry for the late reply.

Yes, your suggestions are good. We can support "Windows-MY-LOCALMACHINE" and 
"Windows-ROOT-LOCALMACHINE".

For read-only access to the keystores, we have already allowed writing and this 
probably will not change. If we do want to support read-only later, it could be 
a new name or a different LoadStoreParameters.

As for Bernd's suggestion. Maybe DomainKeyStore can be used to combine existing 
keystores. I dare not add ADDRESSBOOK at the moment. Are certificates inside it 
fully trusted? Or you need to build a certpath to one of the root CAs to trust 
any of them?

Thanks,
Weijun

> On Apr 11, 2022, at 2:06 PM, Mat Carter  wrote:
>
> Hi Weijun
>
> Did my answers address your concerns?  Also do you have an opinion on Bernd's 
> suggestion?
>
> Thanks in advance
> Mat
>
> Sent from Outlook
> From: security-dev  on behalf of Bernd 
> Eckenfels 
> Sent: Tuesday, April 5, 2022 11:20 AM
> To: security-dev@openjdk.java.net 
> Subject: Re: Proposal: Extend Windows KeyStore support to include access to 
> the local machine location
>
> BTW, since this is Windows specific anyway and since we have also a combining 
> virtual Keystore, why not allow a new naming scheme which allows to access 
> any of the Keystores? like “Windows-ROOT/ADdressbook”?
>
> Gruss
> Bernd
>
> --
> https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fbernd.eckenfels.net%2Fdata=05%7C01%7CMatthew.Carter%40microsoft.com%7C9492ccb410834f633f4708da1beb6d02%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637852995228154570%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=qjRXhchorKHZFaQY4zuwXqSwVjmda7HkWJEN%2FtMeaT4%3Dreserved=0
> Von: security-dev  im Auftrag von Mat 
> Carter 
> Gesendet: Dienstag, April 5, 2022 5:22 PM
> An: Wei-Jun Wang 
> Cc: security-dev@openjdk.java.net 
> Betreff: Re: Proposal: Extend Windows KeyStore support to include access to 
> the local machine location
>
> Hi Weijun
>
> Thank you for the feedback, I'd like to address point 2 first as I think this 
> might also address point 1
>
> >> 2. PrivateKeyEntry is (IMO) mainly used for client auth in TLS. We don't 
> >> want new entries suddenly appear
> >> there and automatically chosen by a key manager.
> >>
> >> It looks OK to enhance Windows-ROOT to cover more root CA certs in your 
> >> organization but including
> >> new entries in Windows-MY is a little dangerous. It's OK to introduce a 
> >> new store type for MY in LOCAL_MACHINE.
>
> I deliberately kept implementation details out of the initial email to focus 
> on the security aspects, but this point makes an assumption that the results 
> of using "Windows-MY" or "Windows-ROOT" would change with this new 
> functionality; this is not what we're proposing.  Specifically we're 
> proposing adding two new strings "Windows-MY-LOCALMACHINE" and
> "Windows-ROOT-LOCALMACHINE" such that developers can now access the key 
> stores in the local machine. To be clear, the implementation would make no 
> attempt to "merge" results when enumerating or to search both locations via a 
> single key store instance; i.e. you can only create and instance for 
> accessing either keystore but not both.
>
> I think this addresses point 1 also, but if not then I have a follow on 
> question:
>
> >> 1. In Java's KeyStore a certificate entry is called 
> >> TrustedCertificateEntry. The name implies that the certificate is
> >> trusted for any purpose. We don't want some certificates that were not 
> >> meant to be trusted shown up.
>
> Our initial analysis leads us to believe that we'll not need to introduce new 
> code paths to handle new certificates; i.e. the only code changes are how the 
> key store is opened, subsequent calls to access certificates is handled by 
> the existing code.
>
> Given the above assumption, your concerns laid out in point 1 and if your 
> concern is not mitigated with our notes for point 2: is it the case that you 
> expect new "types" of certificates to be accessible via local machine that 
> weren't via current user and that some/all of these certs are "bad" (and 
> would need new code paths to handle them)?
>
> While we are talking about implementation, there's another aspect 

Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary

[Bradford Wetmore]

On Wed, 2 Mar 2022 19:04:26 GMT, zzambers  wrote:

> When testing compatibility of jdk TLS implementation with gnutls, I have 
> found a problem. The problem is, that gnutls does not like use of 
> user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() 
> (used by socket.close() unless shutdownOutput was called explicitly) and 
> considers it error. (For more details see: [1])
> 
> As I understand it, usage of user_canceled alert before close is workaround 
> for an issue of not being able to cleanly initialize full (duplex) close of 
> TLS-1.3 connection (other side is not required to immediately close the after 
> receiving close_notify, unlike in earlier TLS versions). Some legacy programs 
> could probably hang or something, expecting socket.close to perform immediate 
> duplex close. Problem is this is not what user_canceled alert is intended for 
> [2] and it is therefore undefined how the other side handles this. (JDK 
> itself replies to close_notify preceded by user_canceled alert by immediately 
> closing its output [3].)
> 
> This fix disables this workaround when it is not necessary (connection is 
> already half-closed by the other side). This way it fixes my case (gnutls 
> client connected to jdk server initiates close) and it should be safe. (As 
> removing workaround altogether could probably reintroduce issues for legacy 
> apps... )
> 
> I also ran jdk_security tests locally, which passed for me.
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473
> [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1
> [3] 
> https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243

Resettting the clock.  Sorry for the delay.

-

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

Re: Proposal: Extend Windows KeyStore support to include access to the local machine location

[Bernd Eckenfels]

Hello,

if you can open/target specific stores dynamically it is up to the 
developer/user what they do with it (very similar to keystore files). 
Addressbook in my post was only an example (but a good one: imagine Java app 
wants to import the addressbook entries)

Gruss
Bernd
--
https://bernd.eckenfels.net

From: Wei-Jun Wang 
Sent: Monday, April 11, 2022 8:45:12 PM
To: Mat Carter 
Cc: Bernd Eckenfels ; security-dev@openjdk.java.net 

Subject: Re: Proposal: Extend Windows KeyStore support to include access to the 
local machine location

Sorry for the late reply.

Yes, your suggestions are good. We can support "Windows-MY-LOCALMACHINE" and 
"Windows-ROOT-LOCALMACHINE".

For read-only access to the keystores, we have already allowed writing and this 
probably will not change. If we do want to support read-only later, it could be 
a new name or a different LoadStoreParameters.

As for Bernd's suggestion. Maybe DomainKeyStore can be used to combine existing 
keystores. I dare not add ADDRESSBOOK at the moment. Are certificates inside it 
fully trusted? Or you need to build a certpath to one of the root CAs to trust 
any of them?

Thanks,
Weijun

> On Apr 11, 2022, at 2:06 PM, Mat Carter  wrote:
>
> Hi Weijun
>
> Did my answers address your concerns?  Also do you have an opinion on Bernd's 
> suggestion?
>
> Thanks in advance
> Mat
>
> Sent from Outlook
> From: security-dev  on behalf of Bernd 
> Eckenfels 
> Sent: Tuesday, April 5, 2022 11:20 AM
> To: security-dev@openjdk.java.net 
> Subject: Re: Proposal: Extend Windows KeyStore support to include access to 
> the local machine location
>
> BTW, since this is Windows specific anyway and since we have also a combining 
> virtual Keystore, why not allow a new naming scheme which allows to access 
> any of the Keystores? like “Windows-ROOT/ADdressbook”?
>
> Gruss
> Bernd
>
> --
> http://bernd.eckenfels.net
> Von: security-dev  im Auftrag von Mat 
> Carter 
> Gesendet: Dienstag, April 5, 2022 5:22 PM
> An: Wei-Jun Wang 
> Cc: security-dev@openjdk.java.net 
> Betreff: Re: Proposal: Extend Windows KeyStore support to include access to 
> the local machine location
>
> Hi Weijun
>
> Thank you for the feedback, I'd like to address point 2 first as I think this 
> might also address point 1
>
> >> 2. PrivateKeyEntry is (IMO) mainly used for client auth in TLS. We don't 
> >> want new entries suddenly appear
> >> there and automatically chosen by a key manager.
> >>
> >> It looks OK to enhance Windows-ROOT to cover more root CA certs in your 
> >> organization but including
> >> new entries in Windows-MY is a little dangerous. It's OK to introduce a 
> >> new store type for MY in LOCAL_MACHINE.
>
> I deliberately kept implementation details out of the initial email to focus 
> on the security aspects, but this point makes an assumption that the results 
> of using "Windows-MY" or "Windows-ROOT" would change with this new 
> functionality; this is not what we're proposing.  Specifically we're 
> proposing adding two new strings "Windows-MY-LOCALMACHINE" and
> "Windows-ROOT-LOCALMACHINE" such that developers can now access the key 
> stores in the local machine. To be clear, the implementation would make no 
> attempt to "merge" results when enumerating or to search both locations via a 
> single key store instance; i.e. you can only create and instance for 
> accessing either keystore but not both.
>
> I think this addresses point 1 also, but if not then I have a follow on 
> question:
>
> >> 1. In Java's KeyStore a certificate entry is called 
> >> TrustedCertificateEntry. The name implies that the certificate is
> >> trusted for any purpose. We don't want some certificates that were not 
> >> meant to be trusted shown up.
>
> Our initial analysis leads us to believe that we'll not need to introduce new 
> code paths to handle new certificates; i.e. the only code changes are how the 
> key store is opened, subsequent calls to access certificates is handled by 
> the existing code.
>
> Given the above assumption, your concerns laid out in point 1 and if your 
> concern is not mitigated with our notes for point 2: is it the case that you 
> expect new "types" of certificates to be accessible via local machine that 
> weren't via current user and that some/all of these certs are "bad" (and 
> would need new code paths to handle them)?
>
> While we are talking about implementation, there's another aspect we'd like 
> to introduce/discuss: this is to allow developers to access the key stores 
> with read only permissions, thus allowing enumeration and reading without 
> requiring administrative permissions be granted to the application (thus 
> increasing security)
>
> Thanks in advance
> Mat
>
> Sent from Outlook
>
>
> From: Wei-Jun Wang 
> Sent: Friday, April 1, 2022 3:15 PM
> To: Mat Carter 
> Cc: security-dev@openjdk.java.net 
> Subject: Re: Proposal: Extend Windows KeyStore support to include access to 
> the local machine location
>

Re: Proposal: Extend Windows KeyStore support to include access to the local machine location

[Wei-Jun Wang]

Sorry for the late reply.

Yes, your suggestions are good. We can support "Windows-MY-LOCALMACHINE" and 
"Windows-ROOT-LOCALMACHINE".

For read-only access to the keystores, we have already allowed writing and this 
probably will not change. If we do want to support read-only later, it could be 
a new name or a different LoadStoreParameters.

As for Bernd's suggestion. Maybe DomainKeyStore can be used to combine existing 
keystores. I dare not add ADDRESSBOOK at the moment. Are certificates inside it 
fully trusted? Or you need to build a certpath to one of the root CAs to trust 
any of them?

Thanks,
Weijun

> On Apr 11, 2022, at 2:06 PM, Mat Carter  wrote:
> 
> Hi Weijun
> 
> Did my answers address your concerns?  Also do you have an opinion on Bernd's 
> suggestion?
> 
> Thanks in advance
> Mat
> 
> Sent from Outlook
> From: security-dev  on behalf of Bernd 
> Eckenfels 
> Sent: Tuesday, April 5, 2022 11:20 AM
> To: security-dev@openjdk.java.net 
> Subject: Re: Proposal: Extend Windows KeyStore support to include access to 
> the local machine location
>  
> BTW, since this is Windows specific anyway and since we have also a combining 
> virtual Keystore, why not allow a new naming scheme which allows to access 
> any of the Keystores? like “Windows-ROOT/ADdressbook”?
> 
> Gruss
> Bernd
> 
> -- 
> http://bernd.eckenfels.net
> Von: security-dev  im Auftrag von Mat 
> Carter 
> Gesendet: Dienstag, April 5, 2022 5:22 PM
> An: Wei-Jun Wang 
> Cc: security-dev@openjdk.java.net 
> Betreff: Re: Proposal: Extend Windows KeyStore support to include access to 
> the local machine location
>  
> Hi Weijun
> 
> Thank you for the feedback, I'd like to address point 2 first as I think this 
> might also address point 1
> 
> >> 2. PrivateKeyEntry is (IMO) mainly used for client auth in TLS. We don't 
> >> want new entries suddenly appear 
> >> there and automatically chosen by a key manager.
> >>
> >> It looks OK to enhance Windows-ROOT to cover more root CA certs in your 
> >> organization but including 
> >> new entries in Windows-MY is a little dangerous. It's OK to introduce a 
> >> new store type for MY in LOCAL_MACHINE.
> 
> I deliberately kept implementation details out of the initial email to focus 
> on the security aspects, but this point makes an assumption that the results 
> of using "Windows-MY" or "Windows-ROOT" would change with this new 
> functionality; this is not what we're proposing.  Specifically we're 
> proposing adding two new strings "Windows-MY-LOCALMACHINE" and 
> "Windows-ROOT-LOCALMACHINE" such that developers can now access the key 
> stores in the local machine. To be clear, the implementation would make no 
> attempt to "merge" results when enumerating or to search both locations via a 
> single key store instance; i.e. you can only create and instance for 
> accessing either keystore but not both.
> 
> I think this addresses point 1 also, but if not then I have a follow on 
> question:
> 
> >> 1. In Java's KeyStore a certificate entry is called 
> >> TrustedCertificateEntry. The name implies that the certificate is 
> >> trusted for any purpose. We don't want some certificates that were not 
> >> meant to be trusted shown up.
> 
> Our initial analysis leads us to believe that we'll not need to introduce new 
> code paths to handle new certificates; i.e. the only code changes are how the 
> key store is opened, subsequent calls to access certificates is handled by 
> the existing code.
> 
> Given the above assumption, your concerns laid out in point 1 and if your 
> concern is not mitigated with our notes for point 2: is it the case that you 
> expect new "types" of certificates to be accessible via local machine that 
> weren't via current user and that some/all of these certs are "bad" (and 
> would need new code paths to handle them)?
> 
> While we are talking about implementation, there's another aspect we'd like 
> to introduce/discuss: this is to allow developers to access the key stores 
> with read only permissions, thus allowing enumeration and reading without 
> requiring administrative permissions be granted to the application (thus 
> increasing security)
> 
> Thanks in advance
> Mat
> 
> Sent from Outlook
> 
> 
> From: Wei-Jun Wang 
> Sent: Friday, April 1, 2022 3:15 PM
> To: Mat Carter 
> Cc: security-dev@openjdk.java.net 
> Subject: Re: Proposal: Extend Windows KeyStore support to include access to 
> the local machine location 
>  
> Hi Mat,
> 
> We have 2 main concerns:
> 
> 1. In Java's KeyStore a certificate entry is called TrustedCertificateEntry. 
> The name implies that the certificate is trusted for any purpose. We don't 
> want some certificates that were not meant to be trusted shown up.
> 
> 2. PrivateKeyEntry is (IMO) mainly used for client auth in TLS. We don't want 
> new entries suddenly appear there and automatically chosen by a key manager.
> 
> It looks OK to enhance Windows-ROOT to cover more root CA certs in your 
> organization but including new 

Re: Proposal: Extend Windows KeyStore support to include access to the local machine location

[Mat Carter]

Hi Weijun

Did my answers address your concerns?  Also do you have an opinion on Bernd's 
suggestion?

Thanks in advance
Mat


Sent from Outlook


From: security-dev  on behalf of Bernd 
Eckenfels 
Sent: Tuesday, April 5, 2022 11:20 AM
To: security-dev@openjdk.java.net 
Subject: Re: Proposal: Extend Windows KeyStore support to include access to the 
local machine location

BTW, since this is Windows specific anyway and since we have also a combining 
virtual Keystore, why not allow a new naming scheme which allows to access any 
of the Keystores? like “Windows-ROOT/ADdressbook”?

Gruss
Bernd

--
http://bernd.eckenfels.net

Von: security-dev  im Auftrag von Mat 
Carter 
Gesendet: Dienstag, April 5, 2022 5:22 PM
An: Wei-Jun Wang 
Cc: security-dev@openjdk.java.net 
Betreff: Re: Proposal: Extend Windows KeyStore support to include access to the 
local machine location

Hi Weijun

Thank you for the feedback, I'd like to address point 2 first as I think this 
might also address point 1

>> 2. PrivateKeyEntry is (IMO) mainly used for client auth in TLS. We don't 
>> want new entries suddenly appear
>> there and automatically chosen by a key manager.
>>
>> It looks OK to enhance Windows-ROOT to cover more root CA certs in your 
>> organization but including
>> new entries in Windows-MY is a little dangerous. It's OK to introduce a new 
>> store type for MY in LOCAL_MACHINE.

I deliberately kept implementation details out of the initial email to focus on 
the security aspects, but this point makes an assumption that the results of 
using "Windows-MY" or "Windows-ROOT" would change with this new functionality; 
this is not what we're proposing.  Specifically we're proposing adding two new 
strings "Windows-MY-LOCALMACHINE" and
"Windows-ROOT-LOCALMACHINE" such that developers can now access the key stores 
in the local machine. To be clear, the implementation would make no attempt to 
"merge" results when enumerating or to search both locations via a single key 
store instance; i.e. you can only create and instance for accessing either 
keystore but not both.

I think this addresses point 1 also, but if not then I have a follow on 
question:

>> 1. In Java's KeyStore a certificate entry is called TrustedCertificateEntry. 
>> The name implies that the certificate is
>> trusted for any purpose. We don't want some certificates that were not meant 
>> to be trusted shown up.

Our initial analysis leads us to believe that we'll not need to introduce new 
code paths to handle new certificates; i.e. the only code changes are how the 
key store is opened, subsequent calls to access certificates is handled by the 
existing code.

Given the above assumption, your concerns laid out in point 1 and if your 
concern is not mitigated with our notes for point 2: is it the case that you 
expect new "types" of certificates to be accessible via local machine that 
weren't via current user and that some/all of these certs are "bad" (and would 
need new code paths to handle them)?

While we are talking about implementation, there's another aspect we'd like to 
introduce/discuss: this is to allow developers to access the key stores with 
read only permissions, thus allowing enumeration and reading without requiring 
administrative permissions be granted to the application (thus increasing 
security)

Thanks in advance
Mat

Sent from Outlook


From: Wei-Jun Wang 
Sent: Friday, April 1, 2022 3:15 PM
To: Mat Carter 
Cc: security-dev@openjdk.java.net 
Subject: Re: Proposal: Extend Windows KeyStore support to include access to the 
local machine location

Hi Mat,

We have 2 main concerns:

1. In Java's KeyStore a certificate entry is called TrustedCertificateEntry. 
The name implies that the certificate is trusted for any purpose. We don't want 
some certificates that were not meant to be trusted shown up.

2. PrivateKeyEntry is (IMO) mainly used for client auth in TLS. We don't want 
new entries suddenly appear there and automatically chosen by a key manager.

It looks OK to enhance Windows-ROOT to cover more root CA certs in your 
organization but including new entries in Windows-MY is a little dangerous. 
It's OK to introduce a new store type for MY in LOCAL_MACHINE.

And we have no plan to add other types like ADDRESSBOOK.

Thanks,
Weijun

> On Mar 31, 2022, at 5:16 PM, Mat Carter  wrote:
>
> Current support for KeyStores on Windows is limited to the current user 
> location [1]
>
> There has been previous request for local machine support [2] along with 
> discussion in the security-dev mailing list [3], further discussions have 
> occurred on stackoverflow in the past [4] and [5]
>
> Using JNI you can access local machine locations but then you are duplicating 
> much of the existing native functionality; this also adds the requirement 
> that developers need to know C/C++ and the Windows cryptography API.
>
> Given the above I propose that we add 

Integrated: 8284105: Update security libraries to use sealed classes

[Sean Mullan]

On Fri, 8 Apr 2022 13:40:37 GMT, Sean Mullan  wrote:

> Please review these changes to update the security libraries to use sealed 
> classes. See JEP 409 for more details on sealed classes.
> 
> No CSR is required as all the changes are to internal classes. A few classes 
> that did not have subclasses were simply marked final instead of sealed.

This pull request has now been integrated.

Changeset: dc6ec2a4
Author:Sean Mullan 
URL:   
https://git.openjdk.java.net/jdk/commit/dc6ec2a46720eaf0cc7ce36a732ba8d4679a50d5
Stats: 139 lines in 23 files changed: 50 ins; 38 del; 51 mod

8284105: Update security libraries to use sealed classes

Reviewed-by: darcy, weijun, xuelei

-

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

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

[Sean Mullan]

On Wed, 6 Apr 2022 00:14:04 GMT, Valerie Peng  wrote:

> Anyone can help review this javadoc update? The main change is the wording 
> for the method javadoc of 
> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording 
> is somewhat restrictive and request is to broaden this to accommodate more 
> scenarios such as when null can be returned.
> The rest are minor things like add {@code } to class name and null, and 
> remove redundant ".". 
> 
> Will file CSR after the review is close to being wrapped up.
> Thanks~

src/java.base/share/classes/javax/crypto/Cipher.java line 488:

> 486:  * A new {@code Cipher} object encapsulating the
> 487:  * {@code CipherSpi} implementation from the first
> 488:  * Provider that supports the specified algorithm is returned.

Since "Provider" is capitalized, I think the intent was that this was the 
classname, so it should also probably be in an `@code` tag. Alternatively, you 
could change this to non-capitalized "provider" (w/o the @code tag) and I think 
it would still be readable (and my vote would be for this).

src/java.base/share/classes/javax/crypto/Cipher.java line 655:

> 653:  *
> 654:  *  A new {@code Cipher} object encapsulating the
> 655:  * {@code CipherSpi} implementation from the specified Provider

Since `Provider` here is a parameter, it is probably better to put this in an 
`@code` tag.

src/java.base/share/classes/javax/crypto/Cipher.java line 2641:

> 2639:  *
> 2640:  * @param transformation the cipher transformation
> 2641:  * @return the maximum key length in bits or Integer.MAX_VALUE

Integer.MAX_VALUE should be inside a `@code` tag.

-

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

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

[Daniel Fuchs]

On Sat, 9 Apr 2022 06:19:12 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 incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains five additional 
> commits since the last revision:
> 
>  - 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 188:

> 186: printableType);
> 187: 
> 188: cleanable = Krb5Util.cleaner.register(this, disposerFor(stub, 
> pName));

I wonder if this line should be moved just after line 159?

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

> 305: }
> 306: };
> 307: }

There seem to be a potential optimization here, since you don't even need to 
create a Cleanable in the case where stub is null or pName is 0? Same for 
earlier duplication of the same code.

-

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

Re: RFR: 8284105: Update security libraries to use sealed classes [v3]

[Sean Mullan]

> Please review these changes to update the security libraries to use sealed 
> classes. See JEP 409 for more details on sealed classes.
> 
> No CSR is required as all the changes are to internal classes. A few classes 
> that did not have subclasses were simply marked final instead of sealed.

Sean Mullan 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 four additional commits since 
the last revision:

 - Make KrbAsRep final.
 - Merge
 - Make some classes package-private instead of sealed.
 - Update security libraries to use sealed classes.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8165/files
  - new: https://git.openjdk.java.net/jdk/pull/8165/files/c27b1ba0..2a1ab2b8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8165=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8165=01-02

  Stats: 4066 lines in 394 files changed: 1983 ins; 791 del; 1292 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8165.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8165/head:pull/8165

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

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v24]

[Maurizio Cimadamore]

On Mon, 11 Apr 2022 10:33:54 GMT, David Holmes  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix TestLinkToNativeRBP
>
> src/hotspot/share/prims/scopedMemoryAccess.cpp line 141:
> 
>> 139: 
>> 140: /*
>> 141:  * This function performs a thread-local handshake against all threads 
>> running at the time
> 
> Nit: thread-local??

I was assuming the term "thread-local handshake" was a thing in the VM, as per:
https://openjdk.java.net/jeps/312

-

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

Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v24]

[David Holmes]

On Mon, 4 Apr 2022 14:57:30 GMT, Maurizio Cimadamore  
wrote:

>> This PR contains the API and implementation changes for JEP-424 [1]. A more 
>> detailed description of such changes, to avoid repetitions during the review 
>> process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix TestLinkToNativeRBP

VM changes look good.

Thanks,
David

src/hotspot/share/prims/scopedMemoryAccess.cpp line 141:

> 139: 
> 140: /*
> 141:  * This function performs a thread-local handshake against all threads 
> running at the time

Nit: thread-local??

-

Marked as reviewed by dholmes (Reviewer).

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

Integrated: 8284641: Doc errors in sun.security.ssl.SSLSessionContextImpl

[John Jiang]

On Mon, 11 Apr 2022 04:02:44 GMT, John Jiang  wrote:

> JDK-8228396 turned stateless resumption on by default, but the JavaDoc was 
> not modified accordingly.
> And a "{" is missing for @systemProperty 
> jdk.tls.server.enableSessionTicketExtension.

This pull request has now been integrated.

Changeset: 40ddb755
Author:John Jiang 
URL:   
https://git.openjdk.java.net/jdk/commit/40ddb7558cd985d49aa5aaedae6c5145ba3d0ac0
Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod

8284641: Doc errors in sun.security.ssl.SSLSessionContextImpl

Reviewed-by: xuelei, ssahoo

-

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

Re: RFR: 8284641: Doc errors in sun.security.ssl.SSLSessionContextImpl

[Sibabrata Sahoo]

On Mon, 11 Apr 2022 04:02:44 GMT, John Jiang  wrote:

> JDK-8228396 turned stateless resumption on by default, but the JavaDoc was 
> not modified accordingly.
> And a "{" is missing for @systemProperty 
> jdk.tls.server.enableSessionTicketExtension.

Marked as reviewed by ssahoo (Committer).

-

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