On 8171135: Include javadoc on JarSigner API in security doc

[Wang Weijun]

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: [PATCH] 8165751: NPE in Signature with java.security.debug=provider

[Wang Weijun]

Hi Adam

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

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().

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 @@
> +/*
> + * 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 

Re: RFR 8075618: Create tests to check jarsigner work with multi-version jar

[Artem Smotrakov]

You can use 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/d4d7f1f0d688/test/lib/security/SecurityTools.java 
which would simplify the code. This lib was added to be used in such tests.


Note that SecurityTools addresses a couple of known issues with running 
security tools on machines with different locales, please see 
"user.language" and "user.country" system properties


Artem


On 12/12/2016 02:43 PM, Amanda Jiang wrote:

Hi All,

Please help to review following changeset, which tests that jarsigner 
tool works with multi-release JAR files.


Webrev: http://cr.openjdk.java.net/~amjiang/8075618/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8075618


Thanks,
Amanda

RFR 8075618: Create tests to check jarsigner work with multi-version jar

[Amanda Jiang]

Hi All,

Please help to review following changeset, which tests that jarsigner 
tool works with multi-release JAR files.


Webrev: http://cr.openjdk.java.net/~amjiang/8075618/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8075618


Thanks,
Amanda

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

[Adam Petcher]

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 @@
+/*
+ * 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 

Re: RFR[9] JDK-8171043: ServerIdentityTest.java fails on Windows

[Xuelei Fan]

Looks fine to me.

Thanks,
Xuelei

On 12/12/2016 2:29 AM, John Jiang wrote:

Hi Xuelei,
Thanks for your comments.
Please review this new webrev:
http://cr.openjdk.java.net/~jjiang/8171043/webrev.01/

Best regards,
John Jiang


On 2016/12/12 13:07, Xuelei Fan wrote:

Hi John,

It's a good catch of the problem.  Looks like the server side should
read the HTTP request at first, and then write the HTTP response.
That's the server call:
 InputStream sslIS = socket.getInputStream();
 sslIS.read();
and then write the "HTTP/1.1, ...".

Thanks,
Xuelei

On 12/11/2016 4:11 AM, John Jiang wrote:

Hi,
Please review this patch for test
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java
Before the client get the response, the server may close.

Webrev: http://cr.openjdk.java.net/~jjiang/8171043/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8171043

Best regards,
John Jiang

Re: RFR 8168979: @implNote for invalid FilePermission

[Wang Weijun]

> 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?

These are 2 different methods: "Plus" and "Using".

> 
> 
> 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.

Ah, yes.

Thanks
Max

> 
> 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: RFR[9] JDK-8171043: ServerIdentityTest.java fails on Windows

[John Jiang]

Hi Xuelei,
Thanks for your comments.
Please review this new webrev: 
http://cr.openjdk.java.net/~jjiang/8171043/webrev.01/


Best regards,
John Jiang


On 2016/12/12 13:07, Xuelei Fan wrote:

Hi John,

It's a good catch of the problem.  Looks like the server side should 
read the HTTP request at first, and then write the HTTP response. 
That's the server call:

 InputStream sslIS = socket.getInputStream();
 sslIS.read();
and then write the "HTTP/1.1, ...".

Thanks,
Xuelei

On 12/11/2016 4:11 AM, John Jiang wrote:

Hi,
Please review this patch for test
sun/net/www/protocol/https/HttpsClient/ServerIdentityTest.java
Before the client get the response, the server may close.

Webrev: http://cr.openjdk.java.net/~jjiang/8171043/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8171043

Best regards,
John Jiang

Re: RFR 8168979: @implNote for invalid FilePermission

[Daniel Fuchs]

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