Re: [8u-dev] Request for Approval: Backport of 8144566: Custom HostnameVerifier disables SNI extension
Changes look good Ramanand. Reviewed. Regards, Sean. On 18/08/2016 07:03, David Buck wrote: Hi Ramanand! As there are (minor) changes between the two change sets, you will need to get a code review of the backported changes. I have included the security-dev alias in the CC list. Cheers, -Buck On Aug 18, 2016, at 14:34, Ramanand Patil wrote: Hi, Please review and approve the backport of 8144566 to 8u-dev. Bug: https://bugs.openjdk.java.net/browse/JDK-8144566 JDK9 Changeset: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/1781aba4f7e3 JDK9 Review Thread: http://mail.openjdk.java.net/pipermail/security-dev/2015-December/013171.html JDK8u-dev Webrev: http://cr.openjdk.java.net/~rpatil/8144566/webrev.00/ Changes apply semi-cleanly to jdk8u-dev after path reshuffling. Below are few reasons for manual edits in the jdk8u-dev patch: 1. JDK9 class name for "JavaNetAccess" is changed to " JavaNetInetAddressAccess". 2. test/javax/net/ssl/ServerName/BestEffortOnLazyConnected.java pathToStores(keystore path) doesn't exist in JDK8u, hence changed the variable to point to available path. 3. test/sun/net/www/protocol/https/HttpsURLConnection/ImpactOnSNI.java pathToStores(keystore path) doesn't exist in JDK8u, hence changed the variable to point to available path. Regards, Ramanand.
Re: RFR: 9: 8164229: Redundant "sun/net/www/protocol/https" tests in jdk_security3 group
> On 17 Aug 2016, at 19:52, Rajan Halade wrote: > > 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). Ok, thanks. No objection from me. -Chris.
Re: RFR: 9: 8164229: Redundant "sun/net/www/protocol/https" tests in jdk_security3 group
> On 17 Aug 2016, at 19:52, Rajan Halade wrote: > > 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). Ok, thanks. No objection from me. -Chris.
Re: RFR [9] 8156841: sun.security.pkcs11.SunPKCS11 poller thread retains a strong reference to the context class loader
Hi Chris,
I agree that resetting the TCCL makes sense, and looks like the
minimal safer thing to do here.
InnocuousThread should probably be considered for the future,
but that's a first step in this direction :-)
best regards,
-- daniel
On 18/08/16 08:58, Chris Hegarty wrote:
The SunPKCS11 poller thread has no need of any user defined class loader,
so should set its context class loader to null before starting, so as to not
inadvertently retain a reference to the creating thread’s context class loader.
In other areas that suffered from a similar issue we changed to use an
InnocuousThread, but I cannot fully satisfy myself that this is a safe
substation here, so I opted for the safest minimal fix. A future refactoring
exercise should consider using InnocuousThread.
diff -r 92c31ec731eb
src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java
--- a/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java
Wed Aug 10 11:54:12 2016 +0100
+++ b/src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/SunPKCS11.java
Wed Aug 10 16:32:41 2016 +0100
@@ -809,20 +809,21 @@
}
}
// create the poller thread, if not already active
private void createPoller() {
if (poller != null) {
return;
}
final TokenPoller poller = new TokenPoller(this);
Thread t = new Thread(null, poller, "Poller " + getName(), 0, false);
+t.setContextClassLoader(null);
t.setDaemon(true);
t.setPriority(Thread.MIN_PRIORITY);
t.start();
this.poller = poller;
}
// destroy the poller thread, if active
private void destroyPoller() {
if (poller != null) {
poller.disable();
https://bugs.openjdk.java.net/browse/JDK-8156841
-Chris.
Re: RFR: 8163126 Wrong @modules in some of jdk/* tests
On 17/08/2016 17:40, Alexandre (Shura) Iline wrote: Thank you! Fixed in place: http://cr.openjdk.java.net/~shurailine/8163126/webrev.00/test/jdk/security/jarsigner/Spec.java.sdiff.html Shura The updated patch looks good to me. Slightly off-topic but how close do you think we are to dropping completely the test groups in the TEST.groups that are defined under the heading "Profile-based Test Group Definition"? -Alan
Re: RFR: 8061842: Package jurisdiction policy files as something other than JAR
Hi Brad,
nice to have this going in. Some comments.
1. Bug synopsis, can you edit it perhaps. "Introduce security property
to control strong crypto" seems more descriptive.
2. Exception handling.
Alot of your new exceptions don't include context. That makes debugging
more difficult than needs be.
+throw new SecurityException(
+"Invalid cryptographic jurisdiction policy directory value");
+if (!Files.isDirectory(path) || !Files.isReadable(path)) {
+throw new Exception("Can't read cryptographic policy directory");
+throw new SecurityException(
+"Unexpected jurisdiction policy filename found");
+} catch (Exception e) {
+throw new SecurityException(
+"Couldn't parse jurisdiction policy files");
Please include the exception 'e' in your last exception here.
+} catch (DirectoryIteratorException ex) {
+// I/O error encountered during the iteration,
+// the cause is an IOException
+throw new SecurityException(
+"Couldn't iterate through the jurisdiction policy files");
+}
Can you add the DirectoryIteratorException to the final exception that
you're throwing ?
3. Test case.
The TestUnlimited.java testcase seems to be lacking. Do you want to test
other values for crypto.policy ? 'limited' would be one. Throwing in
some dummy value would also be good so that the exception handling code
gets exercised.
It needs to be run in ovm mode since you're setting a Security property.
Regards,
Sean.
On 18/08/2016 05:26, Wang Weijun wrote:
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: RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails
Thanks for the tip Artem, Max. No need to modify the policy file then.
Below is the new suggested patch for jdk8u-dev. JPRT results are good.
diff --git a/test/sun/security/krb5/auto/UnboundSSL.java
b/test/sun/security/krb5/auto/UnboundSSL.java
--- a/test/sun/security/krb5/auto/UnboundSSL.java
+++ b/test/sun/security/krb5/auto/UnboundSSL.java
@@ -34,9 +34,9 @@
* @bug 8025123
* @summary Checks if an unbound server can handle connections
* only for allowed service principals
- * @run main/othervm/policy=unbound.ssl.policy UnboundSSL
+ * @run main/othervm/java.security.policy=unbound.ssl.policy UnboundSSL
* unbound.ssl.jaas.conf server_star
- * @run main/othervm/policy=unbound.ssl.policy UnboundSSL
+ * @run main/othervm/java.security.policy=unbound.ssl.policy UnboundSSL
* unbound.ssl.jaas.conf
server_multiple_principals
*/
public class UnboundSSL {
Regards,
Sean.
On 18/08/2016 04:11, Weijun Wang wrote:
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
On 8/17/2016 11:36 PM, Valerie Peng wrote:
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
Hi Valerie -
You know - re-reading this code I'm reminding of why PKCS11 annoys me so
much.
At line 333 (of the "new" P11Key) you grab the Token, Sensitive and
Extractable values and if the private data is sensitive or not
extractable you create a generic P11PrivateKey and return that. However
the contract for RSAKey requires that the public modulus be returned if
available, and, since its not a sensitive attribute it probably should
be available. Also, even if the key is sensitive - if its a sensitive
CRT key, then CKA_PUBLIC_EXPONENT should be available.
That's going to be a surprise if someone tries to cast this return to an
(RSAKey) or (RSAPrivateKey). _This should be changed so a key of the
appropriate type is always created._
Also, checking for CKA_EXTRACTABLE being true, doesn't actually get you
access to the clear text information. If a key is extractable, then it
can be wrapped out under another key. The components themselves aren't
available. It's possible to have a non-sensitive, non-extractable key
where the components are retrievable, but the key can't be wrapped out.
(Hmm... the public exponent is in RSAPublicKey and RSAPrivateCRTKey, but
should probably be in RSAKey instead).
So:
All RSA keys - even the sensitive private ones - should return CKA_MODULUS.
All RSA Private CRT Keys - even the sensitive ones - should also return
CKA_PUBLIC_EXPONENT.
All non-sensitive RSA Private keys - should also return CKA_PRIVATE_EXPONENT
All non-sensitive RSA Private CRT Keys - should also return CKA_PRIME_1,
CKA_PRIME_2, CKA_EXPONENT_1, CKA_EXPONENT_2 and CKA_COEFFICIENT.
This is harder to do than it needs to be due to how
p11_objmgt.c::Java_sun_security_pkcs11_wrapper_PKCS11_C_1GetAttributeValue
is built. At lines 248 and 270, it does a check for an error return and
throws an exception if any error occurs. However, for
C_GetAttributeValue, there are a number of "non-fatal" errors that
indicate either buffer size errors or sensitivity of one or more
components or unavailability of one or more components.
Note that the error codes CKR_ATTRIBUTE_SENSITIVE,
CKR_ATTRIBUTE_TYPE_INVALID, and CKR_BUFFER_TOO_SMALL do not denote
true errors for *C_GetAttributeValue*. If a call to
*C_GetAttributeValue* returns any of these three values, then the call
must nonetheless have processed /every/ attribute in the template
supplied to *C_GetAttributeValue*. Each attribute in the template
whose value /can be/ returned by the call to *C_GetAttributeValue*
/will be/ returned by the call to *C_GetAttributeValue*.
If you updated this slightly - maybe by adding a new method to
wrapper.PKCS11 (say GetAttributeValuesNoError) - to return the
attributes it was able to get in the call with nulls elsewhere, then you
could do all of the above in one pass.
Sorry to complicate this. Mike
ps - I don't have a current build environment set up for the JDK,
otherwise I'd code it and test it myself. I'm happy to take a swing at
it and provide you unverified code you can integrate.
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 i
RSA Key Interfaces Was: Re: [9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation
Hi - Looking at Valeries changes to the above made me take a closer look at the current definitions of the various RSA key interfaces. What would be the impact of the following changes?: Make RSAMultiPrimePrivateCrtKeySpec extend RSAPrivateCrtKeyKeySpec instead of RSAPrivateKeySpec. RSAMultiPrimePrivateCrtKeySpec would then only define the RSAOtherPrimeInfo[] getOtherPrimeInfo() method. Ditto for RSAMultiPrimePrivateCrtKey and RSAPrivateCrtKey Move getPublicExponent() from RSAPublicKey and RSAPrivateCrtKey to RSAKey. Ditto for the Spec versions. I think the first two changes can be done without adverse impact and are more correct than the current definitions. The latter two are more problematic, but can probably be handled using the default method mechanism. The API documents would indicate that these functions would return null if the values are unavailable. Mike
Re: RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails
Hi Sean,
The patch below looks fine to me, but I am not an official reviewer.
Artem
On 08/18/2016 08:28 AM, Seán Coffey wrote:
Thanks for the tip Artem, Max. No need to modify the policy file then.
Below is the new suggested patch for jdk8u-dev. JPRT results are good.
diff --git a/test/sun/security/krb5/auto/UnboundSSL.java
b/test/sun/security/krb5/auto/UnboundSSL.java
--- a/test/sun/security/krb5/auto/UnboundSSL.java
+++ b/test/sun/security/krb5/auto/UnboundSSL.java
@@ -34,9 +34,9 @@
* @bug 8025123
* @summary Checks if an unbound server can handle connections
* only for allowed service principals
- * @run main/othervm/policy=unbound.ssl.policy UnboundSSL
+ * @run main/othervm/java.security.policy=unbound.ssl.policy UnboundSSL
* unbound.ssl.jaas.conf server_star
- * @run main/othervm/policy=unbound.ssl.policy UnboundSSL
+ * @run main/othervm/java.security.policy=unbound.ssl.policy UnboundSSL
* unbound.ssl.jaas.conf
server_multiple_principals
*/
public class UnboundSSL {
Regards,
Sean.
On 18/08/2016 04:11, Weijun Wang wrote:
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: RFR: 9: 8164229: Redundant "sun/net/www/protocol/https" tests in jdk_security3 group
This was probably more important when the Security group had direct responsibility for the https code. Brad On 8/18/2016 12:51 AM, Chris Hegarty wrote: On 17 Aug 2016, at 19:52, Rajan Halade wrote: 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). Ok, thanks. No objection from me. -Chris.
RE: [8u-dev] Request for Approval: Backport of 8144566: Custom HostnameVerifier disables SNI extension
Thank you Sean and David. Regards, Ramanand. -Original Message- From: david buck Sent: Thursday, August 18, 2016 1:26 PM To: Seán Coffey; Ramanand Patil Cc: jdk8u-dev; [email protected] Subject: Re: [8u-dev] Request for Approval: Backport of 8144566: Custom HostnameVerifier disables SNI extension approved for push to 8u-dev Cheers, -Buck On 2016/08/18 16:51, Seán Coffey wrote: > Changes look good Ramanand. Reviewed. > > Regards, > Sean. > > On 18/08/2016 07:03, David Buck wrote: >> Hi Ramanand! >> >> As there are (minor) changes between the two change sets, you will >> need to get a code review of the backported changes. I have included >> the security-dev alias in the CC list. >> >> Cheers, >> -Buck >> >>> On Aug 18, 2016, at 14:34, Ramanand Patil >>> >>> wrote: >>> >>> Hi, >>> >>> Please review and approve the backport of 8144566 to 8u-dev. >>> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8144566 >>> >>> JDK9 Changeset: >>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/1781aba4f7e3 >>> >>> JDK9 Review Thread: >>> http://mail.openjdk.java.net/pipermail/security-dev/2015-December/01 >>> 3171.html >>> >>> >>> JDK8u-dev Webrev: >>> http://cr.openjdk.java.net/~rpatil/8144566/webrev.00/ >>> >>> Changes apply semi-cleanly to jdk8u-dev after path reshuffling. >>> Below are few reasons for manual edits in the jdk8u-dev patch: >>> >>> 1. JDK9 class name for "JavaNetAccess" is changed to " >>> JavaNetInetAddressAccess". >>> 2. test/javax/net/ssl/ServerName/BestEffortOnLazyConnected.java >>> pathToStores(keystore path) doesn't exist in JDK8u, hence changed >>> the variable to point to available path. >>> 3. >>> test/sun/net/www/protocol/https/HttpsURLConnection/ImpactOnSNI.java >>> pathToStores(keystore path) doesn't exist in JDK8u, hence changed >>> the variable to point to available path. >>> >>> >>> Regards, >>> Ramanand. >
Re: [8u-dev] Request for Approval: Backport of 8144566: Custom HostnameVerifier disables SNI extension
Hi Ramanand! As there are (minor) changes between the two change sets, you will need to get a code review of the backported changes. I have included the security-dev alias in the CC list. Cheers, -Buck > On Aug 18, 2016, at 14:34, Ramanand Patil wrote: > > Hi, > > Please review and approve the backport of 8144566 to 8u-dev. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8144566 > > JDK9 Changeset: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/1781aba4f7e3 > > JDK9 Review Thread: > http://mail.openjdk.java.net/pipermail/security-dev/2015-December/013171.html > > JDK8u-dev Webrev: http://cr.openjdk.java.net/~rpatil/8144566/webrev.00/ > > Changes apply semi-cleanly to jdk8u-dev after path reshuffling. Below are few > reasons for manual edits in the jdk8u-dev patch: > > 1. JDK9 class name for "JavaNetAccess" is changed to " > JavaNetInetAddressAccess". > 2. test/javax/net/ssl/ServerName/BestEffortOnLazyConnected.java > pathToStores(keystore path) doesn't exist in JDK8u, hence changed the > variable to point to available path. > 3. test/sun/net/www/protocol/https/HttpsURLConnection/ImpactOnSNI.java > pathToStores(keystore path) doesn't exist in JDK8u, hence changed the > variable to point to available path. > > > Regards, > Ramanand.
Re: [8u-dev] Request for Approval: Backport of 8144566: Custom HostnameVerifier disables SNI extension
approved for push to 8u-dev Cheers, -Buck On 2016/08/18 16:51, Seán Coffey wrote: Changes look good Ramanand. Reviewed. Regards, Sean. On 18/08/2016 07:03, David Buck wrote: Hi Ramanand! As there are (minor) changes between the two change sets, you will need to get a code review of the backported changes. I have included the security-dev alias in the CC list. Cheers, -Buck On Aug 18, 2016, at 14:34, Ramanand Patil wrote: Hi, Please review and approve the backport of 8144566 to 8u-dev. Bug: https://bugs.openjdk.java.net/browse/JDK-8144566 JDK9 Changeset: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/1781aba4f7e3 JDK9 Review Thread: http://mail.openjdk.java.net/pipermail/security-dev/2015-December/013171.html JDK8u-dev Webrev: http://cr.openjdk.java.net/~rpatil/8144566/webrev.00/ Changes apply semi-cleanly to jdk8u-dev after path reshuffling. Below are few reasons for manual edits in the jdk8u-dev patch: 1. JDK9 class name for "JavaNetAccess" is changed to " JavaNetInetAddressAccess". 2. test/javax/net/ssl/ServerName/BestEffortOnLazyConnected.java pathToStores(keystore path) doesn't exist in JDK8u, hence changed the variable to point to available path. 3. test/sun/net/www/protocol/https/HttpsURLConnection/ImpactOnSNI.java pathToStores(keystore path) doesn't exist in JDK8u, hence changed the variable to point to available path. Regards, Ramanand.
Re: RFR: 8061842: Package jurisdiction policy files as something other than JAR
On 08/17/2016 07:22 PM, Bradford Wetmore wrote:
- 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.
Ok, but those are property names.
What about setting the default value to "limited"? And then this would
only be changed to "unlimited" if the build --enable-unlimited-crypto
option is specified?
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().
253 // Sanity check the crypto.policy Security property. Single
254 // directory entry, no pseudo- or subdirectories.
255 String cryptoPolicyDir =
Security.getProperty("crypto.policy");
256
257 if (cryptoPolicyDir == null) {
258 throw new SecurityException(
259 "No cryptographic jurisdiction policy directory
value");
260 }
Instead of throwing an exception here, I wonder if it would make more
sense to assume a default value of "limited" if the property is not set
or is empty.
--Sean
Re: [9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation
Hi Mike, Thanks for the feedback and the detailed write up. The scenario here is complicated by the sensitive/non-extractable keys of PKCS#11 and the fact that java key and key specification classes assume all relevant values being available. Only when all relevant values are available, then we will construct the corresponding key objects. This is necessary as there are other providers which may receive such keys and they can't handle keys like this. I am sure that the current PKCS11 provider code needs many improvement/finer handlings. But I don't see a straightforward way of "making CKA_PUBLIC_EXPONENT available" across various RSA Key classes. This should be tracked in a different issue. Given the current release schedule, the deadline for this fix (P4) is coming up in 10 days and I will be on vacation next week. If you agree with the value of addressing this with the proposed changes for JDK 9, then we can proceed. Otherwise, I will defer this bug to the update release and we can spend more time to polish this. Valerie On 8/18/2016 8:40 AM, Michael StJohns wrote: On 8/17/2016 11:36 PM, Valerie Peng wrote: 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 Hi Valerie - You know - re-reading this code I'm reminding of why PKCS11 annoys me so much. At line 333 (of the "new" P11Key) you grab the Token, Sensitive and Extractable values and if the private data is sensitive or not extractable you create a generic P11PrivateKey and return that. However the contract for RSAKey requires that the public modulus be returned if available, and, since its not a sensitive attribute it probably should be available. Also, even if the key is sensitive - if its a sensitive CRT key, then CKA_PUBLIC_EXPONENT should be available. That's going to be a surprise if someone tries to cast this return to an (RSAKey) or (RSAPrivateKey). _This should be changed so a key of the appropriate type is always created._ Also, checking for CKA_EXTRACTABLE being true, doesn't actually get you access to the clear text information. If a key is extractable, then it can be wrapped out under another key. The components themselves aren't available. It's possible to have a non-sensitive, non-extractable key where the components are retrievable, but the key can't be wrapped out. (Hmm... the public exponent is in RSAPublicKey and RSAPrivateCRTKey, but should probably be in RSAKey instead). So: All RSA keys - even the sensitive private ones - should return CKA_MODULUS. All RSA Private CRT Keys - even the sensitive ones - should also return CKA_PUBLIC_EXPONENT. All non-sensitive RSA Private keys - should also return CKA_PRIVATE_EXPONENT All non-sensitive RSA Private CRT Keys - should also return CKA_PRIME_1, CKA_PRIME_2, CKA_EXPONENT_1, CKA_EXPONENT_2 and CKA_COEFFICIENT. This is harder to do than it needs to be due to how p11_objmgt.c::Java_sun_security_pkcs11_wrapper_PKCS11_C_1GetAttributeValue is built. At lines 248 and 270, it does a check for an error return and throws an exception if any error occurs. However, for C_GetAttributeValue, there are a number of "non-fatal" errors that indicate either buffer size errors or sensitivity of one or more components or unavailability of one or more components. Note that the error codes CKR_ATTRIBUTE_SENSITIVE, CKR_ATTRIBUTE_TYPE_INVALID, and CKR_BUFFER_TOO_SMALL do not denote true errors for *C_GetAttributeValue*. If a call to *C_GetAttributeValue* returns any of these three values, then the call must nonetheless have processed /every/ attribute in the template supplied to *C_GetAttributeValue*. Each attribute in the template whose value /can be/ returned by the call to *C_GetAttributeValue* /will be/ returned by the call to *C_GetAttributeValue*. If you updated this slightly - maybe by adding a new method to wrapper.PKCS11 (say GetAttributeValuesNoError) - to return the attributes it was able to get in the call with nulls elsewhere, then you could do all of the above in one pass. Sorry to complicate this. Mike ps - I don't have a current build environment set up for the JDK, otherwise I'd code it and test it myself. I'm happy to take a swing at it and provide you unverified code you can integrate. 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
[9] RFR 8164398: Add test sun/security/krb5/auto/EmptyPassword.java to ProblemList
Please approve this change to add a failing test to jdk/test/ProblemList.txt so we can investigate further. Thanks. diff --git a/test/ProblemList.txt b/test/ProblemList.txt --- a/test/ProblemList.txt +++ b/test/ProblemList.txt @@ -289,6 +289,8 @@ sun/security/ssl/SSLSocketImpl/AsyncSSLSocketClose.java 8161232 macosx-all +sun/security/krb5/auto/EmptyPassword.java 8164307 solaris-all + # jdk_sound
Re: [9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation
On 8/18/2016 4:49 PM, Valerie Peng wrote: Hi Mike, Thanks for the feedback and the detailed write up. The scenario here is complicated by the sensitive/non-extractable keys of PKCS#11 and the fact that java key and key specification classes assume all relevant values being available. Um... I think that's true for any PublicKey, but not for Secret or Private keys. At worst, the Key object is a handle for the real key that contains all those items, but they might not be available. At best, most of those components will be available. I say at best, because of the language in RSAMultiPrimePrivateCRTKey for getOtherPrimeInfo() which says it can return null. Only when all relevant values are available, then we will construct the corresponding key objects. This is necessary as there are other providers which may receive such keys and they can't handle keys like this. Keys can't generally move across providers AIRC? You can try and use a key factory to convert them, but that's not guaranteed. A PKCS11 derived key isn't going to be portable to another provider without extraction to a keyspec in any case. Hmm.. I went back and read the JDK8 p11 guide (https://docs.oracle.com/javase/8/docs/technotes/guides/security/p11guide.html) and section 3.2 gives the guidance that you only use the generic interfaces for unextractable keys. I actually think that's wrong, given the general guidance in the JCA documentation with respect to Opaque Keys vs transparent KeySpecs. (Hmm... I wonder if this guidance was in the originally submitted code package documentation). Then there's the point that even a generic Public or Private key has a "getEncoded()" method. *bleah* I am sure that the current PKCS11 provider code needs many improvement/finer handlings. But I don't see a straightforward way of "making CKA_PUBLIC_EXPONENT available" across various RSA Key classes. This should be tracked in a different issue. That's just one of the items. As I mentioned in another email, I think the RSA key classes and interfaces need a bit more work and tweaking. I wouldn't try and accomplish that quite yet. Given the current release schedule, the deadline for this fix (P4) is coming up in 10 days and I will be on vacation next week. If you agree with the value of addressing this with the proposed changes for JDK 9, then we can proceed. Otherwise, I will defer this bug to the update release and we can spend more time to polish this. I think you might as well go ahead with this change. The fix you've got should work as long as someone who generates a RSA Key pair on a PKCS11 which is both sensitive and unextractable doesn't try to cast the keys to RSAPublic or RSAPrivate. Mike Valerie On 8/18/2016 8:40 AM, Michael StJohns wrote: On 8/17/2016 11:36 PM, Valerie Peng wrote: 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 Hi Valerie - You know - re-reading this code I'm reminding of why PKCS11 annoys me so much. At line 333 (of the "new" P11Key) you grab the Token, Sensitive and Extractable values and if the private data is sensitive or not extractable you create a generic P11PrivateKey and return that. However the contract for RSAKey requires that the public modulus be returned if available, and, since its not a sensitive attribute it probably should be available. Also, even if the key is sensitive - if its a sensitive CRT key, then CKA_PUBLIC_EXPONENT should be available. That's going to be a surprise if someone tries to cast this return to an (RSAKey) or (RSAPrivateKey). _This should be changed so a key of the appropriate type is always created._ Also, checking for CKA_EXTRACTABLE being true, doesn't actually get you access to the clear text information. If a key is extractable, then it can be wrapped out under another key. The components themselves aren't available. It's possible to have a non-sensitive, non-extractable key where the components are retrievable, but the key can't be wrapped out. (Hmm... the public exponent is in RSAPublicKey and RSAPrivateCRTKey, but should probably be in RSAKey instead). So: All RSA keys - even the sensitive private ones - should return CKA_MODULUS. All RSA Private CRT Keys - even the sensitive ones - should also return CKA_PUBLIC_EXPONENT. All non-sensitive RSA Private keys - should also return CKA_PRIVATE_EXPONENT All non-sensitive RSA Private CRT Keys - should also return CKA_PRIME_1, CKA_PRIME_2, CKA_EXPONENT_1, CKA_EXPONENT_2 and CKA_COEFFICIENT. This is harder to do than it needs to be due to how p11_objmgt.c::Java_sun_security_pkcs11_wrapper_PKCS11_C_1GetAttributeValue is built. At lines 248 and 270, it does a check for an error return and throws an exception if any error occurs. However, for C_GetAttributeValue
Re: [9] RFR: 8164100: com/sun/crypto/provider/KeyFactory/TestProviderLeak.java fails with java.util.concurrent.TimeoutException
Changes look fine. Valerie On 8/17/2016 11:29 AM, Artem Smotrakov wrote: 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
Re: [9] RFR 8078661: [SunPKCS11] Fails to cast into RSAPrivateCrtKey after RSA KeyPair Generation
I share your view on most things. It's just that the APIs are there before the PKCS11 provider is added. So, there are some history reason as to why things are as they are today. Re-structuring the public classes are almost impossible considering the compatibility impact. However, we can explore other functional fixes if necessary. Thanks for the review and feedback, it's very helpful, Valerie On 8/18/2016 2:25 PM, Michael StJohns wrote: On 8/18/2016 4:49 PM, Valerie Peng wrote: Hi Mike, Thanks for the feedback and the detailed write up. The scenario here is complicated by the sensitive/non-extractable keys of PKCS#11 and the fact that java key and key specification classes assume all relevant values being available. Um... I think that's true for any PublicKey, but not for Secret or Private keys. At worst, the Key object is a handle for the real key that contains all those items, but they might not be available. At best, most of those components will be available. I say at best, because of the language in RSAMultiPrimePrivateCRTKey for getOtherPrimeInfo() which says it can return null. Only when all relevant values are available, then we will construct the corresponding key objects. This is necessary as there are other providers which may receive such keys and they can't handle keys like this. Keys can't generally move across providers AIRC? You can try and use a key factory to convert them, but that's not guaranteed. A PKCS11 derived key isn't going to be portable to another provider without extraction to a keyspec in any case. Hmm.. I went back and read the JDK8 p11 guide (https://docs.oracle.com/javase/8/docs/technotes/guides/security/p11guide.html) and section 3.2 gives the guidance that you only use the generic interfaces for unextractable keys. I actually think that's wrong, given the general guidance in the JCA documentation with respect to Opaque Keys vs transparent KeySpecs. (Hmm... I wonder if this guidance was in the originally submitted code package documentation). Then there's the point that even a generic Public or Private key has a "getEncoded()" method. *bleah* I am sure that the current PKCS11 provider code needs many improvement/finer handlings. But I don't see a straightforward way of "making CKA_PUBLIC_EXPONENT available" across various RSA Key classes. This should be tracked in a different issue. That's just one of the items. As I mentioned in another email, I think the RSA key classes and interfaces need a bit more work and tweaking. I wouldn't try and accomplish that quite yet. Given the current release schedule, the deadline for this fix (P4) is coming up in 10 days and I will be on vacation next week. If you agree with the value of addressing this with the proposed changes for JDK 9, then we can proceed. Otherwise, I will defer this bug to the update release and we can spend more time to polish this. I think you might as well go ahead with this change. The fix you've got should work as long as someone who generates a RSA Key pair on a PKCS11 which is both sensitive and unextractable doesn't try to cast the keys to RSAPublic or RSAPrivate. Mike Valerie On 8/18/2016 8:40 AM, Michael StJohns wrote: On 8/17/2016 11:36 PM, Valerie Peng wrote: 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 Hi Valerie - You know - re-reading this code I'm reminding of why PKCS11 annoys me so much. At line 333 (of the "new" P11Key) you grab the Token, Sensitive and Extractable values and if the private data is sensitive or not extractable you create a generic P11PrivateKey and return that. However the contract for RSAKey requires that the public modulus be returned if available, and, since its not a sensitive attribute it probably should be available. Also, even if the key is sensitive - if its a sensitive CRT key, then CKA_PUBLIC_EXPONENT should be available. That's going to be a surprise if someone tries to cast this return to an (RSAKey) or (RSAPrivateKey). _This should be changed so a key of the appropriate type is always created._ Also, checking for CKA_EXTRACTABLE being true, doesn't actually get you access to the clear text information. If a key is extractable, then it can be wrapped out under another key. The components themselves aren't available. It's possible to have a non-sensitive, non-extractable key where the components are retrievable, but the key can't be wrapped out. (Hmm... the public exponent is in RSAPublicKey and RSAPrivateCRTKey, but should probably be in RSAKey instead). So: All RSA keys - even the sensitive private ones - should return CKA_MODULUS. All RSA Private CRT Keys - even the sensitive ones - should also return CKA_PUBLIC_EXPONENT. All non-sensitive RSA Private keys - should also
Re: [9] RFR 8164398: Add test sun/security/krb5/auto/EmptyPassword.java to ProblemList
Looks fine to me. Xuelei On 8/19/2016 5:17 AM, Vincent Ryan wrote: Please approve this change to add a failing test to jdk/test/ProblemList.txt so we can investigate further. Thanks. diff --git a/test/ProblemList.txt b/test/ProblemList.txt --- a/test/ProblemList.txt +++ b/test/ProblemList.txt @@ -289,6 +289,8 @@ sun/security/ssl/SSLSocketImpl/AsyncSSLSocketClose.java 8161232 macosx-all +sun/security/krb5/auto/EmptyPassword.java 8164307 solaris-all + # jdk_sound
Re: RFR: (XS) 8162916:Test sun/security/krb5/auto/UnboundSSL.java fails
This is great. Change looks fine to me.
Thanks
Max
On 8/18/2016 23:28, Seán Coffey wrote:
Thanks for the tip Artem, Max. No need to modify the policy file then.
Below is the new suggested patch for jdk8u-dev. JPRT results are good.
diff --git a/test/sun/security/krb5/auto/UnboundSSL.java
b/test/sun/security/krb5/auto/UnboundSSL.java
--- a/test/sun/security/krb5/auto/UnboundSSL.java
+++ b/test/sun/security/krb5/auto/UnboundSSL.java
@@ -34,9 +34,9 @@
* @bug 8025123
* @summary Checks if an unbound server can handle connections
* only for allowed service principals
- * @run main/othervm/policy=unbound.ssl.policy UnboundSSL
+ * @run main/othervm/java.security.policy=unbound.ssl.policy UnboundSSL
* unbound.ssl.jaas.conf server_star
- * @run main/othervm/policy=unbound.ssl.policy UnboundSSL
+ * @run main/othervm/java.security.policy=unbound.ssl.policy UnboundSSL
* unbound.ssl.jaas.conf
server_multiple_principals
*/
public class UnboundSSL {
Regards,
Sean.
On 18/08/2016 04:11, Weijun Wang wrote:
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
RFR 8164437: Test for JDK-8042900
Please review the code change at http://cr.openjdk.java.net/~weijun/8164437/webrev.00 The updated test shows that with the jdk.security.jgss package, the result of GSSManager::createContext is of an enhanced type. Thanks Max
