On Thu, 28 Apr 2022 06:46:35 GMT, Hai-May Chao <[email protected]> wrote:
>> Please review these changes to add DES/3DES/MD5 to
>> `jdk.security.legacyAlgorithms` security property, and to add the legacy
>> algorithm constraint checking to `keytool` commands that are associated with
>> secret key entries stored in the keystore. These `keytool` commands are
>> -genseckey, -importpass, -list, and -importkeystore. As a result, `keytool`
>> will be able to generate warnings when it detects that the secret key based
>> algorithms and PBE based Mac and cipher algorithms are weak. Also removes
>> the "This algorithm will be disabled in a future update.” from the existing
>> warnings for the asymmetric keys/certificates.
>> Will also file a CSR.
>
> Hai-May Chao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> SecretKeyConstraintsParameters subclass created and property description
> updated
A few more comments.
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1862:
> 1860: keysize = 168;
> 1861: } else if ("RC2".equalsIgnoreCase(keyAlgName)) {
> 1862: keysize = 128;
There are other algorithms that are also weak such as RC4, and maybe more in
the future. Rather than special-casing each of them, I think instead you should
call `keygen.init()` for DES and DESede only where you know that the keysize is
always fixed.
src/java.base/share/conf/security/java.security line 641:
> 639:
> 640: #
> 641: # Legacy cryptographic algorithms and key lengths
Nit: add period at end of sentence.
test/jdk/sun/security/tools/keytool/ReadJar.java line 26:
> 24: /**
> 25: * @test
> 26: * @bug 6890872 8168882 8257722 8255552
Here we are just updating the warning message, so I don't think the bugid needs
to be added.
test/jdk/sun/security/tools/keytool/ReadJar.java line 162:
> 160: .shouldContain("Certificate #2:")
> 161: .shouldContain("Signer #2:")
> 162: .shouldNotMatch("The certificate #.* of signer #.*" +
> "uses the SHA1withRSA.*will be disabled")
You probably don't need to check for a non-occurrence here since the message
has been changed and can no longer occur. I also think it doesn't need to list
the bugid because it is not testing the main fix which is warnings on symmetric
key algs.
-------------
Changes requested by mullan (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8300
