Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v8]
> 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: Skip alg constraint check for PBE secret key entry - Changes: - all: https://git.openjdk.java.net/jdk/pull/8300/files - new: https://git.openjdk.java.net/jdk/pull/8300/files/19afc42e..664f116a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8300=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8300=06-07 Stats: 30 lines in 2 files changed: 13 ins; 13 del; 4 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
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v7]
> 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: Revert changes to StorePasswords.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/8300/files - new: https://git.openjdk.java.net/jdk/pull/8300/files/2fa72aa8..19afc42e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8300=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8300=05-06 Stats: 8 lines in 1 file changed: 0 ins; 5 del; 3 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
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Mon, 2 May 2022 21:42:28 GMT, Valerie Peng wrote:
>>> What kind of additional sentence do you have in mind?
>>
>> It may be fine to put it into the state for 'null" returned value. For
>> example:
>>
>>
>> The returned parameters may be the same that were used to initialize
>> this signature, or may contain additional default or random parameter
>> values used by the underlying signature implementation, or null if the
>> underlying signature implementation does not support returning the
>> parameters as {@code AlgorithmParameters}.
>>
>>
>>
>> The null return conditional in the following sentence may be able to combine
>> together.
>>
>>
>> The returned parameters may be the same that were used to initialize
>> this signature, or may contain additional default or random parameter
>> values used by the underlying signature implementation. {@code null}
>> may be returned if the underlying signature implementation does not
>> support returning the parameters as {@code AlgorithmParameters}, or > conditions>
>
> How about the case when no parameters are given? Say A is the user-supplied
> values, B is the provider specific default or random values, your suggestion
> has A, A+B, and null. Isn't the sentence about B needed (no A and provider
> can generate the parameters)?
I think this sentence covers case B, "... or may contain additional default or
random parameter
values used by the underlying signature implementation."
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v2]
On Tue, 3 May 2022 22:52:49 GMT, Mat Carter wrote: >> On Windows you can now access the local machine keystores using the strings >> "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the >> application requires admin privileges. >> >> "Windows-MY" and "Windows-ROOT" remain unchanged, however given these >> original keystore strings mapped to the current user, I added >> "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer >> can explicitly specify the current user location. These two new strings >> simply map to the original two strings, i.e. no duplication of code paths etc >> >> No new tests added, keystore functionality and API remains unchanged, the >> local machine keystore types would require the tests to run in admin mode >> >> Tested on windows, passes tier1 and tier2 tests > > Mat Carter has updated the pull request incrementally with one additional > commit since the last revision: > > replace string parameter with int and supporting constants Also, please remove trailing spaces and create a new commit. Skara dislikes trailing spaces and TAB characters in source code. - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v2]
On Tue, 3 May 2022 23:38:38 GMT, Mat Carter wrote: >> Mat Carter has updated the pull request incrementally with one additional >> commit since the last revision: >> >> replace string parameter with int and supporting constants > > I don't use this API much so I don't really have an opinion as a customer > regarding the format of the strings used to identify the key stores. I'd be > happy to review a separate PR but I think this falls outside the scope of > this PR which specifically targets the inability to access local machine key > stores (which a bug has been raised against). > > note: there's also a "Windows-PRNG" which isn't a key store but the native > random number generator @macarte I've added my name as reviewer in the CSR. You can either propose it (if you expect some feedback from the CSR reviewers) or just finalize it (if you think the change is simple and obvious). - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v2]
On Tue, 3 May 2022 22:52:49 GMT, Mat Carter wrote:
>> On Windows you can now access the local machine keystores using the strings
>> "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the
>> application requires admin privileges.
>>
>> "Windows-MY" and "Windows-ROOT" remain unchanged, however given these
>> original keystore strings mapped to the current user, I added
>> "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer
>> can explicitly specify the current user location. These two new strings
>> simply map to the original two strings, i.e. no duplication of code paths etc
>>
>> No new tests added, keystore functionality and API remains unchanged, the
>> local machine keystore types would require the tests to run in admin mode
>>
>> Tested on windows, passes tier1 and tier2 tests
>
> Mat Carter has updated the pull request incrementally with one additional
> commit since the last revision:
>
> replace string parameter with int and supporting constants
src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CKeyStore.java line
256:
> 254: private final KeyStoreLocation storeLocation;
> 255:
> 256: CKeyStore(String storeName, KeyStoreLocation storeLocation) {
Why not just an `int` here? The creation of a separate class `keyStoreLocation`
seems not necessary. If you want code to be readable, just add `public static
final int CURRENTUSER = 0`, etc.
-
PR: https://git.openjdk.java.net/jdk/pull/8211
Integrated: 8286069: keytool prints out wrong key algorithm for -importpass command
On Tue, 3 May 2022 17:51:43 GMT, Weijun Wang wrote:
> Since `keytool -importpass` always uses `KeyFactory.getInstance("PBE")` to
> generate the secret key, and "PBE" is an alias of "PBEwithMD5andDES" inside
> the SunJCE security provider, its `getAlgorithm` is always `PBEwithMD5andDES`.
>
> This code change modifies it to "PBE".
>
> Note that I haven't chosen the `-keyalg` option value here because it is
> actually the algorithm used to protect the PBE secret key entry. It's a
> cipher algorithm instead of a key algorithm.
This pull request has now been integrated.
Changeset: 075ce8a0
Author:Weijun Wang
URL:
https://git.openjdk.java.net/jdk/commit/075ce8a0d0ab279049c81d5ce23fcee3711925e2
Stats: 109 lines in 2 files changed: 107 ins; 1 del; 1 mod
8286069: keytool prints out wrong key algorithm for -importpass command
Reviewed-by: hchao, valeriep
-
PR: https://git.openjdk.java.net/jdk/pull/8520
Re: RFR: 8286069: keytool prints out wrong key algorithm for -importpass command [v2]
> Since `keytool -importpass` always uses `KeyFactory.getInstance("PBE")` to
> generate the secret key, and "PBE" is an alias of "PBEwithMD5andDES" inside
> the SunJCE security provider, its `getAlgorithm` is always `PBEwithMD5andDES`.
>
> This code change modifies it to "PBE".
>
> Note that I haven't chosen the `-keyalg` option value here because it is
> actually the algorithm used to protect the PBE secret key entry. It's a
> cipher algorithm instead of a key algorithm.
Weijun Wang has updated the pull request incrementally with one additional
commit since the last revision:
algorithm id
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8520/files
- new: https://git.openjdk.java.net/jdk/pull/8520/files/e99bdc32..a45a500b
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8520=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8520=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/8520.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8520/head:pull/8520
PR: https://git.openjdk.java.net/jdk/pull/8520
Re: RFR: 8286069: keytool prints out wrong key algorithm for -importpass command
On Wed, 4 May 2022 01:50:34 GMT, Valerie Peng wrote:
>> Since `keytool -importpass` always uses `KeyFactory.getInstance("PBE")` to
>> generate the secret key, and "PBE" is an alias of "PBEwithMD5andDES" inside
>> the SunJCE security provider, its `getAlgorithm` is always
>> `PBEwithMD5andDES`.
>>
>> This code change modifies it to "PBE".
>>
>> Note that I haven't chosen the `-keyalg` option value here because it is
>> actually the algorithm used to protect the PBE secret key entry. It's a
>> cipher algorithm instead of a key algorithm.
>
> test/jdk/sun/security/pkcs12/ImportPassKeyAlg.java line 75:
>
>> 73: .shouldContain("Generated PBE secret key");
>> 74:
>> 75: // The aid of a protected entry (at 110c010c01010c0 inside p12)
>> is:
>
> nit: use "algorithm id" instead.
No problem.
-
PR: https://git.openjdk.java.net/jdk/pull/8520
Re: RFR: 8286069: keytool prints out wrong key algorithm for -importpass command
On Tue, 3 May 2022 17:51:43 GMT, Weijun Wang wrote:
> Since `keytool -importpass` always uses `KeyFactory.getInstance("PBE")` to
> generate the secret key, and "PBE" is an alias of "PBEwithMD5andDES" inside
> the SunJCE security provider, its `getAlgorithm` is always `PBEwithMD5andDES`.
>
> This code change modifies it to "PBE".
>
> Note that I haven't chosen the `-keyalg` option value here because it is
> actually the algorithm used to protect the PBE secret key entry. It's a
> cipher algorithm instead of a key algorithm.
Marked as reviewed by valeriep (Reviewer).
test/jdk/sun/security/pkcs12/ImportPassKeyAlg.java line 75:
> 73: .shouldContain("Generated PBE secret key");
> 74:
> 75: // The aid of a protected entry (at 110c010c01010c0 inside p12)
> is:
nit: use "algorithm id" instead.
-
PR: https://git.openjdk.java.net/jdk/pull/8520
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 Thanks for the extra time to review this change. I'm still wondering if there is better pattern that doesn't use user_canceled, but that doesn't need to delay this fix from going in. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
On Tue, 3 May 2022 23:10:51 GMT, Xue-Lei Andrew Fan wrote: > Could someone in Oracle help to run the Mach5 testing for security and > network components (or just tier1 and tier2), and let me know if this update > causes any failures? Builds underway. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v2]
On Tue, 3 May 2022 22:52:49 GMT, Mat Carter wrote: >> On Windows you can now access the local machine keystores using the strings >> "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the >> application requires admin privileges. >> >> "Windows-MY" and "Windows-ROOT" remain unchanged, however given these >> original keystore strings mapped to the current user, I added >> "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer >> can explicitly specify the current user location. These two new strings >> simply map to the original two strings, i.e. no duplication of code paths etc >> >> No new tests added, keystore functionality and API remains unchanged, the >> local machine keystore types would require the tests to run in admin mode >> >> Tested on windows, passes tier1 and tier2 tests > > Mat Carter has updated the pull request incrementally with one additional > commit since the last revision: > > replace string parameter with int and supporting constants I don't use this API much so I don't really have an opinion as a customer regarding the format of the strings used to identify the key stores. I'd be happy to review a separate PR but I think this falls outside the scope of this PR which specifically targets the inability to access local machine key stores (which a bug has been raised against). note: there's also a "Windows-PRNG" which isn't a key store but the native random number generator - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v2]
On Wed, 27 Apr 2022 21:47:15 GMT, Mat Carter wrote: >> Thanks for the feedback, I'm going to incorporate that into the PR > >> And also, is there a ReleaseString missing? > > Yes an error when I "patched" my repo, but based on the feedback there will > no longer be a string to release :) Committed changes to use ints instead of strings - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
On Tue, 3 May 2022 02:07:13 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More note update Could someone in Oracle help to run the Mach5 testing for security and network components (or just tier1 and tier2), and let me know if this update cause any failures? - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v14]
On Tue, 3 May 2022 02:20:23 GMT, Xue-Lei Andrew Fan wrote: >> Hi. Sorry, I should have brought this up earlier, but there is a jtreg test >> library to help with ensuring the GC runs, >> `test/lib/jdk/test/lib/util/ForceGC.java`. You might consider replacing the >> test code that runs/checks the GC with `ForceGC.await()`. > >> Hi. Sorry, I should have brought this up earlier, but there is a jtreg test >> library to help with ensuring the GC runs, >> `test/lib/jdk/test/lib/util/ForceGC.java`. You might consider replacing the >> test code that runs/checks the GC with `ForceGC.await()`. > > It looks like a graceful GC utility. There are also some other test cases I > committed that do no use ForceGC. I filed a [new > bug](https://bugs.openjdk.java.net/browse/JDK-8286045), and will make the > update all together. Thank you. > @XueleiFan Did you check to make sure someone ran Mach5 and the results were > ok before integrating? If I'm right, Weijun made the test. Please let me know if there are mach5 testing failures. BTW, it would be nice if Mach5 testing could be supported to run in Github. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v2]
> On Windows you can now access the local machine keystores using the strings > "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the > application requires admin privileges. > > "Windows-MY" and "Windows-ROOT" remain unchanged, however given these > original keystore strings mapped to the current user, I added > "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer > can explicitly specify the current user location. These two new strings > simply map to the original two strings, i.e. no duplication of code paths etc > > No new tests added, keystore functionality and API remains unchanged, the > local machine keystore types would require the tests to run in admin mode > > Tested on windows, passes tier1 and tier2 tests Mat Carter has updated the pull request incrementally with one additional commit since the last revision: replace string parameter with int and supporting constants - Changes: - all: https://git.openjdk.java.net/jdk/pull/8211/files - new: https://git.openjdk.java.net/jdk/pull/8211/files/86b5ced5..881bc600 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8211=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8211=00-01 Stats: 45 lines in 2 files changed: 21 ins; 7 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/8211.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8211/head:pull/8211 PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8286069: keytool prints out wrong key algorithm for -importpass command
On Tue, 3 May 2022 17:51:43 GMT, Weijun Wang wrote:
> Since `keytool -importpass` always uses `KeyFactory.getInstance("PBE")` to
> generate the secret key, and "PBE" is an alias of "PBEwithMD5andDES" inside
> the SunJCE security provider, its `getAlgorithm` is always `PBEwithMD5andDES`.
>
> This code change modifies it to "PBE".
>
> Note that I haven't chosen the `-keyalg` option value here because it is
> actually the algorithm used to protect the PBE secret key entry. It's a
> cipher algorithm instead of a key algorithm.
LGTM.
-
Marked as reviewed by hchao (Committer).
PR: https://git.openjdk.java.net/jdk/pull/8520
Re: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true'
Hi Andrey, On 4/29/22 11:59 AM, Andrey Turbanov wrote: Hello. I found a suspicious code in CryptoPolicyParser method calls. Method 'isConsistent' is called only from a method 'parsePermissionEntry'. It accepts the 'processedPermissions' parameter from 'parsePermissionEntry'. Method 'parsePermissionEntry' is called only from a method 'parseGrantEntry'. It accepts the 'processedPermissions' parameter from 'parseGrantEntry'. Method 'parseGrantEntry' is called only from a method 'read' and always with null value of parameter 'processedPermissions'. So, it seems in method 'isConsistent' value of parameter 'processedPermissions' will always be 'null'. And the method will always return true. Is this the result of some refactoring? Or did I miss something? No, I don't think so. Good catch. It looks like a bug to me. Can you file a bug report? Thanks, Sean
Re: RFR: 8285380: Fix typos in security
On Thu, 21 Apr 2022 16:10:54 GMT, Alan Bateman wrote: > > @AlanBateman So there is even more 3rd party code in there? :-( I tried to > > ignore fixes for all files that I could identify as 3rd party. It's > > actually a bit annoying that we have imported source code thrown around > > like this in the source tree, so there is no clear boundary between code we > > own and code we import from someone else... > > security-dev can say for sure but the only 3rd party code I see in this > change is in the src/java.xml.crypto/share/classes/com/sun/org/apache tree > (the package name gives a hint has it was it was re-packaged). The `src/java.xml.crypto/share/classes/org/jcp` tree is also 3rd party Apache code. However, I am ok with including the spelling fixes for the 3rd-party Apache code (this tree and the one Alan mentions above which you already reverted, but can put it back) with this PR. I am a Committer on the Apache Santuario project so I can push these spelling fixes (they should be non-controversial) to their code base and there won't be any conflicts when we merge up the JDK to the next Santuario release. - PR: https://git.openjdk.java.net/jdk/pull/8340
RFR: 8002277: Refactor two PBE classes to simplify maintenance
This change refactors the PBES2Core and PKCS12PBECipherCore classes in SunJCE provider as requested in the bug record. Functionality should remain the same with a clearer and simplified code/control flow with less lines of code. This should improve readability and maintenance. I enhanced one existing regression test to test more scenarios. This test would pass before the proposed change and continues to pass with the proposed changes. - Commit messages: - Enhanced test with more decryption w/o parameters scenario. - 8002277: Refactor two PBE classes to simplify maintenance Changes: https://git.openjdk.java.net/jdk/pull/8521/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8521=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8002277 Stats: 542 lines in 3 files changed: 168 ins; 289 del; 85 mod Patch: https://git.openjdk.java.net/jdk/pull/8521.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8521/head:pull/8521 PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v14]
On Tue, 3 May 2022 02:20:23 GMT, Xue-Lei Andrew Fan wrote: >> Hi. Sorry, I should have brought this up earlier, but there is a jtreg test >> library to help with ensuring the GC runs, >> `test/lib/jdk/test/lib/util/ForceGC.java`. You might consider replacing the >> test code that runs/checks the GC with `ForceGC.await()`. > >> Hi. Sorry, I should have brought this up earlier, but there is a jtreg test >> library to help with ensuring the GC runs, >> `test/lib/jdk/test/lib/util/ForceGC.java`. You might consider replacing the >> test code that runs/checks the GC with `ForceGC.await()`. > > It looks like a graceful GC utility. There are also some other test cases I > committed that do no use ForceGC. I filed a [new > bug](https://bugs.openjdk.java.net/browse/JDK-8286045), and will make the > update all together. Thank you. @XueleiFan Did you check to make sure someone ran Mach5 and the results were ok before integrating? - PR: https://git.openjdk.java.net/jdk/pull/8136
RFR: 8286069: keytool prints out wrong key algorithm for -importpass command
Since `keytool -importpass` always uses `KeyFactory.getInstance("PBE")` to
generate the secret key, and "PBE" is an alias of "PBEwithMD5andDES" inside the
SunJCE security provider, its `getAlgorithm` is always `PBEwithMD5andDES`.
This code change modifies it to "PBE".
Note that I haven't chosen the `-keyalg` option value here because it is
actually the algorithm used to protect the PBE secret key entry. It's a cipher
algorithm instead of a key algorithm.
-
Commit messages:
- the fix
Changes: https://git.openjdk.java.net/jdk/pull/8520/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8520=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8286069
Stats: 109 lines in 2 files changed: 107 ins; 1 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/8520.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8520/head:pull/8520
PR: https://git.openjdk.java.net/jdk/pull/8520
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v6]
On Tue, 3 May 2022 14:54:05 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: > > Update per review comments I found a keytool bug. It prints out "PBEWithMD5AndDES" no matter what key algorithm is used in `keytool -importpass`. Together with the code change here, it means a warning will always be shown even if we choose a strong algorithm. https://bugs.openjdk.java.net/browse/JDK-8286069 filed. - PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
On Tue, 3 May 2022 02:07:13 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More note update CSR and release note requests are filed in JBS. May I have them reviewed? CSR: https://bugs.openjdk.java.net/browse/JDK-8286064 Release Note: https://bugs.openjdk.java.net/browse/JDK-8286065 - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v6]
On Tue, 3 May 2022 14:54:05 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: > > Update per review comments src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2530: > 2528: } > 2529: } > 2530: While `c == null` usually means this is a secret key entry, this is not guaranteed. How about we put the check on a secret key entry in its own `instanceof` check, then we can save a cast. Also, the `setEntry` is always called and it can be put outside any if/else block. - 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 [v6]
> 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: Update per review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8300/files - new: https://git.openjdk.java.net/jdk/pull/8300/files/ac92a5f7..2fa72aa8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8300=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8300=04-05 Stats: 50 lines in 3 files changed: 35 ins; 1 del; 14 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
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider
On Wed, 27 Apr 2022 21:41:30 GMT, Mat Carter wrote: >> Same question. Does a new type name automagically add support for CNG? > > Correct, it does enable access to certificates and keys that require next > (second) generation cryptographic providers, that were previously > inaccessible. I've just realized the implication of this on existing > applications and so I'm going to pause and run some test, especially around > enumeration Correction: after looking at wincrypt.h, the documentation [1] and running tests, I can confirm that: 1) this change has no functional impact (i.e. results are unchanged) 2) HCRYPTPROV and HCRYPTPROV_OR_NCRYPT_KEY_HANDLE are both the same type (ULONG_PTR) and so are interchangeable (with the former supporting legacy applications) 3) There is only one function for CryptAcquireCertificatePrivateKey, not two differentiated by this parameter type change 4) support for CNG keys, which was originally thought to be added with this change, has always been true due to the existing use of the flag CRYPT_ACQUIRE_ALLOW_NCRYPT_KEY_FLAG I think this change should stay as it more correctly matches the prototype and the use of CRYPT_ACQUIRE_ALLOW_NCRYPT_KEY_FLAG [1] https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptacquirecertificateprivatekey - PR: https://git.openjdk.java.net/jdk/pull/8211
Integrated: 8284490: Remove finalizer method in java.security.jgss
On Thu, 7 Apr 2022 04:10:55 GMT, Xue-Lei Andrew Fan wrote: > Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. This pull request has now been integrated. Changeset: ffca23a5 Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/ffca23a5313855a6f9797ad6b342bb2e2cb1b49b Stats: 430 lines in 11 files changed: 393 ins; 12 del; 25 mod 8284490: Remove finalizer method in java.security.jgss Reviewed-by: rriggs, dfuchs, weijun - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
On Tue, 3 May 2022 02:07:13 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More note update Given that this proposed change will change the behavior for SSLSocket (& SSLServerSocket?) I would also suggest to file a CSR before integrating. - PR: https://git.openjdk.java.net/jdk/pull/8065
