Re: RFR 8171190: Bump reference of NIST 800-57 Part 1 Rev 3 to Rev 4 in JarSigner API spec

[Xuelei Fan]

Looks fine to me.

Xuelei

On 12/13/2016 6:09 PM, Wang Weijun wrote:

NIST 800-57 Part 1 has a new revision. The lines below are newly introduced in 
jdk9.

diff --git a/src/java.base/share/classes/sun/security/x509/AlgorithmId.java 
b/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
--- a/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
+++ b/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
@@ -1024,7 +1024,7 @@
 }
 }

-// Values from SP800-57 part 1 rev 3 tables 2 and three
+// Values from SP800-57 part 1 rev 4 tables 2 and 3
 private static String ecStrength (int bitLength) {
 if (bitLength >= 512) { // 256 bits of strength
 return "SHA512";
@@ -1035,7 +1035,7 @@
 }
 }

-// same values for RSA and DSA
+// Same values for RSA and DSA
 private static String ifcFfcStrength (int bitLength) {
 if (bitLength > 7680) { // 256 bits
 return "SHA512";
diff --git 
a/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java 
b/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
--- a/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
+++ b/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
@@ -430,7 +430,7 @@
  * SHA384withECDSA for a 384-bit EC key.
  *
  * @implNote This implementation makes use of comparable strengths
- * as defined in Tables 2 and 3 of NIST SP 800-57 Part 1-Rev.3.
+ * as defined in Tables 2 and 3 of NIST SP 800-57 Part 1-Rev.4.
  * Specifically, if a DSA or RSA key with a key size greater than 7680
  * bits, or an EC key with a key size greater than or equal to 512 
bits,
  * SHA-512 will be used as the hash function for the signature.

Thanks
Max

Re: RFR 8168979: @implNote for invalid FilePermission

[Wang Weijun]

> On Dec 14, 2016, at 10:11 AM, Xuelei Fan  wrote:
> 
> On 12/13/2016 5:45 PM, Wang Weijun wrote:
>> A major behavior change is that <> now implies an invalid 
>> permission, I hope this is good to minimize incompatibility.
> Looks like two sides of the same coin.  If there is an invalid <> 
> (not existing in practice, I think),

Not existing in theory either. As the change says, invalid happens when the 
conversion of string to NIO Path fails. For <>, there will be no 
such conversion.

> it now implies all;  if no change on this point, <> does not imply 
> invalid one.  The existing behavior looks more safe to me.  What's you 
> concern to make this behavior change?

Just safer. Sometimes an app can recover from a FileNotExistException but not a 
SecurityException.

Thanks
Max

> 
> Otherwise, looks fine to me.
> 
> Xuelei

Re: RFR 8168979: @implNote for invalid FilePermission

[Xuelei Fan]

On 12/13/2016 5:45 PM, Wang Weijun wrote:

A major behavior change is that <> now implies an invalid 
permission, I hope this is good to minimize incompatibility.
Looks like two sides of the same coin.  If there is an invalid > (not existing in practice, I think), it now implies all;  if no 
change on this point, <> does not imply invalid one.  The 
existing behavior looks more safe to me.  What's you concern to make 
this behavior change?


Otherwise, looks fine to me.

Xuelei

RFR 8171190: Bump reference of NIST 800-57 Part 1 Rev 3 to Rev 4 in JarSigner API spec

[Wang Weijun]

NIST 800-57 Part 1 has a new revision. The lines below are newly introduced in 
jdk9.

diff --git a/src/java.base/share/classes/sun/security/x509/AlgorithmId.java 
b/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
--- a/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
+++ b/src/java.base/share/classes/sun/security/x509/AlgorithmId.java
@@ -1024,7 +1024,7 @@
 }
 }

-// Values from SP800-57 part 1 rev 3 tables 2 and three
+// Values from SP800-57 part 1 rev 4 tables 2 and 3
 private static String ecStrength (int bitLength) {
 if (bitLength >= 512) { // 256 bits of strength
 return "SHA512";
@@ -1035,7 +1035,7 @@
 }
 }

-// same values for RSA and DSA
+// Same values for RSA and DSA
 private static String ifcFfcStrength (int bitLength) {
 if (bitLength > 7680) { // 256 bits
 return "SHA512";
diff --git 
a/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java 
b/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
--- a/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
+++ b/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
@@ -430,7 +430,7 @@
  * SHA384withECDSA for a 384-bit EC key.
  *
  * @implNote This implementation makes use of comparable strengths
- * as defined in Tables 2 and 3 of NIST SP 800-57 Part 1-Rev.3.
+ * as defined in Tables 2 and 3 of NIST SP 800-57 Part 1-Rev.4.
  * Specifically, if a DSA or RSA key with a key size greater than 7680
  * bits, or an EC key with a key size greater than or equal to 512 
bits,
  * SHA-512 will be used as the hash function for the signature.

Thanks
Max

Re: RFR 8157389: Release Note: New default -sigalg for jarsigner and keytool

[Wang Weijun]

I copied the exact @implNote from JarSigner into the release note.

BTW, I noticed NIST 800-57 Part 1 has a new revision 4, which has the same 
tables as in revision 3. A new bug 
https://bugs.openjdk.java.net/browse/JDK-8171190 created and I'll create a 
webrev soon.

Thanks
Max

> On Dec 14, 2016, at 5:09 AM, Sean Mullan  wrote:
> 
> Hi Max,
> 
> Could you include more information that shows what signature algorithm is 
> used based on the key size range and algorithm?
> 
> --Sean
> 
> On 12/11/16 9:53 PM, Wang Weijun wrote:
>> Please take a review at the release note at
>> 
>>   https://bugs.openjdk.java.net/browse/JDK-8157389
>> 
>> Thanks
>> Max
>> 

Re: RFR 8168979: @implNote for invalid FilePermission

[Wang Weijun]

Webrev updated at

   http://cr.openjdk.java.net/~weijun/8168979/webrev.01

One comment is moved to its correct position and a typo fixed.

Thanks Daniel for the comment. Can someone with a reviewer hat take a look?

Thanks
Max

> On Dec 12, 2016, at 6:03 PM, Daniel Fuchs  wrote:
> 
> Hi Max,
> 
> Don't count me as reviewer - but I see a mismatched comment
> in the file:
> 
> 209 /**
> 210  * Creates FilePermission objects with special internals.
> 211  * See {@link FilePermCompat#newPermPlusAltPath(Permission)} and
> 212  * {@link FilePermCompat#newPermUsingAltPath(Permission)}.
> 213  */
> 214
> 215 private static final Path here = builtInFS.getPath(
> 216 GetPropertyAction.privilegedGetProperty("user.dir"));
> 
> I guess the comment is a left over from some merge or previous fix?
> 
> 
> Also I noticed the following later on:
> 
> 541  * invalid {@code FilePermission}. Even if two {@code FilePermission}
> 542  * are created with the same invalid path, one does imply the other.
> 
> should this be:
> 
>Even if two {@code FilePermission} are created with the same
>invalid path, one does *not* imply the other.
> 
> best regards,
> 
> -- daniel
> 
> On 12/12/16 09:01, Wang Weijun wrote:
>> Please take a review at
>> 
>>   http://cr.openjdk.java.net/~weijun/8168979/webrev.00/
>> 
>> This further clarifies what an invalid FilePermission behaves. A major 
>> behavior change is that <> now implies an invalid permission, I 
>> hope this is good to minimize incompatibility.
>> 
>> Thanks
>> Max
>> 
> 

Re: On 8171135: Include javadoc on JarSigner API in security doc

[Wang Weijun]

Jon

Will there be a dedicated page for all non-SE javadocs? We already had some in 
jdk.security.auth, jdk.security.jgss on the security guide home page [1].

Also, I am not sure if this belongs to the docs or build category if the 
javadoc is meant to be generated automatically. Therefore I let it stay in the 
security-libs at the moment.

Thanks
Max

[1] http://download.java.net/java/jdk9/docs/technotes/guides/security/index.html

> On Dec 14, 2016, at 5:32 AM, Sean Mullan  wrote:
> 
> I would check with Jon Gibbons to see if there may be a more "official" place 
> for exported, non-SE javadocs; otherwise it seems ok to me.
> 
> --Sean
> 
> On 12/12/16 9:50 PM, Wang Weijun wrote:
>> Hi All
>> 
>> I've create a new bug to include the javadoc of the non-Java SE JarSigner 
>> API into security doc:
>> 
>>   https://bugs.openjdk.java.net/browse/JDK-8171135
>> 
>> If you think this is OK, I'll move the bug to doc.
>> 
>> Thanks
>> Max
>> 

Re: On 8171135: Include javadoc on JarSigner API in security doc

[Sean Mullan]

I would check with Jon Gibbons to see if there may be a more "official" 
place for exported, non-SE javadocs; otherwise it seems ok to me.


--Sean

On 12/12/16 9:50 PM, Wang Weijun wrote:

Hi All

I've create a new bug to include the javadoc of the non-Java SE JarSigner API 
into security doc:

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

If you think this is OK, I'll move the bug to doc.

Thanks
Max

Re: RFR 8157389: Release Note: New default -sigalg for jarsigner and keytool

[Sean Mullan]

Hi Max,

Could you include more information that shows what signature algorithm 
is used based on the key size range and algorithm?


--Sean

On 12/11/16 9:53 PM, Wang Weijun wrote:

Please take a review at the release note at

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

Thanks
Max

Re: [PATCH] 8165751: NPE in Signature with java.security.debug=provider

[Sean Mullan]

On 12/13/16 1:43 PM, Adam Petcher wrote:

Here is the latest diff that simply adds a null check to the existing
code. When the provider is null, the debug output will be "...algorithm
from: (no provider)". The test code is the same as the last diff.


Looks good to me.

--Sean




diff --git a/src/java.base/share/classes/java/security/Signature.java
b/src/java.base/share/classes/java/security/Signature.java
--- a/src/java.base/share/classes/java/security/Signature.java
+++ b/src/java.base/share/classes/java/security/Signature.java
@@ -452,6 +452,10 @@
 return this.provider;
 }

+private String getProviderName() {
+return (provider == null)  ? "(no provider)" : provider.getName();
+}
+
 void chooseFirstProvider() {
 // empty, overridden in Delegate
 }
@@ -473,7 +477,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" verification algorithm from: " +
this.provider.getName());
+" verification algorithm from: " + getProviderName());
 }
 }

@@ -522,7 +526,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" verification algorithm from: " +
this.provider.getName());
+" verification algorithm from: " + getProviderName());
 }
 }

@@ -543,7 +547,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" signing algorithm from: " + this.provider.getName());
+" signing algorithm from: " + getProviderName());
 }
 }

@@ -566,7 +570,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" signing algorithm from: " + this.provider.getName());
+" signing algorithm from: " + getProviderName());
 }
 }

diff --git a/test/java/security/Signature/NoProvider.java
b/test/java/security/Signature/NoProvider.java
new file mode 100644
--- /dev/null
+++ b/test/java/security/Signature/NoProvider.java
@@ -0,0 +1,99 @@
+/*
+ * Copyright (c) 2016, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License
version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8165751
+ * @summary Verify that that a subclass of Signature that does not
contain a
+ * provider can be used verify.
+ * @run main/othervm -Djava.security.debug=provider NoProvider
+ */
+
+import java.security.*;
+
+public class NoProvider {
+
+private static class NoProviderPublicKey implements PublicKey {
+
+public String getAlgorithm() {
+return "NoProvider";
+}
+public String getFormat() {
+return "none";
+}
+public byte[] getEncoded() {
+return new byte[1];
+}
+}
+
+private static class NoProviderSignature extends Signature {
+
+public NoProviderSignature() {
+super("NoProvider");
+}
+
+protected void engineInitVerify(PublicKey publicKey)
+throws InvalidKeyException {
+// do nothing
+}
+
+protected void engineInitSign(PrivateKey privateKey)
+throws InvalidKeyException {
+// do nothing
+}
+
+protected void engineUpdate(byte b) throws SignatureException {
+// do nothing
+}
+
+protected void engineUpdate(byte[] b, int off, int len)
+throws SignatureException {
+// do nothing
+}
+
+protected byte[] engineSign() throws SignatureException {
+return new byte[1];
+}
+
+protected boolean engineVerify(byte[] sigBytes)
+throws SignatureException {
+return false;
+}
+
+@Deprecated
+protected void engineSetParameter(String param, Object value)
+throws InvalidParameterException {
+// do nothing
+}
+
+@Deprecated
+protected 

Re: [PATCH] 8165751: NPE in Signature with java.security.debug=provider

[Adam Petcher]

Here is the latest diff that simply adds a null check to the existing 
code. When the provider is null, the debug output will be "...algorithm 
from: (no provider)". The test code is the same as the last diff.



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

--- a/src/java.base/share/classes/java/security/Signature.java
+++ b/src/java.base/share/classes/java/security/Signature.java
@@ -452,6 +452,10 @@
 return this.provider;
 }

+private String getProviderName() {
+return (provider == null)  ? "(no provider)" : provider.getName();
+}
+
 void chooseFirstProvider() {
 // empty, overridden in Delegate
 }
@@ -473,7 +477,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" verification algorithm from: " + 
this.provider.getName());

+" verification algorithm from: " + getProviderName());
 }
 }

@@ -522,7 +526,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" verification algorithm from: " + 
this.provider.getName());

+" verification algorithm from: " + getProviderName());
 }
 }

@@ -543,7 +547,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" signing algorithm from: " + this.provider.getName());
+" signing algorithm from: " + getProviderName());
 }
 }

@@ -566,7 +570,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" signing algorithm from: " + this.provider.getName());
+" signing algorithm from: " + getProviderName());
 }
 }

diff --git a/test/java/security/Signature/NoProvider.java 
b/test/java/security/Signature/NoProvider.java

new file mode 100644
--- /dev/null
+++ b/test/java/security/Signature/NoProvider.java
@@ -0,0 +1,99 @@
+/*
+ * Copyright (c) 2016, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License 
version

+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8165751
+ * @summary Verify that that a subclass of Signature that does not 
contain a

+ * provider can be used verify.
+ * @run main/othervm -Djava.security.debug=provider NoProvider
+ */
+
+import java.security.*;
+
+public class NoProvider {
+
+private static class NoProviderPublicKey implements PublicKey {
+
+public String getAlgorithm() {
+return "NoProvider";
+}
+public String getFormat() {
+return "none";
+}
+public byte[] getEncoded() {
+return new byte[1];
+}
+}
+
+private static class NoProviderSignature extends Signature {
+
+public NoProviderSignature() {
+super("NoProvider");
+}
+
+protected void engineInitVerify(PublicKey publicKey)
+throws InvalidKeyException {
+// do nothing
+}
+
+protected void engineInitSign(PrivateKey privateKey)
+throws InvalidKeyException {
+// do nothing
+}
+
+protected void engineUpdate(byte b) throws SignatureException {
+// do nothing
+}
+
+protected void engineUpdate(byte[] b, int off, int len)
+throws SignatureException {
+// do nothing
+}
+
+protected byte[] engineSign() throws SignatureException {
+return new byte[1];
+}
+
+protected boolean engineVerify(byte[] sigBytes)
+throws SignatureException {
+return false;
+}
+
+@Deprecated
+protected void engineSetParameter(String param, Object value)
+throws InvalidParameterException {
+// do nothing
+}
+
+@Deprecated
+protected Object engineGetParameter(String param)
+

Re: [PATCH] 8165751: NPE in Signature with java.security.debug=provider

[Adam Petcher]

Thanks for the review.

After thinking about this some more, I don't like the idea of using 
Optional for a member variable due to the limitations of this class and 
the lack of support for this usage of it. I'll send out a new diff that 
uses a standard null check.


See below for other comments.


On 12/12/2016 9:20 PM, Wang Weijun wrote:

Hi Adam

The only behavior change is with the debug output, right?

Yes. This will be more obvious in my next diff.


Is this a new pattern that internal optional fields should be defined as an 
Optional?

And, when there is no provider the string form "from: " looks strange, I would rather make it "from 
nowhere". I would also move the space before "from" to where the method is called, say, " 
verification algorithm " + getProvidedByString().
It doesn't print the "from: " when there is no provider. So the string 
will look like "Signature.DSA verification algorithm" in this case. I 
don't have a strong opinion on whether we should print "from: no 
provider" (for example), but Sean expressed a preference for leaving 
this entire part blank when there is no provider (as it is in my last 
diff).


Thanks
Max


On Dec 13, 2016, at 4:05 AM, Adam Petcher  wrote:

Okay. I changed getProvider() to return this.provider.orElse(null), which will 
allow this method to return null. For consistency, I allow all other providers 
(in Instance and Service) to be null using Optional.ofNullable(). Hopefully, I 
found and fixed all the whitespace issues, too. Here is the corrected diff:

diff --git a/src/java.base/share/classes/java/security/Signature.java 
b/src/java.base/share/classes/java/security/Signature.java
--- a/src/java.base/share/classes/java/security/Signature.java
+++ b/src/java.base/share/classes/java/security/Signature.java
@@ -134,7 +134,7 @@
 private String algorithm;

 // The provider
-Provider provider;
+Optional provider = Optional.empty();

 /**
  * Possible {@link #state} value, signifying that
@@ -266,7 +266,7 @@
 SignatureSpi spi = (SignatureSpi)instance.impl;
 sig = new Delegate(spi, algorithm);
 }
-sig.provider = instance.provider;
+sig.provider = Optional.ofNullable(instance.provider);
 return sig;
 }

@@ -449,13 +449,17 @@
  */
 public final Provider getProvider() {
 chooseFirstProvider();
-return this.provider;
+return this.provider.orElse(null);
 }

 void chooseFirstProvider() {
 // empty, overridden in Delegate
 }

+private String getProvidedByString() {
+return provider.map(x -> " from: " + x.getName()).orElse("");
+}
+
 /**
  * Initializes this object for verification. If this method is called
  * again with a different argument, it negates the effect
@@ -473,7 +477,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" verification algorithm from: " + this.provider.getName());
+" verification algorithm" + getProvidedByString());
 }
 }

@@ -522,7 +526,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" verification algorithm from: " + this.provider.getName());
+" verification algorithm" + getProvidedByString());
 }
 }

@@ -543,7 +547,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" signing algorithm from: " + this.provider.getName());
+" signing algorithm" + getProvidedByString());
 }
 }

@@ -566,7 +570,7 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("Signature." + algorithm +
-" signing algorithm from: " + this.provider.getName());
+" signing algorithm" + getProvidedByString());
 }
 }

@@ -1087,7 +1091,7 @@
 }
 try {
 sigSpi = newInstance(s);
-provider = s.getProvider();
+provider = Optional.ofNullable(s.getProvider());
 // not needed any more
 firstService = null;
 serviceIterator = null;
@@ -1132,7 +1136,7 @@
 try {
 SignatureSpi spi = newInstance(s);
 init(spi, type, key, random);
-provider = s.getProvider();
+provider = Optional.ofNullable(s.getProvider());
 sigSpi = spi;
 firstService = null;
 serviceIterator = null;
diff --git a/test/java/security/Signature/NoProvider.java 
b/test/java/security/Signature/NoProvider.java
new file mode 100644
--- /dev/null
+++ b/test/java/security/Signature/NoProvider.java
@@ -0,0 +1,99 @@
+/*