Re: RFR: 8061842: Package jurisdiction policy files as something other than JAR

[Wang Weijun]

Before this change, you require default policy in neither export nor import to 
be empty but do not care about the getMinimum() result. In this change, you 
make sure the final result is not empty. I assume this is a fix?

283 // Did we find a default perms?

What does this line mean?

296 // This should never happen

But you can still print out the file name.


Can you rename policydir-tbd to something else? I am afraid it will be confused 
with policy.url.1 etc.

The original README.TXT in unlimited says "are exportable from the United 
States." and you have "is exportable." now. Is this intended? (IANAL)

TestUnlimited.java:
45 "// Use the AES are the test Cipher", you mean "Use AES as the test Cipher"?
51 "throw new Exception ("Unlimited policy is NOT active");". No space before 
"(".

No other comment.

--Max

> On Aug 18, 2016, at 7:22 AM, Bradford Wetmore  
> wrote:
> 
> New webrev:
> 
> https://bugs.openjdk.java.net/browse/JDK-8061842
> http://cr.openjdk.java.net/~wetmore/8061842/webrev.01/

Re: [9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation

[Valerie Peng]

Regression tests are still running, but thought that I will send the 
updated webrev out and see if there are more comments.


Webrev is updated at: 
http://cr.openjdk.java.net/~valeriep/8078661/webrev.01/


Thanks,
Valerie

On 8/17/2016 9:55 AM, Michael StJohns wrote:

On 8/16/2016 9:24 PM, Valerie Peng wrote:


Anyone has time to review a straightforward fix? The current PKCS11 
code assume that if public exponent is available for RSA Private Key, 
then it's a RSA CRT key. However, not all vendor implementation works 
this way. Changing to a tighter check and did minor code-refactoring 
to avoid re-retrieving the attribute values.


Bug: https://bugs.openjdk.java.net/browse/JDK-8078661
Webrev: http://cr.openjdk.java.net/~valeriep/8078661/webrev.00/

Thanks,
Valerie


Given that there's a change to PKCS11 for 2.40 that says that all RSA 
private key objects  MUST also store CKA_PUBLIC_EXPONENT, some change 
needed to be made.


Sorry - I don't think this fix will work.  Or if its working on your 
version of PKCS11, your version of PKCS11 is doing it wrong.  The 
problem is that if you specify attributes that don't exist on the 
object, the underlying PKCS11 library is supposed to return 
CKR_ATTRIBUTE_TYPE_INVALID.  And that should trigger a thrown 
exception before you ever get anything copied to your attributes.


1) Get modulus and private exponent first.  That gives you the stuff 
for a generic RSA private key - and if it fails, there's no reason to 
continue.


2) Then get the rest of the stuff.  If that fails, then you already 
have the stuff you need for a normal private key.




  boolean crtKey;
  try {
  session.token.p11.C_GetAttributeValue
  (session.id(), keyID, attrs2);
- crtKey = (attrs2[0].pValue instanceof byte[]);
+ crtKey = ((attrs2[1].pValue instanceof byte[]) &&
+ (attrs2[3].pValue instanceof byte[]) &&
+ (attrs2[4].pValue instanceof byte[]) &&
+ (attrs2[5].pValue instanceof byte[]) &&
+ (attrs2[6].pValue instanceof byte[]) &&
+ (attrs2[7].pValue instanceof byte[])) ;
  } catch (PKCS11Exception e) {
  // ignore, assume not available
  crtKey = false;
  }


// Change attrs2 so it only has the additional CRT attributes (e.g. 
delete CKA_MODULUS, CKA_PRIVATE_EXPONENT from the list


Replace the above with

CK_ATTRIBUTE[] attrs3 = new CK_ATTRIBUTE[] {
   new CK_ATTRIBUTE(CKA_MODULUS),
   new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT)
};
// no try block needed here - we want to throw the error if it occurs
session.token.p11.C_GetAttributeValue (session.id(), keyID, attrs3);

// So far so good - we have the base attributes, let's see if we can 
get the additional attributes;


try {
session.token.p11.C_GetAttributeValue(session.id(),keyID, attrs2);
} catch (PKCS11Exception e) {
   // we really should check the return value for one of the non-fatal 
values, but let's just assume its not a CRT key
   return new P11RSAPrivateNonCRTKey (session, keyID, algorithm, 
keyLength, attrs2, attrs3);

}

// if we fall through then its a CRT key
// -- we should check for byte[] ness of each of the components, and 
throw an error if they arent - but which error?


return new P11RSAPrivateKey (session, keyID, algorithm, keyLength, 
attrs2, attrs3);


// there are cleanups necessary in other places.  I'd suggest rather 
than depending on the ordering of attributes, you do assignment by 
CKA_ values just so someone coming later doesn't mess things up by 
mistake.  Also, a hell of a lot more readable.


static CK_ATTRIBUTE getAttribute (CK_ATTRIBUTE[] attrs, long attrType) {
for (CK_ATTRIBUTE a : attrs) {
 if (a.type == attrType)
  return a;
 }
return null; // or throw something?
}


coeff = getAtttribute(attrs,CKA_COEFFICIENT).getBigInteger();


The other possibility is to change the C code for C_GetAttributeValues 
so it doesn't error out for the non-fatal error codes and instead 
returns a null value attribute when the attribute is missing.


Mike

Re: RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails

[Weijun Wang]

If I recall correctly, there should be a way to specify a policy file
in @run without overriding the default one. May be it is "@run
main/othervm/java.security.policy=unbound.ssl.policy_new"


Yes, I think this should work. I've also just learned about it and don't 
know from which jtreg it is supported. Hopefully the minimized-required 
version of jtreg for jdk8u already has it.


--Max

Re: [9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation

[Valerie Peng]

Thanks for the input.
SunPKCS11 provider always generate RSA CRT keys if I recall correctly, 
so it's hard to test the non-CRT scenarios. Thus, I made the changes 
basing on the suggestions in the bug report.


I will update the webrev accordingly.
Valerie

On 8/17/2016 9:55 AM, Michael StJohns wrote:

On 8/16/2016 9:24 PM, Valerie Peng wrote:


Anyone has time to review a straightforward fix? The current PKCS11 
code assume that if public exponent is available for RSA Private Key, 
then it's a RSA CRT key. However, not all vendor implementation works 
this way. Changing to a tighter check and did minor code-refactoring 
to avoid re-retrieving the attribute values.


Bug: https://bugs.openjdk.java.net/browse/JDK-8078661
Webrev: http://cr.openjdk.java.net/~valeriep/8078661/webrev.00/

Thanks,
Valerie


Given that there's a change to PKCS11 for 2.40 that says that all RSA 
private key objects  MUST also store CKA_PUBLIC_EXPONENT, some change 
needed to be made.


Sorry - I don't think this fix will work.  Or if its working on your 
version of PKCS11, your version of PKCS11 is doing it wrong.  The 
problem is that if you specify attributes that don't exist on the 
object, the underlying PKCS11 library is supposed to return 
CKR_ATTRIBUTE_TYPE_INVALID.  And that should trigger a thrown 
exception before you ever get anything copied to your attributes.


1) Get modulus and private exponent first.  That gives you the stuff 
for a generic RSA private key - and if it fails, there's no reason to 
continue.


2) Then get the rest of the stuff.  If that fails, then you already 
have the stuff you need for a normal private key.




  boolean crtKey;
  try {
  session.token.p11.C_GetAttributeValue
  (session.id(), keyID, attrs2);
- crtKey = (attrs2[0].pValue instanceof byte[]);
+ crtKey = ((attrs2[1].pValue instanceof byte[]) &&
+ (attrs2[3].pValue instanceof byte[]) &&
+ (attrs2[4].pValue instanceof byte[]) &&
+ (attrs2[5].pValue instanceof byte[]) &&
+ (attrs2[6].pValue instanceof byte[]) &&
+ (attrs2[7].pValue instanceof byte[])) ;
  } catch (PKCS11Exception e) {
  // ignore, assume not available
  crtKey = false;
  }


// Change attrs2 so it only has the additional CRT attributes (e.g. 
delete CKA_MODULUS, CKA_PRIVATE_EXPONENT from the list


Replace the above with

CK_ATTRIBUTE[] attrs3 = new CK_ATTRIBUTE[] {
   new CK_ATTRIBUTE(CKA_MODULUS),
   new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT)
};
// no try block needed here - we want to throw the error if it occurs
session.token.p11.C_GetAttributeValue (session.id(), keyID, attrs3);

// So far so good - we have the base attributes, let's see if we can 
get the additional attributes;


try {
session.token.p11.C_GetAttributeValue(session.id(),keyID, attrs2);
} catch (PKCS11Exception e) {
   // we really should check the return value for one of the non-fatal 
values, but let's just assume its not a CRT key
   return new P11RSAPrivateNonCRTKey (session, keyID, algorithm, 
keyLength, attrs2, attrs3);

}

// if we fall through then its a CRT key
// -- we should check for byte[] ness of each of the components, and 
throw an error if they arent - but which error?


return new P11RSAPrivateKey (session, keyID, algorithm, keyLength, 
attrs2, attrs3);


// there are cleanups necessary in other places.  I'd suggest rather 
than depending on the ordering of attributes, you do assignment by 
CKA_ values just so someone coming later doesn't mess things up by 
mistake.  Also, a hell of a lot more readable.


static CK_ATTRIBUTE getAttribute (CK_ATTRIBUTE[] attrs, long attrType) {
for (CK_ATTRIBUTE a : attrs) {
 if (a.type == attrType)
  return a;
 }
return null; // or throw something?
}


coeff = getAtttribute(attrs,CKA_COEFFICIENT).getBigInteger();


The other possibility is to change the C code for C_GetAttributeValues 
so it doesn't error out for the non-fatal error codes and instead 
returns a null value attribute when the attribute is missing.


Mike

Re: RFR: 8061842: Package jurisdiction policy files as something other than JAR

[Bradford Wetmore]

New webrev:

https://bugs.openjdk.java.net/browse/JDK-8061842
http://cr.openjdk.java.net/~wetmore/8061842/webrev.01/

On 8/10/2016 11:19 AM, Sean Mullan wrote:

Hi Brad,

Looks pretty good. You should also send this to build-dev to review the
Makefile changes. Just a few comments:

- src/java.base/share/conf/security/policy/README.txt

17 contain no restrictions on cryptographic strengths, but they must

s/must/must be/


Ok.


18 specifically activated by updating the "crypto.policy" entry in the

s/entry/Security property/


Ok.  I've updated to:

---begin---
specifically activated by updating the "crypto.policy" Security property
(e.g. /conf/security/java.security) to point to the 
appropriate directory.

end---


33 Please see The Java(TM) Cryptography Architecture (JCA) Reference

Is "TM" really necessary here?


It was required in previous versions of the unlimited crypto policy 
bundle.  We could run it by PM if you feel we should.



src/java.base/share/conf/security/policy/unlimited/default_US_export.policy

1 // Manufacturing policy file.

The term "Manufacturing" is odd. Can we just say this is the "Default
local policy file"?


Sure.


- src/java.base/share/conf/security/java.security

854 crypto.policy=policydir-tbd

The policydir-tbd value is a little confusing in that it isn't a real
value. What about just setting this to the empty string?


It's a similar marker for the string replacement like was done for 
security.provider.tbd.  I could change it to be delineated with <>: 
"" if you like?



- src/java.base/share/classes/javax/crypto/JceSecurity.java

 255 String cryptoPolicyDir =
Security.getProperty("crypto.policy");
 256 Path cryptoPolicyPath = Paths.get(cryptoPolicyDir);

What happens if crypto.policy is not set or is set to ""?


Good catch.  Not set would NPE, "" would simply look at 
/conf/security/policy and fail to iterate the directory if no 
files were actually there.  I've added code for both those conditions, 
and also switched to use Path.resolve().



302 // I/O error encounted during the iteration,

s/encounted/encountered/


Thanks!

Brad



--Sean

On 08/04/2016 03:35 PM, Bradford Wetmore wrote:

https://bugs.openjdk.java.net/browse/JDK-8061842
http://cr.openjdk.java.net/~wetmore/8061842/webrev.00/

The proposal is to move the configuration files from the jar files in
/lib/security to a series of subdirectories under a new
"policy" subdirectory in /conf/security.  Each subdirectory
within that directory will represent a complete policy configuration.
The existing jar files will be split into flat text files such that the
current/existing policies remain.

The default set of policy files (i.e. directory) is configured using a
new java.security.Security property called "crypto.policy" which will be
added to the /conf/security/java.security file.  The default
initial options are "limited" or "unlimited", however additional
directories could potentially be created that specify other
as-yet-unknown policies.

The default value of this property will be "limited" which corresponds
to our current policy for JRE/JDK export/import around the world.
However, the build respects the following "configure" option:

--enable-unlimited-crypto
Enable unlimited crypto policy [disabled]

Within the directory, our implementation will look for files using the
standard filename prefix above ("default_" or "exempt_"), thus new
additional policy restrictions/abstractions can be added with a simple
file addition.

Brad

Re: RFR: 8156192: Provider#compute and #merge methods expect wrong permission & #compute ClassCastException even with wrong permission.

[Anthony Scarpino]

Thanks

Tony

On 08/17/2016 01:47 PM, Sean Mullan wrote:

Looks fine to me.

--Sean

On 08/17/2016 04:11 PM, Anthony Scarpino wrote:

Hi,

I need a simple review on fixing some cut-n-paste errors that the JCK
tests found.

I didn't include the tests because the functionality is covered by the
JCK tests and I don't feel typos need a regression test.

http://cr.openjdk.java.net/~ascarpino/8156192/webrev/

Tony

Re: RFR: 8156192: Provider#compute and #merge methods expect wrong permission & #compute ClassCastException even with wrong permission.

[Sean Mullan]

Looks fine to me.

--Sean

On 08/17/2016 04:11 PM, Anthony Scarpino wrote:

Hi,

I need a simple review on fixing some cut-n-paste errors that the JCK
tests found.

I didn't include the tests because the functionality is covered by the
JCK tests and I don't feel typos need a regression test.

http://cr.openjdk.java.net/~ascarpino/8156192/webrev/

Tony

RFR: 8156192: Provider#compute and #merge methods expect wrong permission & #compute ClassCastException even with wrong permission.

[Anthony Scarpino]

Hi,

I need a simple review on fixing some cut-n-paste errors that the JCK 
tests found.


I didn't include the tests because the functionality is covered by the 
JCK tests and I don't feel typos need a regression test.


http://cr.openjdk.java.net/~ascarpino/8156192/webrev/

Tony

Re: RFR: 9: 8164229: Redundant "sun/net/www/protocol/https" tests in jdk_security3 group

[Rajan Halade]

On 8/17/16 11:36 AM, Chris Hegarty wrote:


On 17 Aug 2016, at 18:54, Rajan Halade  wrote:

sun/net/www/protocol/https tests are redundant in jdk_security3 group, these 
are included in jdk_net group.

Yes they are, but it is very important that anyone touching TLS verify
that HTTPS still works. If the jdk_net tests will be run to verity
such changes, then this should be fine.
Yes, these are useful tests to run with https changes. I don't think 
they need to be duplicated in jdk_security group as jdk_security and 
jdk_net are part of core tests and with continuous integration system in 
place to run all tests with each changeset, this duplication is not 
required.


Out of curiosity, how many tests are in sun/net/www/protocol/https,
and approximately how long do they run for?
There are 27 tests and I see each of those finish within seconds (max I 
saw was 8 secs for ReadTimeout.java).


Thanks,
Rajan


-Chris.

Re: [9] RFR 6877937: The SunJCE PBKDF2KeyImpl is requiring the MAC instance also be from SunJCE.

[Bradford Wetmore]

Looks ok, probably better than the initial proposed fix from April 2014.

Brad




On 8/11/2016 4:55 AM, Vincent Ryan wrote:

Please review this change to unpin the Mac implementation from the
SunJCE provider.
Since the Mac is a private field there are no issues
regarding Clonable implementations for Mac or its MessageDigest.
Thanks.

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

*diff --git
a/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java
b/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java*
*---
a/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java*
*+++
b/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java*
@@ -107,7 +107,7 @@
 throw new InvalidKeySpecException("Key length is negative");
 }
 try {
-this.prf = Mac.getInstance(prfAlgo, SunJCE.getInstance());
+this.prf = Mac.getInstance(prfAlgo);
 } catch (NoSuchAlgorithmException nsae) {
 // not gonna happen; re-throw just in case
 InvalidKeySpecException ike = new InvalidKeySpecException();

Re: RFR: 9: 8164229: Redundant "sun/net/www/protocol/https" tests in jdk_security3 group

[Chris Hegarty]

On 17 Aug 2016, at 18:54, Rajan Halade  wrote:
> 
> sun/net/www/protocol/https tests are redundant in jdk_security3 group, these 
> are included in jdk_net group.

Yes they are, but it is very important that anyone touching TLS verify
that HTTPS still works. If the jdk_net tests will be run to verity
such changes, then this should be fine.

Out of curiosity, how many tests are in sun/net/www/protocol/https,
and approximately how long do they run for? 

-Chris.

[9] RFR: 8164100: com/sun/crypto/provider/KeyFactory/TestProviderLeak.java fails with java.util.concurrent.TimeoutException

[Artem Smotrakov]

Hello,

Please review the following patch for 
com/sun/crypto/provider/KeyFactory/TestProviderLeak.java test.


This is a request to make the test take into account a test timeout 
factor. Timeout factor can be specified with "-timeout" jtreg's command 
line option. This option is used in some test runs to make test runs 
more stable (for example, VM SQE uses it while running regression tests 
with different JVM options).


Bug: https://bugs.openjdk.java.net/browse/JDK-8164100
Webrev: http://cr.openjdk.java.net/~asmotrak/8164100/webrev.00/

Artem

RFR: 9: 8164229: Redundant "sun/net/www/protocol/https" tests in jdk_security3 group

[Rajan Halade]

sun/net/www/protocol/https tests are redundant in jdk_security3 group, 
these are included in jdk_net group.


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

Thanks,
Rajan

Re: RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails

[Artem Smotrakov]

Sorry, my bad, I didn't notice '9-na' label.

I suppose that code from ext directory should have all permissions:

artem@artem-laptop:$ cat ~/jdk/jdk1.8.0_92b14/jre/lib/security/java.policy

// Standard extensions get all permissions by default

grant codeBase "file:${{java.ext.dirs}}/*" {
permission java.security.AllPermission;
};

// default permissions granted to all domains
...

I am wondering if it would be better it the test didn't override the 
default policy.


Artem

On 08/17/2016 10:12 AM, Seán Coffey wrote:

Hi Artem,

Sorry - should have said that this is for jdk8u-dev. The bug is marked 
9-na. The provider loading changes made in this area for 9 mean that 
it's not affected.


Regards,
Sean.

On 17/08/16 18:10, Artem Smotrakov wrote:

Hi Sean,

If I remember correctly, there is no ext directory in JDK 9 any more.

I don't see in jtr file that "java.ext.dirs" system property is 
passed to the test. If I understand correctly, 
"file:${{java.ext.dirs}}/*" becomes "file:/*" which seems to grand 
all permissions to all the code. It doesn't look correct for this test.


It looks like the test overrides the default policy, please see in 
jtr file


-Djava.security.policy==/export/home/gtee/scripts/Results/workDir/scratch_2/unbound.ssl.policy_new 
\\


If I recall correctly, there should be a way to specify a policy file 
in @run without overriding the default one. May be it is "@run 
main/othervm/java.security.policy=unbound.ssl.policy_new"


Artem


On 08/17/2016 09:53 AM, Seán Coffey wrote:
A recently added test case lacks sufficient permissions to read a 
conf file when running with security manager.


bug report : https://bugs.openjdk.java.net/browse/JDK-8162916

proposed patch :
 diff --git a/test/sun/security/krb5/auto/unbound.ssl.policy 
b/test/sun/security/krb5/auto/unbound.ssl.policy

--- a/test/sun/security/krb5/auto/unbound.ssl.policy
+++ b/test/sun/security/krb5/auto/unbound.ssl.policy
@@ -1,7 +1,13 @@
+// Standard extensions get all permissions by default
+
+grant codeBase "file:${{java.ext.dirs}}/*" {
+permission java.security.AllPermission;
+};
+
 grant {
 permission java.util.PropertyPermission "*", "read,write";
 permission java.net.SocketPermission "*:*", 
"listen,resolve,accept,connect";

-permission java.io.FilePermission "*", "read,write,delete";
+permission java.io.FilePermission "<>", 
"read,write,delete";

 permission java.lang.RuntimePermission "accessDeclaredMembers";
 permission java.lang.reflect.ReflectPermission 
"suppressAccessChecks";

 permission java.lang.RuntimePermission "accessClassInPackage.*";

Re: RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails

[Artem Smotrakov]

Hi Sean,

If I remember correctly, there is no ext directory in JDK 9 any more.

I don't see in jtr file that "java.ext.dirs" system property is passed 
to the test. If I understand correctly, "file:${{java.ext.dirs}}/*" 
becomes "file:/*" which seems to grand all permissions to all the code. 
It doesn't look correct for this test.


It looks like the test overrides the default policy, please see in jtr file

-Djava.security.policy==/export/home/gtee/scripts/Results/workDir/scratch_2/unbound.ssl.policy_new 
\\


If I recall correctly, there should be a way to specify a policy file in 
@run without overriding the default one. May be it is "@run 
main/othervm/java.security.policy=unbound.ssl.policy_new"


Artem


On 08/17/2016 09:53 AM, Seán Coffey wrote:
A recently added test case lacks sufficient permissions to read a conf 
file when running with security manager.


bug report : https://bugs.openjdk.java.net/browse/JDK-8162916

proposed patch :
 diff --git a/test/sun/security/krb5/auto/unbound.ssl.policy 
b/test/sun/security/krb5/auto/unbound.ssl.policy

--- a/test/sun/security/krb5/auto/unbound.ssl.policy
+++ b/test/sun/security/krb5/auto/unbound.ssl.policy
@@ -1,7 +1,13 @@
+// Standard extensions get all permissions by default
+
+grant codeBase "file:${{java.ext.dirs}}/*" {
+permission java.security.AllPermission;
+};
+
 grant {
 permission java.util.PropertyPermission "*", "read,write";
 permission java.net.SocketPermission "*:*", 
"listen,resolve,accept,connect";

-permission java.io.FilePermission "*", "read,write,delete";
+permission java.io.FilePermission "<>", 
"read,write,delete";

 permission java.lang.RuntimePermission "accessDeclaredMembers";
 permission java.lang.reflect.ReflectPermission 
"suppressAccessChecks";

 permission java.lang.RuntimePermission "accessClassInPackage.*";

Re: [9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation

[Michael StJohns]

On 8/16/2016 9:24 PM, Valerie Peng wrote:


Anyone has time to review a straightforward fix? The current PKCS11 
code assume that if public exponent is available for RSA Private Key, 
then it's a RSA CRT key. However, not all vendor implementation works 
this way. Changing to a tighter check and did minor code-refactoring 
to avoid re-retrieving the attribute values.


Bug: https://bugs.openjdk.java.net/browse/JDK-8078661
Webrev: http://cr.openjdk.java.net/~valeriep/8078661/webrev.00/

Thanks,
Valerie


Given that there's a change to PKCS11 for 2.40 that says that all RSA 
private key objects  MUST also store CKA_PUBLIC_EXPONENT, some change 
needed to be made.


Sorry - I don't think this fix will work.  Or if its working on your 
version of PKCS11, your version of PKCS11 is doing it wrong. The problem 
is that if you specify attributes that don't exist on the object, the 
underlying PKCS11 library is supposed to return 
CKR_ATTRIBUTE_TYPE_INVALID.  And that should trigger a thrown exception 
before you ever get anything copied to your attributes.


1) Get modulus and private exponent first.  That gives you the stuff for 
a generic RSA private key - and if it fails, there's no reason to continue.


2) Then get the rest of the stuff.  If that fails, then you already have 
the stuff you need for a normal private key.




  boolean crtKey;
  try {
  session.token.p11.C_GetAttributeValue
  (session.id(), keyID, attrs2);
- crtKey = (attrs2[0].pValue instanceof byte[]);
+ crtKey = ((attrs2[1].pValue instanceof byte[]) &&
+ (attrs2[3].pValue instanceof byte[]) &&
+ (attrs2[4].pValue instanceof byte[]) &&
+ (attrs2[5].pValue instanceof byte[]) &&
+ (attrs2[6].pValue instanceof byte[]) &&
+ (attrs2[7].pValue instanceof byte[])) ;
  } catch (PKCS11Exception e) {
  // ignore, assume not available
  crtKey = false;
  }


// Change attrs2 so it only has the additional CRT attributes (e.g. 
delete CKA_MODULUS, CKA_PRIVATE_EXPONENT from the list


Replace the above with

CK_ATTRIBUTE[] attrs3 = new CK_ATTRIBUTE[] {
   new CK_ATTRIBUTE(CKA_MODULUS),
   new CK_ATTRIBUTE(CKA_PRIVATE_EXPONENT)
};
// no try block needed here - we want to throw the error if it occurs
session.token.p11.C_GetAttributeValue (session.id(), keyID, attrs3);

// So far so good - we have the base attributes, let's see if we can get 
the additional attributes;


try {
session.token.p11.C_GetAttributeValue(session.id(),keyID, attrs2);
} catch (PKCS11Exception e) {
   // we really should check the return value for one of the non-fatal 
values, but let's just assume its not a CRT key
   return new P11RSAPrivateNonCRTKey (session, keyID, algorithm, 
keyLength, attrs2, attrs3);

}

// if we fall through then its a CRT key
// -- we should check for byte[] ness of each of the components, and 
throw an error if they arent - but which error?


return new P11RSAPrivateKey (session, keyID, algorithm, keyLength, 
attrs2, attrs3);


// there are cleanups necessary in other places.  I'd suggest rather 
than depending on the ordering of attributes, you do assignment by CKA_ 
values just so someone coming later doesn't mess things up by mistake.  
Also, a hell of a lot more readable.


static CK_ATTRIBUTE getAttribute (CK_ATTRIBUTE[] attrs, long attrType) {
for (CK_ATTRIBUTE a : attrs) {
 if (a.type == attrType)
  return a;
 }
return null; // or throw something?
}


coeff = getAtttribute(attrs,CKA_COEFFICIENT).getBigInteger();


The other possibility is to change the C code for C_GetAttributeValues 
so it doesn't error out for the non-fatal error codes and instead 
returns a null value attribute when the attribute is missing.


Mike

RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails

[Seán Coffey]

A recently added test case lacks sufficient permissions to read a conf 
file when running with security manager.


bug report : https://bugs.openjdk.java.net/browse/JDK-8162916

proposed patch :
 diff --git a/test/sun/security/krb5/auto/unbound.ssl.policy 
b/test/sun/security/krb5/auto/unbound.ssl.policy

--- a/test/sun/security/krb5/auto/unbound.ssl.policy
+++ b/test/sun/security/krb5/auto/unbound.ssl.policy
@@ -1,7 +1,13 @@
+// Standard extensions get all permissions by default
+
+grant codeBase "file:${{java.ext.dirs}}/*" {
+permission java.security.AllPermission;
+};
+
 grant {
 permission java.util.PropertyPermission "*", "read,write";
 permission java.net.SocketPermission "*:*", 
"listen,resolve,accept,connect";

-permission java.io.FilePermission "*", "read,write,delete";
+permission java.io.FilePermission "<>", "read,write,delete";
 permission java.lang.RuntimePermission "accessDeclaredMembers";
 permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
 permission java.lang.RuntimePermission "accessClassInPackage.*";

--
Regards,
Sean.

Re: RFR: 8164071: Default.policy file missing content for solaris

[Erik Joelsson]

On 2016-08-17 18:43, Sean Mullan wrote:


On 8/17/2016 12:33 PM, Erik Joelsson wrote:

Hello Sean,

The change looks ok, but it could also be expressed like this to 
avoid duplication:


ifneq ($(filter $(OPENJDK_TARGET_OS), windows solaris), )
  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy

endif

Thanks. However, shouldn't that be "ifeq" and not "ifneq"?

Actually no, if OPENJDK_TARGET_OS is either windows or solaris, the 
filter function will return exactly that, otherwise it will return 
empty. The if(n)eq compares the result of filter to the empty string. 
This construct is a bit convoluted but is our standard construct for 
this situation.


/Erik

--Sean



/Erik

On 2016-08-17 18:18, Sean Mullan wrote:
Please review this simple fix to append the solaris default.policy 
file to the overall default.policy file.


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

diff -r 551f7617b2c0 make/copy/Copy-java.base.gmk
--- a/make/copy/Copy-java.base.gmkWed Aug 17 10:08:18 2016 +0800
+++ b/make/copy/Copy-java.base.gmkWed Aug 17 12:17:19 2016 -0400
@@ -185,6 +185,8 @@

 ifeq ($(OPENJDK_TARGET_OS), windows)
   DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy

+else ifeq ($(OPENJDK_TARGET_OS), solaris)
+  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy 


 endif

 # Allow imported modules to modify the java.policy


Thanks,
Sean

Re: RFR: 8164071: Default.policy file missing content for solaris

[Sean Mullan]

On 8/17/2016 12:33 PM, Erik Joelsson wrote:

Hello Sean,

The change looks ok, but it could also be expressed like this to avoid 
duplication:


ifneq ($(filter $(OPENJDK_TARGET_OS), windows solaris), )
  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy

endif

Thanks. However, shouldn't that be "ifeq" and not "ifneq"?

--Sean



/Erik

On 2016-08-17 18:18, Sean Mullan wrote:
Please review this simple fix to append the solaris default.policy 
file to the overall default.policy file.


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

diff -r 551f7617b2c0 make/copy/Copy-java.base.gmk
--- a/make/copy/Copy-java.base.gmkWed Aug 17 10:08:18 2016 +0800
+++ b/make/copy/Copy-java.base.gmkWed Aug 17 12:17:19 2016 -0400
@@ -185,6 +185,8 @@

 ifeq ($(OPENJDK_TARGET_OS), windows)
   DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy

+else ifeq ($(OPENJDK_TARGET_OS), solaris)
+  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy 


 endif

 # Allow imported modules to modify the java.policy


Thanks,
Sean

Re: RFR: 8163126 Wrong @modules in some of jdk/* tests

[Alexandre (Shura) Iline]

Thank you!

Fixed in place:
http://cr.openjdk.java.net/~shurailine/8163126/webrev.00/test/jdk/security/jarsigner/Spec.java.sdiff.html

Shura

> On Aug 16, 2016, at 6:48 PM, Wang Weijun  wrote:
> 
> 
>> On Aug 17, 2016, at 9:26 AM, Alexandre (Shura) Iline 
>>  wrote:
>> 
>> Before the suggested fix, the test in question would fail on a system with 
>> no jdk.crypto.pkcs11. That could be emulated by:
>> $ jtreg ... -javaoptions:"-limitmods jdk.jartool" 
>> jdk/security/jarsigner/Spec.java
>> ...
>> FAILED: jdk/security/jarsigner/Spec.java
>> ...
>> $ jtreg ... -javaoptions:"-limitmods jdk.jartool,jdk.crypto.pkcs11" 
>> jdk/security/jarsigner/Spec.java
>> ...
>> Passed: jdk/security/jarsigner/Spec.java
>> ...
>> $ 
> 
> Thanks very much for the detailed answer.
> 
> Just a small suggestion: Can you try using jdk.crypto.ec module instead? It 
> also provides all EC-related algorithms and it is available and loaded 
> out-of-box on all platforms?
> 
> --Max
> 

Re: RFR: 8164071: Default.policy file missing content for solaris

[Erik Joelsson]

Hello Sean,

The change looks ok, but it could also be expressed like this to avoid 
duplication:


ifneq ($(filter $(OPENJDK_TARGET_OS), windows solaris), )
  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy
endif


/Erik

On 2016-08-17 18:18, Sean Mullan wrote:
Please review this simple fix to append the solaris default.policy 
file to the overall default.policy file.


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

diff -r 551f7617b2c0 make/copy/Copy-java.base.gmk
--- a/make/copy/Copy-java.base.gmkWed Aug 17 10:08:18 2016 +0800
+++ b/make/copy/Copy-java.base.gmkWed Aug 17 12:17:19 2016 -0400
@@ -185,6 +185,8 @@

 ifeq ($(OPENJDK_TARGET_OS), windows)
   DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy

+else ifeq ($(OPENJDK_TARGET_OS), solaris)
+  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy 


 endif

 # Allow imported modules to modify the java.policy


Thanks,
Sean

RFR: 8164071: Default.policy file missing content for solaris

[Sean Mullan]

Please review this simple fix to append the solaris default.policy file 
to the overall default.policy file.


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

diff -r 551f7617b2c0 make/copy/Copy-java.base.gmk
--- a/make/copy/Copy-java.base.gmk  Wed Aug 17 10:08:18 2016 +0800
+++ b/make/copy/Copy-java.base.gmk  Wed Aug 17 12:17:19 2016 -0400
@@ -185,6 +185,8 @@

 ifeq ($(OPENJDK_TARGET_OS), windows)
   DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy

+else ifeq ($(OPENJDK_TARGET_OS), solaris)
+  DEF_POLICY_SRC_LIST += 
$(JDK_TOPDIR)/src/java.base/$(OPENJDK_TARGET_OS)/lib/security/default.policy 


 endif

 # Allow imported modules to modify the java.policy


Thanks,
Sean