Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled [v2]

2021-11-18 Thread Weijun Wang
On Thu, 18 Nov 2021 15:03:33 GMT, Sean Mullan  wrote:

>> We should, but the problem is that jarsigner needs to individually test each 
>> algorithm, so it can properly display which algorithm is restricted. So, I 
>> think it will need to parse the RSSASSA params itself, and then call the 
>> constraints code to check each algorithm. Let me see if I can code up 
>> something that does that.
>
> I would like to defer the checking of AlgorithmParameters as part of another 
> bug. There are some major restructuring changes that would need to be made to 
> jarsigner to support this. And for RSASSA-PSS, there should not be any risk 
> for a while since by default jarsigner uses at least SHA-256 for the digest 
> algorithms in the PSS parameters.

Looks so.

-

PR: https://git.openjdk.java.net/jdk/pull/6296


Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled [v2]

2021-11-18 Thread Weijun Wang
On Tue, 16 Nov 2021 18:10:04 GMT, Sean Mullan  wrote:

>> When a signature/digest algorithm was being checked, the algorithm 
>> constraints checked both the signature/digest algorithm and the key to see 
>> if they were restricted. This caused duplicate checks and was also 
>> problematic for `jarsigner` (and `keytool`) which need to distinguish these 
>> two cases, so that the output can properly indicate when the key is disabled 
>> but the signature or digest alg is ok. 
>> 
>> To address this issue, a new `checkKey` parameter is added to the 
>> `DisabledAlgorithmConstraints.permits` methods. When `true` the key (alg and 
>> size) is also checked, otherwise it is not. This flag is always set to 
>> `false` by `jarsigner` when checking algs and by the JDK when checking 
>> digest algorithms. Other small changes include changes in `SignerInfo` to 
>> use a record to store info about the algorithms to be checked, and removing 
>> an unnecessary CRL checking method from `AlgorithmChecker`.
>> 
>> `keytool` will be enhanced in a subsequent CR to call the new methods.
>
> Sean Mullan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use var in for loop in SignerInfo.
>   In TimestampCheck test, combine/simplify what messages should not be emitted
>   when jar is signed with 512-bit RSA key.

Marked as reviewed by weijun (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6296


Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled [v2]

2021-11-18 Thread Sean Mullan
On Tue, 16 Nov 2021 17:53:16 GMT, Sean Mullan  wrote:

>> src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 
>> 1491:
>> 
>>> 1489: private static String checkWeakAlg(String alg, 
>>> CertPathConstraintsParameters cpcp) {
>>> 1490: try {
>>> 1491: CERTPATH_DISABLED_CHECK.permits(alg, cpcp, false);
>> 
>> Do we need to check AlgorithmParamters as well? Ex: if `alg` is RSASSA-PSS.
>
> We should, but the problem is that jarsigner needs to individually test each 
> algorithm, so it can properly display which algorithm is restricted. So, I 
> think it will need to parse the RSSASSA params itself, and then call the 
> constraints code to check each algorithm. Let me see if I can code up 
> something that does that.

I would like to defer the checking of AlgorithmParameters as part of another 
bug. There are some major restructuring changes that would need to be made to 
jarsigner to support this. And for RSASSA-PSS, there should not be any risk for 
a while since by default jarsigner uses at least SHA-256 for the digest 
algorithms in the PSS parameters.

-

PR: https://git.openjdk.java.net/jdk/pull/6296


Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled [v2]

2021-11-16 Thread Sean Mullan
> When a signature/digest algorithm was being checked, the algorithm 
> constraints checked both the signature/digest algorithm and the key to see if 
> they were restricted. This caused duplicate checks and was also problematic 
> for `jarsigner` (and `keytool`) which need to distinguish these two cases, so 
> that the output can properly indicate when the key is disabled but the 
> signature or digest alg is ok. 
> 
> To address this issue, a new `checkKey` parameter is added to the 
> `DisabledAlgorithmConstraints.permits` methods. When `true` the key (alg and 
> size) is also checked, otherwise it is not. This flag is always set to 
> `false` by `jarsigner` when checking algs and by the JDK when checking digest 
> algorithms. Other small changes include changes in `SignerInfo` to use a 
> record to store info about the algorithms to be checked, and removing an 
> unnecessary CRL checking method from `AlgorithmChecker`.
> 
> `keytool` will be enhanced in a subsequent CR to call the new methods.

Sean Mullan has updated the pull request incrementally with one additional 
commit since the last revision:

  Use var in for loop in SignerInfo.
  In TimestampCheck test, combine/simplify what messages should not be emitted
  when jar is signed with 512-bit RSA key.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6296/files
  - new: https://git.openjdk.java.net/jdk/pull/6296/files/6c1f1dd8..ac6d9436

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=6296=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6296=00-01

  Stats: 13 lines in 2 files changed: 0 ins; 8 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6296.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6296/head:pull/6296

PR: https://git.openjdk.java.net/jdk/pull/6296