Re: RFR 8242068: Signed JAR support for RSASSA-PSS and EdDSA

[Weijun Wang]

Ping again. CSR already approved and rdp1 is near...

Thanks,
Max

> On Jun 2, 2020, at 4:07 PM, Weijun Wang  wrote:
> 
> Updated again at
> 
>   https://cr.openjdk.java.net/~weijun/8242068/webrev.02
> 
> Major changes this time:
> 
> 1. Use Signed Attributes in generateNewSignedData(direct==false). This means 
> for Ed448 we will use SHAKE256-LEN so there is no interop issue (BTW, BC 
> recently added support to SHAKE256 in 166b07), hence removed the newly added 
> system property in webrev.01.
> 
> 2. Better interop with providers using Ed448 and Ed25519 as key algorithm 
> names.
> 
> Major changes previously:
> 
> 1. Always encode the whole signature algorithm identifier (instead of 
> encryption algorithmid) in PKCS7 SignerInfo.
> 
> 2. New SignatureUtil methods
> 
> Thanks,
> Max
> 
>> On May 27, 2020, at 8:21 AM, Weijun Wang  wrote:
>> 
>> Webrev updated at
>> 
>>   http://cr.openjdk.java.net/~weijun/8242068/webrev.01/
>> 
>> Two major changes:
>> 
>> 1. Always use the signature algorithm directly in 
>> SignerInfo::signatureAlgorithm:
>> 
>> In PKCS7 SignerInfo
>> 
>> SignerInfo ::= SEQUENCE {
>>   version CMSVersion,
>>   sid SignerIdentifier,
>>   digestAlgorithm DigestAlgorithmIdentifier,
>>   signedAttrs [0] IMPLICIT SignedAttributes OPTIONAL,
>>   signatureAlgorithm SignatureAlgorithmIdentifier,
>>   signature SignatureValue,
>>   unsignedAttrs [1] IMPLICIT UnsignedAttributes OPTIONAL }
>> 
>> We have always been putting "SHA-1" etc into DigestAlgorithmIdentifier, and 
>> "RSA", "DSA", "EC" into signatureAlgorithm.
>> 
>> The latest https://tools.ietf.org/html/rfc5652#section-10.1.2 claims it to be
>> 
>>  The SignatureAlgorithmIdentifier type identifies a signature
>>  algorithm, and it can also identify a message digest algorithm.
>>  Examples include RSA, DSA, DSA with SHA-1, ECDSA, and ECDSA with
>>  SHA-256. 
>> 
>> It's complicated to always divide a signature algorithm into a digest 
>> algorithm and an encryption algorithm (and with the new RSASSA-PSS and EdDSA 
>> it's not easy to define it), therefore I decide to use the signature 
>> algorithm directly from now on. Fortunately Java has been able to parse this 
>> for a very long time so there is no compatibility issue. I noticed 
>> BouncyCastle has been doing the same, and OpenSSL too except for RSA.
>> 
>> 2. Support both SHAKE256 and SHAKE256-LEN while parsing a Ed448 SignerInfo. 
>> They are both described in https://www.rfc-editor.org/rfc/rfc8419.html 
>> although it's a little complicated. To be standard compliant 
>> (https://www.rfc-editor.org/rfc/rfc8419.html#section-3.2 and we don't use 
>> Signed Attributes), by default Java will use SHAKE256 as the 
>> digestAlgorithm. However I noticed BouncyCastle does not recognize it, so if 
>> you set the system property "jdk.security.pkcs7.ed448.digalg.haslen" to 
>> "true", it will use SHAKE256-LEN (len == 512). I haven't described this 
>> system property in the CSR yet.
>> 
>> Thanks,
>> Max
>> 
>> 
>>> On May 22, 2020, at 10:30 PM, Weijun Wang  wrote:
>>> 
>>> Please take a review at
>>> 
>>>CSR : https://bugs.openjdk.java.net/browse/JDK-8245274
>>> webrev : http://cr.openjdk.java.net/~weijun/8242068/webrev.00/
>>> 
>>> Major points in CSR:
>>> 
>>> - new sigalg "RSASSA-PSS", "EdDSA", "Ed25519" and "Ed448" can be used in 
>>> jarsigner
>>> 
>>> - The ".RSA" and ".EC" block extension types (PKCS #7 SignedData inside a 
>>> signed JAR) are reused for new signature algorithms
>>> 
>>> major code changes:
>>> 
>>> - Move signature related utilities methods from AlgorithmId.java to 
>>> SignatureUtil.java
>>> 
>>> - Add new SignatureUtil methods fromKey() and fromSignature() to simplify 
>>> creating Signature and getting its AlgorithmId
>>> 
>>> - Use the new methods in PKCS10, X509CertImpl, and X509CRLImpl signing
>>> 
>>> - Add a new (and intuitive, IMHO) PKCS7::generateNewSignedData capable of 
>>> all old and new signature algorithms
>>> 
>>> - Mark all -altsign related code deprecated and they can be removed once 
>>> ContentSigner is removed
>>> 
>>> Next I'll do some basic interop tests with openssl and BouncyCastle.
>>> 
>>> Thanks,
>>> Max
>>> 
>> 
> 

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Weijun Wang]

Looks fine to me.

For CSR, since there is already a "Note" there for these 2 options, you can add 
a few words about what -keystore and -trustcacerts can do.

Thanks,
Max

> On Jun 8, 2020, at 4:01 AM, Hai-May Chao  wrote:
> 
> Updated webrev -
> 
> https://cr.openjdk.java.net/~hchao/8244148/webrev.02/
> 
> Thanks,
> Hai-May
> 
> 
>> On Jun 5, 2020, at 11:04 PM, Weijun Wang  wrote:
>> 
>> I still think duplicated commands in TrustedCert.java are useless. Line 104 
>> and line 133 are exactly the same, line 109 and line 138 are exactly the 
>> same, and you haven't made any change to these 2 files in between.
>> 
>> Same for line 80 and line 96 of TrustedCRL.java.
>> 
>> Everything else is fine.
>> 
>> Thanks,
>> Max
>> 
>> 
>>> On Jun 6, 2020, at 2:25 AM, Hai-May Chao  wrote:
>>> 
>>> Updated webrev - 
>>> 
>>> https://cr.openjdk.java.net/~hchao/8244148/webrev.01/
>>> 
>>> Added one command line -importcert in TrustCert.java.
>>> Added createCacerts() in test/lib SecurityTools.java.
>>> 
>>> Thanks,
>>> Hai-May
>>> 
>>> 
>>> 
 On Jun 4, 2020, at 5:57 AM, Weijun Wang  wrote:
 
 
 
> On Jun 4, 2020, at 7:29 PM, Hai-May Chao  wrote:
> 
> Hi Max,
> 
>> On Jun 3, 2020, at 12:59 AM, Weijun Wang  wrote:
>> 
>> The source change looks fine to me.
>> 
>> In TrustedCert.java:
>> 
>> - You can use FileOutputStream and Files.copy(Path,OutputStream) in 
>> cat().
> 
> This cat() is taken from WealAlg.java.
> 
>> 
>> - There is no need to recreate root.jks and root.pem.
> 
> The sequences of the commands used in this test scenario allows me to 
> test -printcert for the -trustcacerts and -keytsore options. We had 
> discussion offline about it. The test uses trusted certificates and 
> checks no warnings on the weak algorithms to address the requirement 
> described in the bug. I believe it does serve that purpose, and looks 
> legitimate to me. There could be different ways of testing a 
> functionality, and please let me know if there is a problem with the 
> current approach.
 
 I just meant that the keytool commands generating root.jks and root.pem 
 are exactly the same and there is no need to recreate it.
 
> 
> Please also elaborate your comment about no need to recreate root.jks and 
> root.pem.
> 
>> 
>> - Why not use -trustcacerts below?
>> 
>> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);
> 
> 
> Because here is to import the server (end-entity) cert, and it will not 
> make a difference for the test result whether to use the -trustcacerts or 
> not. It’s the ca (intermediate) cert needs to have it in this test 
> scenario. I intended to leave it out in #160 to distinguish between 
> server and ca certs.
 
 OK.
 
 Then how about we add a new command before line 155?
 
   kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);
 
 This would prove the "-trustcacerts" on line 155 is really useful.
 
> 
>> 
>> - It's probably better to add a " " between cmd and options in 
>> patchcmd(). Same in TrustedCRL.java.
> 
> Ok, will change it.
> 
>> 
>> In TrustedCRL.java:
>> 
>> - No need to recreate ks and ca.crl. Just call "-printcrl" with 
>> different options.
> 
> Same reply as above.
 
 Same question as above.
 
> 
>> 
>> - Why create using MD5withRSA? Do you meant to warn about the weak 
>> algorithm?
> 
> Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA 
> used in root CA where no warning will be emitted. There is another 
> -gencrl in #119 without using MD5withRSA so I’d have two test cases.
> 
>> 
>> Also I would suggest you create a dedicate method (maybe in 
>> SecurityTools.java) to create your own cacerts. There is no need to copy 
>> over the system cacerts, just make sure the file is created with the JKS 
>> storetype. We are thinking of upgrading the storetype of cacerts and 
>> it's nice to do this at a single place so we can modify it easily later.
> 
> I created a method in SecurityTools.java to create the own cacerts. With 
> this keystore, the subsequent importing a certificate reply would not 
> work. It turns out that its caks.size() is zero detected at 
> establishCertChain() in keytool/Main.java after root cert has been 
> imported to that cacerts. At this point I’d like to suggest a separate 
> bug be filed to cover the cacerts enhancement that you suggested.
 
 I meant creating the cacerts in one method, something like
 
  void createCacerts(String ks, String... crt);
 
 and you can call createCacerts("mycacerts", "root.crt") to create it. The 
 method can call KeyStore APIs and not keytool commands.
 
 BTW, what 

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

Updated webrev -

https://cr.openjdk.java.net/~hchao/8244148/webrev.02/ 


Thanks,
Hai-May


> On Jun 5, 2020, at 11:04 PM, Weijun Wang  wrote:
> 
> I still think duplicated commands in TrustedCert.java are useless. Line 104 
> and line 133 are exactly the same, line 109 and line 138 are exactly the 
> same, and you haven't made any change to these 2 files in between.
> 
> Same for line 80 and line 96 of TrustedCRL.java.
> 
> Everything else is fine.
> 
> Thanks,
> Max
> 
> 
>> On Jun 6, 2020, at 2:25 AM, Hai-May Chao  wrote:
>> 
>> Updated webrev - 
>> 
>> https://cr.openjdk.java.net/~hchao/8244148/webrev.01/
>> 
>> Added one command line -importcert in TrustCert.java.
>> Added createCacerts() in test/lib SecurityTools.java.
>> 
>> Thanks,
>> Hai-May
>> 
>> 
>> 
>>> On Jun 4, 2020, at 5:57 AM, Weijun Wang  wrote:
>>> 
>>> 
>>> 
 On Jun 4, 2020, at 7:29 PM, Hai-May Chao  wrote:
 
 Hi Max,
 
> On Jun 3, 2020, at 12:59 AM, Weijun Wang  wrote:
> 
> The source change looks fine to me.
> 
> In TrustedCert.java:
> 
> - You can use FileOutputStream and Files.copy(Path,OutputStream) in cat().
 
 This cat() is taken from WealAlg.java.
 
> 
> - There is no need to recreate root.jks and root.pem.
 
 The sequences of the commands used in this test scenario allows me to test 
 -printcert for the -trustcacerts and -keytsore options. We had discussion 
 offline about it. The test uses trusted certificates and checks no 
 warnings on the weak algorithms to address the requirement described in 
 the bug. I believe it does serve that purpose, and looks legitimate to me. 
 There could be different ways of testing a functionality, and please let 
 me know if there is a problem with the current approach.
>>> 
>>> I just meant that the keytool commands generating root.jks and root.pem are 
>>> exactly the same and there is no need to recreate it.
>>> 
 
 Please also elaborate your comment about no need to recreate root.jks and 
 root.pem.
 
> 
> - Why not use -trustcacerts below?
> 
> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);
 
 
 Because here is to import the server (end-entity) cert, and it will not 
 make a difference for the test result whether to use the -trustcacerts or 
 not. It’s the ca (intermediate) cert needs to have it in this test 
 scenario. I intended to leave it out in #160 to distinguish between server 
 and ca certs.
>>> 
>>> OK.
>>> 
>>> Then how about we add a new command before line 155?
>>> 
>>>   kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);
>>> 
>>> This would prove the "-trustcacerts" on line 155 is really useful.
>>> 
 
> 
> - It's probably better to add a " " between cmd and options in 
> patchcmd(). Same in TrustedCRL.java.
 
 Ok, will change it.
 
> 
> In TrustedCRL.java:
> 
> - No need to recreate ks and ca.crl. Just call "-printcrl" with different 
> options.
 
 Same reply as above.
>>> 
>>> Same question as above.
>>> 
 
> 
> - Why create using MD5withRSA? Do you meant to warn about the weak 
> algorithm?
 
 Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA 
 used in root CA where no warning will be emitted. There is another -gencrl 
 in #119 without using MD5withRSA so I’d have two test cases.
 
> 
> Also I would suggest you create a dedicate method (maybe in 
> SecurityTools.java) to create your own cacerts. There is no need to copy 
> over the system cacerts, just make sure the file is created with the JKS 
> storetype. We are thinking of upgrading the storetype of cacerts and it's 
> nice to do this at a single place so we can modify it easily later.
 
 I created a method in SecurityTools.java to create the own cacerts. With 
 this keystore, the subsequent importing a certificate reply would not 
 work. It turns out that its caks.size() is zero detected at 
 establishCertChain() in keytool/Main.java after root cert has been 
 imported to that cacerts. At this point I’d like to suggest a separate bug 
 be filed to cover the cacerts enhancement that you suggested.
>>> 
>>> I meant creating the cacerts in one method, something like
>>> 
>>>  void createCacerts(String ks, String... crt);
>>> 
>>> and you can call createCacerts("mycacerts", "root.crt") to create it. The 
>>> method can call KeyStore APIs and not keytool commands.
>>> 
>>> BTW, what does caks.size() == 0 matter here?
>>> 
>>> Thanks,
>>> Max
>>> 
>>> 
 
 Thanks,
 Hai-May
 
 
> Thanks,
> Max
> 
> 
>> On Jun 2, 2020, at 2:37 AM, Hai-May Chao  wrote:
>> 
>> Hi,
>> 
>> I’d like to request a review for:
>> 
>> JBS: