Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v5]
On Tue, 15 Mar 2022 20:44:20 GMT, Valerie Peng wrote:
>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java
>> line 122:
>>
>>> 120: default -> {
>>> 121: throw new ProviderException
>>> 122: ("Unrecognized algorithm for checking key
>>> size");
>>
>> If it's an unknown key algorithm, is it possible we just ignore it and keep
>> using `minKeyLen` and `maxKeyLen`?
>
> Well, instead of ignore unknown key algorithm, perhaps safer to throw
> Exception so it can be caught and handled during develop time.
> P11KeyPairGenerator class is only used for known algorithms which it is
> registered for, so probably ok to go either way. I'd prefer to play it safe
> and force a review of this block of code when new algorithm is added.
OK.
-
PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v5]
On Mon, 14 Mar 2022 21:24:15 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update again and undo DSA changes
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java
> line 122:
>
>> 120: default -> {
>> 121: throw new ProviderException
>> 122: ("Unrecognized algorithm for checking key
>> size");
>
> If it's an unknown key algorithm, is it possible we just ignore it and keep
> using `minKeyLen` and `maxKeyLen`?
Well, instead of ignore unknown key algorithm, perhaps safer to throw Exception
so it can be caught and handled during develop time. P11KeyPairGenerator class
is only used for known algorithms which it is registered for, so probably ok to
go either way. I'd prefer to play it safe and force a review of this block of
code when new algorithm is added.
-
PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v5]
On Mon, 14 Mar 2022 21:18:56 GMT, Weijun Wang wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update again and undo DSA changes > > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java > line 101: > >> 99: // set default key sizes and apply our own algorithm-specific >> limits >> 100: // override lower limit to disallow unsecure keys being >> generated >> 101: // override upper limit to deter DOS attack > > Not a P11 expert, but I assume `algorithm` here is already guaranteed to be > in uppercase? Yes, for P11KeyPairGenerator, its algorithm values are all in uppercase. I verified it with an existing regression test. - PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v5]
On Mon, 14 Mar 2022 21:08:30 GMT, Weijun Wang wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update again and undo DSA changes
>
> src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java
> line 121:
>
>> 119: v = max;
>> 120: }
>> 121: } catch (NullPointerException | NoSuchAlgorithmException
>> ne) {
>
> There is no need to mention NPE.
Sure.
-
PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v5]
On Mon, 14 Mar 2022 20:08:31 GMT, Valerie Peng wrote:
>> It's been several years since we increased the default key sizes. Before
>> shifting to PQC, NSA replaced its Suite B cryptography recommendations with
>> the Commercial National Security Algorithm Suite which suggests:
>>
>> - SHA-384 for secure hashing
>> - AES-256 for symmetric encryption
>> - RSA with 3072 bit keys for digital signatures and for key exchange
>> - Diffie Hellman (DH) with 3072 bit keys for key exchange
>> - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures
>> (ECDSA)
>>
>> So, this proposed changes made the suggested key size and algorithm changes.
>> The changes are mostly in keytool, jarsigner and their regression tests, so
>> @wangweij Could you please take a look?
>>
>> Thanks!
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update again and undo DSA changes
Some small comments. Otherwise looks fine.
src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java
line 121:
> 119: v = max;
> 120: }
> 121: } catch (NullPointerException | NoSuchAlgorithmException ne)
> {
There is no need to mention NPE.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java
line 101:
> 99: // set default key sizes and apply our own algorithm-specific
> limits
> 100: // override lower limit to disallow unsecure keys being generated
> 101: // override upper limit to deter DOS attack
No a P11 expert, but I assume `algorithm` here is already guaranteed to be in
uppercase?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java
line 122:
> 120: default -> {
> 121: throw new ProviderException
> 122: ("Unrecognized algorithm for checking key size");
If it's an unknown key algorithm, is it possible we just ignore it and keep
using `minKeyLen` and `maxKeyLen`?
-
PR: https://git.openjdk.java.net/jdk/pull/7652
Re: RFR: 8267319: Use larger default key sizes and algorithms based on CNSA [v5]
> It's been several years since we increased the default key sizes. Before > shifting to PQC, NSA replaced its Suite B cryptography recommendations with > the Commercial National Security Algorithm Suite which suggests: > > - SHA-384 for secure hashing > - AES-256 for symmetric encryption > - RSA with 3072 bit keys for digital signatures and for key exchange > - Diffie Hellman (DH) with 3072 bit keys for key exchange > - Elliptic curve [P-384] for key exchange (ECDH) and for digital signatures > (ECDSA) > > So, this proposed changes made the suggested key size and algorithm changes. > The changes are mostly in keytool, jarsigner and their regression tests, so > @wangweij Could you please take a look? > > Thanks! Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Update again and undo DSA changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7652/files - new: https://git.openjdk.java.net/jdk/pull/7652/files/f728aa7d..48f562ab Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7652=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7652=03-04 Stats: 45 lines in 9 files changed: 8 ins; 2 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/7652.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7652/head:pull/7652 PR: https://git.openjdk.java.net/jdk/pull/7652
