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 tier1/tier2 passed. - PR: https://git.openjdk.java.net/jdk/pull/8065
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 tier1/tier2 tests pass. Did not try infra or JCK yet. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v8]
On Wed, 4 May 2022 05:55:08 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:
>
> Skip alg constraint check for PBE secret key entry
Changes requested by mullan (Reviewer).
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2208:
> 2206: * is not really a new issue as details about secret
> key entries
> 2207: * other than the fact they exist as entries are not
> listed ,
> 2208: * presumably because we may not have the right
> password.
I would leave out this last sentence as that was more of an editorial comment
by me. In the first sentence, I would add at the end "... and we will not be
able to check the constraints because we do not have the keyPass for this
operation."
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5286:
> 5284: @Override
> 5285: public Set getKeys() {
> 5286: return (key == null) ? Set.of() : Set.of(key);
key should never be null, so you don't need to check for this.
test/jdk/sun/security/tools/keytool/WeakSecretKeyTest.java line 92:
> 90: .shouldContain("Warning")
> 91: .shouldMatch("The generated secret key uses a 128-bit AES
> key.*considered a security risk")
> 92: .shouldHaveExitValue(0);
Nice - thanks for adding this test.
-
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 [v2]
On Wed, 4 May 2022 03:18:43 GMT, Weijun Wang 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 > > Also, please remove trailing spaces and create a new commit. Skara dislikes > trailing spaces and TAB characters in source code. @wangweij - I believe the change to be simple enough I'll go ahead and finalize it. However, I've proposed updates to the documentation, how are these actioned, i.e. what steps are required of me? - 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, 4 May 2022 03:18:43 GMT, Weijun Wang 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 > > Also, please remove trailing spaces and create a new commit. Skara dislikes > trailing spaces and TAB characters in source code. > @wangweij - I believe the change to be simple enough I'll go ahead and > finalize it. However, I've proposed updates to the documentation, how are > these actioned, i.e. what steps are required of me? I filed a doc task at https://bugs.openjdk.java.net/browse/JDK-8286141. It will be picked up by the doc team. We will also need to write a release note. If you have any recommended text, you can write here. - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8285516: clearPassword should be called in a finally try block [v2]
On Mon, 25 Apr 2022 14:23:17 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> Could I have the simple update reviewed? >> >> In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() >> should be called in a finally try block. Otherwise, the password cleanup >> could be interrupted by exceptions. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > an extra whitespace added Could someone in Oracle help to run Mach5 testing (tier1 and tier2), just in case unexpected failure happens? - PR: https://git.openjdk.java.net/jdk/pull/8377
Re: RFR: 8285516: clearPassword should be called in a finally try block [v2]
On Mon, 25 Apr 2022 14:23:17 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> Could I have the simple update reviewed? >> >> In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() >> should be called in a finally try block. Otherwise, the password cleanup >> could be interrupted by exceptions. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > an extra whitespace added Please merge your PR with master and I can run it for you. - PR: https://git.openjdk.java.net/jdk/pull/8377
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 Infra + JCK passed. Looks good. - PR: https://git.openjdk.java.net/jdk/pull/7664
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 linux-x64 Infra + JCK passed. Looks good. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8285516: clearPassword should be called in a finally try block [v2]
On Mon, 25 Apr 2022 14:23:17 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> Could I have the simple update reviewed? >> >> In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() >> should be called in a finally try block. Otherwise, the password cleanup >> could be interrupted by exceptions. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > an extra whitespace added Marked as reviewed by hchao (Committer). - PR: https://git.openjdk.java.net/jdk/pull/8377
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v8]
On Wed, 4 May 2022 16:29:09 GMT, Sean Mullan wrote:
>> 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
>
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2208:
>
>> 2206: * is not really a new issue as details about secret
>> key entries
>> 2207: * other than the fact they exist as entries are not
>> listed ,
>> 2208: * presumably because we may not have the right
>> password.
>
> I would leave out this last sentence as that was more of an editorial comment
> by me. In the first sentence, I would add at the end "... and we will not be
> able to check the constraints because we do not have the keyPass for this
> operation."
Comment updated.
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 5286:
>
>> 5284: @Override
>> 5285: public Set getKeys() {
>> 5286: return (key == null) ? Set.of() : Set.of(key);
>
> key should never be null, so you don't need to check for this.
Removed the extra check.
-
PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v9]
> 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 comment and getKeys() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8300/files - new: https://git.openjdk.java.net/jdk/pull/8300/files/664f116a..a77ed4f1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8300&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8300&range=07-08 Stats: 5 lines in 1 file changed: 1 ins; 1 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: 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 [v9]
On Wed, 4 May 2022 20:16:12 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 comment and getKeys() I think there is still one outstanding comment from Max, but the fix looks good for me now. - Marked as reviewed by mullan (Reviewer). 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 [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 Found an issue with my editor where whitespace was not being rendered :( - will check the diff in future - 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, 4 May 2022 03:10:10 GMT, Weijun Wang 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
>
> 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.
I was using type safety to remove the chance of non-expected values being
passed to the C function. Implemented your recommendation as its a simple
contract between two files
-
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 [v3]
> 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: Removed whitespace and simply passing ints between java and C++ - Changes: - all: https://git.openjdk.java.net/jdk/pull/8211/files - new: https://git.openjdk.java.net/jdk/pull/8211/files/881bc600..5b3d4115 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8211&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8211&range=01-02 Stats: 24 lines in 1 file changed: 0 ins; 13 del; 11 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: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v9]
On Wed, 4 May 2022 20:16:12 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 comment and getKeys() Marked as reviewed by weijun (Reviewer). Most of my comments are all resolved. The cast to `SecretKey` one is not a real issue and I'm OK with it now. - PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v9]
On Thu, 28 Apr 2022 13:47:05 GMT, Sean Mullan wrote: >> Changes requested by mullan (Reviewer). > >> @seanjmullan Since we use symmetric keys to encrypt entries and add >> integrity check, should this enhancement cover them as well? For example, if >> a PKCS12 keystore is created with `-J-Dkeystore.pkcs12.legacy=true`, should >> the algorithms used be warned? BTW, in legacy mode, we use >> PBEWithSHA1AndRC2_40 when encrypting keys. Should the security property >> include "RC2" as well? >> >> Not sure if it's doable, because those are PKCS12-specific codes. `keytool` >> is not able to see them. > > Right, I think this would require knowledge of what keystore type is being > used and parsing the PKCS12 encoded bytes which seems beyond the scope of > this RFE. Also, those algorithms are disabled by default, so in some sense > the user is making a decision to use them by enabling the system property and > therefore are taking the risk themselves. @seanjmullan @wangweij Thanks for the review! - PR: https://git.openjdk.java.net/jdk/pull/8300
Integrated: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms
On Tue, 19 Apr 2022 16:08:28 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. This pull request has now been integrated. Changeset: 09e6ee96 Author:Hai-May Chao URL: https://git.openjdk.java.net/jdk/commit/09e6ee96bd448838491e5e8634a898e248f1c44e Stats: 362 lines in 6 files changed: 277 ins; 2 del; 83 mod 822: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms Reviewed-by: mullan, weijun - PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Tue, 3 May 2022 00:17:11 GMT, Weijun Wang wrote: >> An example is RSASSA-PSS, i.e. it requires the caller to explicitly state >> which message digest to use, etc. > > You listed 2 cases when null is returned: 1) not supplied. 2) cannot > generate. My understanding is that the RSASSA-PSS case is 1), and the EdDSA > case is 2). This is why I suggested an "or" relation between them. For EdDSA, it's really the case of the signature impl cannot convert the supplied parameter values to "AlgorithmParameters" object. - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8285516: clearPassword should be called in a finally try block [v3]
> Hi, > > Could I have the simple update reviewed? > > In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() should > be called in a finally try block. Otherwise, the password cleanup could be > interrupted by exceptions. > > Thanks, > Xuelei Xue-Lei Andrew Fan has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge - an extra whitespace added - 8285516: clearPassword should be called in a finally try block - Changes: - all: https://git.openjdk.java.net/jdk/pull/8377/files - new: https://git.openjdk.java.net/jdk/pull/8377/files/99079d30..1df828df Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8377&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8377&range=01-02 Stats: 32346 lines in 832 files changed: 22171 ins; 4511 del; 5664 mod Patch: https://git.openjdk.java.net/jdk/pull/8377.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8377/head:pull/8377 PR: https://git.openjdk.java.net/jdk/pull/8377
Re: RFR: 8285516: clearPassword should be called in a finally try block [v2]
On Wed, 4 May 2022 17:35:13 GMT, Weijun Wang wrote: > Please merge your PR with master and I can run it for you. Merged. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/8377
