Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v5]
On Tue, 3 May 2022 14:54:21 GMT, Hai-May Chao wrote:
>> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2196:
>>
>>> 2194:
>>> 2195: try {
>>> 2196: SecretKey secKey = (SecretKey) keyStore.getKey(alias,
>>> storePass);
>>
>> This means any secret key entries that are protected by a different password
>> than `storePass` will throw an `UnrecoverableKeyException` and we will not
>> be able to check the constraints. For PKCS12 this is not an issue since
>> `storePass` and `keyPass` have to be the same. But for other keystores like
>> JCEKS it might be a problem. However, I note this is not really a new issue
>> as details about secret key entries other than the fact they exist as
>> entries are not listed, presumably because we may not have the right
>> password.
>>
>> I would recommend adding a comment explaining this.
>>
>> For a future RFE, it might be useful to enhance `keytool -list -alias` to
>> have a `-keypass` option so that additional information for entries
>> protected by a different password than `storePass` could be listed, such as
>> their algorithm and key size.
>
> Comment added.
Filed RFE JDK-8286031 as suggested.
-
PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v5]
On Mon, 2 May 2022 15:08:17 GMT, Sean Mullan wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updated spec in java.security
>
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2196:
>
>> 2194:
>> 2195: try {
>> 2196: SecretKey secKey = (SecretKey) keyStore.getKey(alias,
>> storePass);
>
> This means any secret key entries that are protected by a different password
> than `storePass` will throw an `UnrecoverableKeyException` and we will not be
> able to check the constraints. For PKCS12 this is not an issue since
> `storePass` and `keyPass` have to be the same. But for other keystores like
> JCEKS it might be a problem. However, I note this is not really a new issue
> as details about secret key entries other than the fact they exist as entries
> are not listed, presumably because we may not have the right password.
>
> I would recommend adding a comment explaining this.
>
> For a future RFE, it might be useful to enhance `keytool -list -alias` to
> have a `-keypass` option so that additional information for entries protected
> by a different password than `storePass` could be listed, such as their
> algorithm and key size.
Comment added.
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5039:
>
>> 5037: private void checkWeakConstraint(String label, String algName,
>> 5038: ConstraintsParameters cp) {
>> 5039: // Do not check disabled algorithms for symmetric key based
>> algorithms for now
>
> If this method is intended only to be called for symmetric keys, you should
> change the type of `cp` to `SecretKeyConstraintsParameters`.
Done.
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5248:
>
>> 5246: private static class SecretKeyConstraintsParameters implements
>> ConstraintsParameters {
>> 5247: private final Key key;
>> 5248: public SecretKeyConstraintsParameters(Key key) {
>
> This ctor could just be private.
Done.
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5259:
>
>> 5257: @Override
>> 5258: public Set getKeys() {
>> 5259: return null;
>
> You should return `Set.of(key)` here. This allows the constraints to also
> check the key size, if that is also restricted. I would suggest adding a test
> where `jdk.security.legacyAlgorithms` includes a constraint for "AES < 256"
> to see if `keytool` warns about it when generating an AES key of 128 bits.
Fixed this method. Changed to check the key size (via passing checkKey=true).
Added the suggested test to ensure that keytool will emit warning accordingly.
-
PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v5]
On Mon, 2 May 2022 22:38:18 GMT, Weijun Wang wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updated spec in java.security
>
> test/jdk/sun/security/tools/keytool/WeakSecretKeyTest.java line 66:
>
>> 64: .shouldContain("Warning")
>> 65: .shouldMatch(" uses the DESede
>> algorithm.*considered a security risk")
>> 66: .shouldMatch(" uses the DES/CBC
>> algorithm.*considered a security risk")
>
> Please update "DES/CBC" to "DES". I've just fixed it with
> https://github.com/openjdk/jdk/commit/50a4df87c87febdf5fa8561b7d0d21b8d6623943.
Ok, thanks for the heads-up.
-
PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v5]
On Fri, 29 Apr 2022 19:42:27 GMT, Hai-May Chao 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:
>
> Updated spec in java.security
test/jdk/sun/security/tools/keytool/WeakSecretKeyTest.java line 66:
> 64: .shouldContain("Warning")
> 65: .shouldMatch(" uses the DESede
> algorithm.*considered a security risk")
> 66: .shouldMatch(" uses the DES/CBC
> algorithm.*considered a security risk")
Please update "DES/CBC" to "DES". I've just fixed it with
https://github.com/openjdk/jdk/commit/50a4df87c87febdf5fa8561b7d0d21b8d6623943.
-
PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v5]
On Fri, 29 Apr 2022 19:42:27 GMT, Hai-May Chao 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: > > Updated spec in java.security If one generates a secret key with `-keyalg des`, then the warning message also uses lowercase `des`. It probably should be standardized. - PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v5]
On Fri, 29 Apr 2022 19:42:27 GMT, Hai-May Chao 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:
>
> Updated spec in java.security
Changes requested by mullan (Reviewer).
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2196:
> 2194:
> 2195: try {
> 2196: SecretKey secKey = (SecretKey) keyStore.getKey(alias,
> storePass);
This means any secret key entries that are protected by a different password
than `storePass` will throw an `UnrecoverableKeyException` and we will not be
able to check the constraints. For PKCS12 this is not an issue since
`storePass` and `keyPass` have to be the same. But for other keystores like
JCEKS it might be a problem. However, I note this is not really a new issue as
details about secret key entries other than the fact they exist as entries are
not listed, presumably because we may not have the right password.
I would recommend adding a comment explaining this.
For a future RFE, it might be useful to enhance `keytool -list -alias` to have
a `-keypass` option so that additional information for entries protected by a
different password than `storePass` could be listed, such as their algorithm
and key size.
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5039:
> 5037: private void checkWeakConstraint(String label, String algName,
> 5038: ConstraintsParameters cp) {
> 5039: // Do not check disabled algorithms for symmetric key based
> algorithms for now
If this method is intended only to be called for symmetric keys, you should
change the type of `cp` to `SecretKeyConstraintsParameters`.
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5248:
> 5246: private static class SecretKeyConstraintsParameters implements
> ConstraintsParameters {
> 5247: private final Key key;
> 5248: public SecretKeyConstraintsParameters(Key key) {
This ctor could just be private.
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5259:
> 5257: @Override
> 5258: public Set getKeys() {
> 5259: return null;
You should return `Set.of(key)` here. This allows the constraints to also check
the key size, if that is also restricted. I would suggest adding a test where
`jdk.security.legacyAlgorithms` includes a constraint for "AES < 256" to see if
`keytool` warns about it when generating an AES key of 128 bits.
-
PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v5]
> 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: Updated spec in java.security - Changes: - all: https://git.openjdk.java.net/jdk/pull/8300/files - new: https://git.openjdk.java.net/jdk/pull/8300/files/e10ed759..ac92a5f7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8300=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8300=03-04 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8300.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8300/head:pull/8300 PR: https://git.openjdk.java.net/jdk/pull/8300
