Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Sean Mullan]

Looks fine to me.

--Sean

On 11/7/16 10:10 PM, Wang Weijun wrote:

Everything looks fine now.

--Max


On Nov 8, 2016, at 11:09 AM, Artem Smotrakov  wrote:

Here is final version (I hope)

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.04/

Artem

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Wang Weijun]

Everything looks fine now.

--Max

> On Nov 8, 2016, at 11:09 AM, Artem Smotrakov  
> wrote:
> 
> Here is final version (I hope)
> 
> http://cr.openjdk.java.net/~asmotrak/8168882/webrev.04/
> 
> Artem

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Sean, Max,

Please take a look at 
http://cr.openjdk.java.net/~asmotrak/8168882/webrev.03/


It doesn't print a warning anymore, and reset the security property only 
if -jarfile specified. I also updated a couple of tests to check if 
"-printcert" works fine.


Artem


On 11/03/2016 05:47 PM, Artem Smotrakov wrote:

Thank you for review Sean.

I'll remove the warning then. And I'll update it to reset the security 
property only if a jar file has been specified.


Let me also check how "-printcert -file ..." and "-printcert 
-sslserver" work.


Artem


On 11/03/2016 07:27 AM, Wang Weijun wrote:

I agree with Sean.

--Max

On Nov 3, 2016, at 10:00 PM, Sean Mullan  
wrote:


You should only unset the jdk.jar.disabledAlgorithms property if a 
jarfile has been specified.


Also, you are printing the warning message for all usages of the 
-printcert option, -ssl, etc, which is not correct.


But I don't really think the warning message is necessary. The docs 
for the -printcert option are pretty clear that it simply extracts 
the certificate and prints it. If we are going to put a warning in 
for signed JARs, then arguably we should put in a more general, 
simple warning in for all usages of this option to say that the 
certificate, etc is not verified, ex:


"WARNING: The -printcert option does not verify the certificate."

But again, I don't think this is strictly necessary.

Thanks,
Sean

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Thank you for review Sean.

I'll remove the warning then. And I'll update it to reset the security 
property only if a jar file has been specified.


Let me also check how "-printcert -file ..." and "-printcert -sslserver" 
work.


Artem


On 11/03/2016 07:27 AM, Wang Weijun wrote:

I agree with Sean.

--Max


On Nov 3, 2016, at 10:00 PM, Sean Mullan  wrote:

You should only unset the jdk.jar.disabledAlgorithms property if a jarfile has 
been specified.

Also, you are printing the warning message for all usages of the -printcert 
option, -ssl, etc, which is not correct.

But I don't really think the warning message is necessary. The docs for the 
-printcert option are pretty clear that it simply extracts the certificate and 
prints it. If we are going to put a warning in for signed JARs, then arguably 
we should put in a more general, simple warning in for all usages of this 
option to say that the certificate, etc is not verified, ex:

"WARNING: The -printcert option does not verify the certificate."

But again, I don't think this is strictly necessary.

Thanks,
Sean

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Sean Mullan]

You should only unset the jdk.jar.disabledAlgorithms property if a 
jarfile has been specified.


Also, you are printing the warning message for all usages of the 
-printcert option, -ssl, etc, which is not correct.


But I don't really think the warning message is necessary. The docs for 
the -printcert option are pretty clear that it simply extracts the 
certificate and prints it. If we are going to put a warning in for 
signed JARs, then arguably we should put in a more general, simple 
warning in for all usages of this option to say that the certificate, 
etc is not verified, ex:


"WARNING: The -printcert option does not verify the certificate."

But again, I don't think this is strictly necessary.

Thanks,
Sean

On 11/2/16 10:40 PM, Wang Weijun wrote:

Everything is fine now.

Thanks
Max

On 11/3/2016 9:38 AM, Artem Smotrakov wrote:

My bad, I missed that.

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.02/

Artem


On 11/02/2016 06:30 PM, Wang Weijun wrote:

On 11/01/2016 11:59 PM, Wang Weijun wrote:

>> Main.java:
>>
>> The warning (and the subsequent empty line) should be printed
into System.err.

This one?

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Wang Weijun]

Everything is fine now.

Thanks
Max

On 11/3/2016 9:38 AM, Artem Smotrakov wrote:

My bad, I missed that.

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.02/

Artem


On 11/02/2016 06:30 PM, Wang Weijun wrote:

On 11/01/2016 11:59 PM, Wang Weijun wrote:

>> Main.java:
>>
>> The warning (and the subsequent empty line) should be printed into 
System.err.

This one?

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

My bad, I missed that.

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.02/

Artem


On 11/02/2016 06:30 PM, Wang Weijun wrote:

On 11/01/2016 11:59 PM, Wang Weijun wrote:

>>Main.java:
>>
>>The warning (and the subsequent empty line) should be printed into System.err.

This one?

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Wang Weijun]

> On Nov 3, 2016, at 8:27 AM, Artem Smotrakov  
> wrote:
> 
> Hi Max,
> 
> Please see inline.
> 
> 
> On 11/01/2016 11:59 PM, Wang Weijun wrote:
>> Main.java:
>> 
>> The warning (and the subsequent empty line) should be printed into 
>> System.err.

This one?

> 
> Thank you for review Max, please take a look at updated webrev:
> 
> http://cr.openjdk.java.net/~asmotrak/8168882/webrev.01/

Fine except for the System.out one above.

> 
> By the way, I just noticed that we have another version of JarUtils.java 
> which was added by Alan 7 month ago
> 
> http://hg.openjdk.java.net/jdk9/dev/jdk/log/1396fb6d0279/test/lib/testlibrary/JarUtils.java

Looks like it's only used by himself, and has 2 lines of history.

Also, my understanding is that we are switching to test utils in the root repo. 
A lot of same name utils in jdk/test were deprecated.

Thanks
Max

> 
> Artem

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Hi Max,

Please see inline.


On 11/01/2016 11:59 PM, Wang Weijun wrote:

Main.java:

The warning (and the subsequent empty line) should be printed into System.err.

Resources.java:

"This tool accepts any algorithm" is a little confusing (sorry that I originally suggested it). 
Maybe "This tool does not attempt to verify a signed jar file, please run \"jarsigner 
-verify\" if you want to."

Sure, I'll update the message.


Also, ever since the 1st time hard coded strings are changed into dot-connected 
resource keys, newly added keys do not necessarily use the exact same string. 
Make it simple so next time if the value needs to be updated you don't need to 
change the key.
Agree, I was thinking about shorter key, but then noticed that most of 
keys look like messages. I'll make the key shorter.


Test:

- You can also add -Duser.language=en and -Duser.country=US to keytool.

Makes sense to me, will do.


- With my recent update to JarUtils.createJar(), there is no need to create the 
"test" file.
Right. I'll remove it, however creating a file would make it a bit 
clearer to me.


Thank you for review Max, please take a look at updated webrev:

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.01/

By the way, I just noticed that we have another version of JarUtils.java 
which was added by Alan 7 month ago


http://hg.openjdk.java.net/jdk9/dev/jdk/log/1396fb6d0279/test/lib/testlibrary/JarUtils.java

Artem


Everything else looks fine.

Thanks
Max



On Nov 2, 2016, at 7:35 AM, Artem Smotrakov  wrote:

Hello,

Please review this small update for keytool.

"keytool -printcert -jarfile" doesn't work with jars which were signed with algorithms 
listed in "jdk.jar.disabledAlgorithms" security property.

The patch below resets "jdk.jar.disabledAlgorithms" security property before 
reading a jar file, and prints a warning.

I also re-wrote readjar.sh test, and added SecurityTools class with a couple of 
re-usable methods for jarsigner and keytool (those methods are based on methods 
from TimestampCheck.java).

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

Artem

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Wang Weijun]

Main.java:

The warning (and the subsequent empty line) should be printed into System.err.

Resources.java:

"This tool accepts any algorithm" is a little confusing (sorry that I 
originally suggested it). Maybe "This tool does not attempt to verify a signed 
jar file, please run \"jarsigner -verify\" if you want to."

Also, ever since the 1st time hard coded strings are changed into dot-connected 
resource keys, newly added keys do not necessarily use the exact same string. 
Make it simple so next time if the value needs to be updated you don't need to 
change the key.

Test:

- You can also add -Duser.language=en and -Duser.country=US to keytool.

- With my recent update to JarUtils.createJar(), there is no need to create the 
"test" file.

Everything else looks fine.

Thanks
Max


> On Nov 2, 2016, at 7:35 AM, Artem Smotrakov  
> wrote:
> 
> Hello,
> 
> Please review this small update for keytool.
> 
> "keytool -printcert -jarfile" doesn't work with jars which were signed with 
> algorithms listed in "jdk.jar.disabledAlgorithms" security property.
> 
> The patch below resets "jdk.jar.disabledAlgorithms" security property before 
> reading a jar file, and prints a warning.
> 
> I also re-wrote readjar.sh test, and added SecurityTools class with a couple 
> of re-usable methods for jarsigner and keytool (those methods are based on 
> methods from TimestampCheck.java).
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8168882
> Webrev: http://cr.openjdk.java.net/~asmotrak/8168882/webrev.00/
> 
> Artem

[9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

[Artem Smotrakov]

Hello,

Please review this small update for keytool.

"keytool -printcert -jarfile" doesn't work with jars which were signed 
with algorithms listed in "jdk.jar.disabledAlgorithms" security property.


The patch below resets "jdk.jar.disabledAlgorithms" security property 
before reading a jar file, and prints a warning.


I also re-wrote readjar.sh test, and added SecurityTools class with a 
couple of re-usable methods for jarsigner and keytool (those methods are 
based on methods from TimestampCheck.java).


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

Artem