RFR: 8282038: CipherSpi.bufferCrypt leaves plaintext copy on the heap
Clearing buffers and temporary arrays to avoid data leaks in cipher operations. - Commit messages: - the fix Changes: https://git.openjdk.org/jdk/pull/9158/files Webrev: https://webrevs.openjdk.org/?repo=jdk=9158=00 Issue: https://bugs.openjdk.org/browse/JDK-8282038 Stats: 49 lines in 3 files changed: 25 ins; 12 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/9158.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9158/head:pull/9158 PR: https://git.openjdk.org/jdk/pull/9158
RFR: 8288209: SSL debug message wrong about unsupported authentication scheme
At the beginning, this bug was about the incorrect warning message "Unsupported authentication scheme" on line 1051 which should have been "This key algorithm has been checked, skip it". Now, it's a code refactoring that emphasizes only the key algorithm inside a signature scheme is checked in these two methods, and therefore the ignore-if-checked logic in the old code is correct. - Commit messages: - the fix Changes: https://git.openjdk.org/jdk/pull/9140/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9140=00 Issue: https://bugs.openjdk.org/browse/JDK-8288209 Stats: 106 lines in 5 files changed: 9 ins; 57 del; 40 mod Patch: https://git.openjdk.org/jdk/pull/9140.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9140/head:pull/9140 PR: https://git.openjdk.org/jdk/pull/9140
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v6]
On Fri, 10 Jun 2022 21:27:58 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done >> in java.security >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/security > > Mark Powers has updated the pull request incrementally with two additional > commits since the last revision: > > - bad grammar > - Max comments Most of the changes are trivial, like those no-more-public for interfaces. Reviewing 94 files is still easier than fixing 94 out of ...er... 185 files. - PR: https://git.openjdk.org/jdk/pull/8319
Re: Integrated: 8288270: Tier1 build failures after JDK-8287178
On Fri, 10 Jun 2022 23:49:45 GMT, Hai-May Chao wrote: > Please review the small fix in comment. LGTM. Thanks. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.org/jdk/pull/9135
Integrated: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0
On Thu, 9 Jun 2022 21:34:56 GMT, Weijun Wang wrote: > Add comment to the method. This pull request has now been integrated. Changeset: d4b473d8 Author: Weijun Wang URL: https://git.openjdk.org/jdk/commit/d4b473d89046874f25aa6f65f3ae96f7d8397d50 Stats: 8 lines in 1 file changed: 7 ins; 0 del; 1 mod 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0 Reviewed-by: jnimeh - PR: https://git.openjdk.org/jdk/pull/9115
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Fri, 10 Jun 2022 00:35:16 GMT, Mark Powers wrote: >> src/java.base/share/classes/java/security/SecureRandom.java line 905: >> >>> 903: private static final Pattern pattern = >>> 904: Pattern.compile( >>> 905: "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?"); >> >> Don't quite understand this change. Can you explain? > > The author must have thought `:` and `,` were metacharacters that needed to > be escaped to turn them into ordinary characters. I verified with a small > java program that` \:` and` \,` are equivalent to `;` and `,`. > The IntelliJ complaint is "Redundant character escape `'\:'`". I see. Thanks for the explanation. >> src/java.base/share/classes/java/security/cert/CertPathValidator.java line >> 335: >> >>> 333: String cpvtype = >>> 334: AccessController.doPrivileged((PrivilegedAction) >>> () -> >>> 335: Security.getProperty(CPV_TYPE)); >> >> See too many, maybe we need a dedicated method in >> `sun.security.util.SecurityProperties`? > > A privilegedGetProperty method? That would be the subject of a new bug I > think. Yes. - PR: https://git.openjdk.org/jdk/pull/8319
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v6]
On Fri, 10 Jun 2022 21:27:58 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done >> in java.security >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/security > > Mark Powers has updated the pull request incrementally with two additional > commits since the last revision: > > - bad grammar > - Max comments Looks good to me now. For `Provider$ServiceKey::matches`, maybe you can add a comment on why `==` is correct. For other changes suggested by IntelliJ, maybe just follow them to make the IDE silent. You decide it. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.org/jdk/pull/8319
Re: RFR: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0 [v2]
On Thu, 9 Jun 2022 22:29:36 GMT, Jamil Nimeh wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> verb > > src/java.base/share/classes/sun/security/util/math/IntegerModuloP.java line > 161: > >> 159: // This method is used in 2 cases: >> 160: // 1. To calculate the inverse of a number in ECDSAOperations, >> 161: //this number must be non zero (modulus p). > > Should that be "modulo p"? Other than that it looks good to me. Yes, it's a verb here. - PR: https://git.openjdk.org/jdk/pull/9115
Re: RFR: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0 [v2]
> Add comment to the method. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: verb - Changes: - all: https://git.openjdk.org/jdk/pull/9115/files - new: https://git.openjdk.org/jdk/pull/9115/files/36741bba..15ef85fe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9115=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9115=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9115.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9115/head:pull/9115 PR: https://git.openjdk.org/jdk/pull/9115
RFR: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0
Add comment to the method. - Commit messages: - add comment Changes: https://git.openjdk.org/jdk/pull/9115/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9115=00 Issue: https://bugs.openjdk.org/browse/JDK-8287178 Stats: 8 lines in 1 file changed: 7 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9115.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9115/head:pull/9115 PR: https://git.openjdk.org/jdk/pull/9115
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Tue, 7 Jun 2022 15:37:02 GMT, Mark Powers wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done
>> in java.security
>>
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for
>> a single code review, so it was decided to split into smaller chunks. This
>> is one such chunk:
>>
>> open/src/java.base/share/classes/java/security
>
> Mark Powers has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 11 commits:
>
> - Merge
> - fourth iteration
> - Merge
> - Merge
> - third iteration
> - Merge
> - Merge
> - second iteration
> - Merge
> - Merge
> - ... and 1 more:
> https://git.openjdk.java.net/jdk/compare/4d6fb515...44140a01
Some comments. Thanks for the hard work.
src/java.base/share/classes/java/security/MessageDigest.java line 306:
> 304: } else {
> 305: return Delegate.of((MessageDigestSpi)objs[0], algorithm,
> 306: (Provider)objs[1]);
Indent this line.
src/java.base/share/classes/java/security/ProtectionDomain.java line 474:
> 472: return true;
> 473: } catch (SecurityException se) {
> 474: // fall through and return false
How about we just `return false` here and then there is no need for the `return
false` at the end?
src/java.base/share/classes/java/security/ProtectionDomain.java line 474:
> 472: return true;
> 473: } catch (SecurityException se) {
> 474: // fall through and return false
How about we just `return false` here and then there is no need for the `return
false` at the end?
src/java.base/share/classes/java/security/ProtectionDomain.java line 492:
> 490: Policy p = Policy.getPolicyNoCheck();
> 491: return p.getPermissions(ProtectionDomain.this);
> 492: });
First, indent these lines. (or maybe my GitHub view has not reveals the
indentation?).
Second, inline `p` then there is no need to `return`. Simply `() -> P.g.g(P)`.
src/java.base/share/classes/java/security/ProtectionDomain.java line 492:
> 490: Policy p = Policy.getPolicyNoCheck();
> 491: return p.getPermissions(ProtectionDomain.this);
> 492: });
First, indent these lines. (or maybe my GitHub view has not reveals the
indentation?).
Second, inline `p` then there is no need to `return`. Simply `() -> P.g.g(P)`.
src/java.base/share/classes/java/security/Provider.java line 1098:
> 1096: boolean matches(String type, String algorithm) {
> 1097: return (Objects.equals(this.type, type)) &&
> 1098: (Objects.equals(this.originalAlgorithm, algorithm));
In fact, I'm not sure why the original code is brave enough to use `==` instead
of `equals`. If you do believe it's a real bug (and can write a regression
test), let's fix it in a real bug fix. Otherwise, keep using `==` and add a
comment saying it's correct.
src/java.base/share/classes/java/security/Provider.java line 1098:
> 1096: boolean matches(String type, String algorithm) {
> 1097: return (Objects.equals(this.type, type)) &&
> 1098: (Objects.equals(this.originalAlgorithm, algorithm));
In fact, I'm not sure why the original code is brave enough to use `==` instead
of `equals`. If you do believe it's a real bug (and can write a regression
test), let's fix it in a real bug fix. Otherwise, keep using `==` and add a
comment saying it's correct.
src/java.base/share/classes/java/security/SecureClassLoader.java line 221:
> 219: // only), and the fragment is not considered.
> 220: CodeSourceKey key = new CodeSourceKey(cs);
> 221: /* not used */
What is not used? `key1`? How about just rename it to `unused`?
src/java.base/share/classes/java/security/SecureClassLoader.java line 221:
> 219: // only), and the fragment is not considered.
> 220: CodeSourceKey key = new CodeSourceKey(cs);
> 221: /* not used */
What is not used? `key1`? How about just rename it to `unused`?
src/java.base/share/classes/java/security/SecureClassLoader.java line 235:
> 233: }
> 234:
> 235: private record CodeSourceKey(CodeSource cs) {
Really necessary?
src/java.base/share/classes/java/security/SecureClassLoader.java line 235:
> 233: }
> 234:
> 235: private record CodeSourceKey(CodeSource cs) {
Really necessary?
src/java.base/share/classes/java/security/SecureRandom.java line 905:
> 903: private static final Pattern pattern =
> 904: Pattern.compile(
> 905: "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?");
Don't quite understand this change. Can you explain?
src/java.base/share/classes/java/security/SecureRandom.java line 905:
> 903: private static final Pattern pattern =
> 904: Pattern.compile(
> 905:
RFR: 6522064: Aliases from Microsoft CryptoAPI has bad character encoding
Switch to wide char version of `CertGetNameString` to get the non-ASCII name. - Commit messages: - the fix Changes: https://git.openjdk.java.net/jdk/pull/9085/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9085=00 Issue: https://bugs.openjdk.org/browse/JDK-6522064 Stats: 89 lines in 2 files changed: 72 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/9085.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9085/head:pull/9085 PR: https://git.openjdk.java.net/jdk/pull/9085
Re: RFR: 8287246: DSAKeyValue should check for missing params instead of relying on KeyFactory provider
On Tue, 24 May 2022 16:29:02 GMT, Sean Mullan wrote: > Please review this fix to the XML Signature implementation to check for null > or missing DSA parameters and throw a MarshalException before trying to > create a DSA public key from its XML encoding. This will allow the code to > fail earlier and not depend on the underlying provider to detect illegal or > missing parameters. Looks good to me. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8870
Re: RFR: 8287246: DSAKeyValue should check for missing params instead of relying on KeyFactory provider
On Tue, 24 May 2022 16:29:02 GMT, Sean Mullan wrote: > Please review this fix to the XML Signature implementation to check for null > or missing DSA parameters and throw a MarshalException before trying to > create a DSA public key from its XML encoding. This will allow the code to > fail earlier and not depend on the underlying provider to detect illegal or > missing parameters. src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/DOMKeyValue.java line 306: > 304: // Recommendation as they might be known from application > context, > 305: // but this implementation does not support that, so they are > 306: // required. Can you provide more details on why "this implementation" does not support that? Is it because we don't have an API for that or we haven't defined any property for that? src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/DOMKeyValue.java line 306: > 304: // Recommendation as they might be known from application > context, > 305: // but this implementation does not support that, so they are > 306: // required. Can you provide more details on why "this implementation" does not support that? Is it because we don't have an API for that or we haven't defined any property for that? - PR: https://git.openjdk.java.net/jdk/pull/8870
Re: RFR: 8286211: Update PCSC-Lite for Suse Linux to 1.9.5 [v3]
On Mon, 23 May 2022 21:44:39 GMT, Valerie Peng wrote: >> Need to update the 3 header files due to expiring business approval for 3rd >> party. >> >> The header files contain tabs which jcheck disallows, so I have to replace >> them with spaces. >> >> Thanks, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Added a link for COPYING as on the pcsc-lite website LGTM. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8853
Integrated: 8286908: ECDSA signature should not return parameters
On Tue, 17 May 2022 19:56:22 GMT, Weijun Wang wrote: > Let ECDSA's `engineGetParameters()` always return null. At the same time, > remove the remembered `sigParams` field. One behavior change is that after > calling `setParameter()`, one can call `init()` again with a key using > different parameters. I think this should be allowed since we are reusing the > signature object with a brand new key. > > `setParameter` is kept unchanged to be able to deal with certificates still > having parameters after the signature algorithm object identifier. See > https://bugs.openjdk.java.net/browse/JDK-8225745. > > Also added SHA1withECDSA to the no-NULL list in `KnownOIDs`. > > All security-related tests passed. This pull request has now been integrated. Changeset: 8040aa00 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/8040aa0073e7ea22b2fdff5bddff10c244e116ef Stats: 109 lines in 4 files changed: 60 ins; 36 del; 13 mod 8286908: ECDSA signature should not return parameters Reviewed-by: ascarpino, hchao, valeriep - PR: https://git.openjdk.java.net/jdk/pull/8758
Re: RFR: 8286908: ECDSA signature should not return parameters [v3]
> Let ECDSA's `engineGetParameters()` always return null. At the same time, > remove the remembered `sigParams` field. One behavior change is that after > calling `setParameter()`, one can call `init()` again with a key using > different parameters. I think this should be allowed since we are reusing the > signature object with a brand new key. > > `setParameter` is kept unchanged to be able to deal with certificates still > having parameters after the signature algorithm object identifier. See > https://bugs.openjdk.java.net/browse/JDK-8225745. > > Also added SHA1withECDSA to the no-NULL list in `KnownOIDs`. > > All security-related tests passed. Weijun Wang 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 four additional commits since the last revision: - add reference to RFCs - Merge branch 'master' into 8286908 - will not read params for ECDSA - the fix - Changes: - all: https://git.openjdk.java.net/jdk/pull/8758/files - new: https://git.openjdk.java.net/jdk/pull/8758/files/aceeb4a7..e5043a99 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8758=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8758=01-02 Stats: 107875 lines in 1550 files changed: 65071 ins; 35256 del; 7548 mod Patch: https://git.openjdk.java.net/jdk/pull/8758.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8758/head:pull/8758 PR: https://git.openjdk.java.net/jdk/pull/8758
Re: RFR: 8286969: Add a new test library API to execute kinit in SecurityTools.java
On Wed, 18 May 2022 16:19:40 GMT, Sibabrata Sahoo wrote: > A new API to execute kinit. Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8775
Re: RFR: 8286908: ECDSA signature should not return parameters [v2]
> Let ECDSA's `engineGetParameters()` always return null. At the same time, > remove the remembered `sigParams` field. One behavior change is that after > calling `setParameter()`, one can call `init()` again with a key using > different parameters. I think this should be allowed since we are reusing the > signature object with a brand new key. > > `setParameter` is kept unchanged to be able to deal with certificates still > having parameters after the signature algorithm object identifier. See > https://bugs.openjdk.java.net/browse/JDK-8225745. > > Also added SHA1withECDSA to the no-NULL list in `KnownOIDs`. > > All security-related tests passed. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: will not read params for ECDSA - Changes: - all: https://git.openjdk.java.net/jdk/pull/8758/files - new: https://git.openjdk.java.net/jdk/pull/8758/files/b13de5a9..aceeb4a7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8758=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8758=00-01 Stats: 13 lines in 2 files changed: 1 ins; 8 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8758.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8758/head:pull/8758 PR: https://git.openjdk.java.net/jdk/pull/8758
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v9]
On Tue, 17 May 2022 22:22:36 GMT, Valerie Peng wrote: >> This is to update the method javadoc of >> java.security.Signature.getParameters() with the missing `@throws >> UnsupportedOperationException`. In addition, the wording on the returned >> parameters are updated to match those in Cipher and CipherSpi classes. >> >> CSR will be filed later. >> >> Thanks, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > more minor cleanups for consistencies. src/java.base/share/classes/java/security/SignatureSpi.java line 377: > 375: * > 376: * This method is overridden by providers to initialize > 377: * this signature object with the specified parameter set. 1st paragraph: values, 2nd: set. Make them the same. src/java.base/share/classes/java/security/SignatureSpi.java line 397: > 395: * > 396: * This method is overridden by providers to return the parameters > 397: * used with this signature object. The 2 paragraphs above look the same. Of course, if you believe the 1st paragraph should always be a simple description, that's OK. - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8286908: ECDSA signature should not return parameters
On Tue, 17 May 2022 20:27:41 GMT, Jamil Nimeh wrote: > Do the behavioral changes you've cited in the PR description warrant a CSR, > or do you feel this behavioral change is still consistent with the current > Signature API documentation? I think so. In fact, after this change, there's simply no parameters for the signature, so calling `setParameter` should not change any internal state and thus has no effect on other calls. While this is a behavior change, I don't think it has any negative impact to users. - PR: https://git.openjdk.java.net/jdk/pull/8758
RFR: 8286908: ECDSA signature should not return parameters
Let ECDSA's `engineGetParameters()` always return null. At the same time, remove the remembered `sigParams` field. One behavior change is that after calling `setParameter()`, one can call `init()` again with a key using different parameters. I think this should be allowed since we are reusing the signature object with a brand new key. `setParameter` is kept unchanged to be able to deal with certificates still having parameters after the signature algorithm object identifier. See https://bugs.openjdk.java.net/browse/JDK-8225745. Also added SHA1withECDSA to the no-NULL list in `KnownOIDs`. - Commit messages: - the fix Changes: https://git.openjdk.java.net/jdk/pull/8758/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8758=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286908 Stats: 96 lines in 3 files changed: 59 ins; 29 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/8758.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8758/head:pull/8758 PR: https://git.openjdk.java.net/jdk/pull/8758
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v8]
On Thu, 12 May 2022 22:52:59 GMT, Valerie Peng wrote: >> 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. > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > reset ivSpec and minor nit fix. Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v6]
On Wed, 11 May 2022 16:01:39 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: > > Change admin privilege test to reliable method Marked as reviewed by weijun (Reviewer). - 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 [v5]
On Wed, 11 May 2022 15:55:40 GMT, Mat Carter wrote: >> Mat Carter has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add test from wangweij > > @christophbrejla - my goal is to backport to latest (18 or 19), 17 and 11 @macarte I think Sean's comment on your CSR about the scope is correct. The "algorithm" name should be at the JDK level so user knows what to write in their app. Once you think there's no more to update, you can finalize the CSR. - PR: https://git.openjdk.java.net/jdk/pull/8211
Integrated: 8286428: AlgorithmId should understand PBES2
On Mon, 9 May 2022 21:44:10 GMT, Weijun Wang wrote: > `AlgorithmId.getName` is updated for PBES2 algorithm identifiers so it > directly returns the standard algorithm defined by Java (Ex: > `PBEWithHmacSHA256AndAES_256`), instead of a simple "PBES2". > > Please note I specifically update the javadoc for this method to clarify that > this name is meant to be a name that's recognized by various `getInstance()` > methods. This is how we are actually using this method. > > After this change, the `javax.crypto.EncryptedPrivateKeyInfo` API > automatically works with PBES2 encrypted data. As the spec of its > `getAlgName()` methods says, "Standard name is returned". This is shown by > the newly include regression test. > > Existing security-related tests run fine. This pull request has now been integrated. Changeset: 357f990e Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/357f990e3244feaba6d8709b7ea50660220a418b Stats: 149 lines in 3 files changed: 118 ins; 16 del; 15 mod 8286428: AlgorithmId should understand PBES2 Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk/pull/8615
Integrated: 8284194: Allow empty subject fields in keytool
On Wed, 11 May 2022 21:55:45 GMT, Weijun Wang wrote: > This code change allows one entering "." at a distinguished name prompt to > skip a sub-component when running `keytool -genkeyapir`. Several new resource > strings are added. > > There is no detailed description in `keytool.html`, so I think there's no > need to update it. > > I'll file a CSR to describe the behavior change. > > Here is an example after this change: > > $ keytool -genkeypair -keystore ks -storepass changeit -alias b -keyalg EC > Enter the distinguished name. Provide a single dot (.) to leave a > sub-component empty or press ENTER to use the default value in braces. > What is your first and last name? > [Unknown]: . > What is the name of your organizational unit? > [Unknown]: . > What is the name of your organization? > [Unknown]: . > What is the name of your City or Locality? > [Unknown]: . > What is the name of your State or Province? > [Unknown]: . > What is the two-letter country code for this unit? > [Unknown]: . > At least one field must be provided. Enter again. > Enter the distinguished name. Provide a single dot (.) to leave a > sub-component empty or press ENTER to use the default value in braces. > What is your first and last name? > []: Duke > What is the name of your organizational unit? > []: > What is the name of your organization? > []: > What is the name of your City or Locality? > []: > What is the name of your State or Province? > []: > What is the two-letter country code for this unit? > []: > Is CN=Duke correct? > [no]: yes > > Generating 384 bit EC (secp384r1) key pair and self-signed certificate > (SHA384withECDSA) with a validity of 90 days > for: CN=Duke > > In the first round, "." is entered for all fields and keytool rejected it. In > the second round, CN is entered but the others are unchanged (just type > enter, because they are already entered previously). At the end, the name is > "CN=Duke". This pull request has now been integrated. Changeset: f4f1dddf Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/f4f1dddfef6ed3206763bb0e441aebd06a23d6fe Stats: 128 lines in 4 files changed: 91 ins; 3 del; 34 mod 8284194: Allow empty subject fields in keytool Reviewed-by: jnimeh, hchao - PR: https://git.openjdk.java.net/jdk/pull/8667
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v7]
On Thu, 12 May 2022 21:31:39 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 244:
>>
>>> 242: iCount = DEFAULT_COUNT;
>>> 243: }
>>> 244: //if (ivSpec == null) { // old behavior always generate
>>
>> How could `ivSpec` be non-null here? IIUC the only answer is from a previous
>> `engineInit`, and it should not be retained. I suggest removing this check
>> plus adding an explicit `ivSpec = null` at the beginning of this method
>> along with `iCount` and `salt`. Those are the only 3 non final instance
>> fields.
>
> Yeah, it's also possible that ivSpec is non-null if getParameters() is called
> before init(). Now that salt and iCount is reset in the beginning of init(),
> ivSpec should be reset too.
Oh, I didn't realize that. So now whenever `init()` is called every old param
(no matter why it was set) is totally wiped. I like this consistent behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v7]
On Thu, 12 May 2022 03:28:15 GMT, Valerie Peng wrote:
>> 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.
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> trivial syntax fix.
src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 244:
> 242: iCount = DEFAULT_COUNT;
> 243: }
> 244: //if (ivSpec == null) { // old behavior always generate
How could `ivSpec` be non-null here? IIUC the only answer is from a previous
`engineInit`, and it should not be retained. I suggest removing this check plus
adding an explicit `ivSpec = null` at the beginning of this method along with
`iCount` and `salt`. Those are the only 3 non final instance fields.
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v7]
On Thu, 12 May 2022 03:28:15 GMT, Valerie Peng wrote: >> 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. > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > trivial syntax fix. LGTM. src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java line 363: > 361: return core.implGetParameters(); > 362: } > 363: @Override These 3 methods are the same in 3 classes. I wish we had a way to simplify this but I don't know myself. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8284194: Allow empty subject fields in keytool [v3]
> This code change allows one entering "." at a distinguished name prompt to > skip a sub-component when running `keytool -genkeyapir`. Several new resource > strings are added. > > There is no detailed description in `keytool.html`, so I think there's no > need to update it. > > I'll file a CSR to describe the behavior change. > > Here is an example after this change: > > $ keytool -genkeypair -keystore ks -storepass changeit -alias b -keyalg EC > Enter the distinguished name. Provide a single dot (.) to leave a > sub-component empty. > What is your first and last name? > [Unknown]: . > What is the name of your organizational unit? > [Unknown]: . > What is the name of your organization? > [Unknown]: . > What is the name of your City or Locality? > [Unknown]: . > What is the name of your State or Province? > [Unknown]: . > What is the two-letter country code for this unit? > [Unknown]: . > At least one field must be provided. Enter again. > Enter the distinguished name. Provide a single dot (.) to leave a > sub-component empty. > What is your first and last name? > [EMPTY]: Duke > What is the name of your organizational unit? > [EMPTY]: > What is the name of your organization? > [EMPTY]: > What is the name of your City or Locality? > [EMPTY]: > What is the name of your State or Province? > [EMPTY]: > What is the two-letter country code for this unit? > [EMPTY]: > Is CN=Duke correct? > [no]: yes > > Generating 384 bit EC (secp384r1) key pair and self-signed certificate > (SHA384withECDSA) with a validity of 90 days > for: CN=Duke > > In the first round, "." is entered for all fields and keytool rejected it. In > the second round, CN is entered but the others are unchanged (just type > enter, because they are already entered previously). At the end, the name is > "CN=Duke". Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: update the output - Changes: - all: https://git.openjdk.java.net/jdk/pull/8667/files - new: https://git.openjdk.java.net/jdk/pull/8667/files/1894055d..8c592f89 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8667=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8667=01-02 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8667.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8667/head:pull/8667 PR: https://git.openjdk.java.net/jdk/pull/8667
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Thu, 10 Mar 2022 08:52:17 GMT, Сергей Цыпанов wrote:
>> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with
>> smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when
>> called with vararg of size 0, 1, 2.
>>
>> In general replacement of `Arrays.asList()` with `List.of()` is dubious as
>> the latter is null-hostile, however in some cases we are sure that arguments
>> are non-null. Into this PR I've included the following cases (in addition to
>> those where the argument is proved to be non-null at compile-time):
>> - `MethodHandles.longestParameterList()` never returns null
>> - parameter types are never null
>> - interfaces used for proxy construction and returned from
>> `Class.getInterfaces()` are never null
>> - exceptions types of method signature are never null
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8282662: Revert dubious changes in MethodType
src/java.base/share/classes/sun/security/validator/EndEntityChecker.java line
119:
> 117: // TLS key exchange algorithms requiring keyEncipherment key usage
> 118: private static final Collection KU_SERVER_ENCRYPTION =
> 119: Arrays.asList("RSA");
I understand that you haven't modified the `KU_SERVER_KEY_AGREEMENT` on line
122 because it has 4 elements. However, these 2 nearby lines using different
styles look a little strange to me. Why restrict the number to 0, 1, and 2? If
we have defined methods with up to 9 arguments, does that mean the new type is
suitable for 9 elements?
-
PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8284194: Allow empty subject fields in keytool [v2]
On Wed, 11 May 2022 23:40:46 GMT, Weijun Wang wrote: >> This code change allows one entering "." at a distinguished name prompt to >> skip a sub-component when running `keytool -genkeyapir`. Several new >> resource strings are added. >> >> There is no detailed description in `keytool.html`, so I think there's no >> need to update it. >> >> I'll file a CSR to describe the behavior change. >> >> Here is an example after this change: >> >> $ keytool -genkeypair -keystore ks -storepass changeit -alias b -keyalg EC >> Enter the distinguished name. Provide a single dot (.) to leave a >> sub-component empty. >> What is your first and last name? >> [Unknown]: . >> What is the name of your organizational unit? >> [Unknown]: . >> What is the name of your organization? >> [Unknown]: . >> What is the name of your City or Locality? >> [Unknown]: . >> What is the name of your State or Province? >> [Unknown]: . >> What is the two-letter country code for this unit? >> [Unknown]: . >> At least one field must be provided. Enter again. >> Enter the distinguished name. Provide a single dot (.) to leave a >> sub-component empty. >> What is your first and last name? >> [EMPTY]: Duke >> What is the name of your organizational unit? >> [EMPTY]: >> What is the name of your organization? >> [EMPTY]: >> What is the name of your City or Locality? >> [EMPTY]: >> What is the name of your State or Province? >> [EMPTY]: >> What is the two-letter country code for this unit? >> [EMPTY]: >> Is CN=Duke correct? >> [no]: yes >> >> Generating 384 bit EC (secp384r1) key pair and self-signed certificate >> (SHA384withECDSA) with a validity of 90 days >> for: CN=Duke >> >> In the first round, "." is entered for all fields and keytool rejected it. >> In the second round, CN is entered but the others are unchanged (just type >> enter, because they are already entered previously). At the end, the name is >> "CN=Duke". > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > word change I've already modified the prompt a little before the CSR is finalized. How about Enter the distinguished name. Provider a single dot (.) to leave a sub-component empty or press ENTER to use the default value in braces. Hopefully this is not too long, and macOS users know "ENTER" is "return", and people won't debate on "braces" or "brackets". As for the "[EMPTY]" prompt, the user has already entered "." in the first round and we always remember the inputs as the new default values in the next round so that they only need to enter the components they want to update. - PR: https://git.openjdk.java.net/jdk/pull/8667
Integrated: 8286422: Add OIDs for RC2 and Blowfish
On Wed, 11 May 2022 22:35:32 GMT, Weijun Wang wrote: > Add missing OIDs for 2 secret key algorithms. These will be used when storing > secret keys in a PKCS12 keystore. Like DES and DESede, the OIDs were > originally defined for CBC mode cipher algorithms, they are reused here for > key algorithms. > > OpenSSL uses the same OIDs for cipher algorithms. > > 1 3 6 1 4 1 3029 1 2 : BF-CBC: bf-cbc > rsadsi 3 2: RC2-CBC : rc2-cbc > > > Similar to DES, RC2 is only added as an alias so it needs to be specially > treated when getting a secret key entry from a PKCS12 keystore. > > No attempt is made to define these OIDs as aliases to existing cipher > algorithms. This pull request has now been integrated. Changeset: 752ad1c4 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/752ad1c41093645506dd267f618bd46882d0c674 Stats: 118 lines in 4 files changed: 66 ins; 49 del; 3 mod 8286422: Add OIDs for RC2 and Blowfish Reviewed-by: hchao, ascarpino - PR: https://git.openjdk.java.net/jdk/pull/8668
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v5]
On Wed, 11 May 2022 15:55:40 GMT, Mat Carter wrote: > @christophbrejla - my goal is to backport to latest (18 or 19), 17 and 11 Then please add the versions to the "Fix Version(s)" field of the CSR. There are also some questions waiting for you in the comment there. - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v5]
On Wed, 11 May 2022 22:02:42 GMT, Valerie Peng wrote:
>> 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.
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Changed to extend various CipherSpi implementations.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 171:
> 169:
> 170: // holder for the derived key and iv
> 171: private static class KeyAndIv implements Closeable {
You can make this a `record`.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 317:
> 315: Arrays.fill(derivedKey, (byte)0);
> 316:
> 317: KeyAndIv result;
I think the fancy 2022 way is to `return switch (...) { case "RC4" -> new
KeyAndIv(...) ... }`.
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8284194: Allow empty subject fields in keytool [v2]
> This code change allows one entering "." at a distinguished name prompt to > skip a sub-component when running `keytool -genkeyapir`. Several new resource > strings are added. > > There is no detailed description in `keytool.html`, so I think there's no > need to update it. > > I'll file a CSR to describe the behavior change. > > Here is an example after this change: > > $ keytool -genkeypair -keystore ks -storepass changeit -alias b -keyalg EC > Enter the distinguished name. Enter a single dot (.) to leave the > sub-component empty. > What is your first and last name? > [Unknown]: . > What is the name of your organizational unit? > [Unknown]: . > What is the name of your organization? > [Unknown]: . > What is the name of your City or Locality? > [Unknown]: . > What is the name of your State or Province? > [Unknown]: . > What is the two-letter country code for this unit? > [Unknown]: . > At least one field must be provided. Enter again. > Enter the distinguished name. Enter a single dot (.) to leave the > sub-component empty. > What is your first and last name? > [EMPTY]: Duke > What is the name of your organizational unit? > [EMPTY]: > What is the name of your organization? > [EMPTY]: > What is the name of your City or Locality? > [EMPTY]: > What is the name of your State or Province? > [EMPTY]: > What is the two-letter country code for this unit? > [EMPTY]: > Is CN=Duke correct? > [no]: yes > > Generating 384 bit EC (secp384r1) key pair and self-signed certificate > (SHA384withECDSA) with a validity of 90 days > for: CN=Duke > > In the first round, "." is entered for all fields and keytool rejected it. In > the second round, CN is entered but the others are unchanged (just type > enter, because they are already entered previously). At the end, the name is > "CN=Duke". Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: word change - Changes: - all: https://git.openjdk.java.net/jdk/pull/8667/files - new: https://git.openjdk.java.net/jdk/pull/8667/files/abed47cb..1894055d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8667=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8667=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8667.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8667/head:pull/8667 PR: https://git.openjdk.java.net/jdk/pull/8667
Re: RFR: 8284194: Allow empty subject fields in keytool
On Wed, 11 May 2022 22:37:18 GMT, Jamil Nimeh wrote:
>> This code change allows one entering "." at a distinguished name prompt to
>> skip a sub-component when running `keytool -genkeyapir`. Several new
>> resource strings are added.
>>
>> There is no detailed description in `keytool.html`, so I think there's no
>> need to update it.
>>
>> I'll file a CSR to describe the behavior change.
>>
>> Here is an example after this change:
>>
>> $ keytool -genkeypair -keystore ks -storepass changeit -alias b -keyalg EC
>> Enter the distinguished name. Enter a single dot (.) to leave the
>> sub-component empty.
>> What is your first and last name?
>> [Unknown]: .
>> What is the name of your organizational unit?
>> [Unknown]: .
>> What is the name of your organization?
>> [Unknown]: .
>> What is the name of your City or Locality?
>> [Unknown]: .
>> What is the name of your State or Province?
>> [Unknown]: .
>> What is the two-letter country code for this unit?
>> [Unknown]: .
>> At least one field must be provided. Enter again.
>> Enter the distinguished name. Enter a single dot (.) to leave the
>> sub-component empty.
>> What is your first and last name?
>> [EMPTY]: Duke
>> What is the name of your organizational unit?
>> [EMPTY]:
>> What is the name of your organization?
>> [EMPTY]:
>> What is the name of your City or Locality?
>> [EMPTY]:
>> What is the name of your State or Province?
>> [EMPTY]:
>> What is the two-letter country code for this unit?
>> [EMPTY]:
>> Is CN=Duke correct?
>> [no]: yes
>>
>> Generating 384 bit EC (secp384r1) key pair and self-signed certificate
>> (SHA384withECDSA) with a validity of 90 days
>> for: CN=Duke
>>
>> In the first round, "." is entered for all fields and keytool rejected it.
>> In the second round, CN is entered but the others are unchanged (just type
>> enter, because they are already entered previously). At the end, the name is
>> "CN=Duke".
>
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 3781:
>
>> 3779:
>> 3780: private static String dotToNull(String input) {
>> 3781: return ".".equals(input) ? null : input;
>
> Do we deal at all with leading/trailing whitespace (maybe more concerned
> about trailing whitespace than leading)? What happens if we get a ". "
> (trailing space)?
The user must be deliberately doing this. Let's respect their decision. :-)
-
PR: https://git.openjdk.java.net/jdk/pull/8667
RFR: 8286422: Add OIDs for RC2 and Blowfish
Add missing OIDs for 2 secret key algorithms. These will be used when storing secret keys in a PKCS12 keystore. Like DES and DESede, the OIDs were originally defined for CBC mode cipher algorithms, they are reused here for key algorithms. OpenSSL uses the same OIDs for cipher algorithms. 1 3 6 1 4 1 3029 1 2: BF-CBC: bf-cbc rsadsi 3 2 : RC2-CBC : rc2-cbc Similar to DES, RC2 is only added as an alias so it needs to be specially treated when getting a secret key entry from a PKCS12 keystore. No attempt is made to define these OIDs as aliases to existing cipher algorithms. - Commit messages: - add OIDs Changes: https://git.openjdk.java.net/jdk/pull/8668/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8668=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286422 Stats: 118 lines in 4 files changed: 66 ins; 49 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8668.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8668/head:pull/8668 PR: https://git.openjdk.java.net/jdk/pull/8668
RFR: 8284194: Allow empty subject fields in keytool
This code change allows one entering "." at a distinguished name prompt to skip a sub-component when running `keytool -genkeyapir`. Several new resource strings are added. There is no detailed description in `keytool.html`, so I think there's no need to update it. I'll file a CSR to describe the behavior change. Here is an example after this change: $ keytool -genkeypair -keystore ks -storepass changeit -alias b -keyalg EC Enter the distinguished name. Enter a single dot (.) to leave the sub-component empty. What is your first and last name? [Unknown]: . What is the name of your organizational unit? [Unknown]: . What is the name of your organization? [Unknown]: . What is the name of your City or Locality? [Unknown]: . What is the name of your State or Province? [Unknown]: . What is the two-letter country code for this unit? [Unknown]: . At least one field must be provided. Enter again. Enter the distinguished name. Enter a single dot (.) to leave the sub-component empty. What is your first and last name? [EMPTY]: Duke What is the name of your organizational unit? [EMPTY]: What is the name of your organization? [EMPTY]: What is the name of your City or Locality? [EMPTY]: What is the name of your State or Province? [EMPTY]: What is the two-letter country code for this unit? [EMPTY]: Is CN=Duke correct? [no]: yes Generating 384 bit EC (secp384r1) key pair and self-signed certificate (SHA384withECDSA) with a validity of 90 days for: CN=Duke In the first round, "." is entered for all fields and keytool rejected it. In the second round, CN is entered but the others are unchanged (just type enter, because they are already entered previously). At the end, the name is "CN=Duke". - Commit messages: - the fix Changes: https://git.openjdk.java.net/jdk/pull/8667/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8667=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284194 Stats: 128 lines in 4 files changed: 91 ins; 3 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/8667.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8667/head:pull/8667 PR: https://git.openjdk.java.net/jdk/pull/8667
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v3]
On Wed, 11 May 2022 05:53:21 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have this simple example update in the KeyStore specification? >> >> Password protection should be destroyed in the example code in KeyStore >> specification. Otherwise, applications may just copy and past the code, and >> forget to clean up password protection. >> >> It's a trivial update, and may not worthy of a CSR. But please let me know >> if you would like to have a CSR filed. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More udapte Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Wed, 11 May 2022 01:14:00 GMT, Valerie Peng wrote: >> The `core.init(..., cipher)` is actually >> `cipher.init(core.translateKeyAndParams())`. Is it possible we write it this >> way? > > It's possible, more refactoring would be needed and not necessarily less > lines of code. With your suggested change, the caller has to explicitly > destroy the derived key after the cipher.engineInit() call. This would be > repeated in all PKCS12 PBE cipher impl classes, but then there'd be no > casting of the actual classes. I assume this is what you are referring to? > Can code it out and see how it looks. If the returned key-and-iv class implements Closeable, then you can do a try-with-resources to destroy the key, which saves you more lines. - PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v2]
On Tue, 10 May 2022 22:03:19 GMT, Xue-Lei Andrew Fan wrote:
>> src/java.base/share/classes/java/security/KeyStore.java line 165:
>>
>>> 163: *}
>>> 164: *} finally {
>>> 165: *protParam.destroy();
>>
>> `KeyStore.ProtectionParameter` does not have a `destroy` method. Only
>> `PasswordProtection` does.
>
> Oops, I tried to check but finally forgot about it. Thanks!
It's probably better to convert these long example code to snippets, maybe next
time.
-
PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v2]
On Tue, 10 May 2022 22:07:47 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have this simple example update in the KeyStore specification? >> >> Password protection should be destroyed in the example code in KeyStore >> specification. Otherwise, applications may just copy and past the code, and >> forget to clean up password protection. >> >> It's a trivial update, and may not worthy of a CSR. But please let me know >> if you would like to have a CSR filed. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > Use PasswordProtection Approved. Although that `mySecretKey` is out of nowhere, but this is just example code. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v5]
On Tue, 10 May 2022 18:55:50 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:
>
> Add test from wangweij
test/jdk/sun/security/mscapi/AllTypes.java line 60:
> 58: return true;
> 59: } catch (IOException ioe) {
> 60: if
> (ioe.getMessage().trim().endsWith("java.security.KeyStoreException: Access is
> denied.")) {
Could the message be different if the system language is not English?
-
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 Tue, 10 May 2022 18:51:07 GMT, Mat Carter wrote: >> @macarte You need to finalize your CSR soon if you want to include this >> change into JDK 19. RDP1 is 2022/06/09, and all enhancements require >> approval after that. >> >> BTW, is it possible to detect whether you have admin privilege inside the >> test? There is a `NTSystem` class inside Java which can find out user info. >> Maybe the `getGroupIDs()` method will tell you if you are an admin? >> >> We can enhance the test before RC. > > @wangweij - I added your test and updated to handle Access Denied errors due > to admin rights missing, in this case we ignore the test > > The CSR has been finalized and I feel we are ready to close out this PR @macarte Can you explain more on when an "access denied" error could happen? I create a normal user in my local AD and didn't see that. We might need to talk about this in the release note. - 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 [v4]
On Tue, 10 May 2022 17:23:24 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: > > Minor formatting and spelling issues addressed Great. After the CSR is approved I will approve this PR as well. Then you will need to type the `/integrate` command as a comment here, and I will type `/sponsor`. Then the Skara bot will integrate the change and close the PR. - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore
On Tue, 10 May 2022 04:13:43 GMT, Xue-Lei Andrew Fan wrote:
> Hi,
>
> May I have this simple example update in the KeyStore specification?
>
> Password protection should be destroyed in the example code in KeyStore
> specification. Otherwise, applications may just copy and past the code, and
> forget to clean up password protection.
>
> It's a trivial update, and may not worthy of a CSR. But please let me know
> if you would like to have a CSR filed.
>
> Thanks,
> Xuelei
src/java.base/share/classes/java/security/KeyStore.java line 165:
> 163: *}
> 164: *} finally {
> 165: *protParam.destroy();
`KeyStore.ProtectionParameter` does not have a `destroy` method. Only
`PasswordProtection` does.
-
PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v3]
On Thu, 5 May 2022 16:36:04 GMT, Mat Carter wrote:
>> I'd like to contribute a test. Please modify it as much as you like. You can
>> put it inside `test/jdk/sun/security/mscapi/`.
>>
>> /*
>> * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> *
>> * This code is free software; you can redistribute it and/or modify it
>> * under the terms of the GNU General Public License version 2 only, as
>> * published by the Free Software Foundation.
>> *
>> * This code is distributed in the hope that it will be useful, but WITHOUT
>> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
>> * version 2 for more details (a copy is included in the LICENSE file that
>> * accompanied this code).
>> *
>> * You should have received a copy of the GNU General Public License version
>> * 2 along with this work; if not, write to the Free Software Foundation,
>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>> *
>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>> * or visit www.oracle.com if you need additional information or have any
>> * questions.
>> */
>>
>> import jdk.test.lib.Asserts;
>> import jdk.test.lib.SecurityTools;
>>
>> import java.security.KeyStore;
>> import java.util.Collections;
>> import java.util.List;
>> import java.util.Locale;
>>
>> /*
>> * @test
>> * @bug 6782021
>> * @requires os.family == "windows"
>> * @library /test/lib
>> * @summary More keystore types
>> */
>> public class AllTypes {
>> public static void main(String[] args) throws Exception {
>> var nm = test("windows-my");
>> var nr = test("windows-root");
>> var nmu = test("windows-my-currentuser");
>> var nru = test("windows-root-currentuser");
>> var nmm = test("windows-my-localmachine");
>> var nrm = test("windows-root-localmachine");
>> Asserts.assertEQ(nm, nmu);
>> Asserts.assertEQ(nr, nru);
>> }
>>
>> private static List test(String type) throws Exception {
>> var stdType = "Windows-" +
>> type.substring(8).toUpperCase(Locale.ROOT);
>> SecurityTools.keytool("-storetype " + type + " -list")
>> .shouldHaveExitValue(0)
>> .shouldContain("Keystore provider: SunMSCAPI")
>> .shouldContain("Keystore type: " + stdType);
>> KeyStore ks = KeyStore.getInstance(type);
>> ks.load(null, null);
>> var content = Collections.list(ks.aliases());
>> Collections.sort(content);
>> return content;
>> }
>> }
>
> @wangweij - regarding the two tests for localmachine, these will throw a
> KeyStore exception "Access denied" if the test is not run as admin, is there
> anyway in the test to make that a requirement? If so we could split into two
> tests, one in admin that does all and one in non-admin that does the
> currentuser tests
@macarte You need to finalize your CSR soon if you want to include this change
into JDK 19. RDP1 is 2022/06/09, and all enhancements require approval after
that.
BTW, is it possible to detect whether you have admin privilege inside the test?
There is a `NTSystem` class inside Java which can find out user info. Maybe the
`getGroupIDs()` method will tell you if you are an admin?
We can enhance the test before RC.
-
PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Tue, 10 May 2022 01:22:21 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
>> line 314:
>>
>>> 312: } else if (cipher instanceof DESedeCipher
>>> tripleDes) {
>>> 313: tripleDes.engineInit(opmode, cipherKey,
>>> ivSpec, random);
>>> 314: } else {
>>
>> Can we combine the 2 if above? What else type can it be?
>
> Currently, the specified CipherSpi object is one of RC4, RC2, DESede. The
> "else" part is for catching new PKCS12 PBE algorithms support which uses
> other cipher algorithms.
> CipherSpi.engineInit(...) is protected, so that's why we use the instanceof
> to cast the CipherSpi object into the actual impl class in order to call the
> engineInit(...) since the actual impl class is in the same package of this
> class.
The `core.init(..., cipher)` is actually
`cipher.init(core.translateKeyAndParams())`. Is it possible we write it this
way?
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v5]
On Mon, 9 May 2022 18:28:04 GMT, Valerie Peng wrote:
>> Anyone can help review this javadoc update? The main change is the wording
>> for the method javadoc of
>> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording
>> is somewhat restrictive and request is to broaden this to accommodate more
>> scenarios such as when null can be returned.
>> The rest are minor things like add {@code } to class name and null, and
>> remove redundant ".".
>>
>> Will file CSR after the review is close to being wrapped up.
>> Thanks~
>
> Valerie Peng 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 five additional
> commits since the last revision:
>
> - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8209038
> - update to address review feedback and minor javadoc cleanup
> - Update for getParameters()
> - Update w/ review feedbacks
> - 8209038: Clarify the javadoc of Cipher.getParameters()
Marked as reviewed by weijun (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v4]
On Mon, 9 May 2022 18:45:05 GMT, Valerie Peng wrote: >> This is to update the method javadoc of >> java.security.Signature.getParameters() with the missing `@throws >> UnsupportedOperationException`. In addition, the wording on the returned >> parameters are updated to match those in Cipher and CipherSpi classes. >> >> CSR will be filed later. >> >> Thanks, >> Valerie > > Valerie Peng 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 four additional > commits since the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8253176 > - Sync'ed w/ the wording in the other Cipher.getParameters() PR. > - Undo un-intentional changes. > - 8253176: Signature.getParameters should specify that it can throw > UnsupportedOperationException Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Mon, 9 May 2022 23:23:05 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 229:
>>
>>> 227: if (key instanceof javax.crypto.interfaces.PBEKey
>>> pbeKey) {
>>> 228: salt = check(pbeKey.getSalt()); // may be null
>>> 229: iCount = check(pbeKey.getIterationCount()); // may
>>> be 0
>>
>> It seems the return value is never 0.
>
> Oh, the comment about "may be 0" is meant toward the
> pbeKey.getInterationCount() call... Hmm, I will make it clearer.
I see. Another question, shall we reset `salt` and `iCount` at the beginning?
If `params` is null and `key` is not `PBEKey` and there is an existing positive
`iCount`, it will not be set to `DEFAULT_COUNT`.
-
PR: https://git.openjdk.java.net/jdk/pull/8521
RFR: 8286428: AlgorithmId should understand PBES2
`AlgorithmId.getName` is updated for PBES2 algorithm identifiers so it directly returns the standard algorithm defined by Java (Ex: `PBEWithHmacSHA256AndAES_256`), instead of a simple "PBES2". Please note I specifically update the javadoc for this method to clarify that this name is meant to be a name that's recognized by various `getInstance()` methods. This is how we are actually using this method. After this change, the `javax.crypto.EncryptedPrivateKeyInfo` API automatically works with PBES2 encrypted data. As the spec of its `getAlgName()` methods says, "Standard name is returned". This is shown by the newly include regression test. Existing security-related tests run fine. - Commit messages: - typo - the fix Changes: https://git.openjdk.java.net/jdk/pull/8615/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8615=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286428 Stats: 149 lines in 3 files changed: 118 ins; 16 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/8615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8615/head:pull/8615 PR: https://git.openjdk.java.net/jdk/pull/8615
Integrated: 8285743: Ensure each IntegerPolynomial object is only created once
On Fri, 29 Apr 2022 22:30:04 GMT, Weijun Wang wrote: > All `IntegerPolynimial`s are singletons now. Also, hand-coded implementations > for Ed25519 and Ed448 are removed. They were not used since `FieldGen` starts > generating classes for them. > > No new regression test. This is a clean-up. This pull request has now been integrated. Changeset: 397d095f Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/397d095f661e9d9c98b8254fb7867dc87047b0b8 Stats: 507 lines in 10 files changed: 16 ins; 465 del; 26 mod 8285743: Ensure each IntegerPolynomial object is only created once Reviewed-by: xuelei, ascarpino - PR: https://git.openjdk.java.net/jdk/pull/8476
Re: RFR: 8002277: Refactor two PBE classes to simplify maintenance [v2]
On Thu, 5 May 2022 19:38:06 GMT, Valerie Peng wrote:
>> 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.
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update copyright year for PBES2Core.java
Is it possible to rewrite `PKCS12PBECipherCore.java` so that the implementation
inside is `CipherCore` instead of `CipherSpi`? I'd rather create a `CipherSpi`
child inside this package as the base for all implementations that does nothing
more but simply is able to expose all those `engineXYZ` methods to other
classes in the same package. Sigh.
src/java.base/share/classes/com/sun/crypto/provider/PBES2Core.java line 229:
> 227: if (key instanceof javax.crypto.interfaces.PBEKey
> pbeKey) {
> 228: salt = check(pbeKey.getSalt()); // may be null
> 229: iCount = check(pbeKey.getIterationCount()); // may
> be 0
It seems the return value is never 0.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 183:
> 181:"for PBEWithSHA1And" + algo);
> 182: }
> 183: }
How about using switch/case or at least put the `if` in same indentation level?
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 214:
> 212:
> 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params,
> 214: SecureRandom random, CipherSpi cipher)
Why rename `cipherImpl` to `cipher`? I think `cipher` is usually a `Cipher`
object and `cipherImpl` is a good name for a `CipherSpi` object.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 215:
> 213: void implInit(int opmode, Key key, AlgorithmParameterSpec params,
> 214: SecureRandom random, CipherSpi cipher)
> 215: throws InvalidKeyException, InvalidAlgorithmParameterException {
Indent the line above.
src/java.base/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java
line 314:
> 312: } else if (cipher instanceof DESedeCipher tripleDes)
> {
> 313: tripleDes.engineInit(opmode, cipherKey, ivSpec,
> random);
> 314: } else {
Can we combine the 2 if above? What else type can it be?
-
PR: https://git.openjdk.java.net/jdk/pull/8521
Re: RFR: 8285743: Ensure each IntegerPolynomial object is only created once [v2]
On Fri, 29 Apr 2022 22:57:20 GMT, Weijun Wang wrote: >> All `IntegerPolynimial`s are singletons now. Also, hand-coded >> implementations for Ed25519 and Ed448 are removed. They were not used since >> `FieldGen` starts generating classes for them. >> >> No new regression test. This is a clean-up. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > move singleton back into impl and make constructor private make/jdk/src/classes/build/tools/intpoly/FieldGen.java line 837: > 835: // c[i + j] += 2 * a[i] * a[j] > 836: // } > 837: // } The comment was in the hand-coded curve25519 code. Moved here. - PR: https://git.openjdk.java.net/jdk/pull/8476
Re: RFR: JDK-8284688 Minor cleanup could be done in java.security.jgss [v5]
On Thu, 5 May 2022 21:05:40 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8284688 >> >> [JDK-8273046](https://bugs.openjdk.java.net/browse/JDK-8273046) is the >> umbrella bug for this bug. The changes were too large for a single code >> review, so it was decided to split into smaller chunks. This is one such >> chunk: >> >> open/src/java.security.jgss/share/classes/javax/security >> open/src/java.security.jgss/share/classes/org/ietf >> open/src/java.security.jgss/share/classes/sun/security > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Max comments OK. But let's wait until next week so loom can be integrated smoothly. I doubt loom will touch any of the files here but this commit is still quite big. - PR: https://git.openjdk.java.net/jdk/pull/7746
Re: RFR: 8285516: clearPassword should be called in a finally try block [v3]
On Thu, 5 May 2022 06:02:14 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 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 Tests run fine. No unexpected failure. - PR: https://git.openjdk.java.net/jdk/pull/8377
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v3]
On Wed, 4 May 2022 20:32:30 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: > > Removed whitespace and simply passing ints between java and C++ Oops, I didn't realized that. Yes, you can divide it to 2 tests. The one needs admin privilege can be tagged `@run main/manual Test`. It probably won't get a lot of chance to run. - 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 Wed, 4 May 2022 20:32:30 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:
>
> Removed whitespace and simply passing ints between java and C++
I'd like to contribute a test. Please modify it as much as you like. You can
put it inside `test/jdk/sun/security/mscapi/`.
/*
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
import jdk.test.lib.Asserts;
import jdk.test.lib.SecurityTools;
import java.security.KeyStore;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
/*
* @test
* @bug 6782021
* @requires os.family == "windows"
* @library /test/lib
* @summary More keystore types
*/
public class AllTypes {
public static void main(String[] args) throws Exception {
var nm = test("windows-my");
var nr = test("windows-root");
var nmu = test("windows-my-currentuser");
var nru = test("windows-root-currentuser");
var nmm = test("windows-my-localmachine");
var nrm = test("windows-root-localmachine");
Asserts.assertEQ(nm, nmu);
Asserts.assertEQ(nr, nru);
}
private static List test(String type) throws Exception {
var stdType = "Windows-" + type.substring(8).toUpperCase(Locale.ROOT);
SecurityTools.keytool("-storetype " + type + " -list")
.shouldHaveExitValue(0)
.shouldContain("Keystore provider: SunMSCAPI")
.shouldContain("Keystore type: " + stdType);
KeyStore ks = KeyStore.getInstance(type);
ks.load(null, null);
var content = Collections.list(ks.aliases());
Collections.sort(content);
return content;
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8285516: clearPassword should be called in a finally try block [v3]
On Thu, 5 May 2022 06:02:14 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 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 Running now... - PR: https://git.openjdk.java.net/jdk/pull/8377
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v3]
On Wed, 4 May 2022 20:32:30 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: > > Removed whitespace and simply passing ints between java and C++ src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CKeyStore.java line 860: > 858: > 859: /** > 860: * Load keys and/or certificates from keystore into Collection. Take this chance to modify `Load` to `Loads`. src/jdk.crypto.mscapi/windows/classes/sun/security/mscapi/CKeyStore.java line 866: > 864: */ > 865: private native void loadKeysOrCertificateChains(String name, > 866: int location) throws KeyStoreException; We usually indent 8 spaces in a continuation line. - 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: 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: 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: 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
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: 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: JDK-8284688 Minor cleanup could be done in java.security.jgss [v4]
On Mon, 2 May 2022 22:39:09 GMT, Mark Powers wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8284688
>>
>> [JDK-8273046](https://bugs.openjdk.java.net/browse/JDK-8273046) is the
>> umbrella bug for this bug. The changes were too large for a single code
>> review, so it was decided to split into smaller chunks. This is one such
>> chunk:
>>
>> open/src/java.security.jgss/share/classes/javax/security
>> open/src/java.security.jgss/share/classes/org/ietf
>> open/src/java.security.jgss/share/classes/sun/security
>
> Mark Powers has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 11 commits:
>
> - sixth iteration
> - Merge
> - fifth iteration
> - Merge
> - fourth iteration
> - third iteration
> - Merge
> - Merge
> - Merge
> - second iteration
> - ... and 1 more:
> https://git.openjdk.java.net/jdk/compare/64225e19...5ce46ffb
Many many thanks for this huge code change. No problem found. I have several
comments and you're free to make your own decision.
src/java.security.jgss/share/classes/org/ietf/jgss/GSSException.java line 333:
> 331: public String getMajorString() {
> 332:
> 333: return Objects.requireNonNullElseGet(majorString, () ->
> messages[major - 1]);
Isn't this too fancy?
src/java.security.jgss/share/classes/sun/security/jgss/GSSHeader.java line 267:
> 265: throw new IOException("DerInputStream.getLength():
> lengthTag="
> 266: + tmp + ", "
> 267: + "too big.");
Combine the 2 lines above.
src/java.security.jgss/share/classes/sun/security/jgss/krb5/SubjectComber.java
line 154:
> 152: while (iterator.hasNext()) {
> 153: Object obj = iterator.next();
> 154: if (!(obj instanceof
> @SuppressWarnings("unchecked")KerberosTicket ticket)) {
Does not look fluent to me.
src/java.security.jgss/share/classes/sun/security/jgss/spnego/SpNegoContext.java
line 876:
> 874:
> 875: // pass token
> 876: tok = Objects.requireNonNullElseGet(token, () -> new byte[0]);
Is this how `requireNonNullElseGet` is meant to be used?
-
Marked as reviewed by weijun (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7746
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Mon, 2 May 2022 21:14:21 GMT, Valerie Peng wrote: >> Then what does "cannot generate parameter values" mean? Any example? > > 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. - PR: https://git.openjdk.java.net/jdk/pull/8396
Integrated: 8286024: PKCS12 keystore shows "DES/CBC" as the algorithm of a DES SecretKeyEntry
On Mon, 2 May 2022 17:41:52 GMT, Weijun Wang wrote: > PKCS12 stores the object identifier of a SecretKey along with it, and when > retrieved, translate the object identifier to an algorithm name. > Unfortunately, inside `KnownOIDs.java`, "DES" is [only registered > as](https://github.com/wangweij/jdk/blob/7a6cbef157b67bb4fb877617f2a23228aade9a5d/src/java.base/share/classes/sun/security/util/KnownOIDs.java#L368-L368) > an alias of another name "DES/CBC". We should modify it to "DES" before > returning the secret key. This pull request has now been integrated. Changeset: 50a4df87 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/50a4df87c87febdf5fa8561b7d0d21b8d6623943 Stats: 53 lines in 2 files changed: 52 ins; 0 del; 1 mod 8286024: PKCS12 keystore shows "DES/CBC" as the algorithm of a DES SecretKeyEntry Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk/pull/8505
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
RFR: 8286024: PKCS12 keystore shows "DES/CBC" as the algorithm of a DES SecretKeyEntry
PKCS12 stores the object identifier of a SecretKey along with it, and when retrieved, translate the object identifier to an algorithm name. Unfortunately, inside `KnownOIDs.java`, "DES" is [only registered as](https://github.com/wangweij/jdk/blob/7a6cbef157b67bb4fb877617f2a23228aade9a5d/src/java.base/share/classes/sun/security/util/KnownOIDs.java#L368-L368) an alias of another name "DES/CBC". We should modify it to "DES" before returning the secret key. - Commit messages: - remove exe bits - fix Changes: https://git.openjdk.java.net/jdk/pull/8505/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8505=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286024 Stats: 53 lines in 2 files changed: 52 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8505.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8505/head:pull/8505 PR: https://git.openjdk.java.net/jdk/pull/8505
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: 8284490: Remove finalizer method in java.security.jgss [v13]
On Thu, 28 Apr 2022 18:32:31 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. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > add sleep back LGTM - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8136
Integrated: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
On Thu, 28 Apr 2022 14:35:54 GMT, Weijun Wang wrote: > We added a new system property back in > https://bugs.openjdk.java.net/browse/JDK-8153005 but it's better to describe > it in the `java.security` file as well. > > Please review the text. I especially added the last sentence so that people > won't set `-Dkeystore.pkcs12.legacy=false`. This pull request has now been integrated. Changeset: cfcba1fc Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/cfcba1fccc8e3e6a68e1cb1826b70e076d5d83c4 Stats: 24 lines in 1 file changed: 20 ins; 0 del; 4 mod 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: 8285743: Ensure each IntegerPolynomial object is only created once [v2]
> All `IntegerPolynimial`s are singletons now. Also, hand-coded implementations > for Ed25519 and Ed448 are removed. They were not used since `FieldGen` starts > generating classes for them. > > No new regression test. This is a clean-up. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: move singleton back into impl and make constructor private - Changes: - all: https://git.openjdk.java.net/jdk/pull/8476/files - new: https://git.openjdk.java.net/jdk/pull/8476/files/b24ca783..331e9519 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8476=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8476=00-01 Stats: 54 lines in 8 files changed: 7 ins; 19 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/8476.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8476/head:pull/8476 PR: https://git.openjdk.java.net/jdk/pull/8476
RFR: 8285743: Ensure each IntegerPolynomial object is only created once
All `IntegerPolynimial`s are singletons now. Also, hand-coded implementations for Ed25519 and Ed448 are removed. They were not used since `FieldGen` starts generating classes for them. No new regression test. This is a clean-up. - Commit messages: - the fix Changes: https://git.openjdk.java.net/jdk/pull/8476/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8476=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285743 Stats: 530 lines in 10 files changed: 28 ins; 465 del; 37 mod Patch: https://git.openjdk.java.net/jdk/pull/8476.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8476/head:pull/8476 PR: https://git.openjdk.java.net/jdk/pull/8476
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file [v2]
On Fri, 29 Apr 2022 20:47:08 GMT, Sean Mullan wrote: >> The reason I added the last sentence is because this property has no value. >> Someone might think they can set it to false to disable it, but that is >> equivalent to set it to true. > > Ah I see. Maybe put in the previous sentence, ex: "When set, this system > property (which can only be enabled and has no value) is equivalent to:" > > Just a suggestion. Suggested accepted. I just pushed a new commit. I modified "PKCS #12" to "PKCS12" because that's the word we used throughout the file. - PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file [v2]
> We added a new system property back in > https://bugs.openjdk.java.net/browse/JDK-8153005 but it's better to describe > it in the `java.security` file as well. > > Please review the text. I especially added the last sentence so that people > won't set `-Dkeystore.pkcs12.legacy=false`. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: clearer text - Changes: - all: https://git.openjdk.java.net/jdk/pull/8452/files - new: https://git.openjdk.java.net/jdk/pull/8452/files/08700389..8a24c745 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8452=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8452=00-01 Stats: 18 lines in 1 file changed: 7 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8452.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8452/head:pull/8452 PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
On Fri, 29 Apr 2022 20:35:14 GMT, Sean Mullan wrote: >> Can we say both? All these properties are only used when creating the file >> (key-related ones when creating the key). If a compatibility issue already >> happens, users need to downgrade their keystore. >> >> So, the full text will be something like >> >> Some legacy PKCS #12 tools or libraries do not support the new algorithms >> based on >> PBES2 and AES. In order to create a PKCS #12 keystore for them, the system >> property >> "keystore.pkcs12.legacy" can be set which overrides the properties defined >> here with >> legacy algorithm. Setting this system property is equivalent to >> >> >> >> Also, you can downgrade an existing PKCS #12 keystore that already uses new >> algorithms >> to use legacy algorithms with >> >>keytool -J-Dkeystore.pkcs12.legacy -importkeystore -srckeystore ks >> -destkeystore ks >> >> This system property should be used at your own risk. Please note there is >> no value defined for this system property, i.e. "-Dkeystore.pkcs12.legacy" >> has the same effect as "-Dkeystore.pkcs12.legacy=". >> >> I'll double check if the command can indeed downgrade key algorithms as >> well. *Update*: it works. All 3 algorithms (key, cert, mac) downgraded to >> legacy ones. > > It's a little long, but I can see why it is useful, so I think it's good. I > would avoid the word "new" as this won't be new in a few years time. Here is > an edit where I removed words which I thought were not essential: > >> Some PKCS #12 tools and libraries may not support algorithms based on PBES2 >> and AES. >> To create a PKCS #12 keystore which they can load, set the system property >> "keystore.pkcs12.legacy" which overrides the values of the properties >> defined below with >> legacy algorithms. Setting this system property is equivalent to >> >> >> >> Also, you can downgrade an existing PKCS #12 keystore created with stronger >> algorithms >> to legacy algorithms with >> >>keytool -J-Dkeystore.pkcs12.legacy -importkeystore -srckeystore ks >> -destkeystore ks >> >> This system property should be used at your own risk. > > Don't think you really need the sentence below, as you have already given > several examples: > >> Please note there is >> no value defined for this system property, i.e. "-Dkeystore.pkcs12.legacy" >> has the same effect as "-Dkeystore.pkcs12.legacy=". The reason I added the last sentence is because this property has no value. Someone might think they can set it to false to disable it, but that is equivalent to set it to true. - PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
On Fri, 29 Apr 2022 13:17:55 GMT, Sean Mullan wrote: >> How about this? >> >> To work with legacy PKCS #12 tools that does not support the new algorithms, >> the system property "keystore.pkcs12.legacy" can be set >> which will override the properties defined here with old settings. >> This system property is equivalent to > > I think the text above might still make some users concerned that they should > always set this property. > Maybe we can be less specific, and just say: "If you encounter compatibility > issues with software that doesn't support the stronger algorithms, the system > property ..." Can we say both? All these properties are only used when creating the file (key-related ones when creating the key). If a compatibility issue already happens, users need to downgrade their keystore. So, the full text will be something like To work with legacy PKCS #12 tools that does not support the new algorithms, the system property "keystore.pkcs12.legacy" can be set which will override the properties defined here with old settings. If you encounter compatibility issues with software that doesn't support the stronger algorithms, you can downgrade the keystore with keytool -J-Dkeystore.pkcs12.legacy -importkeystore -keystore ks ... I'll double check if the command can indeed downgrade key algorithms as well. - PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 28 Apr 2022 23:22:30 GMT, Valerie Peng wrote: >> I suggest the last sentence to be "null is returned if the required >> parameters were not supplied **or** the underlying signature implementation >> cannot generate the parameter values." I used "or" because for EdDSA >> parameters are supplied but the impl cannot generate parameter values. > > The impl does not need to generate parameter values, but rather cannot > convert the supplied parameter values into AlgorithmParameter objects. By > parameter values, I mean the components of the parameters. Then what does "cannot generate parameter values" mean? Any example? - PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
On Thu, 28 Apr 2022 19:59:07 GMT, Sean Mullan wrote: >> OpenSSL's help page shows >> >> -legacy Use legacy encryption: 3DES_CBC for keys, RC2_CBC for >> certs >> >> Can we also say "To work with legacy PKCS #12 files"? > > But isn't it mostly an issue when creating new keystores and not reading > existing ones? I would want to avoid users thinking that they had to set this > in more cases than needed. How about this? To work with legacy PKCS #12 tools that does not support the new algorithms, the system property "keystore.pkcs12.legacy" can be set which will override the properties defined here with old settings. This system property is equivalent to - PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 28 Apr 2022 23:08:17 GMT, Valerie Peng wrote:
>> So, "the underlying signature implementation supports returning the
>> parameters as {@code AlgorithmParameters}" is quite necessary. Xuelei's
>> suggestion is quite good, just change the last "and" to "or".
>
> I assume you were suggesting this? `"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. {https://github.com/code null} is returned if the required
> parameters were not supplied and the underlying signature implementation
> cannot generate the parameter values."`
> But the "the underlying signature implementation supports returning the
> parameters as {https://github.com/code AlgorithmParameters}" is necessary.
> Strictly speaking, this is somewhat different than the "cannot generate
> parameter values" though. Perhaps we should go a bit broader for the last
> sentence regarding null return value?
I suggest the last sentence to be "null is returned if the required parameters
were not supplied **or** the underlying signature implementation cannot
generate the parameter values." I used "or" because for EdDSA parameters are
supplied but the impl cannot generate parameter values.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
On Thu, 28 Apr 2022 19:48:38 GMT, Sean Mullan wrote: >> We added a new system property back in >> https://bugs.openjdk.java.net/browse/JDK-8153005 but it's better to describe >> it in the `java.security` file as well. >> >> Please review the text. I especially added the last sentence so that people >> won't set `-Dkeystore.pkcs12.legacy=false`. > > src/java.base/share/conf/security/java.security line 1174: > >> 1172: # If the property is not set or empty, a default value will be used. >> 1173: # >> 1174: # For compatibility, the system property "keystore.pkcs12.legacy" can >> be set > > Was wondering if we should add why you might want to set this property, ex: > "For compatibility with JDK or PKCS12 implementations that do not support the > stronger algorithms ..." > > Compatibility with prior JDK versions should be less of an issue over time as > these stronger settings and algs have been backported to prior JDKs. OpenSSL's help page shows -legacy Use legacy encryption: 3DES_CBC for keys, RC2_CBC for certs Can we also say "To work with legacy PKCS #12 files"? - PR: https://git.openjdk.java.net/jdk/pull/8452
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v3]
On Thu, 28 Apr 2022 19:11:23 GMT, Valerie Peng wrote:
>> Anyone can help review this javadoc update? The main change is the wording
>> for the method javadoc of
>> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording
>> is somewhat restrictive and request is to broaden this to accommodate more
>> scenarios such as when null can be returned.
>> The rest are minor things like add {@code } to class name and null, and
>> remove redundant ".".
>>
>> Will file CSR after the review is close to being wrapped up.
>> Thanks~
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update for getParameters()
src/java.base/share/classes/javax/crypto/Cipher.java line 1056:
> 1054: * parameters were not supplied and the underlying cipher
> implementation
> 1055: * can generate the parameter values, it will be returned.
> Otherwise,
> 1056: * {@code null} returned.
Should this be "null is returned"?
src/java.base/share/classes/javax/crypto/Cipher.java line 1787:
> 1785: * Ensures that Cipher is in a valid state for update() and
> doFinal()
> 1786: * calls - should be initialized and in ENCRYPT_MODE or
> DECRYPT_MODE.
> 1787: * @throws IllegalStateException if Cipher object is not in valid
> state
"Cipher" in `{@code}`? Or make it lowercase.
src/java.base/share/classes/javax/crypto/CipherSpi.java line 449:
> 447: *
> 448: * Note that when a Cipher object is initialized, it loses all
> 449: * previously-acquired state. In other words, initializing a Cipher
> is
Two `{@code Cipher}` above.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 04:34:36 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. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > change to use othervm My understanding is such kind of tests that rely on GC is fundementally unreliable. I would preemptively add an intermittent keyword to it. :-) - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 04:34:36 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. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > change to use othervm I'm sure you have well established testing environment as well, but if you need any help to test out your "other proposals", I'm glad to help. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 04:34:36 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. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > change to use othervm I see you removed the `Thread.sleep(100)` calls. Given the failure of another similar test, maybe it's safer to add them back? - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms [v2]
On Thu, 28 Apr 2022 06:46:35 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: > > SecretKeyConstraintsParameters subclass created and property description > updated There is no way to `-genseckey` an RC2 key in a PKCS12 keystore but the only reason is that we don't have a known RC2 OID registered. (In fact, I was preparing to add one in the attempted code change to add OIDs into the standard names doc). You can add an RC4 key to PKCS12. Also, you can add both RC2 and RC4 to a JCEKS keystore. - PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 02:33:49 GMT, Mark Powers wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8285504
>>
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for
>> a single code review, so it was decided to split into smaller chunks. This
>> is one such chunk:
>>
>> open/src/java.base/share/classes/java/net
>
> Mark Powers 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 eight additional
> commits since the last revision:
>
> - Merge
> - Max and Brad comments
> - jaikiran comments
> - Alan Bateman comments
> - second iteration
> - Merge
> - Merge
> - first iteration
I made some wrong suggestions earlier.
src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70:
> 68: String type;
> 69: type = GetPropertyAction.privilegedGetProperty(
> 70: "ssl.KeyManagerFactory.algorithm");
So sorry I got it wrong here, this is a security property.
`GetPropertyAction.privilegedGetProperty` is for system properties.
src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:
> 90: static String getSecurityProperty(final String name) {
> 91: return AccessController.doPrivileged((PrivilegedAction)
> () -> {
> 92: return Security.getProperty(name);
I assume we still need to do the if-empty-then-null conversion?
src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82:
> 80: String type;
> 81: type = GetPropertyAction.privilegedGetProperty(
> 82: "ssl.TrustManagerFactory.algorithm");
Sorry I got it wrong here, this is a security property.
-
PR: https://git.openjdk.java.net/jdk/pull/8384
RFR: 8285827: Describe the keystore.pkcs12.legacy system property in the java.security file
We added a new system property back in https://bugs.openjdk.java.net/browse/JDK-8153005 but it's better to describe it in the `java.security` file as well. Please review the text. I especially added the last sentence so that people won't set `-Dkeystore.pkcs12.legacy=false`. - Commit messages: - add description Changes: https://git.openjdk.java.net/jdk/pull/8452/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8452=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285827 Stats: 13 lines in 1 file changed: 13 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8452.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8452/head:pull/8452 PR: https://git.openjdk.java.net/jdk/pull/8452
