Re: AlgorithmConstraints caching [ was Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice]

[Seán Coffey]

I think the work done with 8284694 will help alot in any case since I 
suspect the same SSLAlgorithmConstraints Object will be shared much more 
now (rather than spin off new copies)


Some recent JFRs I looked at show that alot of CPU cycles[1] get taken 
in the HandshakeContext methods of :

sun.security.ssl.HandshakeContext#getActiveCipherSuites
sun.security.ssl.HandshakeContext#getActiveProtocols

Both methods get called per Handshakecontext construction and I think 
each TLS handshake gets a new Handshakecontext.I was thinking that a 
cache of the last known variables used to deduce the 
getActiveCipherSuites and getActiveProtocols Lists could be created. 
That might have the potential to avoid alot of needless CPU cycles in 
this area since the parameters used to produce these Lists don't really 
change that often. I'm still looking into this potential and hope to 
share a patch shortly.


regards,
Sean.

[1]

Stack Trace    Count    Percentage
TreeMap$Entry java.util.TreeMap.getEntryUsingComparator(Object) 1035    
100 %

TreeMap$Entry java.util.TreeMap.getEntry(Object)    1035    100 %
boolean java.util.TreeMap.containsKey(Object)    1035    100 %
boolean java.util.TreeSet.contains(Object)    1035    100 %
boolean 
sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(Set, 
String, AlgorithmDecomposer)    1035    100 %
boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, 
String, AlgorithmParameters)    1035    100 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, 
AlgorithmParameters)    1035    100 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, 
AlgorithmParameters)    553    53.4 %
boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite, 
AlgorithmConstraints, Map)    309    29.9 %
List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, List, 
AlgorithmConstraints)    302    29.2 %
void sun.security.ssl.HandshakeContext.(SSLContextImpl, 
TransportContext)    302    29.2 %
void sun.security.ssl.ClientHandshakeContext.(SSLContextImpl, 
TransportContext)    302    29.2 %

void sun.security.ssl.TransportContext.kickstart()    302 29.2 %
void sun.security.ssl.SSLSocketImpl.startHandshake(boolean) 302    29.2 %


On 13/04/2022 18:05, Anthony Scarpino wrote:

Hi Sean,

Caching is an interesting idea.  I've wondered for a while off and on 
about how to speed it up, but hadn't come up with a solution I liked. 
The complication with caching is while something like an algorithm 
name only could be easy in a hashmap, it gets more complicated when 
you get into key sizes. Such as, how to represent RSA 1k being 
disallowed and but 2k allowed.. or certificate usage..


Tony

On 4/13/22 2:03 AM, Sean Coffey wrote:
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński 
 wrote:


During TLS handshake, hundreds of constraints are evaluated to 
determine which cipher suites are usable. Most of the evaluations 
are performed using `HandshakeContext#algorithmConstraints` object. 
By default that object contains a `SSLAlgorithmConstraints` instance 
wrapping another `SSLAlgorithmConstraints` instance. As a result the 
constraints defined in `SSLAlgorithmConstraints` are evaluated twice.


This PR improves the default case; if the user-specified constraints 
are left at defaults, we use a single `SSLAlgorithmConstraints` 
instance, and avoid duplicate checks.


Nice work. I've been looking at this area myself in recent weeks also 
while debugging some JDK 11u performance issues. JFR shows this area 
as costly. Some other JDK fixes in this area have already improved 
performance. This will certainly help. Pasting a stacktrace[1] from 
an old Oracle JDK 11.0.12 report for history purposes.


I think there are other improvements that can be made in the TLS 
constraints code. Caching the last known values from a constraints 
permits test is something I've begun looking into.


[1]
Stack Trace    Count    Percentage
boolean java.lang.StringLatin1.regionMatchesCI(byte[], int, byte[], 
int, int)    1637    100 %
boolean java.lang.String.regionMatches(boolean, int, String, int, 
int)    1637    100 %

boolean java.lang.String.equalsIgnoreCase(String)    1637 100 %
boolean 
sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List, 
String, AlgorithmDecomposer)    1631    99.6 %
boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, 
String, AlgorithmParameters)    1631    99.6 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, 
AlgorithmParameters)    1631    99.6 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, 
AlgorithmParameters)    836    51.1 %
boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite, 
AlgorithmConstraints, Map)    428    26.1 %
List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, 
List, AlgorithmConstraints)    418    25.5 %
void sun.security.ssl.HandshakeContext.(SSLContextImpl, 
TransportContext)    418    25.5 %
void 

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads [v2]

[Seán Coffey]

Hi Valerie,

many thanks for the thorough review. I've taken all your feedback on 
board with the latest push. Some of the test anomalies were a result of 
previous iterations of test edits I had been making.


Regarding the extra edits in 
"src/java.base/share/lib/security/default.policy", I had assumed it 
would be ok to tidy up the module under edit but I've reverted the 
unrelated changes now.


I was doubtful about removing the AccessController.doPrivileged blocks 
around the InnocuousThread.newSystemThread calls given that I wasn't 
sure which path the calling code would come from but on re-examination, 
I think it's ok to remove. The module provides the necessary permissions 
already and use of InnocuousThread solves the original permissions 
issue. Thanks for spotting!


In test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java 
:


> +if (args.length > 0) {
+Policy.setPolicy(new SimplePolicy());
+System.setSecurityManager(new SecurityManager());
+}


Just curious, why split the loop into 2 and set the SecurityManager in 
between the two loops? Can't we just set the policy/security manager 
before the loop?
The test infra requires various permissions including the problem 
setContextClassLoader permission. I figured it was better to set up the 
test infra first and then trigger the security manager.


New edits just pushed for review.

regards,
Sean.


On 25/06/2021 23:31, Valerie Peng wrote:

On Tue, 22 Jun 2021 20:08:03 GMT, Sean Coffey  wrote:


Sufficient permissions missing if this code was ever to run with 
SecurityManager.

Cleanest approach appears to be use of InnocuousThread to create the 
cleaner/poller threads.
Test case coverage extended to cover the SecurityManager scenario.

Reviewer request: @valeriepeng

Sean Coffey has updated the pull request incrementally with one additional 
commit since the last revision:

   Move TokenPoller to Runnable

src/java.base/share/lib/security/default.policy line 131:


129: permission java.lang.RuntimePermission 
"accessClassInPackage.com.sun.crypto.provider";
130: permission java.lang.RuntimePermission 
"accessClassInPackage.jdk.internal.misc";
131: permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.*";

Can we just do necessary changes? I noticed that this file seems to have mixed 
style, i.e. some lines are longer than 80 chars and some break into 2 lines 
with length less than 80 chars. Since the whole file is mixed, maybe just do 
what must be changed.

src/java.base/share/lib/security/default.policy line 142:


140: permission java.security.SecurityPermission 
"clearProviderProperties.*";
141: permission java.security.SecurityPermission "removeProviderProperty.*";
142: permission java.security.SecurityPermission 
"getProperty.auth.login.defaultCallbackHandler";

Same "avoid unnecessary changes" comment here.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
952:


950: AccessController.doPrivileged((PrivilegedAction) () -> {
951: Thread t = InnocuousThread.newSystemThread(
952: "Poller " + getName(),

nit: "Poller " -> "Poller-" (like before)?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
956:


954: assert t.getContextClassLoader() == null;
955: t.setDaemon(true);
956: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1033:


1031: }
1032: cleaner = new NativeResourceCleaner();
1033: AccessController.doPrivileged((PrivilegedAction) () -> {

It seems that the AccessController.doPrivileged((PrivilegedAction) () -> {} is 
un-necessary? I tried your test without it and test still passes.

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1039:


1037: assert t.getContextClassLoader() == null;
1038: t.setDaemon(true);
1039: t.setPriority(Thread.MIN_PRIORITY);

nit: supply this priority value as an argument to the 
InnocuousThread.newSystemThread() call instead?

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line 
1212:


1210:
1211: this.token = token;
1212: if (cleaner == null) {

This check seems duplicate to the one in createCleaner() call.

test/jdk/sun/security/pkcs11/Provider/MultipleLogins.java line 56:


54: System.out.println("No NSS config found. Skipping.");
55: return;
56: }

Move this if-check block of code up before the for-loop?

-

PR: 

Re: [jdk17] RFR: 8269034: AccessControlException for SunPKCS11 daemon threads

[Seán Coffey]

Thank for the feedback Peter. Comments inline.

On 22/06/2021 22:40, Peter Firmstone wrote:

Was ever to run with SecurityManager?
I found the issue while porting to jdk8u where Solaris uses a 
configuration file with the SunPKCS11 Provider by default - We have 
tests to register Providers while SecurityManager is in place. This 
failed for SunPKCS11.


When you see an AccessControlException, I'd recommend setting the 
following security debug property, so you can capture the 
ProtectionDomain that failed the access check: 
-Djava.security.debug=access:failure  Clearly there's a 
ProtectionDomain on the calling threads stack that doesn't have the 
necessary permission.  If you knew which one it was, you could just 
grant it java.lang.RuntimePermission "setContextClassLoader" 
permission in the policy file.
Yes - that was one of my first actions. [1]. The jdk.crypto.cryptoki 
ProtectionDomain lacks the permission and rightly so IMO. The default 
policy doesn't grant "setContextClassLoader" permission to any JDK 
module. It's not required when we use InnocuousThread.


In NativeResourceCleaner the original constructor is setting the 
Context ClassLoader of the calling thread to null, prior to calling 
the Thread superclass constructor, so that both the calling thread and 
new thread will nave a null context ClassLoader.  In your new 
implementation, you are asserting the context class loader of the 
created thread is null, without setting the context ClassLoader of the 
original calling thread, is that the intended behaviour?


Alternatively you could set the context ClassLoader of the calling 
thread to null using a PrivilegedAction, prior to creating the new 
thread and restore it?
Use of InnocuousThread is made in various JDK classes for similar 
purpose where daemon threads need to be run with limited privilege. 
Similar use seen in networking and ldap classes.




If the original intent was to set the context ClassLoader of the new 
thread to null, then why not just do that, rather than use an assertion?
InnocuousThread sets this to null. The assert is just a belt and braces 
approach which is a useful check during test runs. Again, similar 
approach done in other JDK libraries.


If assertions are not enabled it may run with a non null context 
ClassLoader?   What are the consequences?  It appears to me, the fix 
has created a bigger problem, rather than fixed it.  Just my 2 cents.


see above. We shouldn't have an issue. A non-null classloader would lead 
to classloader memory leak in some environments.


regards,
Sean.



We use SecurityManager by default following principles of least 
privilege (only the code paths we need to execute), and the original 
reported bug is a non problem for us, we would capture the missing 
permission and grant it, these are permission grants for Java 16:


grant codebase "jrt:/jdk.crypto.cryptoki"
{
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util";

};

grant codebase "jrt:/jdk.crypto.ec"
{
    permission java.security.SecurityPermission 
"putProviderProperty.SunEC";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.jca";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.pkcs";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util.math";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.util.math.intpoly";
    permission java.lang.RuntimePermission 
"accessClassInPackage.sun.security.x509";

};

Good call making NativeResourceCleaner implement Runnable instead of 
extending Thread though.



[1]

access: domain that failed ProtectionDomain (jrt:/jdk.crypto.cryptoki 
)

 jdk.internal.loader.ClassLoaders$PlatformClassLoader@5433274e
 
 java.security.Permissions@7006c658 (
 ("java.io.FilePermission" "<>" "read")
 ("java.net.SocketPermission" "localhost:0" "listen,resolve")
 ("java.security.SecurityPermission" "clearProviderProperties.*")
 ("java.security.SecurityPermission" 
"getProperty.auth.login.defaultCallbackHandler")

 ("java.security.SecurityPermission" "putProviderProperty.*")
 ("java.security.SecurityPermission" "authProvider.*")
 ("java.security.SecurityPermission" "removeProviderProperty.*")
 ("java.util.PropertyPermission" "java.specification.version" "read")
 ("java.util.PropertyPermission" "java.vm.vendor" "read")
 ("java.util.PropertyPermission" "path.separator" "read")
 ("java.util.PropertyPermission" "os.version" "read")
 ("java.util.PropertyPermission" "java.vendor.url" "read")
 ("java.util.PropertyPermission" "java.vm.name" "read")
 ("java.util.PropertyPermission" "java.vm.specification.version" "read")
 ("java.util.PropertyPermission" "os.name" "read")
 ("java.util.PropertyPermission" 
"sun.security.pkcs11.allowSingleThreadedModules" "read")
 ("java.util.PropertyPermission" 

Re: [External] : Re: [11u] RFR: 8267599: Revert the change to the default PKCS12 macAlgorithm and macIterationCount props for 11u/8u/7u

[Seán Coffey]

Looks good!

regards,
Sean.

On 28/05/2021 17:17, Doerr, Martin wrote:


Hi,

here’s my new webrev for reverting the pkcs12.macAlgorithm and 
macIterationCount changes from the JDK-8153005 
backport:


http://cr.openjdk.java.net/~mdoerr/8267599_revert_8153005_11u/webrev.01/ 



Oracle’s JBS issue:

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



and CSR:

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



Thanks to Sean for confirming that there’s nothing else to do in the 
security file and PKCS12KeyStore.java.


So, I only had to adapt the tests.

Please review.

Best regards,

Martin

(JDK-8266351) Re: [External] : Re: RFR: 8236671: NullPointerException in JKS keystore [v2]

[Seán Coffey]

Thanks for the pointers Will.

I've added your details to the JDK-8266351 bug report.
https://bugs.openjdk.java.net/browse/JDK-8266351

regards,
Sean.

On 24/05/2021 18:53, Will Sargent wrote:
I have tried to sign up to the bug tracking system (through reset 
password I think?) but I'm not getting an email out, so I can't add to 
the bug.


I have created a test case in Github:

https://github.com/wsargent/jca-key-failure/ 
<https://urldefense.com/v3/__https://github.com/wsargent/jca-key-failure/__;!!GqivPVa7Brio!KZTaOe6TkXX8t-ZTaptDzm3RETFWZV4O6xj-7_iS2CF-NV4g7FxSSzYEweeXM5lj3g$>


The stack trace shows the invalid key store entry after saving and 
loading it again.


https://github.com/wsargent/jca-key-failure/blob/main/src/main/java/com/tersesystems/jcakeyfailure/JcaKeyFailure.java#L68 
<https://urldefense.com/v3/__https://github.com/wsargent/jca-key-failure/blob/main/src/main/java/com/tersesystems/jcakeyfailure/JcaKeyFailure.java*L68__;Iw!!GqivPVa7Brio!KZTaOe6TkXX8t-ZTaptDzm3RETFWZV4O6xj-7_iS2CF-NV4g7FxSSzYEweeC27YT_w$>


On Fri, Apr 30, 2021 at 12:40 PM Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Thanks for the feedback Will. It would be useful if you can
provide a testcase and/or add comments to JDK-8266351
<https://bugs.openjdk.java.net/browse/JDK-8266351> on your experience.

regards,
Sean.

On 30/04/2021 17:54, Will Sargent wrote:

> KeyStore specification will be tightened up via another bug record

This would be super helpful, as one thing that confuses me is
what the relationship is between a key entry and a key alias --
in particular, the existence alias doesn't seem to guarantee a
valid entry that can be retrieved.

In JDK 11 it's possible to create a private key with a keystore
using pkcs12.setKeyEntry() (see link below):


https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L135

<https://urldefense.com/v3/__https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java*L135__;Iw!!GqivPVa7Brio!KZTaOe6TkXX8t-ZTaptDzm3RETFWZV4O6xj-7_iS2CF-NV4g7FxSSzYEweeUj8qrfw$>

and then have a null pointer exception when retrieving the entry
from the alias because the certificate chain is null (see
commented out "testSystem" use case):


https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L27

<https://urldefense.com/v3/__https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java*L27__;Iw!!GqivPVa7Brio!KZTaOe6TkXX8t-ZTaptDzm3RETFWZV4O6xj-7_iS2CF-NV4g7FxSSzYEwedESajqLA$>

I can write this up into a formal bug if that helps.

On Fri, Apr 30, 2021 at 2:30 AM Sean Coffey
mailto:coff...@openjdk.java.net>> wrote:

On Wed, 28 Apr 2021 12:39:42 GMT, Sean Coffey
mailto:coff...@openjdk.org>> wrote:

>> Trivial enough change. Improved the exception thrown from
JceKeyStore also.
>
> Sean Coffey 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:
>
>  - Check for null before try block
>  - Merge branch 'master' of https://github.com/openjdk/jdk

<https://urldefense.com/v3/__https://github.com/openjdk/jdk__;!!GqivPVa7Brio!KZTaOe6TkXX8t-ZTaptDzm3RETFWZV4O6xj-7_iS2CF-NV4g7FxSSzYEweeOltfJww$>
into JDK-8236671-NPE
>  - Fix white space
>  - 8236671: NullPointerException in JKS keystore

KeyStore specification will be tightened up via another bug
record: https://bugs.openjdk.java.net/browse/JDK-8266351
<https://bugs.openjdk.java.net/browse/JDK-8266351>

-

PR: https://git.openjdk.java.net/jdk/pull/3588
<https://git.openjdk.java.net/jdk/pull/3588>

Re: [External] : AW: [11u] RFR: 8267599: Revert the change to the default PKCS12 macAlgorithm and macIterationCount props for 11u/8u/7u

[Seán Coffey]

here are the main changes that we pushed for JDK 11u:


diff --git 
a/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java 
b/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java
index a62452bdcd..441f2b651e 100644
--- a/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java
+++ b/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java
@@ -101,10 +101,10 @@ public final class PKCS12KeyStore extends KeyStoreSpi {
  = "PBEWithHmacSHA256AndAES_256";
  private static final String DEFAULT_KEY_PBE_ALGORITHM
  = "PBEWithHmacSHA256AndAES_256";
-private static final String DEFAULT_MAC_ALGORITHM = "HmacPBESHA256";
+private static final String DEFAULT_MAC_ALGORITHM = "HmacPBESHA1";
  private static final int DEFAULT_CERT_PBE_ITERATION_COUNT = 1;
  private static final int DEFAULT_KEY_PBE_ITERATION_COUNT = 1;
-private static final int DEFAULT_MAC_ITERATION_COUNT = 1;
+private static final int DEFAULT_MAC_ITERATION_COUNT = 10;
  
  // Legacy settings. Used when "keystore.pkcs12.legacy" is set.

  private static final String LEGACY_CERT_PBE_ALGORITHM
diff --git a/src/java.base/share/conf/security/java.security 
b/src/java.base/share/conf/security/java.security
index b0c5beccf6..893567071c 100644
--- a/src/java.base/share/conf/security/java.security
+++ b/src/java.base/share/conf/security/java.security
@@ -1200,12 +1200,12 @@ jceks.key.serialFilter = 
java.base/java.lang.Enum;java.base/java.security.KeyRep
  # The algorithm used to calculate the optional MacData at the end of a PKCS12
  # file. This can be any HmacPBE algorithm defined in the Mac section of the
  # Java Security Standard Algorithm Names Specification. When set to "NONE",
-# no Mac is generated. The default value is "HmacPBESHA256".
-#keystore.pkcs12.macAlgorithm = HmacPBESHA256
+# no Mac is generated. The default value is "HmacPBESHA1".
+#keystore.pkcs12.macAlgorithm = HmacPBESHA1
  
  # The iteration count used by the MacData algorithm. This value must be a

-# positive integer. The default value is 1.
-#keystore.pkcs12.macIterationCount = 1
+# positive integer. The default value is 10.
+#keystore.pkcs12.macIterationCount = 10
  
  #

  # Enhanced exception message information


regards,
Sean.

On 28/05/2021 15:02, Doerr, Martin wrote:


Hi Sean,

thank you for your quick reply. I was already hoping to get such feedback.

I had read the CSR and I had already thought that you guys didn’t 
revert the complete change.


My problem is that I can’t see what exactly you have done.

I’m concerned about making it insecure by creating a mixture of old 
and new behavior. How can I ensure to get the same behavior as 
11.0.12-oracle?


Would it be possible to publish your security file and 
PKCS12KeyStore.java?


Otherwise, wouldn’t it be safer to stick with the old behavior until 
we switch to the new one in a future release?


Best regards,

Martin

*Von: *Seán Coffey 
*Datum: *Freitag, 28. Mai 2021 um 15:42
*An: *Doerr, Martin , 
jdk-updates-...@openjdk.java.net , 
security-dev , Hohensee, Paul 

*Betreff: *Re: [11u] RFR: 8267599: Revert the change to the default 
PKCS12 macAlgorithm and macIterationCount props for 11u/8u/7u


Martin,

you seem to be suggesting a full revert of the JDK-8153005 changes. Note
that the Oracle JDK changes only relate to to the default PKCS12
macAlgorithm and macIterationCount (back to HmacPBESHA1 and 10
respectively). While there are other interoperability concerns with the
keystore.pkcs12.certProtectionAlgorithm and
keystore.pkcs12.keyProtectionAlgorithm values [1], they relate to JDK
8u/7u where PKCS12 is not the default keystore type.

regards,
Sean.

[1] https://bugs.openjdk.java.net/browse/JDK-8267837 
<https://bugs.openjdk.java.net/browse/JDK-8267837>


On 28/05/2021 13:52, Doerr, Martin wrote:
> Hi,
>
> Oracle has reverted the changes from JDK-8153005 backport in 
11.0.12-oracle for interoperability reasons. See:
> https://bugs.openjdk.java.net/browse/JDK-8267599 
<https://bugs.openjdk.java.net/browse/JDK-8267599>

> and CSR:
> https://bugs.openjdk.java.net/browse/JDK-8267701 
<https://bugs.openjdk.java.net/browse/JDK-8267701>

>
> I had to adapt the small test addition from JDK-8266293 (see "// 
8266293" comment in ParamsPreferences.java):
> 
http://cr.openjdk.java.net/~mdoerr/8267599_revert_8153005_11u/webrev.00/ 
<http://cr.openjdk.java.net/~mdoerr/8267599_revert_8153005_11u/webrev.00/>

>
> Please review.
> Comments?
>
> Best regards,
> Martin
>

Re: RFR: 8240256: Better resource cleaning for SunPKCS11 Provider [v2]

[Seán Coffey]

Thanks for the review Valerie. I've gone ahead and updated the test. 
You've a good point in that the PKCS11Test framework didn't suit the 
test that I needed. The new test no longer extends PKCS11Test as a 
result. I have kept the refactoring in PKCS11Test thought since it can 
offer up some useful helper static methods. I did clean up the 
testNSS(..) static methods. You're right that the 2nd Provider parameter 
was redundant. Removed that.


Changes pushed.

regards,
Sean.

On 12/05/2021 19:55, Valerie Peng wrote:

On Fri, 7 May 2021 13:51:05 GMT, Sean Coffey  wrote:


Added capability to allow the PKCS11 Token to be destroyed once a session is 
logged out from. New configuration properties via pkcs11 config file. Cleaned 
up the native resource poller also.

New unit test case to test behaviour. Some PKCS11 tests refactored to allow 
pkcs11 provider to be configured (and tested) with a config file of choice.

Reviewer request @valeriepeng

Sean Coffey has updated the pull request incrementally with one additional 
commit since the last revision:

   Initial corrections from RFR

The update changes look good. Will wait for your test edit then.

-

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

Re: [11u] RFR: 8267599: Revert the change to the default PKCS12 macAlgorithm and macIterationCount props for 11u/8u/7u

[Seán Coffey]

Martin,

you seem to be suggesting a full revert of the JDK-8153005 changes. Note 
that the Oracle JDK changes only relate to to the default PKCS12 
macAlgorithm and macIterationCount (back to HmacPBESHA1 and 10 
respectively). While there are other interoperability concerns with the 
keystore.pkcs12.certProtectionAlgorithm and 
keystore.pkcs12.keyProtectionAlgorithm values [1], they relate to JDK 
8u/7u where PKCS12 is not the default keystore type.


regards,
Sean.

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

On 28/05/2021 13:52, Doerr, Martin wrote:

Hi,

Oracle has reverted the changes from JDK-8153005 backport in 11.0.12-oracle for 
interoperability reasons. See:
https://bugs.openjdk.java.net/browse/JDK-8267599
and CSR:
https://bugs.openjdk.java.net/browse/JDK-8267701

I had to adapt the small test addition from JDK-8266293 (see "// 8266293" 
comment in ParamsPreferences.java):
http://cr.openjdk.java.net/~mdoerr/8267599_revert_8153005_11u/webrev.00/

Please review.
Comments?

Best regards,
Martin

Re: RFR: 8236671: NullPointerException in JKS keystore [v2]

[Seán Coffey]

Thanks for the feedback Will. It would be useful if you can provide a 
testcase and/or add comments to JDK-8266351 
 on your experience.


regards,
Sean.

On 30/04/2021 17:54, Will Sargent wrote:

> KeyStore specification will be tightened up via another bug record

This would be super helpful, as one thing that confuses me is what the 
relationship is between a key entry and a key alias -- in particular, 
the existence alias doesn't seem to guarantee a valid entry that can 
be retrieved.


In JDK 11 it's possible to create a private key with a keystore using 
pkcs12.setKeyEntry() (see link below):


https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L135 



and then have a null pointer exception when retrieving the entry from 
the alias because the certificate chain is null (see commented out 
"testSystem" use case):


https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L27 



I can write this up into a formal bug if that helps.

On Fri, Apr 30, 2021 at 2:30 AM Sean Coffey > wrote:


On Wed, 28 Apr 2021 12:39:42 GMT, Sean Coffey mailto:coff...@openjdk.org>> wrote:

>> Trivial enough change. Improved the exception thrown from
JceKeyStore also.
>
> Sean Coffey 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:
>
>  - Check for null before try block
>  - Merge branch 'master' of https://github.com/openjdk/jdk
 into JDK-8236671-NPE
>  - Fix white space
>  - 8236671: NullPointerException in JKS keystore

KeyStore specification will be tightened up via another bug
record: https://bugs.openjdk.java.net/browse/JDK-8266351


-

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

Re: JSSE reference guide issue

[Seán Coffey]

Another option for reporting is to use the https://bugreport.java.com 
portal. Component = documentation.


regards,
Sean.

On 05/02/2021 14:03, Sean Mullan wrote:
If you have a JBS account, you can file a bug in the docs/guides 
category.


However, I have also forwarded your email to our internal docs 
engineer, so we will follow-up on it.


Thanks,
Sean

On 2/5/21 2:42 AM, Daniel Jeliński wrote:

Hi all,
What's the right spot to report documentation issues?

I've been reading the JSSE reference guide and noticed that in section
"Resuming Session Without Server-Side State"
(https://docs.oracle.com/en/java/javase/15/security/java-secure-socket-extension-jsse-reference-guide.html#GUID-64D7EAF6-D2EE-4719-8616-25E2829CF810) 


it says "This feature is not enabled by default", which appears to be
a leftover from Java 13.

Also the note about TLS 1.3 in the same section isn't entirely clear
to me. What does it mean when the docs say "the contents of stateless
tickets, in particular, the contents of a NewSessionTicket message,
depend on the value of jdk.tls.server.enableSessionTicketExtension"?
How exactly does the contents change and why should I care?
Regards,
Daniel

Re: RFR: 8255348: NPE in PKIXCertPathValidator event logging code

[Seán Coffey]

Thanks for the comments Sean. Code updated.

https://github.com/openjdk/jdk/pull/2150/commits/8a344a85303297f17a6e75e7c1f423c2d16c451a

regards,
Sean.

On 21/01/2021 19:47, Sean Mullan wrote:

On Tue, 19 Jan 2021 17:54:33 GMT, Sean Coffey  wrote:


Correction of NPE and updating of test cases. Minor refactoring of test library 
also.

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java
 line 248:


246: int anchorCertId = 0;
247: X509Certificate trustedCert = anchor.getTrustedCert();
248: if (trustedCert != null) {

You could use the `anchorCert` variable which was set earlier instead of 
calling `getTrustedCert()` again.

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java
 line 251:


249: anchorCertId = trustedCert.hashCode();
250: } else {
251: if (anchor.getCAPublicKey() != null) {

Per the TrustAnchor API, you can't create a TrustAnchor with a null public key, 
so I don't think you need this check.

-

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

Re: RFR: 8253368: TLS connection always receives close_notify exception

[Seán Coffey]

On 13/11/2020 21:23, David Lloyd wrote:

Thinking about this some more, I suppose I tend to interpret the
shutdownInput() API as indicating that the caller doesn't really care
about any further data being received on the socket.  I expect that a
truncation attack would only be significant if the socket EOF was
received before close_notify while the caller is trying to read data
from the socket.  So in that light the change is probably OK (since it
only exists when explicitly shutting down the input) from the
perspective that I was thinking of.  Whether it has any protocol-level
implications I don't know and won't comment on.

That said, maybe shutdownInput(boolean) should be collapsed into
shutdownInput() since the parameter is no longer used?
Thanks for the comments. Fair point on the method overload. One I'll 
look into.


I'm looking to further study the consequences of the overall change some 
more and will reach out to other engineers for opinion also. Hope to 
revert to the list with an update in next 1-2 weeks.


regards,
Sean.



On Fri, Nov 13, 2020 at 3:01 PM Seán Coffey  wrote:

Open to discussion on that. It's also highlighted in the TLSv1.3 RFC.

The TLS spec doesn't require a fatal alert to be issued when closing
inbound without receiving the peer's close_notify. I've seen a handful
of applications making JDK upgrades which are seeing regression as a
result of this new JDK check (like the one described in this bug). If
we're to keep the check in the JDK, should we restrict it to the v1.3
protocol or should we implement it based on a system property perhaps ?

regards,
Sean.

On 13/11/2020 17:11, David Lloyd wrote:

How would a truncation attack be avoided in this case?

On Fri, Nov 13, 2020 at 8:23 AM Sean Coffey  wrote:

removing the "closing inbound before receiving peer's close_notify" exception 
that can be seen with TLS stack if calling close on inbound. After reading the relevant 
parts of the TLS v1.2/v1.3 RFCs, I believe the local end point doesn't have to wait for 
close_notify alert from remote end.

-

Commit messages:
   - 8253368: TLS connection always receives close_notify exception

Changes: https://git.openjdk.java.net/jdk/pull/1205/files
   Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1205=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8253368
Stats: 25 lines in 2 files changed: 12 ins; 10 del; 3 mod
Patch: https://git.openjdk.java.net/jdk/pull/1205.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/1205/head:pull/1205

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

Re: RFR: 8253368: TLS connection always receives close_notify exception

[Seán Coffey]

Open to discussion on that. It's also highlighted in the TLSv1.3 RFC.

The TLS spec doesn't require a fatal alert to be issued when closing 
inbound without receiving the peer's close_notify. I've seen a handful 
of applications making JDK upgrades which are seeing regression as a 
result of this new JDK check (like the one described in this bug). If 
we're to keep the check in the JDK, should we restrict it to the v1.3 
protocol or should we implement it based on a system property perhaps ?


regards,
Sean.

On 13/11/2020 17:11, David Lloyd wrote:

How would a truncation attack be avoided in this case?

On Fri, Nov 13, 2020 at 8:23 AM Sean Coffey  wrote:

removing the "closing inbound before receiving peer's close_notify" exception 
that can be seen with TLS stack if calling close on inbound. After reading the relevant 
parts of the TLS v1.2/v1.3 RFCs, I believe the local end point doesn't have to wait for 
close_notify alert from remote end.

-

Commit messages:
  - 8253368: TLS connection always receives close_notify exception

Changes: https://git.openjdk.java.net/jdk/pull/1205/files
  Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1205=00
   Issue: https://bugs.openjdk.java.net/browse/JDK-8253368
   Stats: 25 lines in 2 files changed: 12 ins; 10 del; 3 mod
   Patch: https://git.openjdk.java.net/jdk/pull/1205.diff
   Fetch: git fetch https://git.openjdk.java.net/jdk pull/1205/head:pull/1205

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

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

Thanks for the review Hai-May. I've implemented all your suggestions.

The CSR was approved late on Friday so I'll now submit this via PR on 
github infra.


regards,
Sean.

On 28/08/2020 21:08, Hai-May Chao wrote:
JarSigner.java #953: The output debug message can be removed from the 
code.

JavaUtilZipFileAccess.java #44: Change posixPerms to extraAttrs.
ZipFile.java #661: Suggest to keep the comment and update it with the 
additional 4 bits for symlink.


The rest of code changes and CSR look good.

Thanks,
Hai-May


On Aug 28, 2020, at 7:17 AM, Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


I've been poking around the zip internals and am now able to locate 
the 16 bits of interest. The position of these actual bits does 
appear to move around from one test run to another. For now, I guess 
it's sufficient to look for the pattern of interest in the signed zip 
file. New testcase added.


http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/

regards,
Sean.

On 27/08/2020 15:58, Weijun Wang wrote:

Looks like it was a conscious design decision to only allow recording of POSIX 
permission bits for this field (& 0xFFF). I don't see anything about symlink 
support in zipfs docs.

As long as that*byte*  is there and it’s not difficult to locate, we can 
manually add the*bit*  for symlink and see if jarsigner can keep it.

—Max

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

On 28/08/2020 16:18, Weijun Wang wrote:

1. Add a comment on how to generate ZIPBYTES in the test. Not the byte[] 
declaration but how the original ZIP file is generated.

I'll add a comment block to the test:

    /*
 * Created using the createByteArray utility method.
 * The zipfile itself was created via this example:
 * $ ls -l z
 * lrwxrwxrwx 1 test test 4 Aug 27 18:33 z -> ../z
 * $ zip -ry test.zip z
 */




2. Does this require a CSR? The POSIX permission one had one.


Fair point. I've logged one, just to be safe.

regards,
Sean.



Thanks,
Max


On Aug 28, 2020, at 10:17 AM, Seán Coffey  wrote:

I've been poking around the zip internals and am now able to locate the 16 bits 
of interest. The position of these actual bits does appear to move around from 
one test run to another. For now, I guess it's sufficient to look for the 
pattern of interest in the signed zip file. New testcase added.

http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/

regards,
Sean.

On 27/08/2020 15:58, Weijun Wang wrote:

Looks like it was a conscious design decision to only allow recording of POSIX 
permission bits for this field (& 0xFFF). I don't see anything about symlink 
support in zipfs docs.


As long as that *byte* is there and it’s not difficult to locate, we can 
manually add the *bit*
  for symlink and see if jarsigner can keep it.

—Max

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

I've been poking around the zip internals and am now able to locate the 
16 bits of interest. The position of these actual bits does appear to 
move around from one test run to another. For now, I guess it's 
sufficient to look for the pattern of interest in the signed zip file. 
New testcase added.


http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/

regards,
Sean.

On 27/08/2020 15:58, Weijun Wang wrote:

Looks like it was a conscious design decision to only allow recording of POSIX 
permission bits for this field (& 0xFFF). I don't see anything about symlink 
support in zipfs docs.

As long as that*byte*  is there and it’s not difficult to locate, we can 
manually add the*bit*  for symlink and see if jarsigner can keep it.

—Max

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

Thanks for the review Max. Comments inline..

On 27/08/2020 14:45, Weijun Wang wrote:

I’m OK with using one warning, but prefer it to a little more formal like 
"POSIX file permission and/or symlink attributes detected…”.

One nit in ZipFile.java:

1098 // only set posix perms value via ZipEntry constructor for 
now
1099 @Override
1100 public int getExtraAttributes(ZipEntry ze) {

Maybe you can just remove the comment.

Do you also want to rename the “posixPermsDetected" field and loacl variable 
“perms” in JarSigner.java?


Good points. Edits made.

http://cr.openjdk.java.net/~coffeys/webrev.8250968.v3/webrev/



I’m not sure about the test but if zipfs is able to keep permissions inside a 
zip file then that POSIX byte (or whatever it’s named) is already there and we 
can modify it to include more bits.


Looks like it was a conscious design decision to only allow recording of 
POSIX permission bits for this field (& 0xFFF). I don't see anything 
about symlink support in zipfs docs.


regards,
Sean.



No other comment.

Thanks,
Max



On Aug 27, 2020, at 3:26 AM, Seán Coffey  wrote:

updated webrev:
http://cr.openjdk.java.net/~coffeys/webrev.8250968.v2/webrev/

regards,
Sean.

On 27/08/2020 07:42, Seán Coffey wrote:

Hi Max,

I looked at updating the warning string but figured that it might have been of 
no interest to end users. How about this edit then ?

+{"posix.attributes.detected", "POSIX file permission attributes detected. 
These attributes are ignored when signing and are not protected by the signature."},


replace with:

+{"extra.attributes.detected", "POSIX file permission/symlink attributes 
detected. These attributes are ignored when signing and are not protected by the signature."},

regards,
Sean.

On 26/08/2020 23:15, Weijun Wang wrote:

Are you going to update the warning text or create a new one?

Thanks,
Max


On Aug 26, 2020, at 2:26 PM, Seán Coffey  wrote:

This is a follow on from the recent 8218021 fix. The jarsigner tool removes 
symlink attribute data from zipfiles when signing them. jarsigner should 
preserve this data. The fix involves preserving the 16 bits associated with the 
file attributes (instead of the current 12). That's done in ZipFile. All other 
changes are just a refactor of the variable name.

I haven't been able to automate a test for this since zipfs doesn't seem to 
support symlinks. Manual testing looks good.

https://bugs.openjdk.java.net/browse/JDK-8250968
http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html

regards,
Sean.

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

updated webrev:
http://cr.openjdk.java.net/~coffeys/webrev.8250968.v2/webrev/

regards,
Sean.

On 27/08/2020 07:42, Seán Coffey wrote:

Hi Max,

I looked at updating the warning string but figured that it might have 
been of no interest to end users. How about this edit then ?


+    {"posix.attributes.detected", "POSIX file permission 
attributes detected. These attributes are ignored when signing and are 
not protected by the signature."},


>> replace with:
+    {"extra.attributes.detected", "POSIX file permission/symlink 
attributes detected. These attributes are ignored when signing and are 
not protected by the signature."},


regards,
Sean.

On 26/08/2020 23:15, Weijun Wang wrote:

Are you going to update the warning text or create a new one?

Thanks,
Max

On Aug 26, 2020, at 2:26 PM, Seán Coffey  
wrote:


This is a follow on from the recent 8218021 fix. The jarsigner tool 
removes symlink attribute data from zipfiles when signing them. 
jarsigner should preserve this data. The fix involves preserving the 
16 bits associated with the file attributes (instead of the current 
12). That's done in ZipFile. All other changes are just a refactor 
of the variable name.


I haven't been able to automate a test for this since zipfs doesn't 
seem to support symlinks. Manual testing looks good.


https://bugs.openjdk.java.net/browse/JDK-8250968
http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html 



regards,
Sean.

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

Hi Max,

I looked at updating the warning string but figured that it might have 
been of no interest to end users. How about this edit then ?


+    {"posix.attributes.detected", "POSIX file permission attributes 
detected. These attributes are ignored when signing and are not 
protected by the signature."},


>> replace with:
+    {"extra.attributes.detected", "POSIX file permission/symlink 
attributes detected. These attributes are ignored when signing and are 
not protected by the signature."},


regards,
Sean.

On 26/08/2020 23:15, Weijun Wang wrote:

Are you going to update the warning text or create a new one?

Thanks,
Max


On Aug 26, 2020, at 2:26 PM, Seán Coffey  wrote:

This is a follow on from the recent 8218021 fix. The jarsigner tool removes 
symlink attribute data from zipfiles when signing them. jarsigner should 
preserve this data. The fix involves preserving the 16 bits associated with the 
file attributes (instead of the current 12). That's done in ZipFile. All other 
changes are just a refactor of the variable name.

I haven't been able to automate a test for this since zipfs doesn't seem to 
support symlinks. Manual testing looks good.

https://bugs.openjdk.java.net/browse/JDK-8250968
http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html

regards,
Sean.

RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

This is a follow on from the recent 8218021 fix. The jarsigner tool 
removes symlink attribute data from zipfiles when signing them. 
jarsigner should preserve this data. The fix involves preserving the 16 
bits associated with the file attributes (instead of the current 12). 
That's done in ZipFile. All other changes are just a refactor of the 
variable name.


I haven't been able to automate a test for this since zipfs doesn't seem 
to support symlinks. Manual testing looks good.


https://bugs.openjdk.java.net/browse/JDK-8250968
http://hmsjpse.uk.oracle.com/home/scoffey/ws/jdk-jdk/open/webrev.8250968/webrev/index.html

regards,
Sean.

Re: RFR[15] 8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider

[Seán Coffey]

Your patch looks ok to me Valerie. I note that John mentions an anomaly 
with your patch - I'm thinking that may need further investigation.


regards,
Sean.

On 06/07/2020 17:33, Valerie Peng wrote:

Hi Max,

The suggested fix is not much different than the suggested webrev.

The essential change is to call getService(...) for the returned 
service in Provider.getDefaultSecureRandomService(). The only 
difference here is using the hardcoded type "SecureRandom" vs the one 
returned by getType() call. Is this what you are referring to?


I re-write the rest to store String instead of Service as it may seem 
strange why the stored Service is not used but re-queried through 
getService(...). Also, looks cleaner to me this way.


Thanks,
Valerie
On 7/2/2020 9:05 PM, Weijun Wang wrote:

Hi Valerie,

How about the suggested fix from the bug reporter?

Thanks,
Max

On Jul 3, 2020, at 4:52 AM, Valerie Peng  
wrote:


Hi Max and Sean,

Can you help reviewing this fix for JDK-8248505? This is the 
followup fix for JDK-8246613 "Choose the default SecureRandom algo 
based on registration ordering" which you reviewed earlier. Based on 
the feedback, BCFIPS provider overrides putService/getService() 
calls which does not work well with the fix for JDK-8246613. Thus, I 
adapted to store the SecureRandom algorithm names internally and 
then return the result from getService(...) when 
Provider.getDefaultSecureRandomService() is called. Updated the 
regression test to include a custom provider which simulates the 
BCFIPS provider behavior.


Bug: https://bugs.openjdk.java.net/browse/JDK-8248505

Webrev: http://cr.openjdk.java.net/~valeriep/8248505/webrev.00/

Thanks,
Valerie

Re: RFR: 8218021: Have jarsigner preserve posix permission attributes

[Seán Coffey]

Thanks for the review Alan. I'm in contact with Max already about 
possible follow up enhancements in this area. It would be worked via a 
follow on JBS record.


Regarding the error message, I'm fine with your suggestion. We can go 
with this then:
"POSIX file permission attributes detected. These attributes are ignored 
when signing and are not protected by the signature."


regards,
Sean.

On 02/07/2020 08:59, Alan Bateman wrote:

On 30/06/2020 14:51, Seán Coffey wrote:


:

During the CSR review, a suggestion was made to have jarsigner 
preserve such attributes by default. Warnings about these attributes 
will also be added during signing and verify operations (if detected).


Yes, signing should be additive so the original proposal to drop 
information from the UNIX extra block would be surprising. The 
intersection of those using zip/other tools to create zip files and 
then signing them with jarsigner is probably small but it would still 
be confusing for signing to loose information. Having jarsigner refuse 
to sign these zip files by default, with an option to override, would 
be a reasonable approach. The current proposal to printing a warning 
seems okay too.


I've skimmed through webrev.8218021.v5 which has this warning:

"POSIX file permission attributes detected. Note that these attributes 
are unsigned and not protected by the signature."


I realize you've agreed this with the other Reviewers but I think that 
"Note that these attributes are unsigned ..." is confusing as it could 
be interpreted to mean that they have to be signed by some other 
means, or even that the warning is because they are using unsigned 
values.


It might be better to tweak the second part to make it a bit clearer, 
up to you but something like "These attributes are ignored when 
signing and are not protected by the signature".


-Alan

Re: RFR: 8218021: Have jarsigner preserve posix permission attributes

[Seán Coffey]

Thanks for the review Max. All edits made bar the 
"Event.clearReportListener(Event.ReporterCategory.POSIXPERMS);" 
suggested edit. That's already in a finally block.


latest webrev: 
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v5/webrev/


I plan to push once I have a clean test run.

regards,
Sean.

On 01/07/2020 03:02, Weijun Wang wrote:

-
src/java.base/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java
@@ -248,7 +248,7 @@ private static X509CRL getCRL(URIName name) throws 
CertStoreException {
 debug.println("Trying to fetch CRL from DP " + uri);
 }

 Event.report("event.crl.check", uri.toString());
 Event.report(Event.ReporterCategory.CRLCHECK,"event.crl.check", 
uri.toString());
  
Please add a whitespace after CRLCHECK.

-
src/java.base/share/classes/sun/security/provider/certpath/OCSP.java
@@ -234,7 +234,7 @@ static OCSPResponse check(List certIds, URI 
responderURI,
 debug.println("connecting to OCSP service at: " + url);
 }

 Event.report("event.ocsp.check", url.toString());
 Event.report(Event.ReporterCategory.CRLCHECK,"event.ocsp.check", 
url.toString());
  
White space after CRLCHECK.

-
src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
 int perms = JUZFA.getPosixPerms(ze);
 if (!posixPermsDetected && perms != -1) {
 posixPermsDetected = true;
 Event.report(Event.ReporterCategory.POSIXPERMS, "true");
  
The method signature is "report(category, type, values...)". There is no need to define a type or value here but I feel like more normal to call 'report(POSIXPERMS, "detected")' because the 2nd argument is type instead of value.

-
src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java
@@ -1054,7 +1063,7 @@ private void displayMessagesAndResult(boolean isSigning) {
 notYetValidCert || chainNotValidated || hasExpiredCert ||
 hasUnsignedEntry || signerSelfSigned || (legacyAlg != 0) ||
 (disabledAlg != 0) || aliasNotInStore || notSignedByAlias ||
 tsaChainNotValidated || permsDetected ||
  
I'd prefer move the permsDetected check and the if block for it out of this big block, since it's not a fatal warning and just informational. You can move it into "if (hasExpiringCert".

-
src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java
@@ -1805,6 +1820,7 @@ void signJar(String jarName, String alias)
 fos.close();
 }

 Event.clearReportListener(Event.ReporterCategory.POSIXPERMS);
  
Maybe in a final block?

-
test/jdk/sun/security/util/Resources/Usages.java
@@ -75,7 +75,7 @@
 "(?m)rb[ \\n]*\\.getString[ \\n]*\\([ \\n]*\"(.*?)\"\\)");

 static Pattern EVENT_OCSP_CRL = Pattern.compile(
 "Event\\.report\\(.*,\"(.*?)\",");
  
If you agree to add a whitespace in the calls in OSCP.java and DistributionFetcher.java, you might want to add a whitespace (or "\s*") here as well.


Thanks,
Max



On Jun 30, 2020, at 9:51 PM, Seán Coffey  wrote:

Thanks Lance.

During the CSR review, a suggestion was made to have jarsigner preserve such 
attributes by default. Warnings about these attributes will also be added 
during signing and verify operations (if detected).

webrev: https://cr.openjdk.java.net/~coffeys/webrev.8218021.v4/webrev/

regards,
Sean.

On 22/06/2020 17:17, Lance Andersen wrote:

HI Sean,

Looks OK based on our exchanges.  Thank you for your time on this one!

Best
Lance


On Jun 22, 2020, at 7:22 AM, Seán Coffey  wrote:

Thanks Lance.

I've updated the patch with some extra offline feedback from yourself and Max.
A new warning is printed with use of the new flag. A warning is also printed 
when file posix permissions are detected on resources being signed. Test 
updated for that also.

https://cr.openjdk.java.net/~coffeys/webrev.8218021.v3/webrev/

regards,
Sean.

On 12/06/2020 17:05, Lance Andersen wrote:

Hi Sean,

I think your changes look fine so all good FMPOV.

Best
Lance


On Jun 12, 2020, at 6:21 AM, Seán Coffey  wrote:

Hi,

I'd like to reboot this jarsigner enhancement request[1]. I've removed the 
problem references to zip file name extensions. Instead, there's a new JDK 
implementation specific jarsigner option: -keepposixperms

https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/

regards,
Sean.

[1] http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html




Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com






Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8218021: Have jarsigner preserve posix permission attributes

[Seán Coffey]

Thanks Lance.

During the CSR review, a suggestion was made to have jarsigner preserve 
such attributes by default. Warnings about these attributes will also be 
added during signing and verify operations (if detected).


webrev: https://cr.openjdk.java.net/~coffeys/webrev.8218021.v4/webrev/

regards,
Sean.

On 22/06/2020 17:17, Lance Andersen wrote:

HI Sean,

Looks OK based on our exchanges.  Thank you for your time on this one!

Best
Lance

On Jun 22, 2020, at 7:22 AM, Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Thanks Lance.

I've updated the patch with some extra offline feedback from yourself 
and Max.
A new warning is printed with use of the new flag. A warning is also 
printed when file posix permissions are detected on resources being 
signed. Test updated for that also.


https://cr.openjdk.java.net/~coffeys/webrev.8218021.v3/webrev/

regards,
Sean.

On 12/06/2020 17:05, Lance Andersen wrote:

Hi Sean,

I think your changes look fine so all good FMPOV.

Best
Lance

On Jun 12, 2020, at 6:21 AM, Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Hi,

I'd like to reboot this jarsigner enhancement request[1]. I've 
removed the problem references to zip file name extensions. 
Instead, there's a new JDK implementation specific jarsigner 
option: -keepposixperms


https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/

regards,
Sean.

[1] 
http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html




 
<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>





<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

Re: RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Thanks Lance.

I've updated the patch with some extra offline feedback from yourself 
and Max.
A new warning is printed with use of the new flag. A warning is also 
printed when file posix permissions are detected on resources being 
signed. Test updated for that also.


https://cr.openjdk.java.net/~coffeys/webrev.8218021.v3/webrev/

regards,
Sean.

On 12/06/2020 17:05, Lance Andersen wrote:

Hi Sean,

I think your changes look fine so all good FMPOV.

Best
Lance

On Jun 12, 2020, at 6:21 AM, Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Hi,

I'd like to reboot this jarsigner enhancement request[1]. I've 
removed the problem references to zip file name extensions. Instead, 
there's a new JDK implementation specific jarsigner option: 
-keepposixperms


https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/

regards,
Sean.

[1] 
http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html




<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi,

I'd like to reboot this jarsigner enhancement request[1]. I've removed 
the problem references to zip file name extensions. Instead, there's a 
new JDK implementation specific jarsigner option: -keepposixperms


https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/

regards,
Sean.

[1] 
http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html

Re: Thread leak by LdapLoginModule

[Seán Coffey]

If 8217606 is your issue, then it's fixed in JDK 11.0.8 which is due for 
release in mid July.


regards,
Sean.

On 09/06/2020 22:15, Mkrtchyan, Tigran wrote:

Hi all,

with Java-11 we have notice a thread leak with ldap module.
We use LDAP to authenticate users with username+pasword by
directly calling LdapLoginModule. This was ok with java 7 and
java 8. With java 11 we see threads getting accumulated. here is a
test case that demonstrates it:

```

 private static final String USERNAME_KEY = 
"javax.security.auth.login.name";
 private static final String PASSWORD_KEY = 
"javax.security.auth.login.password";

 String ldapUrl = "ldap://;;
 String peopleOU = "ou= ... o= ... c=...");

String user = ...;
String pass = ...;


 @Test
 public void threadLeakTest() throws AuthenticationException, 
NoSuchPrincipalException, LoginException {

 Map threadsBefore = 
Thread.getAllStackTraces();

 Map  globalLoginOptions = Map.of(
 "userProvider", ldapUrl + "/" + peopleOU,
 "useSSL", "false",
 "userFilter", "(uid={USERNAME})",
 "useFirstPass", "true"
 );

 for (int i = 0; i < 10; i++) {

 Map loginOptions = Map.of(
 USERNAME_KEY, user,
 PASSWORD_KEY, pass.toCharArray());

 Subject subject = new Subject();

 LdapLoginModule loginModule = new LdapLoginModule();
 loginModule.initialize(subject, null, loginOptions, 
globalLoginOptions);
 loginModule.login();
 loginModule.commit();
 loginModule.logout();
 }

 Map threadsAfter = 
Thread.getAllStackTraces();

 assertEquals("Thread leak detected",  threadsBefore.size() + 1, 
threadsAfter.size());
 }

```

The thread count difference is always equals to the number of iterations in the 
loop, e.g. on each call a
thread is created and stays around. Eventually our server crashes with:

[19497.011s][warning][os,thread] Attempt to protect stack guard pages failed 
(0x7fcc4c65c000-0x7fcc4c66).
OpenJDK 64-Bit Server VM warning: INFO: os::commit_memory(0x7fcc4c55b000, 
16384, 0) failed; error='Not enough space' (errno=12)

The issue is not observed with java-14, thus I assume that the fix is related 
to commit

http://hg.openjdk.java.net/jdk/jdk/rev/6717d7e59db4

As java-11 is LTS, what is the procedure to get it fix back-ported?

Regards,
Tigran.

Re: [15] RFR JDK-8246031: Hang observed with coherence SSLNIOServer test

[Seán Coffey]

Looks like the right approach to me Prasad. Reviewed.

regards,
Sean.

On 04/06/2020 16:13, Prasadrao Koppula wrote:


Hi,

Could you please review this patch. For timeout/interrupts, JDK11u+ 
releases, SSLSocket:getSession behavior is different, compare to 
JDK8u. i.e, connection is in open state for timeout/interrupts 
exception. For comparability reasons, this fix will close connection 
for getSession timeout/ interrupts.


Bug: https://bugs.openjdk.java.net/browse/JDK-8246031

Webrev: http://cr.openjdk.java.net/~pkoppula/8246031/webrev.00/

Thanks,

Prasad.K

Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable

[Seán Coffey]

I share the same concern. clone() is a heavy weight operation in 
constructors that can be called alot during intensive crypto operations.


Now that you've done work on Delegate class - why not also keep the 
(instanceof Cloneable) test ? That can serve as your fastpath for the 
default JDK configuration AFAIK.


regards,
Sean.

On 05/06/2020 00:16, Weijun Wang wrote:



在 2020年6月5日，03:19，Valerie Peng  写道：


Can you give an example when these 2 rules have different results? Is this only 
true for those implementation that directly subclass MessageDigest?

Before this fix, even the Spi impl implements Cloneable the instanceof check 
will always fail because the wrapper class, i.e. MessageDigest.Delegate does 
not. However, if you call the clone() (made public by the MessageDigest class), 
it will succeed because Delegate.clone() checks to see if the spi object 
implements the Cloneable interface, if yes, it will proceed to call the spi 
clone(). So, for this scenario, the results are different, e.g. instanceof 
returns false, but clone() succeeds. This is just one example. Is this what you 
are asking?

No.

I understand this case, but this has already been fixed. Is there any other 
example? Or are you only follow the words in the spec? i.e. try clone() to see 
if it’s cloneable.

I am worried that try clone() is much heavier than just check instanof 
Cloneable.

Thanks,
Max

Re: RFR[jdk] 8237474: Default SSLEngine should create in server role

[Seán Coffey]

Looks good to me also Prasad. Trivially, I think you can drop the 
'object' word in this implNote:


"for a new {@code SSLEngine} object"

Also, don't forget to create the release note sub-task for this change.

regards,
Sean.

On 02/04/2020 16:56, Prasadrao Koppula wrote:

Thanks for review Xuelei, I will incorporate your suggestions.

Thanks,
Prasad.K


-Original Message-
From: Xuelei Fan
Sent: Thursday, April 2, 2020 9:12 PM
To: security-dev@openjdk.java.net
Subject: Re: RFR[jdk] 8237474: Default SSLEngine should create in server role

Please update copyright year to 2020.

SSLEngine.java
--
@@ -1109,10 +1115,14 @@
+ * @implNote
+ * The JDK SunJSSE provider implementation returns false unless
{@link setUseClientMode}
+ *  is used to change the mode to true.

For the link, I may add parameter, and limit the line under 80 characters, and
don't indent the lines.

+ * @implNote
- * The JDK SunJSSE provider implementation returns false unless
{@link setUseClientMode}
- *  is used to change the mode to true.
+ * The JDK SunJSSE provider implementation returns false unless
+ * {@link setUseClientMode(boolean)} is used to change the mode
+  * to true.

It's fine to leave the CSR  as it is.

Otherwise, looks fine to me.

Xuelei

On 3/30/2020 6:50 AM, Prasadrao Koppula wrote:

Hi,

Added @implnote and updated test changes, here is the new webrev,
please review it.

Webrev: http://cr.openjdk.java.net/~pkoppula/8237474/webrev.01/

issue: https://bugs.openjdk.java.net/browse/JDK-8237474

CSR: https://bugs.openjdk.java.net/browse/JDK-8238593

Thanks,

Prasad.K

*From:* Prasadrao Koppula
*Sent:* Friday, February 7, 2020 5:03 PM
*To:* security-dev@openjdk.java.net
*Subject:* RFR[jdk] 8237474: Default SSLEngine should create in server
role

Hi,

Could you please review this patch. Default server role mode was
flipped in SSLEngine, to client role mode as part of SSL package code
refactoring for TLSv1.3, this patch flips back default client role to
server role in SSLEngine.

webrev: http://cr.openjdk.java.net/~pkoppula/8237474/webrev.00/

issue: https://bugs.openjdk.java.net/browse/JDK-8237474

CSR: https://bugs.openjdk.java.net/browse/JDK-8238593

Thanks,

Prasad.K

Re: Possible regression in JDK 14 related to SSLSessionContext / SSLSession on the server side

[Seán Coffey]

Looks interesting Norman. Do you want to share some more details about 
the peculiarities of this handshake before considering a fully fledged 
testcase ?


regards,
Sean.

On 27/03/2020 12:48, Norman Maurer wrote:

Hi there,

I am just about to add JDK14 to the test matrix for netty and think I found a 
regression. Before I will invest time to write a standalone reproducer please 
let me know if you think this is a regression or not.
Basically after the handshake is complete 
SSLEngine.getSession().getSessionContext() returns null on the serverside when 
using JDK14. Running the same test with any previous version (JDK13 and 
earlier) doesn’t show the same result.

Does this sounds like a regression and if so should I provide a standalone 
reproducer here ?

Bye
Norman

Re: RFR JDK-8240988 : Incorrect copyright header in CertificateValidation.java

[Seán Coffey]

Looks fine to me Ravi.

regards,
Sean.

On 30/03/2020 12:14, Ravi Reddy wrote:

Hello All,
  
Could you please review this attached patch. This patch fixes the "Incorrect copyright header in CertificateValidation.java".




Issue :https://bugs.openjdk.java.net/browse/JDK-8240988  




Thanks,
Ravi

Re: RFR[jdk] 8237474: Default SSLEngine should create in server role

[Seán Coffey]

Looks ok to me Prasad. This may also be worthy of highlighting via 
release note. You might be able to expand test coverage to capture the 
TLSContext scenario. Something like below patch might work ?



--- 
a/test/jdk/sun/security/ssl/SSLEngineImpl/EngineEnforceUseClientMode.java
+++ 
b/test/jdk/sun/security/ssl/SSLEngineImpl/EngineEnforceUseClientMode.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2004, 2018, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2004, 2020, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,7 +28,7 @@

 /*
  * @test
- * @bug 4980882 8207250
+ * @bug 4980882 8207250 8237474
  * @summary SSLEngine should enforce setUseClientMode
  * @run main/othervm EngineEnforceUseClientMode
  * @author Brad R. Wetmore
@@ -87,6 +87,9 @@
  * Note, these are not initialized to client/server
  */
 ssle3 = sslc.createSSLEngine();
+if (ssle3.getUseClientMode()) {
+throw new RuntimeException("Expected default role to be 
server");

+}
 ssle4 = sslc.createSSLEngine();
 ssle5 = sslc.createSSLEngine();
 }

Regards,
Sean.

On 07/02/20 11:32, Prasadrao Koppula wrote:


Hi,

Could you please review this patch. Default server role mode was 
flipped in SSLEngine, to client role mode as part of SSL package code 
refactoring for TLSv1.3, this patch flips back default client role to 
server role in SSLEngine.


webrev: http://cr.openjdk.java.net/~pkoppula/8237474/webrev.00/ 



issue: https://bugs.openjdk.java.net/browse/JDK-8237474

CSR: https://bugs.openjdk.java.net/browse/JDK-8238593

Thanks,

Prasad.K

Re: RFR(jdk): 8238502: sunmscapi.dll causing EXCEPTION_ACCESS_VIOLATION

[Seán Coffey]

Looks fine to me also. Thanks for handling.

Sean.

On 4 February 2020 21:11:04 GMT, Bradford Wetmore  
wrote:
>Ah yes, defaulted to enhancement.  Thanks.
>
>Brad
>
>
>On 2/4/2020 11:48 AM, Sean Mullan wrote:
>> Looks good. I assume this should be a Bug and not an Enhancement
>though?
>> 
>> --Sean
>> 
>> On 2/4/20 2:42 PM, Bradford Wetmore wrote:
>>> (Sigh, forgot location)
>>>
>>>
>>> Simple review request.  Weijun has provided the fix, so pushing for
>him.
>>>
>>> 8238502: sunmscapi.dll causing EXCEPTION_ACCESS_VIOLATION
>>> Reviewed-by: wetmore, coffeys
>>>
>>> A JNI call isn't properly sizing its arguments, and is causing VM 
>>> crashes.
>>>
>>> http://cr.openjdk.java.net/~wetmore/8238502/
>>>
>>> Brad

Re: RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi Philipp,

thanks for the reply .. comments inline

On 19/01/20 16:09, Philipp Kunz wrote:

Hi Sean,

I figure that distinguishing zips and jars is ambiguous in a certain
way. After having signed a zip, it contains a manifest and is also a
jar file according to the specs. That would mean we would end up with
two kinds of jar files, those with permissions and those without,
depending on the file name extension of the files. That a tool named
jarsigner is successfully applied to a file is for me convincing enough
to consider that file a jar file or jarsigned would reject it however
it name ends.

Yes, both zip and jar files enjoy the same format. jar files being created
predominantly with the java based API/tools.


Now when signing jar files (namely those with zip file name extension
but depending on the further discussion not necessary limited to those)
with permissions, I wonder if those permissions should also be subject
of the signature. I would consider a change of an executable flag for
example a change of the signed file just the same as a changed byte of
contents. The way jarsigner works now, loosing the permissions, we know
at least that the files contained in signed jars have certain
permission flags (none or defaults) immediately after having signed
them even though later manipulation would not be detected and, hence,
cannot be trusted even with a valid signature.
historically, jar tool never recorded these permissions. As such, it 
should make

no difference to how jarsigner operates on such files. The original report
highlights the loss of data on files created with the zip utility. It 
think it's reasonable

for jarsigner to preserve attributes in such a case.


Far as I understand the current situation, jars and zips could be told
from one another only before signing depending on the presence or
absence of a manifest. After signing, however, a manifest is always
there. Looking at it that way, the "much more consideration" you
mention would have to take place now, wouldn't it?

I'm not sure if there's much change in this area post fix. The classic
jar tool doesn't add or read the file attributes at hand. The name check
was useful to prevent any unintended behavioural changes in this area.


I also wonder why the "if (filename.endsWith(".zip"))" piece of code
does not occur only in JarSigner. Why is it in ZipFile, too? One such
kind of if statement should have been enough, at first glance. There
may be a good and convincing explanation but I don't see it just like
that. On the other hand, having this if condition duplicated into
ZipFile looks like it could have an undesired side-effect elsewhere. I
also kind of miss another test case to make sure permission flags
continue to be disregarded for "non-zip" jar files.

fair point. I should be able to remove this code. I can also improve the
test case.


And the comment of Michael Osipov is really frightening. Did you know
that i and I are two different characters in turkish both of which have
yet other capital/small counterparts? At that point maybe discussing
the file name extension distinction could easily become lengthier than
a discussion about consequences of supporting permissions for all jar
files.

frightening ??  Yes, I've worked on such issues before. Thanks to Michael
for pointing that out. I should be able to do a comparison using Locale.ROOT


You mention "the case, at hand is unique here" and that made to occur
to me that if there is maybe only one person affected that he or she
could work around it more easily than changing the jdk. Is it really
important for most others? And is it really a bug at all?

Another idea to work around compatibility issues would be to introduce
a flag for jarsigner, for example -p like with tar. Yet another idea
could be to refire the jar file specs and jar and jarsigner tools docs
accordingly.

Let me have a think about this. A new flag in jarsigner may help.

regards,
Sean.


Regards,
Philipp


On Fri, 2020-01-17 at 13:07 +, Seán Coffey wrote:

Hi Philipp,

On 17/01/2020 12:40, Philipp Kunz wrote:

Hi Sean,

Nice patch. I wonder why permissions should be preserved only in
zip
files. Jar files also are zip files, according to the jar file
specs,
and hence, shouldn't jar files benefit of preserving permissions,
too?

Thanks for your comments. The jar tool has never been interested in
the
posix
permissions fields for the individual entries. Such a change could
yield
more
interoperability issues. Such a change would also need much more
consideration

The zip tool on the other hand has always populated this field and I
think the case
at hand is unique here (preserving attributes already created by
non-java tools)

The file name extension is most often zip for zip files and jar for
jar
files but is that really a safe assumption? I would not expect it
always. Removing

Yes, I didn't see any easy way to distinguish a zip file from a jar
file
withou

Re: RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi Philipp,

On 17/01/2020 12:40, Philipp Kunz wrote:

Hi Sean,

Nice patch. I wonder why permissions should be preserved only in zip
files. Jar files also are zip files, according to the jar file specs,
and hence, shouldn't jar files benefit of preserving permissions, too?
Thanks for your comments. The jar tool has never been interested in the 
posix
permissions fields for the individual entries. Such a change could yield 
more
interoperability issues. Such a change would also need much more 
consideration


The zip tool on the other hand has always populated this field and I 
think the case
at hand is unique here (preserving attributes already created by 
non-java tools)


The file name extension is most often zip for zip files and jar for jar
files but is that really a safe assumption? I would not expect it
always. Removing
Yes, I didn't see any easy way to distinguish a zip file from a jar file 
without being
more invasive and scanning file attributes for that file. I could take 
that approach

if it's deemed necessary.

regards,
Sean.



if (zf.getName().toLowerCase().endsWith(".zip")) {

along with similar code in ZipFile would avoid discussing that question
and the test would not have to check that files with another name
extension than zip don't preserve permissions.

Philipp


On Fri, 2020-01-17 at 10:59 +0000, Seán Coffey wrote:

Hi,

Looking to introduce some JDK private functionality which will help
preserve internal zip file attribute permissions when jarsigner is
run
on a zip file. Some of the logic is taken from the recent work
carried
out in this area for zipfs API.

https://bugs.openjdk.java.net/browse/JDK-8218021
http://cr.openjdk.java.net/~coffeys/webrev.8218021/webrev/

regards,
Sean.

RFR: 8218021: jarsigner strips the execute permission when signing a .zip file

[Seán Coffey]

Hi,

Looking to introduce some JDK private functionality which will help 
preserve internal zip file attribute permissions when jarsigner is run 
on a zip file. Some of the logic is taken from the recent work carried 
out in this area for zipfs API.


https://bugs.openjdk.java.net/browse/JDK-8218021
http://cr.openjdk.java.net/~coffeys/webrev.8218021/webrev/

regards,
Sean.

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

Thanks Alan. Updates made and changes pushed.

regards,
Sean.

On 13/01/2020 18:50, Alan Bateman wrote:

On 13/01/2020 10:28, Seán Coffey wrote:
some off line comments suggested that I could move the jar 
initialization checks to the EventHelper class. With that in place, 
the EventHelper utility class should never initialize the logging 
framework early during jar initialization.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/
Thanks for the update. JAR file verification is tricky and important 
not to attempt to run arbitrary code while doing that, esp. anything 
that might need to load a class or resource from the class path. So I 
think the approach (in v5) looks okay.  A minor nit in JarFile is that 
it should be "static final".  Also you might want to replace or change 
the @summary in both tests to make it clearer that the tests attempt 
to trigger class loading from the class loader during JAR file 
verification.


-Alan.

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

Thanks for the reviews. All callers of EventHelper log methods are 
ensuring that isLoggingSecurity() is true before proceeding. I've added 
an assert null check in the 4 logger methods to ensure expectations are 
in place.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v5/webrev/

Hope this helps,
Sean.

On 13/01/2020 14:38, Daniel Fuchs wrote:

On 13/01/2020 14:06, Chris Hegarty wrote:
I’m going to ask, since I cannot find the answer myself. Why are some 
securityLogger::log invocations guarded with isLoggingSecurity, and 
others not? With this change shouldn’t all invocations be guarded, 
since it is isLoggingSecurity that assigns securityLogger a value?


Argh! Well spotted chris!

-- daniel

Re: RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

some off line comments suggested that I could move the jar 
initialization checks to the EventHelper class. With that in place, the 
EventHelper utility class should never initialize the logging framework 
early during jar initialization.


http://cr.openjdk.java.net/~coffeys/webrev.8234466.v4/webrev/

regards,
Sean.

On 16/12/2019 14:15, Seán Coffey wrote:
The recent crypto event logging mechanism (JDK-8148188) has introduced 
a regression whereby the System Logger may be invoked too early in the 
bootstrap phase. This causes issue when JarFile objects are locked by 
JarFile verifier initialization code. The logging work records an X509 
Certificate which is used during the jar file 
verification/initialization phase and hence leads to an early 
System.Logger call.


One thread invokes the initialization of the Logger framework via 
ServiceLoader and waits to lock a JarFile in use via another thread. 
Unfortunately that other thread is also waiting for the System Logger 
to initialize. For now, I think we can avoid the early Logger 
initialization via use of a ThreadLocal. I've tried reproducing the 
reported issue through manual and automated tests but to no avail. 
I've added a new ServiceLoader test which has concurrent threads. One 
is loading providers and another is initializing JarFile verifiers. 
Hope it helps improve code coverage for the future.


JBS record: https://bugs.openjdk.java.net/browse/JDK-8234466
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8234466/webrev/

RFR: 8234466: Class loading deadlock involving X509Factory#commitEvent()

[Seán Coffey]

The recent crypto event logging mechanism (JDK-8148188) has introduced a 
regression whereby the System Logger may be invoked too early in the 
bootstrap phase. This causes issue when JarFile objects are locked by 
JarFile verifier initialization code. The logging work records an X509 
Certificate which is used during the jar file 
verification/initialization phase and hence leads to an early 
System.Logger call.


One thread invokes the initialization of the Logger framework via 
ServiceLoader and waits to lock a JarFile in use via another thread. 
Unfortunately that other thread is also waiting for the System Logger to 
initialize. For now, I think we can avoid the early Logger 
initialization via use of a ThreadLocal. I've tried reproducing the 
reported issue through manual and automated tests but to no avail. I've 
added a new ServiceLoader test which has concurrent threads. One is 
loading providers and another is initializing JarFile verifiers. Hope it 
helps improve code coverage for the future.


JBS record: https://bugs.openjdk.java.net/browse/JDK-8234466
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8234466/webrev/

--
Regards,
Sean.

Re: RFR [14] JDK-8235655, Clean the duplicated block in SSLContextImpl

[Seán Coffey]

Looks fine to me.

regards,
Sean.

On 10/12/2019 14:35, Xuelei Fan wrote:



On 12/10/2019 6:34 AM, Xuelei Fan wrote:

Hi,

Could I get the following code-cleanup patch reviewed?
    cr.openjdk.java.net/~xuelei/8235655/webrev.00


missing the protocol part:
 http://cr.openjdk.java.net/~xuelei/8235655/webrev.00/

In the SSLContextImpl.java, duplicated arrays are used in the same 
block, which could be simplified.


Simple update, code cleanup, no new regression test.

Thanks,
Xuelei

Re: SunPKCS11 connection lost after Decrypt doFinal (noPadding) openjdk 8_232

[Seán Coffey]

Also,

which JDK distro version of 8 are you using ? Note that the OpenJDK 
version has an extra few pkcs11 fixes in OpenJDK in this area (compared 
to the Oracle JDK 8 Updates) - Below being some of those :


https://bugs.openjdk.java.net/browse/JDK-8216597
https://bugs.openjdk.java.net/browse/JDK-6913047
https://bugs.openjdk.java.net/browse/JDK-6946830

Regards,
Sean.

On 03/12/19 00:56, Valerie Peng wrote:

Hi Hubert,

I've filed https://bugs.openjdk.java.net/browse/JDK-8235215 to keep 
track of this issue.


I have not yet tried if this can be reproduced in house with NSS yet.

Just curious, which HSM vendor did you use? It'd be helpful to include 
in the bug report.


Thanks,
Valerie
On 12/2/2019 8:50 AM, DEBORDEAUX Hubert wrote:

Hello,
Following the update to OpenJDK 8_232, we did face a problem after a 
DECRYPT with no padding.

We use a SunPKCS11 provider linked to a Network HSM.
After a DECRYPT command (DES or AES) NOPADDING, we noticed the log : 
"Killing session 
(sun.security.pkcs11.P11Cipher.cancelOperation(P11Cipher.java:428)) 
active: 1"

All following commands failed with error : CKR_USER_NOT_LOGGED_IN

After a quick investigation, it looks like the fix JDK-8228565 done 
in P11Cipher.java is the root cause of this new behavior.


// Special handling to match SunJCE provider behavior
 if (bytesBuffered == 0 && padBufferLen == 0) {
 return 0;
 }

} finally {
 reset(doCancel);   // doCancel is true, so 
killSession is called.

 }

This is a source code to reproduce the problem:
SunPKCS11 p = new SunPKCS11(configName);   // config to 
Network HSM

p.setCallbackHandler(handler);// Handler for password
 Security.addProvider(p);
  KeyStore.CallbackHandlerProtection chp =
 new KeyStore.CallbackHandlerProtection(handler);
 KeyStore.Builder builder = 
KeyStore.Builder.newInstance("PKCS11", p, chp);

 KeyStore keystore = builder.getKeyStore();
SecretKeyEntry entry = (SecretKeyEntry) 
keystore.getEntry("MyKeyAlias", null);
  Cipher cipher = 
Cipher.getInstance("DESede/CBC/NOPADDING", p.getName());
 IvParameterSpec ivParameterSpec = new 
IvParameterSpec(new byte[8]);

 // cipher a text
 cipher.init(Cipher.ENCRYPT_MODE, entry.getSecretKey(), 
ivParameterSpec);

 byte[] clearData = "clear text11".getBytes();
 byte[] cipheredData = cipher.doFinal(clearData);
// Decipher the result
 cipher.init(Cipher.DECRYPT_MODE, entry.getSecretKey(), 
ivParameterSpec);

 byte[] clearTextResult = cipher.doFinal(cipheredData);
// display the result
System.out.println(new String(clearTextResult));  // So far, no 
problem

// Try another cipher
 cipher.init(Cipher.ENCRYPT_MODE, entry.getSecretKey(), 
ivParameterSpec);

byte[] clearData2 = "clear text22".getBytes();
byte[] cipheredData2 = cipher.doFinal(clearData);
// --> Failed with sun.security.pkcs11.wrapper.PKCS11Exception: 
CKR_USER_NOT_LOGGED_IN


Caused by: sun.security.pkcs11.wrapper.PKCS11Exception: 
CKR_USER_NOT_LOGGED_IN

at sun.security.pkcs11.wrapper.PKCS11.C_EncryptUpdate(Native Method)
at sun.security.pkcs11.P11Cipher.implUpdate(P11Cipher.java:581)

 Workarounds:
. use the SunPkcs11 jar file from openJDK 8_222
. add a login after every decrypt commands
. use PKCS5Padding when possible

Could you tell me if you can reproduce this problem and what is the 
best way for me to report it ?


Thanks you
Best Regards,
Hubert

RFR: 8233801:GCMEmptyIv.java test fails on Solaris 11.4

[Seán Coffey]

Seeing an internal test failure on Solaris 11.4. Appears connected with 
the recent upgrade of PKCS11 libraries to v2.40. The test coverage has 
increased since SunPKCS11-Solaris now supports  AES/GCM. Unfortunately 
the bug details are not public but I'll give a summary here.


The new test code coverage provokes a CKR_MECHANISM_PARAM_INVALID error 
from the underlying provider and the test expects 
InvalidAlgorithmParameterException to be thrown by the Provider. 
InvalidKeyException is currently thrown by SunPKCS11-Solaris. The patch 
is quite trivial:



+++ b/src/share/classes/sun/security/pkcs11/P11AEADCipher.java
@@ -322,6 +322,9 @@
 try {
 initialize();
 } catch (PKCS11Exception e) {
+ if (e.getErrorCode() == CKR_MECHANISM_PARAM_INVALID) {
+ throw new InvalidAlgorithmParameterException("Bad params", e);
+ }
 throw new InvalidKeyException("Could not initialize 
cipher", e);

 }
 }


regards,
Sean.

Re: RFR 8233884 : Avoid looking up standard charsets in security libraries

[Seán Coffey]

On 11/11/2019 20:56, Ivan Gerasimov wrote:

Thank you Seán for reviewing!

On 11/11/19 7:56 AM, Seán Coffey wrote:

Nice work Ivan.

I see you've some clean up done on exception handling also. I might 
have a concern on this change in SSLLogger. You're catching 
IOException now instead of Exception. Given that it's a logger and 
the intent seems to be to ignore any type of exception - should we 
leave what's there ?



IOException seems to be the only type of Exception that may be thrown 
here (otherwise javac would have complained during a build), but I 
agree it can be left as it was before.


Sure - I'm thinking the original code was also protecting against 
possible unchecked exceptions. (to what value is questionable!)




Here's the updated webrev with this only change, comparing to the 
previous one:


http://cr.openjdk.java.net/~igerasim/8233884/01/webrev/


thanks. Looks fine to me.

regards,
Sean.



With kind regards,
Ivan



Everything else looked fine to me.

Regards,
Sean.

On 10/11/19 09:47, Ivan Gerasimov wrote:

Hello!

There are many places in the security libraries where string are 
converted from/to byte arrays using standard charsets.


It would be beneficial, if those are not looked up by their name 
every time, and constants from java.nio.charset.StandardCharsets.* 
are used instead.


Would you please help review this relatively lengthy (in terms of 
the affected files), though mostly straight-forward cleanup?


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8233884
WEBREV: http://cr.openjdk.java.net/~igerasim/8233884/00/webrev/

Thanks in advance!

Re: RFR 8233884 : Avoid looking up standard charsets in security libraries

[Seán Coffey]

Nice work Ivan.

I see you've some clean up done on exception handling also. I might have 
a concern on this change in SSLLogger. You're catching IOException now 
instead of Exception. Given that it's a logger and the intent seems to 
be to ignore any type of exception - should we leave what's there ?


Everything else looked fine to me.

Regards,
Sean.

On 10/11/19 09:47, Ivan Gerasimov wrote:

Hello!

There are many places in the security libraries where string are 
converted from/to byte arrays using standard charsets.


It would be beneficial, if those are not looked up by their name every 
time, and constants from java.nio.charset.StandardCharsets.* are used 
instead.


Would you please help review this relatively lengthy (in terms of the 
affected files), though mostly straight-forward cleanup?


BUGURL: https://bugs.openjdk.java.net/browse/JDK-8233884
WEBREV: http://cr.openjdk.java.net/~igerasim/8233884/00/webrev/

Thanks in advance!

Re: CSR Review request (11-pool): JDK-8233825: Update SunPKCS11 provider with PKCS11 v2.40 support

[Seán Coffey]

Hey Christoph,

I've added myself as reviewer for this CSR. Hope that's ok.

There was a bug tail with this PKCS11 upgrade (interoperability issues 
due to ambiguity in the spec)

See JDK-8229243 also (and the JDK-8225695 regression)

Regards,
Sean.

On 07/11/19 22:19, Langer, Christoph wrote:


Hi Valerie, Sean,

may I please ask you to add yourself as reviewer to the backport CSR 
JDK-8233825: “Update SunPKCS11 provider with PKCS11 v2.40 support” 
[0]. It is a CSR for backporting JDK-8080462 to OpenJKD 11u. Oracle 
did that already for 11.0.6 but with the internal ccc process. Joe 
indicated, though, that it should be clearer to do an 11u backport CSR 
still for OpenJDK. I’ve linked the backport CSR to the Oracle 11.0.6 
backport as well.


Thanks

Christoph

[0] https://bugs.openjdk.java.net/browse/JDK-8233825 



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

Re: Is there any reason not to have 8223269 in JDK baseline?

[Seán Coffey]

Was OOTO. No issues from me on this one!

regards,
Sean.

On 31/10/2019 15:01, Martin Balao wrote:

Hi Sean,

Thanks for your answer.

Even though newer JDKs should be using the PKCS12 keystore format, there
are still customers using the old JKS format and they would benefit from
this feature.

Is there any objection if I propose this for JDK base line?

Thanks,
Martin.-


On 10/30/19 8:14 PM, Seán Coffey wrote:

Martin,

Newer JDK families should be using the PKCS12 keystore format. For that
reason, I kept it 8u only. It was a solution to address a particular use
case reported by an Oracle JDK user.

regards,
Sean.

On 30/10/2019 18:21, Martin Balao wrote:

Hi,

I've noticed that 8223269 [1] (not public) has been included in Oracle's
8u231 JDK [2].

Is there a reason not to have this in JDK baseline? (and, thus, create a
backport for the open JDKs)

In case there is not, I'll proceed with a new ticket (unless you want to
make [1] public), a CSR and a patch.

Thanks,
Martin.-

--
[1] - https://bugs.openjdk.java.net/browse/JDK-8223269
[2] -
https://www.oracle.com/technetwork/java/javase/8u231-relnotes-5592812.html#JDK-8223269

Re: Is there any reason not to have 8223269 in JDK baseline?

[Seán Coffey]

Martin,

Newer JDK families should be using the PKCS12 keystore format. For that 
reason, I kept it 8u only. It was a solution to address a particular use 
case reported by an Oracle JDK user.


regards,
Sean.

On 30/10/2019 18:21, Martin Balao wrote:

Hi,

I've noticed that 8223269 [1] (not public) has been included in Oracle's
8u231 JDK [2].

Is there a reason not to have this in JDK baseline? (and, thus, create a
backport for the open JDKs)

In case there is not, I'll proceed with a new ticket (unless you want to
make [1] public), a CSR and a patch.

Thanks,
Martin.-

--
[1] - https://bugs.openjdk.java.net/browse/JDK-8223269
[2] -
https://www.oracle.com/technetwork/java/javase/8u231-relnotes-5592812.html#JDK-8223269

Re: JDK 14 RFR of JDK-8231368: Suppress warnings on non-serializable non-transient instance fields in java.security.jgss

[Seán Coffey]

Looks fine Joe.

regards,
Sean.

On 23/09/2019 17:15, Joe Darcy wrote:

Hello,

Another module, another review request as part of making serial 
warnings more robust:


    JDK-8231368: Suppress warnings on non-serializable non-transient 
instance fields in java.security.jgss

    http://cr.openjdk.java.net/~darcy/8231368.0/

(Related earlier review 
https://mail.openjdk.java.net/pipermail/security-dev/2019-September/020672.html.)


In this latest review, I included a comment in KRBError.java that its 
writeObject method uses a different encoding scheme.


Thanks,

-Joe

Re:(JDK-8230297) Slow LoginContext.login() due to repeated ServiceLoader lookups

[Seán Coffey]

Thanks Florent. Now visible at 
https://bugs.openjdk.java.net/browse/JDK-8230297


regards,
Sean.

On 27/08/2019 20:04, Florent Guillaume wrote:
I'm not an Author so I created the ticket on bugreport.java.com 
<http://bugreport.java.com>, it has the internal review ID 9062061


Thanks,
Florent

On Tue, Aug 27, 2019 at 6:04 PM Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Probably best to log a bug to capture this issue.

It reminds me of another issue I've been meaning to wrap up:
https://bugs.openjdk.java.net/browse/JDK-8223260

Similar scenario in how the ContextFactory is searched for. My
proposed
patch is to cache a factory per classloader (for the NamingManager
issue at least)

regards,
Sean.

On 27/08/2019 16:16, Florent Guillaume wrote:

Hi,

When switching from Java 8 to Java 11, we're experiencing an
important slowdown when executing LoginContext.login(),
especially under concurrency.

We tracked this down to JDK-8047789 which changed the way the
lookup of LoginModules is done in LoginContext.invoke.
Previously, it was a simple Class.forName that is of course
extremely optimized. After JDK-8047789, there is first a
ServiceLoader-based lookup for the class. This lookup doesn't
seem to be cached. In our case, it has to open the 400+ JARs in
our classpath (we're not using modules yet) to check the content
of META-INF/services/javax.security.auth.spi.LoginModule, and in
addition this hits a synchronized block in ZipFile.getEntry which
prevents any performance under concurrency.

Is there anything we can do to improve LoginContext.login() in
this context?

For reference, the code path to the synchronized block:
at
java.util.zip.ZipFile.getEntry(java.base@11.0.4/ZipFile.java:346
<mailto:java.base@11.0.4/ZipFile.java:346>)
- locked <0x00068b18bdd0> (a java.util.jar.JarFile)
at
java.util.zip.ZipFile$1.getEntry(java.base@11.0.4/ZipFile.java:1121
<mailto:java.base@11.0.4/ZipFile.java:1121>)
at
java.util.jar.JarFile.getEntry0(java.base@11.0.4/JarFile.java:576
<mailto:java.base@11.0.4/JarFile.java:576>)
at
java.util.jar.JarFile.getEntry(java.base@11.0.4/JarFile.java:506
<mailto:java.base@11.0.4/JarFile.java:506>)
at
java.util.jar.JarFile.getJarEntry(java.base@11.0.4/JarFile.java:468
<mailto:java.base@11.0.4/JarFile.java:468>)
at

jdk.internal.loader.URLClassPath$JarLoader.getResource(java.base@11.0.4/URLClassPath.java:929
<mailto:java.base@11.0.4/URLClassPath.java:929>)
at

jdk.internal.loader.URLClassPath$JarLoader.findResource(java.base@11.0.4/URLClassPath.java:912
<mailto:java.base@11.0.4/URLClassPath.java:912>)
at

jdk.internal.loader.URLClassPath$1.next(java.base@11.0.4/URLClassPath.java:341
<mailto:java.base@11.0.4/URLClassPath.java:341>)
at

jdk.internal.loader.URLClassPath$1.hasMoreElements(java.base@11.0.4/URLClassPath.java:351
<mailto:java.base@11.0.4/URLClassPath.java:351>)
at
java.net.URLClassLoader$3$1.run(java.base@11.0.4/URLClassLoader.java:687
<mailto:java.base@11.0.4/URLClassLoader.java:687>)
at
java.net.URLClassLoader$3$1.run(java.base@11.0.4/URLClassLoader.java:685
<mailto:java.base@11.0.4/URLClassLoader.java:685>)
at
java.security.AccessController.doPrivileged(java.base@11.0.4/Native
<mailto:java.base@11.0.4/Native> Method)
at
java.net.URLClassLoader$3.next(java.base@11.0.4/URLClassLoader.java:684
<mailto:java.base@11.0.4/URLClassLoader.java:684>)
at

java.net.URLClassLoader$3.hasMoreElements(java.base@11.0.4/URLClassLoader.java:709
<mailto:java.base@11.0.4/URLClassLoader.java:709>)
at
java.lang.CompoundEnumeration.next(java.base@11.0.4/ClassLoader.java:3022
<mailto:java.base@11.0.4/ClassLoader.java:3022>)
at

java.lang.CompoundEnumeration.hasMoreElements(java.base@11.0.4/ClassLoader.java:3031
<mailto:java.base@11.0.4/ClassLoader.java:3031>)
at

org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.inc(WebappClassLoaderBase.java:2670)
at

org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.hasMoreElements(WebappClassLoaderBase.java:2655)
at

java.util.ServiceLoader$LazyClassPathLookupIterator.nextProviderClass(java.base@11.0.4/ServiceLoader.java:1202
<mailto:java.base@11.0.4/ServiceLoader.java:1202>)
at

java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(java.base@11.0.4/ServiceLoader.java:1220
<mailto:java.base@11.0.4/ServiceLoader.java:1220>)
at

java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(java.base@11.0.4/ServiceLoader.java:1264
<mailto:java.base@11.0.4/ServiceLoader.java:1264>)
at
java.util.ServiceLoader$2.hasNext(java.

Re: Slow LoginContext.login() due to repeated ServiceLoader lookups

[Seán Coffey]

Probably best to log a bug to capture this issue.

It reminds me of another issue I've been meaning to wrap up:
https://bugs.openjdk.java.net/browse/JDK-8223260

Similar scenario in how the ContextFactory is searched for. My proposed
patch is to cache a factory per classloader (for the NamingManager issue 
at least)


regards,
Sean.

On 27/08/2019 16:16, Florent Guillaume wrote:

Hi,

When switching from Java 8 to Java 11, we're experiencing an important 
slowdown when executing LoginContext.login(), especially under 
concurrency.


We tracked this down to JDK-8047789 which changed the way the lookup 
of LoginModules is done in LoginContext.invoke. Previously, it was a 
simple Class.forName that is of course extremely optimized. 
After JDK-8047789, there is first a ServiceLoader-based lookup for the 
class. This lookup doesn't seem to be cached. In our case, it has to 
open the 400+ JARs in our classpath (we're not using modules yet) to 
check the content 
of META-INF/services/javax.security.auth.spi.LoginModule, and in 
addition this hits a synchronized block in ZipFile.getEntry which 
prevents any performance under concurrency.


Is there anything we can do to improve LoginContext.login() in this 
context?


For reference, the code path to the synchronized block:
at java.util.zip.ZipFile.getEntry(java.base@11.0.4/ZipFile.java:346)
- locked <0x00068b18bdd0> (a java.util.jar.JarFile)
at java.util.zip.ZipFile$1.getEntry(java.base@11.0.4/ZipFile.java:1121)
at java.util.jar.JarFile.getEntry0(java.base@11.0.4/JarFile.java:576)
at java.util.jar.JarFile.getEntry(java.base@11.0.4/JarFile.java:506)
at java.util.jar.JarFile.getJarEntry(java.base@11.0.4/JarFile.java:468)
at 
jdk.internal.loader.URLClassPath$JarLoader.getResource(java.base@11.0.4/URLClassPath.java:929)
at 
jdk.internal.loader.URLClassPath$JarLoader.findResource(java.base@11.0.4/URLClassPath.java:912)
at 
jdk.internal.loader.URLClassPath$1.next(java.base@11.0.4/URLClassPath.java:341)
at 
jdk.internal.loader.URLClassPath$1.hasMoreElements(java.base@11.0.4/URLClassPath.java:351)
at 
java.net.URLClassLoader$3$1.run(java.base@11.0.4/URLClassLoader.java:687)
at 
java.net.URLClassLoader$3$1.run(java.base@11.0.4/URLClassLoader.java:685)
at java.security.AccessController.doPrivileged(java.base@11.0.4/Native 
Method)
at 
java.net.URLClassLoader$3.next(java.base@11.0.4/URLClassLoader.java:684)
at 
java.net.URLClassLoader$3.hasMoreElements(java.base@11.0.4/URLClassLoader.java:709)
at 
java.lang.CompoundEnumeration.next(java.base@11.0.4/ClassLoader.java:3022)
at 
java.lang.CompoundEnumeration.hasMoreElements(java.base@11.0.4/ClassLoader.java:3031)
at 
org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.inc(WebappClassLoaderBase.java:2670)
at 
org.apache.catalina.loader.WebappClassLoaderBase$CombinedEnumeration.hasMoreElements(WebappClassLoaderBase.java:2655)
at 
java.util.ServiceLoader$LazyClassPathLookupIterator.nextProviderClass(java.base@11.0.4/ServiceLoader.java:1202)
at 
java.util.ServiceLoader$LazyClassPathLookupIterator.hasNextService(java.base@11.0.4/ServiceLoader.java:1220)
at 
java.util.ServiceLoader$LazyClassPathLookupIterator.hasNext(java.base@11.0.4/ServiceLoader.java:1264)
at 
java.util.ServiceLoader$2.hasNext(java.base@11.0.4/ServiceLoader.java:1299)
at 
java.util.ServiceLoader$3.hasNext(java.base@11.0.4/ServiceLoader.java:1384)
at 
javax.security.auth.login.LoginContext.invoke(java.base@11.0.4/LoginContext.java:691)
at 
javax.security.auth.login.LoginContext$4.run(java.base@11.0.4/LoginContext.java:665)
at 
javax.security.auth.login.LoginContext$4.run(java.base@11.0.4/LoginContext.java:663)
at java.security.AccessController.doPrivileged(java.base@11.0.4/Native 
Method)
at 
javax.security.auth.login.LoginContext.invokePriv(java.base@11.0.4/LoginContext.java:663)
at 
javax.security.auth.login.LoginContext.login(java.base@11.0.4/LoginContext.java:574)


Thanks,

Florent

--
Nuxeo Logo    

Florent Guillaume  Head of R LinkedIn 
 Twitter 
 Github 


Nuxeo Content Services Platform. Stay ahead.

Re: [14] RFR JDK-8229243 "SunPKCS11-Solaris provider tests failing on Solaris 11.4"

[Seán Coffey]

Thanks for taking this on Valerie. It's a shame to see such confusion 
arise from the new PKCS11 spec.


The changes look fine to me. It'll certainly benefit from widespread 
interoperability testing.


Regards,
Sean.

On 16/08/19 00:49, Valerie Peng wrote:


Anyone has time to help review this fix? PKCS#11 v2.40 has 
inconsistent definition for CK_GCM_PARAMS struct. The mechanism spec 
(sec 2..12.3) has:


typedef struct CK_GCM_PARAMS {
CK_BYTE_PTR   pIv;
CK_ULONG  ulIvLen;
CK_BYTE_PTR   pAAD;
CK_ULONG  ulAADLen;
CK_ULONG  ulTagBits;
} CK_GCM_PARAMS;

However, the header file pkcs11t.h has an extra "ulIvBits" field:

typedef struct CK_GCM_PARAMS {
CK_BYTE_PTR   pIv;
CK_ULONG  ulIvLen;
*CK_ULONG  ulIvBits;*
CK_BYTE_PTR   pAAD;
CK_ULONG  ulAADLen;
CK_ULONG  ulTagBits;
} CK_GCM_PARAMS;

Some vendors such OpenHSM2 and Solaris go with the header file while 
some vendor such as NSS go with the mech spec. Current SunPKCS11 
provider impl works with NSS but not with other vendors whose 
CK_GCM_PARAMS struct contains ulIvBits field. Based on testing 
results, OpenHSM2 and Solaris error out at init time when the 
parameter length does not have their expected value. Thus, one way to 
workaround this inconsistency is to re-try with a different structure 
for AES GCM when the init failed. In addition, given the parameters 
contains Iv and AAD which some vendor may use during subsequent 
enc/dec operations, I changed to use the same model as RSASSA-PSS to 
defer the freeing after the enc/dec operation is finished. Verified 
the changes on Solaris 11.4 against existing GCM regression tests.


Bug: https://bugs.openjdk.java.net/browse/JDK-8229243

Webrev: http://cr.openjdk.java.net/~valeriep/8229243/webrev.00/

Thanks,
Valerie

RFR: 8228645: Don't run sun/security/pkcs11/Cipher/TestKATForGCM.java on buggy NSS solaris versions

[Seán Coffey]

The sun/security/pkcs11/Cipher/TestKATForGCM.java testcase can fail when 
running
on older, buggy NSS libraries. In particular on Solaris 11.2 and 
earlier. Most of our
solaris testing occurs on 11.3 OS but we should discount failures seen 
in Solaris 11.2

test runs.

https://bugs.openjdk.java.net/browse/JDK-8228645
http://cr.openjdk.java.net/~coffeys/webrev.8228645/webrev/

regards,
Sean.

Re: RFR (XS) : 8133489: Better messaging for PKIX path validation matching

[Seán Coffey]

Thanks for the review Xuelei,

edits made:

--- a/src/java.base/share/classes/java/security/cert/X509CertSelector.java
+++ b/src/java.base/share/classes/java/security/cert/X509CertSelector.java
@@ -2115,8 +2115,11 @@
 if (certSubjectKeyID == null ||
 !Arrays.equals(subjectKeyID, certSubjectKeyID)) {
 if (debug != null) {
-    debug.println("X509CertSelector.match: "
-    + "subject key IDs don't match");
+    debug.println("X509CertSelector.match: subject key 
IDs " +
+    "don't match\nX509CertSelector.match: 
subjectKeyID: " +

+    Arrays.toString(subjectKeyID) +
+    "\nX509CertSelector.match: certSubjectKeyID: " +
+    Arrays.toString(certSubjectKeyID));
 }
 return false;

regards,
Sean.

On 20/06/2019 15:14, Xuelei Fan wrote:

On 6/20/2019 5:56 AM, Seán Coffey wrote:
A simple debugging enhancement to print out subjectkey ID details 
when mismatch is encountered. I encountered a DER encoding issue with 
an application server team a good while back and needed such a patch 
to debug the issue correctly. I added -Djava.security.debug=certpath 
to a testcase which tests this functionality. Sample output :


certpath: X509CertSelector.match: subject key IDs don't match
certpath: 509CertSelector.match: subjectKeyID: [4, 20, -12, -2, 115, 
79, -15, 106, 114, -58, 102, 43, 32, 26, 120, -76, -33, 50, -45, -56, 
-16, -38]
certpath: 509CertSelector.match: certSubjectKeyID: [4, 20, -111, 93, 
-48, -86, -39, 59, -128, -118, 45, -10, 126, -76, -115, 126, -99, 
-106, -116, 107, 124, -63]


regards,
Sean.

diff --git 
a/src/java.base/share/classes/java/security/cert/X509CertSelector.java 
b/src/java.base/share/classes/java/security/cert/X509CertSelector.java
--- 
a/src/java.base/share/classes/java/security/cert/X509CertSelector.java
+++ 
b/src/java.base/share/classes/java/security/cert/X509CertSelector.java

@@ -2117,6 +2117,10 @@
  if (debug != null) {
  debug.println("X509CertSelector.match: "
  + "subject key IDs don't match");
+    debug.println("509CertSelector.match:" +
+    " subjectKeyID: " + 
Arrays.toString(subjectKeyID));

+    debug.println("509CertSelector.match:" +
+    " certSubjectKeyID: " + 
Arrays.toString(certSubjectKeyID));

  }
  return false;
  }

Is it a typo "509CertSelector" -> "X509CertSelector"?

I may use one call to debug.println() in case the information are 
separated in multi-thread environment.


Otherwise, looks good to me.

Thanks,
Xuelei

diff --git 
a/test/jdk/java/security/cert/CertPathBuilder/selfIssued/KeyUsageMatters.java 
b/test/jdk/java/security/cert/CertPathBuilder/selfIssued/KeyUsageMatters.java 

--- 
a/test/jdk/java/security/cert/CertPathBuilder/selfIssued/KeyUsageMatters.java 

+++ 
b/test/jdk/java/security/cert/CertPathBuilder/selfIssued/KeyUsageMatters.java 


@@ -29,13 +29,13 @@

  /**
   * @test
- * @bug 6852744
+ * @bug 6852744 8133489
   * @summary PIT b61: PKI test suite fails because self signed 
certificates

   *  are being rejected
   * @modules java.base/sun.security.util
- * @run main/othervm KeyUsageMatters subca
- * @run main/othervm KeyUsageMatters subci
- * @run main/othervm KeyUsageMatters alice
+ * @run main/othervm -Djava.security.debug=certpath KeyUsageMatters 
subca
+ * @run main/othervm -Djava.security.debug=certpath KeyUsageMatters 
subci
+ * @run main/othervm -Djava.security.debug=certpath KeyUsageMatters 
alice

   * @author Xuelei Fan
   */

RFR (XS) : 8133489: Better messaging for PKIX path validation matching

[Seán Coffey]

A simple debugging enhancement to print out subjectkey ID details when 
mismatch is encountered. I encountered a DER encoding issue with an 
application server team a good while back and needed such a patch to 
debug the issue correctly. I added -Djava.security.debug=certpath to a 
testcase which tests this functionality. Sample output :


certpath: X509CertSelector.match: subject key IDs don't match
certpath: 509CertSelector.match: subjectKeyID: [4, 20, -12, -2, 115, 79, 
-15, 106, 114, -58, 102, 43, 32, 26, 120, -76, -33, 50, -45, -56, -16, -38]
certpath: 509CertSelector.match: certSubjectKeyID: [4, 20, -111, 93, 
-48, -86, -39, 59, -128, -118, 45, -10, 126, -76, -115, 126, -99, -106, 
-116, 107, 124, -63]


regards,
Sean.

diff --git 
a/src/java.base/share/classes/java/security/cert/X509CertSelector.java 
b/src/java.base/share/classes/java/security/cert/X509CertSelector.java

--- a/src/java.base/share/classes/java/security/cert/X509CertSelector.java
+++ b/src/java.base/share/classes/java/security/cert/X509CertSelector.java
@@ -2117,6 +2117,10 @@
 if (debug != null) {
 debug.println("X509CertSelector.match: "
 + "subject key IDs don't match");
+    debug.println("509CertSelector.match:" +
+    " subjectKeyID: " + Arrays.toString(subjectKeyID));
+    debug.println("509CertSelector.match:" +
+    " certSubjectKeyID: " + 
Arrays.toString(certSubjectKeyID));

 }
 return false;
 }
diff --git 
a/test/jdk/java/security/cert/CertPathBuilder/selfIssued/KeyUsageMatters.java 
b/test/jdk/java/security/cert/CertPathBuilder/selfIssued/KeyUsageMatters.java
--- 
a/test/jdk/java/security/cert/CertPathBuilder/selfIssued/KeyUsageMatters.java
+++ 
b/test/jdk/java/security/cert/CertPathBuilder/selfIssued/KeyUsageMatters.java

@@ -29,13 +29,13 @@

 /**
  * @test
- * @bug 6852744
+ * @bug 6852744 8133489
  * @summary PIT b61: PKI test suite fails because self signed certificates
  *  are being rejected
  * @modules java.base/sun.security.util
- * @run main/othervm KeyUsageMatters subca
- * @run main/othervm KeyUsageMatters subci
- * @run main/othervm KeyUsageMatters alice
+ * @run main/othervm -Djava.security.debug=certpath KeyUsageMatters subca
+ * @run main/othervm -Djava.security.debug=certpath KeyUsageMatters subci
+ * @run main/othervm -Djava.security.debug=certpath KeyUsageMatters alice
  * @author Xuelei Fan
  */

RFR (XS) : 8042904: apple.security.KeychainStore.getSalt() calling generateSeed()

[Seán Coffey]

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

Looking to correct this issue. SecureRandom.nextBytes looks like the method
which should be in use rather than generateSeed(int)

--- a/src/java.base/macosx/classes/apple/security/KeychainStore.java
+++ b/src/java.base/macosx/classes/apple/security/KeychainStore.java
@@ -1050,7 +1050,7 @@
 if (random == null) {
 random = new SecureRandom();
 }
-    salt = random.generateSeed(SALT_LEN);
+    random.nextBytes(salt);
 return salt;
 }

regards,
Sean.

Re: RFR: 8191808: Configurable read timeout for CRLs

[Seán Coffey]

Thanks. Looks good.

regards,
Sean.

On 09/05/2019 17:38, Sean Mullan wrote:

On 5/9/19 11:29 AM, Seán Coffey wrote:

Nice feature to have Sean. Thanks.

What do you think about adding a debug print if such a value is set ? 
(perhaps in initializeTimeout method)


Good suggestion. I've added the following to the intializeTimeout method:

+    private static int initializeTimeout(String prop, int def) {
+    Integer tmp = GetIntegerAction.privilegedGetProperty(prop);
 if (tmp == null || tmp < 0) {
-    return DEFAULT_CRL_CONNECT_TIMEOUT;
+    return def;
+    }
+    if (debug != null) {
+    debug.println(prop + " set to " + tmp + " seconds");
 }

--Sean



regards,
Sean.

On 07/05/2019 17:16, Sean Mullan wrote:

On 5/7/19 11:28 AM, Xuelei Fan wrote:
What do you think if com.sun.security.crl.readtimeout is not set, 
CRL_READ_TIMEOUT is set as the same value as CRL_CONNECT_TIMEOUT?


There may be good reasons you would want different values for these 
two properties, so if com.sun.security.crl.readtimeout is not set, I 
think it is better to have a fixed default value and not have it be 
the same as CRL_CONNECT_TIMEOUT.




Otherwise, looks fine to me.


Thanks. Could you also add your name as the CSR Reviewer?

--Sean



Xuelei

On 5/7/2019 7:28 AM, Sean Mullan wrote:
Please review this change to add a system property for configuring 
the read timeout when downloading CRLs with a default value of 15 
seconds. Currently there is no timeout so downloads of large CRLs 
can block for a long time or indefinitely. Current workaround is 
to use the sun.net.client.defaultReadTimeout system property but 
that affects all connections.


bug: https://bugs.openjdk.java.net/browse/JDK-8191808
CSR: https://bugs.openjdk.java.net/browse/JDK-8223310
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8191808/webrev.00/

Thanks,
Sean

Re: RFR: 8191808: Configurable read timeout for CRLs

[Seán Coffey]

Nice feature to have Sean. Thanks.

What do you think about adding a debug print if such a value is set ? 
(perhaps in initializeTimeout method)


regards,
Sean.

On 07/05/2019 17:16, Sean Mullan wrote:

On 5/7/19 11:28 AM, Xuelei Fan wrote:
What do you think if com.sun.security.crl.readtimeout is not set, 
CRL_READ_TIMEOUT is set as the same value as CRL_CONNECT_TIMEOUT?


There may be good reasons you would want different values for these 
two properties, so if com.sun.security.crl.readtimeout is not set, I 
think it is better to have a fixed default value and not have it be 
the same as CRL_CONNECT_TIMEOUT.




Otherwise, looks fine to me.


Thanks. Could you also add your name as the CSR Reviewer?

--Sean



Xuelei

On 5/7/2019 7:28 AM, Sean Mullan wrote:
Please review this change to add a system property for configuring 
the read timeout when downloading CRLs with a default value of 15 
seconds. Currently there is no timeout so downloads of large CRLs 
can block for a long time or indefinitely. Current workaround is to 
use the sun.net.client.defaultReadTimeout system property but that 
affects all connections.


bug: https://bugs.openjdk.java.net/browse/JDK-8191808
CSR: https://bugs.openjdk.java.net/browse/JDK-8223310
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8191808/webrev.00/

Thanks,
Sean

Re: RFR: 8222137: Remove T-Systems root CA certificate

[Seán Coffey]

On 02/05/2019 09:27, Langer, Christoph wrote:


Hi Sean,

sounds good. I’ll create an 11.0.4 bug before pushing to jkd11u-dev.

I assume you’ll take care of jdk12u?


Sure - I'm happy to push 12u. I'll add the necessary request label shortly.

regards,
Sean.


Thanks

Christoph

*From:*Seán Coffey 
*Sent:* Donnerstag, 2. Mai 2019 10:25
*To:* Langer, Christoph 
*Cc:* Rajan Halade ; Sean Mullan 
; security-dev 

*Subject:* Re: RFR: 8222137: Remove T-Systems root CA certificate

Christoph,

if possible, can you create an "11.0.4" fixVersion backport before 
pushing your changeset? That will help with JBS records. 
Alternatively, I'll just ensure there's an 11-pool or 11.0.5 record 
before I push.


regards,
Sean.

On 02/05/2019 08:48, Langer, Christoph wrote:

Thanks, Raj.

Hi Seán,

since there is an 11-pool request, I guess you should push the
change to some 11.o.-oracle repository first such that you’ll
get this item resolved by hgupdater. Please let me know how to
proceed.

Thanks

Christoph

*From:*Rajan Halade 
<mailto:rajan.hal...@oracle.com>
*Sent:* Mittwoch, 1. Mai 2019 21:08
*To:* Langer, Christoph 
<mailto:christoph.lan...@sap.com>
*Cc:* Sean Mullan 
<mailto:sean.mul...@oracle.com>; security-dev

<mailto:security-dev@openjdk.java.net>; Seán Coffey
 <mailto:sean.cof...@oracle.com>
*Subject:* Re: RFR: 8222137: Remove T-Systems root CA certificate

I have created needed backports including 12-pool, 11-pool,
8-pool, and 7-pool. These are assigned to Sean C. for now. Please
co-ordinate with him.

Thanks,
Rajan

On 5/1/19 6:19 AM, Langer, Christoph wrote:

Hi Rajan,

I’ll take this to jdk11 updates. What about jdk12 updates? I
can process both releases, if you want.

Best regards

Christoph

*From:*security-dev 
<mailto:security-dev-boun...@openjdk.java.net> *On Behalf Of
*Rajan Halade
*Sent:* Dienstag, 30. April 2019 20:39
*To:* Sean Mullan 
<mailto:sean.mul...@oracle.com>; security-dev

<mailto:security-dev@openjdk.java.net>
*Subject:* RFR: 8222137: Remove T-Systems root CA certificate

Please review this fix for removal of T-Systems Deutsche
Telekom Root CA 2 certificate.

Webrev: http://cr.openjdk.java.net/~rhalade/8222137/webrev.00/

Release note is at -
https://bugs.openjdk.java.net/browse/JDK-8223161

Thanks,
Rajan

Re: RFR: 8222137: Remove T-Systems root CA certificate

[Seán Coffey]

Christoph,

if possible, can you create an "11.0.4" fixVersion backport before 
pushing your changeset? That will help with JBS records. Alternatively, 
I'll just ensure there's an 11-pool or 11.0.5 record before I push.


regards,
Sean.

On 02/05/2019 08:48, Langer, Christoph wrote:


Thanks, Raj.

Hi Seán,

since there is an 11-pool request, I guess you should push the change 
to some 11.o.-oracle repository first such that you’ll get this 
item resolved by hgupdater. Please let me know how to proceed.


Thanks

Christoph

*From:*Rajan Halade 
*Sent:* Mittwoch, 1. Mai 2019 21:08
*To:* Langer, Christoph 
*Cc:* Sean Mullan ; security-dev 
; Seán Coffey 

*Subject:* Re: RFR: 8222137: Remove T-Systems root CA certificate

I have created needed backports including 12-pool, 11-pool, 8-pool, 
and 7-pool. These are assigned to Sean C. for now. Please co-ordinate 
with him.


Thanks,
Rajan

On 5/1/19 6:19 AM, Langer, Christoph wrote:

Hi Rajan,

I’ll take this to jdk11 updates. What about jdk12 updates? I can
process both releases, if you want.

Best regards

Christoph

*From:*security-dev 
<mailto:security-dev-boun...@openjdk.java.net> *On Behalf Of
*Rajan Halade
*Sent:* Dienstag, 30. April 2019 20:39
*To:* Sean Mullan 
<mailto:sean.mul...@oracle.com>; security-dev
 <mailto:security-dev@openjdk.java.net>
*Subject:* RFR: 8222137: Remove T-Systems root CA certificate

Please review this fix for removal of T-Systems Deutsche Telekom
Root CA 2 certificate.

Webrev: http://cr.openjdk.java.net/~rhalade/8222137/webrev.00/

Release note is at - https://bugs.openjdk.java.net/browse/JDK-8223161

Thanks,
Rajan

Re: [13] RFR JDK-8218780 "Update MUSCLE PCSC-Lite header files"

[Seán Coffey]

Looks fine to me Valerie.

regards,
Sean.

On 15/02/2019 20:04, Valerie Peng wrote:

Hi Sean (Coffey),

Can you please help reviewing this fix? It's about updating the PC/SC 
lite header files under the MUSCLE directory inside smartcardio 
module. The header files are from https://pcsclite.apdu.fr/ and minor 
adjustments are made to pcsc_md.h and pcsc.c to match the header 
update and replace the old types, i.e. LPTSTR/LPCTSTR, with new ones 
to avoid compilation fails on MacOs.


Bug: https://bugs.openjdk.java.net/browse/JDK-8218780

Webrev: http://cr.openjdk.java.net/~valeriep/8218780/webrev.00/

Mach5 run is clean.

Thanks,
Valerie

Re: RFR: 8218553: Enhance keystore load debug output

[Seán Coffey]

On 06/02/2019 16:24, Weijun Wang wrote:

Hi Séan,

Change looks fine.

I usually think there is no need to provide data in the debug output that were already 
available from public APIs (i.e. they are not something that can only be revealed from 
"internally"), but these numbers could help a supporter engineer to quickly 
find out if there is some really simple error in a customer's environment. So, good to 
have them.

And I definitely believe you know better that I what a support engineer needs.


Thanks for review. Yes - use of APIs to retrieve such data is not an 
option when someone is trying to debug a live production issue. I think 
this will help admins to confirm if keystore configuration looks correct 
in such environments.


regards,
Sean.



Thanks,
Max


On Feb 6, 2019, at 11:32 PM, Seán Coffey  wrote:

Looking to improve debug output in the keystore area.

It's useful to know how many keys/certs are found in such stores
to ensure correct set up.

https://bugs.openjdk.java.net/browse/JDK-8218553
http://cr.openjdk.java.net/~coffeys/webrev.8218553/webrev/

--
Regards,
Sean.

RFR: 8218553: Enhance keystore load debug output

[Seán Coffey]

Looking to improve debug output in the keystore area.

It's useful to know how many keys/certs are found in such stores
to ensure correct set up.

https://bugs.openjdk.java.net/browse/JDK-8218553
http://cr.openjdk.java.net/~coffeys/webrev.8218553/webrev/

--
Regards,
Sean.

Re: [12] RFR: 8216280: Allow later Symantec Policy distrust date for two Apple SubCAs

[Seán Coffey]

Looks good to me Sean.

regards,
Sean.

On 16/01/2019 19:53, Sean Mullan wrote:
Please review this change to allow a later Symantec Policy distrust 
date for two Apple subordinate CAs.


webrev: http://cr.openjdk.java.net/~mullan/webrevs/8216280/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8216280

For some background, the JDK will stop trusting TLS Server 
certificates chaining back to Symantec roots, in line with similar 
plans announced by Google, Mozilla, Apple, and Microsoft. The list of 
affected certificates includes certificates branded as GeoTrust, 
Thawte, and VeriSign, which were managed by Symantec. Any TLS Server 
certificate issued after April 16, 2019 will be restricted. This 
change has already been implemented and is in JDK 12 (see JDK-8207258 
for more info).


Apple are actively working with DigiCert on a transition plan and have 
requested a later distrust date: December 31, 2019. This later 
distrust date would only apply to TLS Server certificates issued from 
(or chaining back to) two Apple subordinate CAs: "Apple IST CA 2 - G1" 
and "Apple IST CA 8 - G1" issued by GeoTrust root CAs. Any certificate 
issued after that date will be distrusted. This change would be in 
line with other vendors such as Mozilla that have granted similar 
exemptions to these Apple subCAs.


Thanks,
Sean

Re: RFR: 8179943 Typo in javax.net.ssl.SSLSession.removeValue(String) method documentation

[Seán Coffey]

Looks fine to me. I can sponsor this patch if required.

regards,
Sean.

On 03/01/2019 16:51, Roger Calnan wrote:
please review straightforward fix in the javadoc 
for javax.net.ssl.SSLSession.removeValue


Thanks,

Roger

*https://bugs.openjdk.java.net/browse/JDK-8179943 Typo in 
javax.net.ssl.SSLSession.removeValue(String) method documentation*

*
*
diff -r 3d0f6ef91216 
src/java.base/share/classes/javax/net/ssl/SSLSession.java
--- a/src/java.base/share/classes/javax/net/ssl/SSLSession.javaWed Jan 
02 13:37:55 2019 -0500
+++ b/src/java.base/share/classes/javax/net/ssl/SSLSession.javaWed Jan 
02 13:19:38 2019 -0800

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1997, 2019, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -187,7 +187,7 @@
      * Removes the object bound to the given name in the session's
      * application layer data.  Does nothing if there is no object
      * bound to the given name.  If the bound existing object
-     * implements the {@code SessionBindingListener} interface,
+     * implements the {@code SSLSessionBindingListener} interface,
      * it is notified appropriately.
      * 
      * For security reasons, the same named values may not be

Re: RFR: 8214532,Update RFC 2459 references in javadoc to RFC 5280

[Seán Coffey]

Thanks Sean. I've made those edits and pushed to JDK 12 repo.

Regards,
Sean.

On 19/12/18 14:21, Sean Mullan wrote:

Just a couple of minor comments:

* DNSName

184  * draft-ietf-pkix-new-part1-00.txt:  DNSName restrictions are 
expressed as foo.bar.com.


Change "draft-ietf-pkix-new-part1-00.txt" to RFC 5280.

* X509CertImpl

67  * * can be referenced in RFC 5280.

remove extra '*'.

--Sean

On 12/19/18 6:47 AM, Seán Coffey wrote:
Some doc edits to update RFC 2459 to 5280. I've updated the relevant 
section numbers where necessary.


No edits to Java SE specification docs. I've taken the opportunity to 
put a space in some "RFC" references also as it seems to be 
preferred approach. I've removed a paragraph in 
src/java.base/share/classes/sun/security/x509/X509CertImpl.java also 
which seemed obsolete.


I'd like to push this doc edit to JDK 12.

http://cr.openjdk.java.net/~coffeys/webrev.8214532/webrev/

RFR: 8214532,Update RFC 2459 references in javadoc to RFC 5280

[Seán Coffey]

Some doc edits to update RFC 2459 to 5280. I've updated the relevant 
section numbers where necessary.


No edits to Java SE specification docs. I've taken the opportunity to 
put a space in some "RFC" references also as it seems to be 
preferred approach. I've removed a paragraph in 
src/java.base/share/classes/sun/security/x509/X509CertImpl.java also 
which seemed obsolete.


I'd like to push this doc edit to JDK 12.

http://cr.openjdk.java.net/~coffeys/webrev.8214532/webrev/

--
Regards,
Sean.

Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

[Seán Coffey]

whoops:

latest webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v4/webrev/


Regards,
Sean.

On 03/12/18 17:10, Seán Coffey wrote:


The CSR related to this change has been approved.

I made further edits to update the DNSName comment code to reference 
RFC 5280 rather than the obsoleted RFC 2459. I also updated the test 
case with a few extra tests per suggestion from Chris and others. 
Moved the dataprovider into a good and bad data set also.


A follow on bug has been logged to update all references of RFC 2459 
to RFC 5280 (JDK-8214532)


Regards,
Sean.
On 27/11/18 14:27, Seán Coffey wrote:
Thanks for the feedback Max. I'll ping Joe Darcy and check if a CSR 
is required for this type of change. I'll revert back to this list 
once I have an answer.


keytool works well with the new change. I've modified the test as per 
your suggestion :


--- a/test/jdk/sun/security/tools/keytool/KeyToolTest.java
+++ b/test/jdk/sun/security/tools/keytool/KeyToolTest.java
@@ -1436,6 +1436,7 @@
 testOK("", pre+"san3 -ext san=dns:me.org");
 testOK("", pre+"san4 -ext san=ip:192.168.0.1");
 testOK("", pre+"san5 -ext san=oid:1.2.3.4");
+testOK("", pre+"san6 -ext san=dns:1abc.com"); //begin with 
digit
 testOK("", pre+"san235 -ext 
san=uri:http://me.org,dns:me.org,oid:1.2.3.4;);


Regards,
Sean.

On 27/11/18 01:29, Weijun Wang wrote:


On Nov 26, 2018, at 11:15 AM, Weijun Wang  
wrote:




On Nov 24, 2018, at 12:45 AM, Seán Coffey  
wrote:


Thanks for your review Max. I've made the suggested edits.
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v3/webrev/

The change looks fine. Thanks.

I've also submitted a CSR for this change just to cover the 
behavioural aspect of the edit. Will push if/once that's approved 
by CSR team.
The CSR focuses on keytool but this is actually a library change. 
There is no change with "add/remove/modify command line option" at 
all.


I would suggest talking about the DNSName class instead of keytool. 
I understand it's internal but we can describe this change as a 
non-minimal change on behavior so it's still worth a CSR. Or, you 
can consult Joe what the best way is. Maybe he can spare you from 
filing a CSR at all.


BTW, you did try out keytool after this code change and "-ext 
dns=1abc.com" is working now, right?
You can add an extra line after line 1437 of 
test/jdk/sun/security/tools/keytool/KeyToolTest.java.



Thanks
Max


Regards,
Sean.

On 22/11/18 14:49, Weijun Wang wrote:

* DNSName.java:

  91 if ((endIndex - startIndex) < 1)

No need for inner parentheses.

And, is there really a need to define alphaDigitsAndHyphen? How 
about just (== '-' || inside alphaDigits)?


* DNSNameTest.java:

Space cannot appear anywhere, please add a test case like "a c.com".

BTW, I assume you want to reuse this test for other sub-tasks of 
JDK-8054380? I see the @summary is more general.


No other comments.

Thanks
Max

On Nov 22, 2018, at 12:51 AM, Seán Coffey 
 wrote:


p.s I've updated the testcase to test the IOException message 
for presence of "DNSName". Webrev updated in place.


Regards,
Sean.

On 21/11/18 15:42, Seán Coffey wrote:

Thanks for the comments Bernd. Comments inline..

On 16/11/18 21:27, Bernd Eckenfels wrote:

Hello Sean,

I was only looking at the inspected DNSName class, it still 
has some variations (but start now with DNSNames which is good 
already):


  76 throw new IOException("DNSName must not be null or empty");
  78 throw new IOException("DNSNames or NameConstraints with 
blank components are not permitted");
  80 throw new IOException("DNSNames or NameConstraints may 
not begin or end with a .");
  92 throw new IOException("DNSName SubjectAltNames with empty 
components are not permitted");
96 throw new IOException("DNSName components must begin with a 
letter or digit");
101 throw new IOException("DNSName components must consist of 
letters, digits, and hyphens");


I did not check if those exceptions are catched and rethrown 
with something like „while validating the SubjectAltName 
Extension  of certificate ...“, in my experience 
it helps if you do not have to find and review the actual 
certificates (in this case it might not be a problem if SAN is 
only in the end entity). You can probably remove „or 
NameConstraints“ and „SubjectAltNames“ from lines 78-92 (this 
is good if DNsNa

Ok - I've cleaned up the exception messages on line 78, 80, 92.
webrev updated in place : 
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/



me applies to SAN or NameConstrained context and the 
validation logic does not know — so it’s not only more unified 
but also less missleading)


BTW: probably not inthe scope of this fix but a subtype for 
validation errors which have getters for context/loca

Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

[Seán Coffey]

The CSR related to this change has been approved.

I made further edits to update the DNSName comment code to reference RFC 
5280 rather than the obsoleted RFC 2459. I also updated the test case 
with a few extra tests per suggestion from Chris and others. Moved the 
dataprovider into a good and bad data set also.


A follow on bug has been logged to update all references of RFC 2459 to 
RFC 5280 (JDK-8214532)


Regards,
Sean.

On 27/11/18 14:27, Seán Coffey wrote:
Thanks for the feedback Max. I'll ping Joe Darcy and check if a CSR is 
required for this type of change. I'll revert back to this list once I 
have an answer.


keytool works well with the new change. I've modified the test as per 
your suggestion :


--- a/test/jdk/sun/security/tools/keytool/KeyToolTest.java
+++ b/test/jdk/sun/security/tools/keytool/KeyToolTest.java
@@ -1436,6 +1436,7 @@
 testOK("", pre+"san3 -ext san=dns:me.org");
 testOK("", pre+"san4 -ext san=ip:192.168.0.1");
 testOK("", pre+"san5 -ext san=oid:1.2.3.4");
+testOK("", pre+"san6 -ext san=dns:1abc.com"); //begin with digit
 testOK("", pre+"san235 -ext 
san=uri:http://me.org,dns:me.org,oid:1.2.3.4;);


Regards,
Sean.

On 27/11/18 01:29, Weijun Wang wrote:


On Nov 26, 2018, at 11:15 AM, Weijun Wang  
wrote:




On Nov 24, 2018, at 12:45 AM, Seán Coffey  
wrote:


Thanks for your review Max. I've made the suggested edits.
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v3/webrev/

The change looks fine. Thanks.

I've also submitted a CSR for this change just to cover the 
behavioural aspect of the edit. Will push if/once that's approved 
by CSR team.
The CSR focuses on keytool but this is actually a library change. 
There is no change with "add/remove/modify command line option" at all.


I would suggest talking about the DNSName class instead of keytool. 
I understand it's internal but we can describe this change as a 
non-minimal change on behavior so it's still worth a CSR. Or, you 
can consult Joe what the best way is. Maybe he can spare you from 
filing a CSR at all.


BTW, you did try out keytool after this code change and "-ext 
dns=1abc.com" is working now, right?
You can add an extra line after line 1437 of 
test/jdk/sun/security/tools/keytool/KeyToolTest.java.



Thanks
Max


Regards,
Sean.

On 22/11/18 14:49, Weijun Wang wrote:

* DNSName.java:

  91 if ((endIndex - startIndex) < 1)

No need for inner parentheses.

And, is there really a need to define alphaDigitsAndHyphen? How 
about just (== '-' || inside alphaDigits)?


* DNSNameTest.java:

Space cannot appear anywhere, please add a test case like "a c.com".

BTW, I assume you want to reuse this test for other sub-tasks of 
JDK-8054380? I see the @summary is more general.


No other comments.

Thanks
Max

On Nov 22, 2018, at 12:51 AM, Seán Coffey 
 wrote:


p.s I've updated the testcase to test the IOException message for 
presence of "DNSName". Webrev updated in place.


Regards,
Sean.

On 21/11/18 15:42, Seán Coffey wrote:

Thanks for the comments Bernd. Comments inline..

On 16/11/18 21:27, Bernd Eckenfels wrote:

Hello Sean,

I was only looking at the inspected DNSName class, it still has 
some variations (but start now with DNSNames which is good 
already):


  76 throw new IOException("DNSName must not be null or empty");
  78 throw new IOException("DNSNames or NameConstraints with 
blank components are not permitted");
  80 throw new IOException("DNSNames or NameConstraints may not 
begin or end with a .");
  92 throw new IOException("DNSName SubjectAltNames with empty 
components are not permitted");
96 throw new IOException("DNSName components must begin with a 
letter or digit");
101 throw new IOException("DNSName components must consist of 
letters, digits, and hyphens");


I did not check if those exceptions are catched and rethrown 
with something like „while validating the SubjectAltName 
Extension  of certificate ...“, in my experience 
it helps if you do not have to find and review the actual 
certificates (in this case it might not be a problem if SAN is 
only in the end entity). You can probably remove „or 
NameConstraints“ and „SubjectAltNames“ from lines 78-92 (this 
is good if DNsNa

Ok - I've cleaned up the exception messages on line 78, 80, 92.
webrev updated in place : 
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/



me applies to SAN or NameConstrained context and the validation 
logic does not know — so it’s not only more unified but also 
less missleading)


BTW: probably not inthe scope of this fix but a subtype for 
validation errors which have getters for context/location and 
maybe error code and getValue() would allow callers to print 
nicer validation reports without relying on the message or 
Stacktraces.
That's 

Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

[Seán Coffey]

Thanks for the feedback Max. I'll ping Joe Darcy and check if a CSR is 
required for this type of change. I'll revert back to this list once I 
have an answer.


keytool works well with the new change. I've modified the test as per 
your suggestion :


--- a/test/jdk/sun/security/tools/keytool/KeyToolTest.java
+++ b/test/jdk/sun/security/tools/keytool/KeyToolTest.java
@@ -1436,6 +1436,7 @@
 testOK("", pre+"san3 -ext san=dns:me.org");
 testOK("", pre+"san4 -ext san=ip:192.168.0.1");
 testOK("", pre+"san5 -ext san=oid:1.2.3.4");
+testOK("", pre+"san6 -ext san=dns:1abc.com"); //begin with digit
 testOK("", pre+"san235 -ext 
san=uri:http://me.org,dns:me.org,oid:1.2.3.4;);


Regards,
Sean.

On 27/11/18 01:29, Weijun Wang wrote:



On Nov 26, 2018, at 11:15 AM, Weijun Wang  wrote:




On Nov 24, 2018, at 12:45 AM, Seán Coffey  wrote:

Thanks for your review Max. I've made the suggested edits.
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v3/webrev/

The change looks fine. Thanks.


I've also submitted a CSR for this change just to cover the behavioural aspect 
of the edit. Will push if/once that's approved by CSR team.

The CSR focuses on keytool but this is actually a library change. There is no change with 
"add/remove/modify command line option" at all.

I would suggest talking about the DNSName class instead of keytool. I 
understand it's internal but we can describe this change as a non-minimal 
change on behavior so it's still worth a CSR. Or, you can consult Joe what the 
best way is. Maybe he can spare you from filing a CSR at all.

BTW, you did try out keytool after this code change and "-ext dns=1abc.com" is 
working now, right?

You can add an extra line after line 1437 of 
test/jdk/sun/security/tools/keytool/KeyToolTest.java.


Thanks
Max


Regards,
Sean.

On 22/11/18 14:49, Weijun Wang wrote:

* DNSName.java:

  91 if ((endIndex - startIndex) < 1)

No need for inner parentheses.

And, is there really a need to define alphaDigitsAndHyphen? How about just (== 
'-' || inside alphaDigits)?

* DNSNameTest.java:

Space cannot appear anywhere, please add a test case like "a c.com".

BTW, I assume you want to reuse this test for other sub-tasks of JDK-8054380? I 
see the @summary is more general.

No other comments.

Thanks
Max


On Nov 22, 2018, at 12:51 AM, Seán Coffey  wrote:

p.s I've updated the testcase to test the IOException message for presence of 
"DNSName". Webrev updated in place.

Regards,
Sean.

On 21/11/18 15:42, Seán Coffey wrote:

Thanks for the comments Bernd. Comments inline..

On 16/11/18 21:27, Bernd Eckenfels wrote:

Hello Sean,

I was only looking at the inspected DNSName class, it still has some variations 
(but start now with DNSNames which is good already):

  76 throw new IOException("DNSName must not be null or empty");
  78 throw new IOException("DNSNames or NameConstraints with blank components are 
not permitted");
  80 throw new IOException("DNSNames or NameConstraints may not begin or end with a 
.");
  92 throw new IOException("DNSName SubjectAltNames with empty components are not 
permitted");
96 throw new IOException("DNSName components must begin with a letter or 
digit");
101 throw new IOException("DNSName components must consist of letters, digits, and 
hyphens");

I did not check if those exceptions are catched and rethrown with something like „while 
validating the SubjectAltName Extension  of certificate ...“, in 
my experience it helps if you do not have to find and review the actual certificates (in 
this case it might not be a problem if SAN is only in the end entity). You can probably 
remove „or NameConstraints“ and „SubjectAltNames“ from lines 78-92 (this is good if DNsNa

Ok - I've cleaned up the exception messages on line 78, 80, 92.
webrev updated in place : 
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/



me applies to SAN or NameConstrained context and the validation logic does not 
know — so it’s not only more unified but also less missleading)

BTW: probably not inthe scope of this fix but a subtype for validation errors 
which have getters for context/location and maybe error code and getValue() 
would allow callers to print nicer validation reports without relying on the 
message or Stacktraces.

That's a nice idea and one that should be followed up in separate enhancement. 
We could lean on the new `jdk.includeInExceptions` Security property which 
other component areas have started to use lately.

e.g. https://bugs.openjdk.java.net/browse/JDK-8207768

regards,
Sean.

Gruss
Bernd
--
http://bernd.eckenfels.net
Von: Seán Coffey 
Gesendet: Freitag, November 16, 2018 5:15 PM
An: Bernd Eckenfels; security-dev@openjdk.java.net
Betreff: Re: RFR: 8213952: Relax DNSName restriction as per R

Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

[Seán Coffey]

Thanks for your review Max. I've made the suggested edits.
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v3/webrev/

I've also submitted a CSR for this change just to cover the behavioural 
aspect of the edit. Will push if/once that's approved by CSR team.


Regards,
Sean.

On 22/11/18 14:49, Weijun Wang wrote:

* DNSName.java:

   91 if ((endIndex - startIndex) < 1)

No need for inner parentheses.

And, is there really a need to define alphaDigitsAndHyphen? How about just (== 
'-' || inside alphaDigits)?

* DNSNameTest.java:

Space cannot appear anywhere, please add a test case like "a c.com".

BTW, I assume you want to reuse this test for other sub-tasks of JDK-8054380? I 
see the @summary is more general.

No other comments.

Thanks
Max


On Nov 22, 2018, at 12:51 AM, Seán Coffey  wrote:

p.s I've updated the testcase to test the IOException message for presence of 
"DNSName". Webrev updated in place.

Regards,
Sean.

On 21/11/18 15:42, Seán Coffey wrote:

Thanks for the comments Bernd. Comments inline..

On 16/11/18 21:27, Bernd Eckenfels wrote:

Hello Sean,

I was only looking at the inspected DNSName class, it still has some variations 
(but start now with DNSNames which is good already):

   76 throw new IOException("DNSName must not be null or empty");
   78 throw new IOException("DNSNames or NameConstraints with blank components are 
not permitted");
   80 throw new IOException("DNSNames or NameConstraints may not begin or end with a 
.");
   92 throw new IOException("DNSName SubjectAltNames with empty components are not 
permitted");
  96 throw new IOException("DNSName components must begin with a letter or 
digit");
101 throw new IOException("DNSName components must consist of letters, digits, and 
hyphens");

I did not check if those exceptions are catched and rethrown with something like „while 
validating the SubjectAltName Extension  of certificate ...“, in 
my experience it helps if you do not have to find and review the actual certificates (in 
this case it might not be a problem if SAN is only in the end entity). You can probably 
remove „or NameConstraints“ and „SubjectAltNames“ from lines 78-92 (this is good if DNsNa

Ok - I've cleaned up the exception messages on line 78, 80, 92.
webrev updated in place : 
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/



me applies to SAN or NameConstrained context and the validation logic does not 
know — so it’s not only more unified but also less missleading)

BTW: probably not inthe scope of this fix but a subtype for validation errors 
which have getters for context/location and maybe error code and getValue() 
would allow callers to print nicer validation reports without relying on the 
message or Stacktraces.

That's a nice idea and one that should be followed up in separate enhancement. 
We could lean on the new `jdk.includeInExceptions` Security property which 
other component areas have started to use lately.

e.g. https://bugs.openjdk.java.net/browse/JDK-8207768

regards,
Sean.

Gruss
Bernd
--
http://bernd.eckenfels.net
  
Von: Seán Coffey 

Gesendet: Freitag, November 16, 2018 5:15 PM
An: Bernd Eckenfels; security-dev@openjdk.java.net
Betreff: Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123
  
Thanks for the comments Bernd. comments inline..


On 16/11/18 12:40, Bernd Eckenfels wrote:

You could also add (a..b, false) and (.a, false), (a., false) to the testcases.

good point. test updated.

I noticed that there are different types of Exception messages (DNS name, 
DNSName, DNS Name or name constrained, DNS name and SAN), would be good if all 
of them have the same keyword?

I cleaned up some other references to DNSName in the sun.security.x509 package. 
I'm not sure what classes you were referencing the above examples from.

new webrev : http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/

regards,
Sean.

Gruss
Bernd
--
http://bernd.eckenfels.net
  
Von: security-dev  im Auftrag von Seán Coffey 

Gesendet: Freitag, November 16, 2018 12:25 PM
An: OpenJDK Dev list
Betreff: RFR: 8213952: Relax DNSName restriction as per RFC 1123
  
Looking to make an adjustment to DNSName constructor to help comply with

RFC 1123

https://bugs.openjdk.java.net/browse/JDK-8213952
http://cr.openjdk.java.net/~coffeys/webrev.8213952/webrev/

regards,
Sean.

Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

[Seán Coffey]

p.s I've updated the testcase to test the IOException message for 
presence of "DNSName". Webrev updated in place.


Regards,
Sean.

On 21/11/18 15:42, Seán Coffey wrote:


Thanks for the comments Bernd. Comments inline..

On 16/11/18 21:27, Bernd Eckenfels wrote:

Hello Sean,

I was only looking at the inspected DNSName class, it still has some 
variations (but start now with DNSNames which is good already):


  76 throw new IOException("DNSName must not be null or empty");
  78 throw new IOException("*DNSNames or NameConstraints* with blank 
components are not permitted");
  80 throw new IOException("*DNSNames or NameConstraints* may not 
begin or end with a .");
  92 throw new IOException("*DNSName SubjectAltNames* with empty 
components are not permitted");
 96 throw new IOException("DNSName components must begin with a 
letter or digit");
101 throw new IOException("DNSName components must consist of 
letters, digits, and hyphens");


I did not check if those exceptions are catched and rethrown with 
something like „while validating the SubjectAltName Extension  
of certificate ...“, in my experience it helps if you do not 
have to find and review the actual certificates (in this case it 
might not be a problem if SAN is only in the end entity). You can 
probably remove „or NameConstraints“ and „SubjectAltNames“ from lines 
78-92 (this is good if DNsNa

Ok - I've cleaned up the exception messages on line 78, 80, 92.
webrev updated in place : 
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/



me applies to SAN or NameConstrained context and the validation logic 
does not know — so it’s not only more unified but also less missleading)


BTW: probably not inthe scope of this fix but a subtype for 
validation errors which have getters for context/location and maybe 
error code and getValue() would allow callers to print nicer 
validation reports without relying on the message or Stacktraces.


That's a nice idea and one that should be followed up in separate 
enhancement. We could lean on the new `jdk.includeInExceptions` 
Security property which other component areas have started to use lately.


e.g. https://bugs.openjdk.java.net/browse/JDK-8207768

regards,
Sean.


Gruss
Bernd
--
http://bernd.eckenfels.net
--------
*Von:* Seán Coffey 
*Gesendet:* Freitag, November 16, 2018 5:15 PM
*An:* Bernd Eckenfels; security-dev@openjdk.java.net
*Betreff:* Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

Thanks for the comments Bernd. comments inline..

On 16/11/18 12:40, Bernd Eckenfels wrote:
You could also add (a..b, false) and (.a, false), (a., false) to the 
testcases.

good point. test updated.


I noticed that there are different types of Exception messages (DNS 
name, DNSName, DNS Name or name constrained, DNS name and SAN), 
would be good if all of them have the same keyword?
I cleaned up some other references to DNSName in the 
sun.security.x509 package. I'm not sure what classes you were 
referencing the above examples from.


new webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/


regards,
Sean.


Gruss
Bernd
--
http://bernd.eckenfels.net
----
*Von:* security-dev  im 
Auftrag von Seán Coffey 

*Gesendet:* Freitag, November 16, 2018 12:25 PM
*An:* OpenJDK Dev list
*Betreff:* RFR: 8213952: Relax DNSName restriction as per RFC 1123
Looking to make an adjustment to DNSName constructor to help comply 
with

RFC 1123

https://bugs.openjdk.java.net/browse/JDK-8213952
http://cr.openjdk.java.net/~coffeys/webrev.8213952/webrev/

regards,
Sean.

Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

[Seán Coffey]

Thanks for the comments Bernd. Comments inline..

On 16/11/18 21:27, Bernd Eckenfels wrote:

Hello Sean,

I was only looking at the inspected DNSName class, it still has some 
variations (but start now with DNSNames which is good already):


  76 throw new IOException("DNSName must not be null or empty");
  78 throw new IOException("*DNSNames or NameConstraints* with blank 
components are not permitted");
  80 throw new IOException("*DNSNames or NameConstraints* may not 
begin or end with a .");
  92 throw new IOException("*DNSName SubjectAltNames* with empty 
components are not permitted");
 96 throw new IOException("DNSName components must begin with a letter 
or digit");
101 throw new IOException("DNSName components must consist of letters, 
digits, and hyphens");


I did not check if those exceptions are catched and rethrown with 
something like „while validating the SubjectAltName Extension  of 
certificate ...“, in my experience it helps if you do not 
have to find and review the actual certificates (in this case it might 
not be a problem if SAN is only in the end entity). You can probably 
remove „or NameConstraints“ and „SubjectAltNames“ from lines 78-92 
(this is good if DNsNa

Ok - I've cleaned up the exception messages on line 78, 80, 92.
webrev updated in place : 
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/



me applies to SAN or NameConstrained context and the validation logic 
does not know — so it’s not only more unified but also less missleading)


BTW: probably not inthe scope of this fix but a subtype for validation 
errors which have getters for context/location and maybe error code 
and getValue() would allow callers to print nicer validation reports 
without relying on the message or Stacktraces.


That's a nice idea and one that should be followed up in separate 
enhancement. We could lean on the new `jdk.includeInExceptions` Security 
property which other component areas have started to use lately.


e.g. https://bugs.openjdk.java.net/browse/JDK-8207768

regards,
Sean.


Gruss
Bernd
--
http://bernd.eckenfels.net
----
*Von:* Seán Coffey 
*Gesendet:* Freitag, November 16, 2018 5:15 PM
*An:* Bernd Eckenfels; security-dev@openjdk.java.net
*Betreff:* Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

Thanks for the comments Bernd. comments inline..

On 16/11/18 12:40, Bernd Eckenfels wrote:
You could also add (a..b, false) and (.a, false), (a., false) to the 
testcases.

good point. test updated.


I noticed that there are different types of Exception messages (DNS 
name, DNSName, DNS Name or name constrained, DNS name and SAN), would 
be good if all of them have the same keyword?
I cleaned up some other references to DNSName in the sun.security.x509 
package. I'm not sure what classes you were referencing the above 
examples from.


new webrev : http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/

regards,
Sean.


Gruss
Bernd
--
http://bernd.eckenfels.net
--------
*Von:* security-dev  im 
Auftrag von Seán Coffey 

*Gesendet:* Freitag, November 16, 2018 12:25 PM
*An:* OpenJDK Dev list
*Betreff:* RFR: 8213952: Relax DNSName restriction as per RFC 1123
Looking to make an adjustment to DNSName constructor to help comply with
RFC 1123

https://bugs.openjdk.java.net/browse/JDK-8213952
http://cr.openjdk.java.net/~coffeys/webrev.8213952/webrev/

regards,
Sean.

Re: RFR: 8210838: Override javax.crypto.Cipher.toString()

[Seán Coffey]

Thanks Sean. StringBuilder use added :

http://cr.openjdk.java.net/~coffeys/webrev.8210838.v4/webrev/

Regards,
Sean.

On 16/11/18 17:33, Sean Mullan wrote:
Looks ok. If there are no disadvantages to using a StringBuilder, I 
would probably do that, since you are creating 4-5 separate Strings in 
the toString method.


--Sean

On 11/16/18 11:35 AM, Seán Coffey wrote:


On 16/11/18 16:16, Sean Mullan wrote:

On 11/16/18 9:04 AM, Seán Coffey wrote:

That's a good example and point Max. How does this revision look ?

http://cr.openjdk.java.net/~coffeys/webrev.8210838.v2/webrev/


2832  * This implementation returns a String containing the 
transformation
2833  * used by this Cipher, the Cipher mode and the Cipher 
Provider.


I would suggest rewording this as: "This implementation returns a 
String containing the transformation, mode, and provider of this 
Cipher."

Good suggestion. Changed.


2840 String m = "not initialized";
2841 if (initialized)
2842 m = getOpmodeString(opmode);

Or:

String m = initialized ? getOpmodeString(opmode) : "not initialized";
I've moved the switch expression into the toString() call now as per 
suggestion from Max. I've just
seen that we should never have an unexpected opmode given the 
checkOpmode(int) method check.
I've let an unexpected opmode register with "error" in the switch 
statement. // should never happen.


webrev updated in place:
  http://cr.openjdk.java.net/~coffeys/webrev.8210838.v3/webrev/

regards,
Sean.



Also, it might be worthwhile to use a StringBuilder if you think 
this method may be called often.


--Sean



Regards,
Sean.

On 16/11/18 03:35, Weijun Wang wrote:

Signature's toString looks like

public String toString() {
 String initState = "";
 switch (state) {
 case UNINITIALIZED:
 initState = "";
 break;
 case VERIFY:
 initState = "";
 break;
 case SIGN:
 initState = "";
 break;
 }
 return "Signature object: " + getAlgorithm() + initState;
}

Maybe you can add some similar info.

In fact, it looks like you can extract the debug output at the end 
of each init() methods into a new toString() method.


Thanks
Max

On Nov 16, 2018, at 12:35 AM, Seán Coffey 
 wrote:


A simple enhancement to override toString() for 
javax.crypto.Cipher class


https://bugs.openjdk.java.net/browse/JDK-8210838
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8210838/webrev/

regards,
Sean.

Re: RFR: 8210838: Override javax.crypto.Cipher.toString()

[Seán Coffey]

On 16/11/18 16:16, Sean Mullan wrote:

On 11/16/18 9:04 AM, Seán Coffey wrote:

That's a good example and point Max. How does this revision look ?

http://cr.openjdk.java.net/~coffeys/webrev.8210838.v2/webrev/


2832  * This implementation returns a String containing the 
transformation

2833  * used by this Cipher, the Cipher mode and the Cipher Provider.

I would suggest rewording this as: "This implementation returns a 
String containing the transformation, mode, and provider of this Cipher."

Good suggestion. Changed.


2840 String m = "not initialized";
2841 if (initialized)
2842 m = getOpmodeString(opmode);

Or:

String m = initialized ? getOpmodeString(opmode) : "not initialized";
I've moved the switch expression into the toString() call now as per 
suggestion from Max. I've just
seen that we should never have an unexpected opmode given the 
checkOpmode(int) method check.
I've let an unexpected opmode register with "error" in the switch 
statement. // should never happen.


webrev updated in place:
 http://cr.openjdk.java.net/~coffeys/webrev.8210838.v3/webrev/

regards,
Sean.



Also, it might be worthwhile to use a StringBuilder if you think this 
method may be called often.


--Sean



Regards,
Sean.

On 16/11/18 03:35, Weijun Wang wrote:

Signature's toString looks like

public String toString() {
 String initState = "";
 switch (state) {
 case UNINITIALIZED:
 initState = "";
 break;
 case VERIFY:
 initState = "";
 break;
 case SIGN:
 initState = "";
 break;
 }
 return "Signature object: " + getAlgorithm() + initState;
}

Maybe you can add some similar info.

In fact, it looks like you can extract the debug output at the end 
of each init() methods into a new toString() method.


Thanks
Max

On Nov 16, 2018, at 12:35 AM, Seán Coffey  
wrote:


A simple enhancement to override toString() for javax.crypto.Cipher 
class


https://bugs.openjdk.java.net/browse/JDK-8210838
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8210838/webrev/

regards,
Sean.

Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

[Seán Coffey]

Thanks for the comments Bernd. comments inline..

On 16/11/18 12:40, Bernd Eckenfels wrote:
You could also add (a..b, false) and (.a, false), (a., false) to the 
testcases.

good point. test updated.


I noticed that there are different types of Exception messages (DNS 
name, DNSName, DNS Name or name constrained, DNS name and SAN), would 
be good if all of them have the same keyword?
I cleaned up some other references to DNSName in the sun.security.x509 
package. I'm not sure what classes you were referencing the above 
examples from.


new webrev : http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/

regards,
Sean.


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

*Von:* security-dev  im Auftrag 
von Seán Coffey 

*Gesendet:* Freitag, November 16, 2018 12:25 PM
*An:* OpenJDK Dev list
*Betreff:* RFR: 8213952: Relax DNSName restriction as per RFC 1123
Looking to make an adjustment to DNSName constructor to help comply with
RFC 1123

https://bugs.openjdk.java.net/browse/JDK-8213952
http://cr.openjdk.java.net/~coffeys/webrev.8213952/webrev/

regards,
Sean.

Re: RFR: 8210838: Override javax.crypto.Cipher.toString()

[Seán Coffey]

webrev updated!

http://cr.openjdk.java.net/~coffeys/webrev.8210838.v3/webrev/

Regards,
Sean.

On 16/11/18 14:19, Weijun Wang wrote:

Do you want to update the init() methods to make use of this new toString() 
output? This avoids duplicated code and we can even inline the 
getOpmodeString() method.

--Max


On Nov 16, 2018, at 10:04 PM, Seán Coffey  wrote:

That's a good example and point Max. How does this revision look ?

http://cr.openjdk.java.net/~coffeys/webrev.8210838.v2/webrev/

Regards,
Sean.

On 16/11/18 03:35, Weijun Wang wrote:

Signature's toString looks like

public String toString() {
 String initState = "";
 switch (state) {
 case UNINITIALIZED:
 initState = "";
 break;
 case VERIFY:
 initState = "";
 break;
 case SIGN:
 initState = "";
 break;
 }
 return "Signature object: " + getAlgorithm() + initState;
}

Maybe you can add some similar info.

In fact, it looks like you can extract the debug output at the end of each 
init() methods into a new toString() method.

Thanks
Max


On Nov 16, 2018, at 12:35 AM, Seán Coffey  wrote:

A simple enhancement to override toString() for javax.crypto.Cipher class

https://bugs.openjdk.java.net/browse/JDK-8210838
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8210838/webrev/

regards,
Sean.

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Seán Coffey]

I've renamed the 'peerCertificateId' variable and label in 
TLSHandshakeEvent to 'certificateId'. This allows the event data to be 
co-displayed in JMC views which share the same type of data 
(@CertificateId). I've uploaded an example screenshot [1]


I've also made an adjustment to the TestModuleEvents test to disregard 
the jdk.proxy1 module for test purposes.


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v10/webrev/

[1] 
http://cr.openjdk.java.net/~coffeys/jmc_screenshots/shared_selection_certificateId.png/Screenshot%20from%202018-11-16%2015%3a28%3a59.png


Regards,
Sean.

On 14/11/18 21:09, Seán Coffey wrote:

Hi Sean,

comments inline..

On 13/11/2018 18:53, Sean Mullan wrote:

Looking good, a couple of comments/questions:

* src/java.base/share/classes/java/security/Security.java

The isJdkSecurityProperty method could return false positives, for 
example there may be a non-JDK property starting with "security.". I 
was thinking it would be better to put all the JDK property names in 
a HashSet which is populated by the static initialize() method, and 
only if event logging is enabled. Then setProperty can just check if 
the property name is in this set.
after further discussion, we've decided to log all properties operated 
on via Security.setProperty(String, String). It think it makes sense. 
The contents of a JFR recording should always be treated as sensitive. 
Keep in mind also that these new JFR Events will be off by default.


* src/java.base/share/classes/sun/security/x509/X509CertImpl.java
Good points here. I've taken on board your suggestion to move this 
code logic to the sun/security/provider/X509Factory.java class as per 
your later email suggestion.



45 l.addExpected("SunJSSE Test Serivce");

Is that a typo? "Serivce"

That's a typo in the test certificate details. We should fix it up via 
another bug record.



* test/jdk/jdk/security/logging/TestX509ValidationLog.java

54: remove blank line

* test/lib/jdk/test/lib/security/SSLSocketTest.java

Why is this file included? It looks like a duplicate of 
SSLSocketTemplate.
Yes - it's almost identical to SSLSocketTemplate except that 
SSLSocketTemplate doesn't belong to a package. I had a hard time 
incorporating its use into the jtreg tests via the @lib syntax. I 
think it's best if we open another JBS issue to migrate other uses of 
SSLSocketTemplate to jdk.test.lib.security.SSLSocketTemplate


I've cleaned up the various typo/syntax issues that you've highlighted 
also.

http://cr.openjdk.java.net/~coffeys/webrev.8148188.v9/webrev/index.html

regards,
Sean.


The rest of my comments are mostly minor stuff, and nits:

* 
src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java


245 if(xve.shouldCommit() || EventHelper.isLoggingSecurity()) {

add space before "(xve"

* src/java.base/share/classes/sun/security/ssl/Finished.java

1097 }

indent

1098 if(event.shouldCommit()) {

add space before "(event"

* src/java.base/share/classes/sun/security/x509/X509CertImpl.java

update copyright

* src/java.base/share/classes/jdk/internal/event/EventHelper.java

35  * A helper class to have events logged to a JDK Event Logger

Add '.' at end of sentence.

* src/java.base/share/classes/jdk/internal/event/TLSHandshakeEvent.java

38: remove blank line

* 
src/java.base/share/classes/jdk/internal/event/X509ValidationEvent.java


30  * used in X509 cert path validation

Add '.' at end of sentence.

* src/jdk.jfr/share/classes/jdk/jfr/events/X509ValidationEvent.java

47: remove blank line

* test/jdk/jdk/security/logging/TestTLSHandshakeLog.java


--Sean


On 11/6/18 3:46 AM, Seán Coffey wrote:
With JDK-8203629 now pushed, I've re-based my patch on latest 
jdk/jdk code.


Some modifications also made based on off-thread suggestions :

src/java.base/share/classes/java/security/Security.java

* Only record JDK security properties for now (i.e. those in 
java.security conf file)
   - we can consider a new event to record non-JDK security events 
if demand comes about

   - new event renamed to *Jdk*SecurityPropertyModificationEvent

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

* Use hashCode() equality test for storing certs in List.

Tests updated to capture these changes

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java 



* Verify that self signed certs exercise the modified code paths

I've added new test functionality to test use of self signed cert.

webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/

Regards,
Sean.

On 17/10/18 11:25, Seán Coffey wrote:
I'd like to re-boot this review. I've been working with Erik off 
thread who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEv

Re: RFR: 8210838: Override javax.crypto.Cipher.toString()

[Seán Coffey]

That's a good example and point Max. How does this revision look ?

http://cr.openjdk.java.net/~coffeys/webrev.8210838.v2/webrev/

Regards,
Sean.

On 16/11/18 03:35, Weijun Wang wrote:

Signature's toString looks like

public String toString() {
 String initState = "";
 switch (state) {
 case UNINITIALIZED:
 initState = "";
 break;
 case VERIFY:
 initState = "";
 break;
 case SIGN:
 initState = "";
 break;
 }
 return "Signature object: " + getAlgorithm() + initState;
}

Maybe you can add some similar info.

In fact, it looks like you can extract the debug output at the end of each 
init() methods into a new toString() method.

Thanks
Max


On Nov 16, 2018, at 12:35 AM, Seán Coffey  wrote:

A simple enhancement to override toString() for javax.crypto.Cipher class

https://bugs.openjdk.java.net/browse/JDK-8210838
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8210838/webrev/

regards,
Sean.

RFR: 8213952: Relax DNSName restriction as per RFC 1123

[Seán Coffey]

Looking to make an adjustment to DNSName constructor to help comply with 
RFC 1123


https://bugs.openjdk.java.net/browse/JDK-8213952
http://cr.openjdk.java.net/~coffeys/webrev.8213952/webrev/

regards,
Sean.

RFR: 8210838: Override javax.crypto.Cipher.toString()

[Seán Coffey]

A simple enhancement to override toString() for javax.crypto.Cipher class

https://bugs.openjdk.java.net/browse/JDK-8210838
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8210838/webrev/

regards,
Sean.

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Seán Coffey]

Thanks for the comments Weijun.

As per other review thread, I'm now recording all properties set via 
Security.setProperty(String, String).


regards,
Sean.


On 14/11/2018 01:11, Weijun Wang wrote:

Confused. Aren't all Security properties security-related? This is not about 
normal system properties.

And the method name in the latest webrev is "isSecurityProperty" without the 
"JDK" word. I assume this means you don't care about the difference between SE properties 
and JDK properties.

--Max

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Seán Coffey]

Erik,

comments inline..
On 08/11/18 15:12, Erik Gahlin wrote:

Hi Sean,

I think we could still call the event 
"jdk.SecurityPropertyModification", but add a @Description that 
explains that events are only emitted for the JDK due to security 
concerns. If we at a later stage want to include user events, we could 
add those and remove the @Description, possibly with a setting where 
you can specify scope, or perhaps a new event all together.

sounds fine. I've made those changes.


Perhaps we could find a better name for the field 
validationEventNumber? It sounds like we have an event number, which 
is not really the case. We have a counter for the validation id.

How about 'validationCounter' ?


I noticed that you use hashCode as an id. I assume this is to simplify 
the implementation? That is probably OK, the risk of a collision is 
perhaps low, but could you make the field a long, so we in the future 
could add something more unique (Flight Recorder uses compressed 
integers, so a long uses the same amount of disk space as an int). 
Could you also rename the field and the annotation, perhaps 
certificationId could work, so we don't leak how the id was implemented.
Yes - originally, I was using the certificate serial number but that may 
not always be unique (esp. for test generated certs). I've changed the 
variable name to 'certificateId' and made it a long. Renamed the 
annotation also.


I could not find the file: 
test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java

whoops - forgot to add it to mercurial tracking. there now :
http://cr.openjdk.java.net/~coffeys/webrev.8148188.v8/webrev/

regards,
Sean.

Re: Additional debug log message in KeyTab

[Seán Coffey]

On 07/11/18 13:12, Lars Francke wrote:

Seán,

thank you!

I've already sent the OCA and am in the process of getting it filed.
According to the docs[0] the next step would be to provide a patch to 
this mailing list.


I have to admit that I'm a bit overwhelmed still (having never built 
OpenJDK myself) so it'll take a while.


Would this patch also require a test?
you might get away without a test and add the noreg-trivial to the 
eventual bug report. However, it might be no harm to examine the current 
test code to see if the debug flag is exercised. If it is, it might be 
the case of a simple edit to an existing test.


regards,
Sean.


Assuming it's just of the form
if (debug) {
  System.out.println(...);;
}

Cheers,
Lars

<https://openjdk.java.net/contribute/>

On Wed, Nov 7, 2018 at 1:44 PM Seán Coffey <mailto:sean.cof...@oracle.com>> wrote:


Looks like a reasonable minor enhancement to me.

To contribute, you'll need to sign the OCA first. More information
at :

https://openjdk.java.net/contribute/

Regards,
Sean.

On 07/11/18 11:40, Lars Francke wrote:
> Hi,
>
> I have to preface this by saying that this would be my first
> contribution to OpenJDK and I'm still learning the ways. Not
sure for
> example if this is the correct mailing list or if a more generic
JDK
> one would be appropriate.
>
> While working on Hadoop based systems I frequently need to debug
> Kerberos related issues. And while there are
sun.security.krb5.debug
> and sun.security.spnego.debug the error messages could be more
helpful
> at times.
>
> One of these instance
> is sun.security.krb5.internal.ktab.Keytab#readServiceKeys.
>
> It logs that it's looking for keys but (at least for me) it
would be
> very helpful if it also logged how many keys it found after the
for-loop.
>
> If it finds keys it also logs them but if it can't find any then
> there's no message so it's easy to miss.
>
> What do you think?
>
> Cheers,
> Lars

Re: Additional debug log message in KeyTab

[Seán Coffey]

Looks like a reasonable minor enhancement to me.

To contribute, you'll need to sign the OCA first. More information at :

https://openjdk.java.net/contribute/

Regards,
Sean.

On 07/11/18 11:40, Lars Francke wrote:

Hi,

I have to preface this by saying that this would be my first 
contribution to OpenJDK and I'm still learning the ways. Not sure for 
example if this is the correct mailing list or if a more generic JDK 
one would be appropriate.


While working on Hadoop based systems I frequently need to debug 
Kerberos related issues. And while there are sun.security.krb5.debug 
and sun.security.spnego.debug the error messages could be more helpful 
at times.


One of these instance 
is sun.security.krb5.internal.ktab.Keytab#readServiceKeys.


It logs that it's looking for keys but (at least for me) it would be 
very helpful if it also logged how many keys it found after the for-loop.


If it finds keys it also logs them but if it can't find any then 
there's no message so it's easy to miss.


What do you think?

Cheers,
Lars

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Seán Coffey]

With JDK-8203629 now pushed, I've re-based my patch on latest jdk/jdk code.

Some modifications also made based on off-thread suggestions :

src/java.base/share/classes/java/security/Security.java

* Only record JDK security properties for now (i.e. those in 
java.security conf file)
  - we can consider a new event to record non-JDK security events if 
demand comes about

  - new event renamed to *Jdk*SecurityPropertyModificationEvent

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

* Use hashCode() equality test for storing certs in List.

Tests updated to capture these changes

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java

* Verify that self signed certs exercise the modified code paths

I've added new test functionality to test use of self signed cert.

webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/

Regards,
Sean.

On 17/10/18 11:25, Seán Coffey wrote:
I'd like to re-boot this review. I've been working with Erik off 
thread who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name : 
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the 
@Relational JFR annotation.
* Keep calls for JFR event operations local to call site to optimize 
jvm compilation


webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/

This code is dependent on the JDK-8203629 enhancement being integrated.

Regards,
Sean.

On 10/07/18 13:50, Seán Coffey wrote:

Erik,

After some trial edits, I'm not so sure if moving the event & logger 
commit code into the class where it's used works too well after all.


In the code you suggested, there's an assumption that calls such as 
EventHelper.certificateChain(..) are low cost. While that might be 
the case here, it's something we can't always assume. It's called 
once for the JFR operation and once for the Logger operation. That 
pattern multiplies itself further in other Events. i.e. set up and 
assign the variables for a JFR event without knowing whether they'll 
be needed again for the Logger call. We could work around it by 
setting up some local variables and re-using them in the Logger code 
but then, we're back to where we were. The current EventHelper design 
eliminates this problem and also helps to abstract the 
recording/logging functionality away from the functionality of the 
class itself.


It also becomes a problem where we record events in multiple 
different classes (e.g. SecurityPropertyEvent). While we could leave 
the code in EventHelper for this case, we then have a mixed design 
pattern.


Are you ok with leaving as is for now? A future wish might be one 
where JFR would handle Logger via its own framework/API in a future 
JDK release.


I've removed the variable names using underscore. Also optimized some 
variable assignments in X509Impl.commitEvent(..)


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/

regards,
Sean.

On 09/07/2018 18:01, Seán Coffey wrote:

Erik,

Thanks for reviewing. Comments inline..

On 09/07/18 17:21, Erik Gahlin wrote:

Thanks Sean.

Some feedback on the code in the security libraries.

- We should use camel case naming convention for variables (not 
underscore).

Sure. I see two offending variable names which I'll fix up.


- Looking at sun/security/ssl/Finished.java,

I wonder if it wouldn't be less code and more easy to read, if we 
would commit the event in a local private function and then use the 
EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the 
recording/logging logic away in a helper class also had benefits but 
I'll re-factor to your suggested style unless I hear objections.


regards,
Sean.

Re: RFR [12]: 8195793: Remove GTE CyberTrust Global Root

[Seán Coffey]

Looks fine.

Regards,
Sean.

On 18/10/18 16:04, Sean Mullan wrote:
Please review this change to remove the GTE CyberTrust Global Root 
from the cacerts keystore. This root is expired and all certificates 
that chain back to this root have expired.


Note that retaining roots past their expiration date may make sense in 
some cases. For example, if we removed a root it could break signed 
code that had been previously timestamped. It may make sense to allow 
for a transition period for those apps to be signed and re-deployed 
using new certificates.


However, this is much less of a risk going forward. Applets have been 
deprecated since JDK 9 and WebStart apps are not supported as of 
(Oracle) JDK 11. These were the primary use cases for signed and 
timestamped code that I am aware of.


webrev: http://cr.openjdk.java.net/~mullan/webrevs/8195793/webrev.00/

Thanks,
Sean

Re: Upgrade to RSAKeyGenParameterSpec.F4 for RSA Keypair generation test?

[Seán Coffey]

Hi Xin,

looks like a reasonable backport candidate for jdk8u. I guess the 
changeset will apply cleanly once you correct the paths.


You should have no problem with a push request on jdk8u-dev : 
http://openjdk.java.net/projects/jdk8u/approval-template.html


Regards,
Sean.

On 18/10/18 23:34, Liu, Xin wrote:


Hi, Security developers,

We can’t pass the following test on our platform for OpenJDK8.

Test:http://hg.openjdk.java.net/jdk8u/jdk8u-dev/jdk/file/4a782529d712/test/sun/security/pkcs11/rsa/TestKeyPairGenerator.java#l106

Error Message:

Generating 512 bit keypair...

STDERR:

java.security.ProviderException: 
sun.security.pkcs11.wrapper.PKCS11Exception: CKR_ARGUMENTS_BAD


at 
sun.security.pkcs11.P11KeyPairGenerator.generateKeyPair(P11KeyPairGenerator.java:424)


at 
java.security.KeyPairGenerator$Delegate.generateKeyPair(KeyPairGenerator.java:697)


at TestKeyPairGenerator.main(TestKeyPairGenerator.java:119)

at PKCS11Test.premain(PKCS11Test.java:88)

at PKCS11Test.testNSS(PKCS11Test.java:403)

at PKCS11Test.main(PKCS11Test.java:98)

at TestKeyPairGenerator.main(TestKeyPairGenerator.java:97)

at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

at 
sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)


at 
sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)


at java.lang.reflect.Method.invoke(Method.java:498)

at 
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)


at java.lang.Thread.run(Thread.java:748)

Caused by: sun.security.pkcs11.wrapper.PKCS11Exception: CKR_ARGUMENTS_BAD

at sun.security.pkcs11.wrapper.PKCS11.C_GenerateKeyPair(Native Method)

at 
sun.security.pkcs11.P11KeyPairGenerator.generateKeyPair(P11KeyPairGenerator.java:416)


... 12 more

We believe the problem is caused by the NSS standard[1].

There’s a bug related to Solaris[2]. Our platform also rejects this 
test for the same reason.   Is it okay backport this patch to jdk8u? 
  I think it’s backward-compatible.



Here is difference between FIPS 186-2 and FIPS 186-4

RSA: restrict n size to 1024 2048 3072, restrict e to 2^16+1 to 
2^256−1, and specify RSA private key generation in detail with several 
options. *This prohibits one traditionally popular e namely 3;* F4 
(65537) is allowed and IME more popular anyway.[1]


References:

1.https://crypto.stackexchange.com/questions/35388/what-is-the-major-difference-between-fips-186-2-and-fips-186-4

2. https://bugs.openjdk.java.net/browse/JDK-8129560

Re: RFR[12] JDK-8212562: To remove lib/security from test/jdk/TEST.groups

[Seán Coffey]

Looks fine to me.

Regards,
Sean.

On 17/10/18 08:03, sha.ji...@oracle.com wrote:

Hi,
test/jdk/lib/security doesn't exist, so this directory could be 
removed from jdk_security3


diff -r f54dcfc5a5f8 test/jdk/TEST.groups
--- a/test/jdk/TEST.groupsFri Oct 05 15:12:37 2018 -0700
+++ b/test/jdk/TEST.groupsWed Oct 17 15:01:36 2018 +0800
@@ -218,8 +218,7 @@
 -sun/security/krb5 \
 -sun/security/jgss \
 javax/net \
-com/sun/net/ssl \
-lib/security
+com/sun/net/ssl

 jdk_security4 = \
 com/sun/security/jgss \

Best regards,
John Jiang

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Seán Coffey]

I'd like to re-boot this review. I've been working with Erik off thread 
who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name : 
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the @Relational 
JFR annotation.
* Keep calls for JFR event operations local to call site to optimize jvm 
compilation


webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/

This code is dependent on the JDK-8203629 enhancement being integrated.

Regards,
Sean.

On 10/07/18 13:50, Seán Coffey wrote:

Erik,

After some trial edits, I'm not so sure if moving the event & logger 
commit code into the class where it's used works too well after all.


In the code you suggested, there's an assumption that calls such as 
EventHelper.certificateChain(..) are low cost. While that might be the 
case here, it's something we can't always assume. It's called once for 
the JFR operation and once for the Logger operation. That pattern 
multiplies itself further in other Events. i.e. set up and assign the 
variables for a JFR event without knowing whether they'll be needed 
again for the Logger call. We could work around it by setting up some 
local variables and re-using them in the Logger code but then, we're 
back to where we were. The current EventHelper design eliminates this 
problem and also helps to abstract the recording/logging functionality 
away from the functionality of the class itself.


It also becomes a problem where we record events in multiple different 
classes (e.g. SecurityPropertyEvent). While we could leave the code in 
EventHelper for this case, we then have a mixed design pattern.


Are you ok with leaving as is for now? A future wish might be one 
where JFR would handle Logger via its own framework/API in a future 
JDK release.


I've removed the variable names using underscore. Also optimized some 
variable assignments in X509Impl.commitEvent(..)


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/

regards,
Sean.

On 09/07/2018 18:01, Seán Coffey wrote:

Erik,

Thanks for reviewing. Comments inline..

On 09/07/18 17:21, Erik Gahlin wrote:

Thanks Sean.

Some feedback on the code in the security libraries.

- We should use camel case naming convention for variables (not 
underscore).

Sure. I see two offending variable names which I'll fix up.


- Looking at sun/security/ssl/Finished.java,

I wonder if it wouldn't be less code and more easy to read, if we 
would commit the event in a local private function and then use the 
EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the 
recording/logging logic away in a helper class also had benefits but 
I'll re-factor to your suggested style unless I hear objections.


regards,
Sean.

Re: RFR: 8209862:CipherCore performance improvement

[Seán Coffey]

Thanks for the review Adam. I've corrected those style issues.

Now waiting on 2nd Reviewer.

Regards,
Sean.

On 08/10/18 19:18, Adam Petcher wrote:


The organization is better now, thanks. The code looks good to me, but 
I would like to request another review from Tony (or someone else who 
is familiar with this code) before you push. There is a lot of 
complexity here, and it is hard for me to be sure that everything will 
behave the same after the change. It will be helpful to have another 
person examine the new code to ensure that it is correct.


Minor style issues(addressing them is optional):

Lines 848, 915, and 969 are longer than 80 characters
Line 1075: no space before ? character


On 10/2/2018 1:51 PM, Seán Coffey wrote:


Good points from Adam and Tony. I've taken them both on board. Sergey 
has already eliminated a lot of duplicate code but there was further 
optimizing possible in the two doFinal(..) methods. I've introduced a 
new 'fillOutputBuffer' method to help. Please review :


https://cr.openjdk.java.net/~coffeys/webrev.8209862.v2/webrev/

regards,
Sean.


On 01/10/2018 15:32, Adam Petcher wrote:
Looks like a nice improvement, but is it possible to do this without 
duplicating code? For example, code almost identical to this also 
appears starting on line 860:


971 } else { // encrypting
972 try {
973 outLen = finalNoPadding(finalBuf, finalOffset, output,
974 outputOffset, finalBufLen);
975 } finally {
976 // reset after doFinal() for GCM encryption
977 requireReinit = (cipherMode == GCM_MODE);
978 if (finalBuf != input) {
979 // done with internal finalBuf array. Copied to output
980 Arrays.fill(finalBuf, (byte) 0x00);
981 }
982 }
983 }
It may be possible to factor out the entire if/else statement and 
some of the code around it into a separate method and do the short 
buffer check and save/restore around it. Ideally, each doFinal 
method would have only a small amount of code to either allocate the 
array or check array lengths, and then they would call the same 
private method to do most of the work.


I would suggest a noreg-sqe label on the ticket to document that 
existing regression tests are used to ensure correctness of the 
modified code.


Minor code style comments: check for long lines (e.g. line 856) and 
missing spaces after commas in actual parameter lists (also e.g. 
line 856). Also, line 1076 is missing space around the '?' and ':' 
characters. I can check the code again and give you the complete 
list after we sort out the code duplication.



On 10/1/2018 9:11 AM, Seán Coffey wrote:
JDK-8207775 introduced some performance regressions in the 
ciphercore area. Sergey Kuksenko has been looking at this and has 
contributed the following patch:


http://cr.openjdk.java.net/~skuksenko/crypto/8209862/
bug report : https://bugs.openjdk.java.net/browse/JDK-8209862

I've been reviewing it and ran functionality and TCK testing. 
Didn't see any issues. Sergey has also confirmed that the patch 
helps to alleviate the performance issues introduced. More details 
regards approach for fix are in the bug description.


Thanks Sergey! I'm looking for another review from security team.

Re: RFR: 8209862:CipherCore performance improvement

[Seán Coffey]

Good points from Adam and Tony. I've taken them both on board. Sergey 
has already eliminated a lot of duplicate code but there was further 
optimizing possible in the two doFinal(..) methods. I've introduced a 
new 'fillOutputBuffer' method to help. Please review :


https://cr.openjdk.java.net/~coffeys/webrev.8209862.v2/webrev/

regards,
Sean.


On 01/10/2018 15:32, Adam Petcher wrote:
Looks like a nice improvement, but is it possible to do this without 
duplicating code? For example, code almost identical to this also 
appears starting on line 860:


971 } else { // encrypting
972 try {
973 outLen = finalNoPadding(finalBuf, finalOffset, output,
974 outputOffset, finalBufLen);
975 } finally {
976 // reset after doFinal() for GCM encryption
977 requireReinit = (cipherMode == GCM_MODE);
978 if (finalBuf != input) {
979 // done with internal finalBuf array. Copied to output
980 Arrays.fill(finalBuf, (byte) 0x00);
981 }
982 }
983 }
It may be possible to factor out the entire if/else statement and some 
of the code around it into a separate method and do the short buffer 
check and save/restore around it. Ideally, each doFinal method would 
have only a small amount of code to either allocate the array or check 
array lengths, and then they would call the same private method to do 
most of the work.


I would suggest a noreg-sqe label on the ticket to document that 
existing regression tests are used to ensure correctness of the 
modified code.


Minor code style comments: check for long lines (e.g. line 856) and 
missing spaces after commas in actual parameter lists (also e.g. line 
856). Also, line 1076 is missing space around the '?' and ':' 
characters. I can check the code again and give you the complete list 
after we sort out the code duplication.



On 10/1/2018 9:11 AM, Seán Coffey wrote:
JDK-8207775 introduced some performance regressions in the ciphercore 
area. Sergey Kuksenko has been looking at this and has contributed 
the following patch:


http://cr.openjdk.java.net/~skuksenko/crypto/8209862/
bug report : https://bugs.openjdk.java.net/browse/JDK-8209862

I've been reviewing it and ran functionality and TCK testing. Didn't 
see any issues. Sergey has also confirmed that the patch helps to 
alleviate the performance issues introduced. More details regards 
approach for fix are in the bug description.


Thanks Sergey! I'm looking for another review from security team.

RFR: 8209862:CipherCore performance improvement

[Seán Coffey]

JDK-8207775 introduced some performance regressions in the ciphercore 
area. Sergey Kuksenko has been looking at this and has contributed the 
following patch:


http://cr.openjdk.java.net/~skuksenko/crypto/8209862/
bug report : https://bugs.openjdk.java.net/browse/JDK-8209862

I've been reviewing it and ran functionality and TCK testing. Didn't see 
any issues. Sergey has also confirmed that the patch helps to alleviate 
the performance issues introduced. More details regards approach for fix 
are in the bug description.


Thanks Sergey! I'm looking for another review from security team.

--
Regards,
Sean.

Re: RFR: JDK-8209129 :Further improvements to cipher buffer management

[Seán Coffey]

I made those minor edits in the end and pushed the changes. We should 
examine usage of Arrays.fill(passwd, '0') in other parts of the JDK as a 
follow up.


regards,
Sean.

On 22/08/2018 23:01, Seán Coffey wrote:


On 22 August 2018 19:22:49 IST, Ivan Gerasimov  
wrote:

Hi Seán!

Just a minor comment.

I don't know if it's even measurable in this context, but I was under
impression that filling memory with zero *bytes* might be a slightly
more efficient then filling with any other constant.

Maybe it is better to use Arrays.fill(passwd, '\0') instead of
Arrays.fill(passwd, '0') to give the JVM a chance to optimize filling
if
it's possible?

Interesting comment Ivan. I was not aware of such an effect! If you've further 
references on that, I'd appreciate it. '0' is used in other, similar, fill 
operations IIRC. Perhaps we can optimize such code across all security libs 
code via another JBS issue.

Regards,
Sean.

Re: RFR: JDK-8209129 :Further improvements to cipher buffer management

[Seán Coffey]

On 22 August 2018 19:22:49 IST, Ivan Gerasimov  
wrote:
>Hi Seán!
>
>Just a minor comment.
>
>I don't know if it's even measurable in this context, but I was under 
>impression that filling memory with zero *bytes* might be a slightly 
>more efficient then filling with any other constant.
>
>Maybe it is better to use Arrays.fill(passwd, '\0') instead of 
>Arrays.fill(passwd, '0') to give the JVM a chance to optimize filling
>if 
>it's possible?

Interesting comment Ivan. I was not aware of such an effect! If you've further 
references on that, I'd appreciate it. '0' is used in other, similar, fill 
operations IIRC. Perhaps we can optimize such code across all security libs 
code via another JBS issue.

Regards,
Sean.

>
>With kind regards,
>
>Ivan
>
>
>On 8/22/18 9:25 AM, Seán Coffey wrote:
>> Thanks for reviewing. comments inline..
>>
>> On 22/08/18 15:50, Weijun Wang wrote:
>>> PBES2Core.java:
>>>
>>>   181 byte[] passwdBytes = key.getEncoded();
>>>   182 char[] passwdChars = null;
>>>   183 PBEKeySpec pbeSpec;
>>>   184 try {
>>>   185 if ((passwdBytes == null) ||
>>>   186 !(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3))) {
>>>   187 throw new InvalidKeyException("Missing
>password");
>>>   188 }
>>>   
>>>   272 } finally {
>>>   273 if (passwdChars != null) Arrays.fill(passwdChars,
>' 
>>> ');
>>>   274 Arrays.fill(passwdBytes, (byte)0x00);
>>>   275 }
>>>
>>> If passwdBytes == null, line 274 would throw an NPE.
>> Good catch. Corrected.
>>>
>>> PBKDF2KeyImpl.java:
>>>
>>>87 char[] passwd = keySpec.getPassword();
>>>88 if (passwd == null) {
>>>89 // Should allow an empty password.
>>>90 this.passwd = new char[0];
>>>91 } else {
>>>92 this.passwd = passwd.clone();
>>>93 }
>>>94 // Convert the password from char[] to byte[]
>>>95 byte[] passwdBytes = getPasswordBytes(this.passwd);
>>>
>>>96 // remove local copy
>>>97 Arrays.fill(passwd, '0');
>>>
>>> If passwd == null, line 97 would throw an NPE.
>> Another good catch!
>>
>> updated webrev : 
>> http://cr.openjdk.java.net/~coffeys/webrev.8209129.v3/webrev/
>>
>> regards,
>> Sean.
>>
>>>
>>> Otherwise fine.
>>>
>>> Thanks
>>> Max
>>>
>>>
>>>> On Aug 17, 2018, at 12:53 AM, Seán Coffey  
>>>> wrote:
>>>>
>>>> Find new webrev here Max :
>>>>
>>>> http://cr.openjdk.java.net/~coffeys/webrev.8209129.v2/webrev/
>>>>
>>>> regards :
>>>>
>>
>>

Re: RFR: JDK-8209129 :Further improvements to cipher buffer management

[Seán Coffey]

Thanks for reviewing. comments inline..

On 22/08/18 15:50, Weijun Wang wrote:

PBES2Core.java:

  181 byte[] passwdBytes = key.getEncoded();
  182 char[] passwdChars = null;
  183 PBEKeySpec pbeSpec;
  184 try {
  185 if ((passwdBytes == null) ||
  186 !(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 
3))) {
  187 throw new InvalidKeyException("Missing password");
  188 }
  
  272 } finally {
  273 if (passwdChars != null) Arrays.fill(passwdChars, ' ');
  274 Arrays.fill(passwdBytes, (byte)0x00);
  275 }

If passwdBytes == null, line 274 would throw an NPE.

Good catch. Corrected.


PBKDF2KeyImpl.java:

   87 char[] passwd = keySpec.getPassword();
   88 if (passwd == null) {
   89 // Should allow an empty password.
   90 this.passwd = new char[0];
   91 } else {
   92 this.passwd = passwd.clone();
   93 }
   94 // Convert the password from char[] to byte[]
   95 byte[] passwdBytes = getPasswordBytes(this.passwd);

   96 // remove local copy
   97 Arrays.fill(passwd, '0');

If passwd == null, line 97 would throw an NPE.

Another good catch!

updated webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8209129.v3/webrev/


regards,
Sean.



Otherwise fine.

Thanks
Max



On Aug 17, 2018, at 12:53 AM, Seán Coffey  wrote:

Find new webrev here Max :

http://cr.openjdk.java.net/~coffeys/webrev.8209129.v2/webrev/

regards :

Re: RFR: JDK-8209129 :Further improvements to cipher buffer management

[Seán Coffey]

Ping. I'd like to push on with this review.

Regards,
Sean.

On 16/08/18 17:53, Seán Coffey wrote:

Find new webrev here Max :

http://cr.openjdk.java.net/~coffeys/webrev.8209129.v2/webrev/

regards :


MD4.java:

  110 void implReset() {

  Add @Override. 
There's a whole bunch of annotations missing in these files, I'd 
prefer if it was fixed up with different bug ID.


another thing we're to be cautious about is the nulling out of values 
returned from some Key's such as PBEKey subclasses. We're assuming 
that getPassword() returns cloned copies. That should be the case for 
any production fit code.


Edit to test/jdk/com/sun/crypto/provider/Cipher/PBE/PKCS12Cipher.java 
was necessary as a result.


regards,
Sean.


On 10/08/2018 15:18, Seán Coffey wrote:

Thanks for the review Max - comments inline..

Regards,
Sean.

On 10/08/18 11:53, Weijun Wang wrote:

HmacPKCS12PBESHA1.java:

   76 byte[] passwdBytes = key.getEncoded();
   77 if ((passwdBytes == null) ||
   78 !(key.getAlgorithm().regionMatches(true, 0, 
"PBE", 0, 3))) {
   79 throw new InvalidKeyException("Missing 
password");

   80 }

  Please also cleanup passwdBytes. Maybe divide the if check above 
into 2 to avoid passwdBytes being assigned but algorithm is not PBE.
Yes - I had thought about refactoring this code during edits. Will 
make that change.


  132 Arrays.fill(passwdChars, '0');

  In a finally block?

PBES1Core.java:

  250 Arrays.fill(passwdBytes, (byte) 0x00);

  In a finally block?

PBES2Core.java:

  273 Arrays.fill(passwdChars, ' ');
  274 Arrays.fill(passwdBytes, (byte)0x00);

  In a finally block?

Yes - makes sense to capture above in finally blocks. Will re-factor.


PBKDF2KeyImpl.java:

   88 if (passwd == null) {
   89 // Should allow an empty password.
   90 this.passwd = new char[0];
   91 } else {
   92 this.passwd = passwd.clone();
   93 }
   94 // Convert the password from char[] to byte[]
   95 byte[] passwdBytes = getPasswordBytes(this.passwd);
   96 // remove local copy
   97 Arrays.fill(passwd, '0');

  Strange, why passwd.clone()?
Not sure - it's original behaviour. I'll check if it makes sense to 
change.


  129 Arrays.fill(passwdBytes, (byte)0x00);

  In a finally block?

PBMAC1Core.java:

  111 byte[] passwdBytes = key.getEncoded();
  112 if ((passwdBytes == null) ||
  113 !(key.getAlgorithm().regionMatches(true, 0, 
"PBE", 0, 3))) {
  114 throw new InvalidKeyException("Missing 
password");

  115 }

  170 Arrays.fill(passwdChars, ' ');

  Same comments as for HmacPKCS12PBESHA1.java.

PKCS12PBECipherCore.java:

  262 char[] passwdChars = null;

  Deal with the same way as in PBMAC1Core.java?

MD4.java:

  110 void implReset() {

  Add @Override.
All above comments make sense. I've come across one other possible 
issue in MessageDigest functionality. I'm debugging that to be sure. 
Will get a new webrev out once that's done.


regards,
Sean.


Thanks
Max


On Aug 9, 2018, at 6:42 PM, Seán Coffey  
wrote:


Adding RFR to title..

On 09/08/2018 11:37, Seán Coffey wrote:
I've been looking further at how private/temporary buffers are 
used in cipher/keystore management and identified some more areas 
that could benefit with a more aggressive nulling out of contents.


I've been testing through use of stepping through debugging 
sessions while setting/getting keys and capturing process memory 
via tooling like gcore.


JBS report : https://bugs.openjdk.java.net/browse/JDK-8209129

webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8209129.v1/webrev/index.html 



TCK and regression tests are green.

regards,
Sean.

RFR: 8u-dev : 8206911: javax/xml/crypto/dsig/GenerationTests.java fails in 8u-dev

[Seán Coffey]

This test is currently on the ProblemList for solaris-all in jdk8u-dev. 
The test can fail on Solaris installs where the native pkcs11 library 
might not be fully patched. I've modified the testcase to allow for such 
failure and to re-run with the SunPKCS11-Solaris Provider removed.


Testing looks good. This failure is mainly seen on older build/test 
systems which are used by 8u/7u.


https://bugs.openjdk.java.net/browse/JDK-8206911
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8206911.8u/webrev/

--
Regards,
Sean.

RFR: JDK-8208675: Remove legacy sun.security.key.serial.interop property

[Seán Coffey]

"sun.security.key.serial.interop" is an old JDK 1.4 -1.5 
interoperability property. It can be safely removed now.


JBS report : https://bugs.openjdk.java.net/browse/JDK-8208675
webrev : http://cr.openjdk.java.net/~coffeys/webrev.8208675/webrev/

regards,
Sean.