[jdk18] RFR: 8278186: throw StringIndexOutOfBoundsException when calling substring method
Add check on `xpointer(id('name'))` format.
-
Commit messages:
- 8278186: throw StringIndexOutOfBoundsException when calling substring method
Changes: https://git.openjdk.java.net/jdk18/pull/1/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk18&pr=1&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8278186
Stats: 75 lines in 4 files changed: 66 ins; 0 del; 9 mod
Patch: https://git.openjdk.java.net/jdk18/pull/1.diff
Fetch: git fetch https://git.openjdk.java.net/jdk18 pull/1/head:pull/1
PR: https://git.openjdk.java.net/jdk18/pull/1
Re: RFR: 8277932: Subject:callAs() not throwing NPE when action is null
On Tue, 7 Dec 2021 07:14:53 GMT, Alan Bateman wrote: > Is there a test for this? (I see noreg-trivial is added but a test should be > easy to add). I can add one, just thought it's not necessary. I didn't say noreg-hard. :-) - PR: https://git.openjdk.java.net/jdk/pull/6728
Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Tue, 7 Dec 2021 00:08:03 GMT, Valerie Peng wrote:
>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
>> line 405:
>>
>>> 403: private void cancelOperation() {
>>> 404: // cancel operation by finishing it; avoid killSession as some
>>> 405: // hardware vendors may require re-login
>>
>> The new `cancelOperation()` methods seems identical everywhere. Is it
>> possible to consolidate it to a helper method like `trySessionCancel(token,
>> session, flags)`? It can return true if canceled successfully, false if
>> needs a fallback, and can still throw a `ProviderException`.
>
> I assume you mean the if-() block of code? I can move the code into a helper
> method inside the P11Util class.
Yes, just keep duplicated lines as few as possible.
-
PR: https://git.openjdk.java.net/jdk/pull/6655
Integrated: 8277932: Subject:callAs() not throwing NPE when action is null
On Mon, 6 Dec 2021 22:22:14 GMT, Weijun Wang wrote: > Add null check. I must have thought the NPE will be thrown anyway but the > `catch Exception` block swallows it. > > I added a noreg-trivial label. If you think differently can add one. This pull request has now been integrated. Changeset: 10db0e41 Author: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/10db0e41634b62be5c1a931bd54ac4260108670d Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8277932: Subject:callAs() not throwing NPE when action is null Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/6728
RFR: 8277932: Subject:callAs() not throwing NPE when action is null
Add null check. I must have thought the NPE will be thrown anyway but the `catch Exception` block swallows it. I added a noreg-trivial label. If you think differently can add one. - Commit messages: - 8277932: Subject:callAs() not throwing NPE when action is null Changes: https://git.openjdk.java.net/jdk/pull/6728/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6728&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277932 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6728.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6728/head:pull/6728 PR: https://git.openjdk.java.net/jdk/pull/6728
Integrated: 8275082: Update XML Security for Java to 2.3.0
On Wed, 1 Dec 2021 17:31:37 GMT, Weijun Wang wrote: > Import Apache Santuario 2.3.0 without the secure validation changes since in > OpenJDK we are using the `jdk.xml.dsig.secureValidationPolicy` security > property for XML Signature secure validation protection. > > Two commits are pushed: > > - 2.3.0: Import 2.3.0 code changes > - revert: revert the Santuario secure validation changes This pull request has now been integrated. Changeset: 2c31a173 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/2c31a1735d5b8646ed8984a5475d5c8c9c91c19d Stats: 409 lines in 32 files changed: 56 ins; 268 del; 85 mod 8275082: Update XML Security for Java to 2.3.0 Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/6644
Re: RFR: 8277353: java/security/MessageDigest/ThreadSafetyTest.java test times out [v2]
On Fri, 3 Dec 2021 06:14:49 GMT, Sibabrata Sahoo wrote: >> This Test gets timeout during low cpu availability. It is modified to >> support extended timeout period during JTREG execution. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > 8277353: java/security/MessageDigest/ThreadSafetyTest.java test times out Looks good. Thanks. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6626
Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Wed, 1 Dec 2021 21:42:51 GMT, Valerie Peng wrote:
> PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE,
> it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the
> C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not
> explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API
> for cancelling session operations, so I've also updated various classes to
> call this new API if the PKCS#11 library version is 3.0. Otherwise, these
> classes will try to cancel by finishing off current operations as before. The
> support for the new C_LoginUser() has not been tested, so I commented it out
> for now. Given the current release schedule, support for other new PKCS#11
> APIs (such as message-based ones and parameters structure) and options for
> C_GetInterface (if needed) will be handled later.
>
> I validated the current changes against different NSS releases (supports
> PKCS#11 v2.40 and v3..0 respectively) with existing regression tests.
>
> Thanks,
> Valerie
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java line 418:
> 416: }
> 417: String word = st.sval;
> 418: switch (word) {
Since every case has a break it's probably better to use the enhanced switch
(`case "x" -> ...;`). It's safer and also saves quite some lines. An IDE can
help you with the conversion.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
line 405:
> 403: private void cancelOperation() {
> 404: // cancel operation by finishing it; avoid killSession as some
> 405: // hardware vendors may require re-login
The new `cancelOperation()` methods seems identical everywhere. Is it possible
to consolidate it to a helper method like `trySessionCancel(token, session,
flags)`? It can return true if canceled successfully, false if needs a
fallback, and can still throw a `ProviderException`.
-
PR: https://git.openjdk.java.net/jdk/pull/6655
Integrated: 8278247: KeyStoreSpi::engineGetAttributes does not throws KeyStoreException
On Fri, 3 Dec 2021 19:36:51 GMT, Weijun Wang wrote: > The specification wrongly claims there could be an exception thrown, but it's > not true. This pull request has now been integrated. Changeset: e1cde19d Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/e1cde19dbdbbca365ecfea6d1e2e85a42ed8bde0 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod 8278247: KeyStoreSpi::engineGetAttributes does not throws KeyStoreException Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/6706
RFR: 8278247: KeyStoreSpi::engineGetAttributes does not throws KeyStoreException
The specification wrongly claims there could be an exception thrown, but it's not true. - Commit messages: - 8278247: KeyStoreSpi::engineGetAttributes does not throws KeyStoreException Changes: https://git.openjdk.java.net/jdk/pull/6706/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6706&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278247 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6706.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6706/head:pull/6706 PR: https://git.openjdk.java.net/jdk/pull/6706
Integrated: 8225181: KeyStore should have a getAttributes method
On Wed, 20 Oct 2021 02:08:24 GMT, Weijun Wang wrote: > Add `KeyStore::getAttributes` so that one can get the attributes of an entry > without retrieving the entry first. This is especially useful for a private > key entry which can only be retrieved with a password. This pull request has now been integrated. Changeset: a729a70c Author: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/a729a70c0119ed071ff490b0dfd4e3e2cb1a5ae4 Stats: 169 lines in 6 files changed: 167 ins; 0 del; 2 mod 8225181: KeyStore should have a getAttributes method Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8275082: Update XML Security for Java to 2.3.0 [v2]
> Import Apache Santuario 2.3.0 without the secure validation changes since in > OpenJDK we are using the `jdk.xml.dsig.secureValidationPolicy` security > property for XML Signature secure validation protection. > > Two commits are pushed: > > - 2.3.0: Import 2.3.0 code changes > - revert: revert the Santuario secure validation changes Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: update comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/6644/files - new: https://git.openjdk.java.net/jdk/pull/6644/files/26a8a428..318a7c89 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6644&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6644&range=00-01 Stats: 3 lines in 1 file changed: 2 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6644.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6644/head:pull/6644 PR: https://git.openjdk.java.net/jdk/pull/6644
Re: RFR: 8278099: two sun/security/pkcs11/Signature tests failed with AssertionError
On Thu, 2 Dec 2021 01:20:30 GMT, Valerie Peng wrote: > Can someone help reviewing this trivial one-line fix? The assert check in > CK_MECHANISM.java is too strict and fail unexpectedly when digest-specific > PSS signature mechanisms are supported by the underlying PKCS#11 library. The > fix is to remove this assert check. No new regression test added with this > fix as this is already covered by existing regression tests. > > Thanks, > Valerie Not a PKCS11 expert, but does a "digest-specific PSS signature mechanism" needs `setParameter` anymore? Or, must it be the same as the existing parameters dictated by the specified digest? - PR: https://git.openjdk.java.net/jdk/pull/6656
Re: RFR: 8277353: java/security/MessageDigest/ThreadSafetyTest.java test times out
On Wed, 1 Dec 2021 19:29:36 GMT, Sibabrata Sahoo wrote: > > Can you lower the `threadsFactor` or `duration`? Or set an upper limit for > > `nTasks`? > > I can reduce the threadFactor and duration to close to half(threadsFactor=2 > and duration=2 Or hardcode nTasks=20) and i think there still will be enough > threads to verify threadsafety. In that case default JTREG timeout period > should be enough and no need for any additional timeout with @run tag. Maybe you can contact the author of this test to see if that's enough. - PR: https://git.openjdk.java.net/jdk/pull/6626
RFR: 8275082: Update XML Security for Java to 2.3.0
Import Apache Santuario 2.3.0 without the secure validation changes since in OpenJDK we are using the `jdk.xml.dsig.secureValidationPolicy` security property for XML Signature secure validation protection. Two commits are pushed: - 2.3.0: Import 2.3.0 code changes - revert: revert the Santuario secure validation changes - Commit messages: - revert - 2.3.0 Changes: https://git.openjdk.java.net/jdk/pull/6644/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6644&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275082 Stats: 406 lines in 32 files changed: 54 ins; 267 del; 85 mod Patch: https://git.openjdk.java.net/jdk/pull/6644.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6644/head:pull/6644 PR: https://git.openjdk.java.net/jdk/pull/6644
RFR: 8255266: 2021-11-27 public suffix list update v 3c213aa
Update Public Suffix List data to the latest version at https://github.com/publicsuffix/list. - Commit messages: - 8255266: 2021-11-27 public suffix list update v 3c213aa Changes: https://git.openjdk.java.net/jdk/pull/6643/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6643&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255266 Stats: 1254 lines in 5 files changed: 887 ins; 215 del; 152 mod Patch: https://git.openjdk.java.net/jdk/pull/6643.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6643/head:pull/6643 PR: https://git.openjdk.java.net/jdk/pull/6643
Re: RFR: 8277353: java/security/MessageDigest/ThreadSafetyTest.java test times out
On Wed, 1 Dec 2021 06:26:58 GMT, Sibabrata Sahoo wrote: > This Test gets timeout during low cpu availability. It is modified to support > extended timeout period during JTREG execution. Can you lower the `threadsFactor` or `duration`? Or set an upper limit for `nTasks`? - PR: https://git.openjdk.java.net/jdk/pull/6626
Re: RFR: 8225181: KeyStore should have a getAttributes method [v5]
> Add `KeyStore::getAttributes` so that one can get the attributes of an entry > without retrieving the entry first. This is especially useful for a private > key entry which can only be retrieved with a password. 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 seven additional commits since the last revision: - final spec change - Merge branch 'master' into 8225181 - Merge branch 'master' into 8225181 - redine spec - more clear and precise spec - clarification on protected attributes - 8225181: KeyStore should have a getAttributes method 8225181: KeyStore should have a getAttributes method - Changes: - all: https://git.openjdk.java.net/jdk/pull/6026/files - new: https://git.openjdk.java.net/jdk/pull/6026/files/901bdf83..702168db Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6026&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6026&range=03-04 Stats: 929909 lines in 2271 files changed: 483097 ins; 432951 del; 13861 mod Patch: https://git.openjdk.java.net/jdk/pull/6026.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6026/head:pull/6026 PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8225181: KeyStore should have a getAttributes method [v4]
On Thu, 4 Nov 2021 19:34:50 GMT, Weijun Wang wrote: >> Add `KeyStore::getAttributes` so that one can get the attributes of an entry >> without retrieving the entry first. This is especially useful for a private >> key entry which can only be retrieved with a password. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > redine spec New commits pushed. Except for the merge commits, the only change is method spec update to match the text in the approved CSR. - PR: https://git.openjdk.java.net/jdk/pull/6026
Integrated: 8231107: Allow store password to be null when saving a PKCS12 KeyStore
On Thu, 14 Oct 2021 14:43:32 GMT, Weijun Wang wrote: > You can create a password-less PKCS12 KeyStore file now by calling > `ks.store(outStream, null)` no matter what the default cert protection > algorithm and Mac algorithm are defined in `java.security`. > > Note: the system properties set in `ToolsJDK.gmk` to generate `cacerts` must > be retained (at the moment) because the tool is launched with BOOT_JDK. This pull request has now been integrated. Changeset: 7049c13c Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/7049c13cf4bf4cdfcd0c8f0fa96bf4c3748ae1e7 Stats: 70 lines in 4 files changed: 37 ins; 16 del; 17 mod 8231107: Allow store password to be null when saving a PKCS12 KeyStore Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/5950
Re: RFR: 8276660: Scalability bottleneck in java.security.Provider.getService() [v2]
On Wed, 24 Nov 2021 21:17:34 GMT, Valerie Peng wrote: >> It is observed that when running crypto benchmark with large number of >> threads, a lot of time is spent on the synchronized block inside the >> Provider.getService() method. The cause for this is that >> Provider.getService() method first uses the 'serviceMap' field to find the >> requested service. However, when the requested service is not supported by >> this provider, e.g. requesting Cipher.RSA from SUN provider, the impl >> continues to try searching the legacy registrations whose processing is >> guarded by the "synchronized" keyword. When apps use getInstance() calls >> without the provider argument, Provider class has to iterate through >> existing providers trying to find one that supports the requested service. >> >> Now that the parent class of Provider no longer synchronizes all of its >> methods, Provider class should follow suit and de-synchronize its methods. >> Parsing of the legacy registration is done eagerly (at the time of put(...) >> calls) instead of lazily (at the time of getService(...) calls). This also >> makes "legacyStrings" redundant as the registration is parsed and stored >> directly into "legacyMap". >> >> The bug reporter has confirmed that the changes resolve the performance >> bottleneck and all regression tests pass. >> >> Please review and thanks in advance, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Updated to use pattern matching with instanceof operator. Since all legacy registration are done eagerly, I assume the original `ensureLegacyParsed()` method should be super fast now. Maybe we don't need to change any synchronized keyword. - PR: https://git.openjdk.java.net/jdk/pull/6513
Integrated: 8272162: S4U2Self ticket without forwardable flag
On Fri, 22 Oct 2021 16:31:02 GMT, Weijun Wang wrote: > The S4U2proxy extension requires that the service ticket to the first service > has the forwardable flag set, but some versions of Windows Server do not set > the forwardable flag in a S4U2self response and accept it in a S4U2proxy > request. > > There are 2 commits now. The 1st is a refactoring that sends more info into > the methods (Ex: `KdcComm::send(byte[])` -> `KdcComm::send(KrbKdcReq)`, and > `Ticket` -> `Credentials` in multiple places) so that inside `KdcComm::send` > there is enough info to decide how to deal with various errors. The 2nd is > the actual fix to this issue, i.e. ignore the flag and retry another KDC. This pull request has now been integrated. Changeset: ab867f6c Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/ab867f6c7c578ae7e65af2989b6836d523a41c5a Stats: 413 lines in 17 files changed: 218 ins; 38 del; 157 mod 8272162: S4U2Self ticket without forwardable flag Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk/pull/6082
Re: RFR: 8276660: Scalability bottleneck in java.security.Provider.getService() [v2]
On Wed, 24 Nov 2021 21:17:34 GMT, Valerie Peng wrote: >> It is observed that when running crypto benchmark with large number of >> threads, a lot of time is spent on the synchronized block inside the >> Provider.getService() method. The cause for this is that >> Provider.getService() method first uses the 'serviceMap' field to find the >> requested service. However, when the requested service is not supported by >> this provider, e.g. requesting Cipher.RSA from SUN provider, the impl >> continues to try searching the legacy registrations whose processing is >> guarded by the "synchronized" keyword. When apps use getInstance() calls >> without the provider argument, Provider class has to iterate through >> existing providers trying to find one that supports the requested service. >> >> Now that the parent class of Provider no longer synchronizes all of its >> methods, Provider class should follow suit and de-synchronize its methods. >> Parsing of the legacy registration is done eagerly (at the time of put(...) >> calls) instead of lazily (at the time of getService(...) calls). This also >> makes "legacyStrings" redundant as the registration is parsed and stored >> directly into "legacyMap". >> >> The bug reporter has confirmed that the changes resolve the performance >> bottleneck and all regression tests pass. >> >> Please review and thanks in advance, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Updated to use pattern matching with instanceof operator. Consider this case, two threads are changing a value at the same time. Since the method is not synchonized, thread1 might finish the first part of the method (`super.replace`) earlier than thread2, but it finishes the second part (`parseLegacy`) later than thread2. At the end, the internal entrySet has thread2's value but the legacy map has thread1's value. - PR: https://git.openjdk.java.net/jdk/pull/6513
Re: RFR: 8276660: Scalability bottleneck in java.security.Provider.getService() [v2]
On Tue, 30 Nov 2021 02:40:22 GMT, Valerie Peng wrote: >> src/java.base/share/classes/java/security/Provider.java line 832: >> >>> 830: // NOTE: may need extra mechanism for providers to indicate their >>> 831: // preferred ordering of SecureRandom algorithms since registration >>> 832: // ordering info is lost once serialized >> >> Why is the ordering info lost once serialized? Weren't all entries re-added >> again in their original order? > > The serialized bytes are just the mappings, i.e. key + value pairs. There are > no ordering info associated with the key + value pair. IIRC, the particular > thing about SecureRandom is that the first registration of SecureRandom is > deemed to be the most preferred. However, if given only the serialized bytes, > the entries are added based on the resulting order stored by the parent > class, not necessarily the ordering of the initial insertion/add calls. I see. How about serializing `prngAlgos` as a list? - PR: https://git.openjdk.java.net/jdk/pull/6513
Re: RFR: 8276660: Scalability bottleneck in java.security.Provider.getService() [v2]
On Tue, 30 Nov 2021 02:47:45 GMT, Valerie Peng wrote: >> src/java.base/share/classes/java/security/Provider.java line 979: >> >>> 977: parseLegacy(sk, sv, OPType.REPLACE); >>> 978: } >>> 979: } >> >> If you are going through all the entries, should we also clean up the legacy >> sets and restart? > > Do you mean simply wipe out the legacyMap and just do ADD instead of REPLACE? Yes, since you are simply iterating through all the entries instead of only the changed ones. - PR: https://git.openjdk.java.net/jdk/pull/6513
Re: RFR: 8276660: Scalability bottleneck in java.security.Provider.getService() [v2]
On Wed, 24 Nov 2021 21:17:34 GMT, Valerie Peng wrote: >> It is observed that when running crypto benchmark with large number of >> threads, a lot of time is spent on the synchronized block inside the >> Provider.getService() method. The cause for this is that >> Provider.getService() method first uses the 'serviceMap' field to find the >> requested service. However, when the requested service is not supported by >> this provider, e.g. requesting Cipher.RSA from SUN provider, the impl >> continues to try searching the legacy registrations whose processing is >> guarded by the "synchronized" keyword. When apps use getInstance() calls >> without the provider argument, Provider class has to iterate through >> existing providers trying to find one that supports the requested service. >> >> Now that the parent class of Provider no longer synchronizes all of its >> methods, Provider class should follow suit and de-synchronize its methods. >> Parsing of the legacy registration is done eagerly (at the time of put(...) >> calls) instead of lazily (at the time of getService(...) calls). This also >> makes "legacyStrings" redundant as the registration is parsed and stored >> directly into "legacyMap". >> >> The bug reporter has confirmed that the changes resolve the performance >> bottleneck and all regression tests pass. >> >> Please review and thanks in advance, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Updated to use pattern matching with instanceof operator. Some comments. I'm more concerned about the `parseLegacy()` method which is called everywhere. Without the synchronized keyword, is it safe to call into this method by multiple threads at the same time? Do we have tests around this? src/java.base/share/classes/java/security/Provider.java line 832: > 830: // NOTE: may need extra mechanism for providers to indicate their > 831: // preferred ordering of SecureRandom algorithms since registration > 832: // ordering info is lost once serialized Why is the ordering info lost once serialized? Weren't all entries re-added again in their original order? src/java.base/share/classes/java/security/Provider.java line 901: > 899: legacyChanged = true; > 900: } > 901: return true; Better put this "return" line into the else block. src/java.base/share/classes/java/security/Provider.java line 979: > 977: parseLegacy(sk, sv, OPType.REPLACE); > 978: } > 979: } If you are going through all the entries, should we also clean up the legacy sets and restart? - PR: https://git.openjdk.java.net/jdk/pull/6513
Re: RFR: 8272162: S4U2Self ticket without forwardable flag [v2]
On Mon, 22 Nov 2021 21:26:05 GMT, Valerie Peng wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> some word changes > > src/java.security.jgss/share/classes/sun/security/krb5/Credentials.java line > 69: > >> 67: private static boolean alreadyTried = false; >> 68: >> 69: public final static boolean S4U2PROXY_ACCEPT_NON_FORWARDABLE > > nit: swap to use "static final" Done. - PR: https://git.openjdk.java.net/jdk/pull/6082
Re: RFR: 8272162: S4U2Self ticket without forwardable flag [v2]
> The S4U2proxy extension requires that the service ticket to the first service > has the forwardable flag set, but some versions of Windows Server do not set > the forwardable flag in a S4U2self response and accept it in a S4U2proxy > request. > > There are 2 commits now. The 1st is a refactoring that sends more info into > the methods (Ex: `KdcComm::send(byte[])` -> `KdcComm::send(KrbKdcReq)`, and > `Ticket` -> `Credentials` in multiple places) so that inside `KdcComm::send` > there is enough info to decide how to deal with various errors. The 2nd is > the actual fix to this issue, i.e. ignore the flag and retry another KDC. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: some word changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/6082/files - new: https://git.openjdk.java.net/jdk/pull/6082/files/c07e6f64..1f93a881 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6082&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6082&range=00-01 Stats: 5 lines in 3 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6082.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6082/head:pull/6082 PR: https://git.openjdk.java.net/jdk/pull/6082
Re: RFR: 8277246: Check for NonRepudiation as well when validating a TSA certificate [v3]
On Wed, 17 Nov 2021 14:06:00 GMT, Weijun Wang wrote: >> There is no need to check for the KeyUsage extension when validating a TSA >> certificate. >> >> A test is modified where a TSA cert has a KeyUsage but without the >> DigitalSignature bit. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > check either KU_SIGNATURE or KU_NON_REPUDIATION Great. Thanks a lot for your suggestion to them. I really appreciate it. - PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8272162: S4U2Self ticket without forwardable flag
On Thu, 28 Oct 2021 19:21:02 GMT, Martin Balao wrote: > * The names 'second' and 'secondTicket' -that were used before- don't look > ideal to me. I've not seen them used neither in RFC 4120 nor in MS-SFU > (v.20.0). In the case of 'additionalTickets', it's defined in RFC 4120 but > more from a message-format perspective than with any specific semantics. Many > of us are familiar to these names at this point but perhaps we can take this > opportunity to find a better replacement. These are service tickets used for > impersonation, from a user principal probably. I used 'userSvcTicket' when > working on the Referrals feature. It could be that or something different. > I'm okay if you want to postpone this consideration, but I wanted to raise it > just in case. I see what you mean. I'll go through them. > * While I see the need of introducing a new class to the hierarchy > (KrbKdcReq), I'd rearrange that a bit if possible. In particular, I'd make > ::getMessage part of the interface, instead of ::encoding, and then delegate > to the message (KDCReq --> ASReq | TGSReq) the encoding. In fact, the message > already does the encoding; it's just caching it in the same place where it's > done -which should be fine as the message is non-mutable-. This would > simplify things a bit and we can have only one 'obuf' field in KDCReq, > instead of caching it both in KrbAsReq and KrbTgsReq. We are not loosing > encapsulation because the message is already accessible. What do you think? Good idea. > * In CredentialsUtil.java, I see how the checks 'additionalTickets == null || > additionalTickets.length == 0' were replaced by 'second == null', but what > about the check 'credsInOut[1] == null'? Sorry, missed it. > * I want to notice that 'additionalTickets' was technically an 'in/out' > parameter in CredentialsUtil::serviceCredsReferrals. I couldn't find any > consequence of making it an 'in' parameter, but just in case I want to > mention this. I believe the only function that could have used that > information is CredentialsUtil::acquireS4U2proxyCreds but it's an r-value > there so it should be fine. That's right. Although the content of the original array argument could be modified, the caller has not used it. In fact, changing it from array to a normal object makes me feel relieved. I always needed to remind myself that this was not meant to be an '[out]' parameter. > * A KDC_ERR_BADOPTION error meaning that the KDC requires the 'FORWARDABLE' > option in the ticket is what the code suggests. Is it possible that this is > not what really happened and there is something else wrong? In other words, > is it possible that a DS_BEHAVIOR_WIN2012 DC returns this error? I don't know. > * The FORWARDABLE check removed is the one in S4U2Self. Apparently, for > S4U2Proxy with non-S4U2Self second-tickets there were no checks. Now we check > at S4U2Proxy level (for all 'second' tickets, S4U2Self and non-S4U2Self > ones). Is that okay? Or do we need to be more specific and check for S4U2Self > second-tickets only (in a S4U2Proxy communication)? That's what I asked you about a more precise way to ensure a cred is a S4U2self one. I thought about stuff the `S4U2Type` value as a "type" field into the credentials returned by `serviceCreds()` but it looks a little ugly. > > Otherwise, looks good to me. Thanks. > > Thanks, Martin.- Hi @martinuy, I looked at the variable names. Some can be named `clientCreds` or `proxyCreds`. Some are for a general `additionalTickets`. I can name it to `additionalCreds` but this "creds" is only one object and not an array. - PR: https://git.openjdk.java.net/jdk/pull/6082
Re: RFR: 8272162: S4U2Self ticket without forwardable flag
On Fri, 19 Nov 2021 23:34:11 GMT, Valerie Peng wrote: >> The S4U2proxy extension requires that the service ticket to the first >> service has the forwardable flag set, but some versions of Windows Server do >> not set the forwardable flag in a S4U2self response and accept it in a >> S4U2proxy request. >> >> There are 2 commits now. The 1st is a refactoring that sends more info into >> the methods (Ex: `KdcComm::send(byte[])` -> `KdcComm::send(KrbKdcReq)`, and >> `Ticket` -> `Credentials` in multiple places) so that inside `KdcComm::send` >> there is enough info to decide how to deal with various errors. The 2nd is >> the actual fix to this issue, i.e. ignore the flag and retry another KDC. > > src/java.security.jgss/share/classes/sun/security/krb5/internal/CredentialsUtil.java > line 64: > >> 62: PrincipalName sname = middleTGT.getClient(); >> 63: String uRealm = user.getRealmString(); >> 64: String localRealm = middleTGT.getClient().getRealmString(); > > nit: can just use sname on line 64? Sure. - PR: https://git.openjdk.java.net/jdk/pull/6082
Re: RFR: 8272162: S4U2Self ticket without forwardable flag
On Fri, 22 Oct 2021 16:31:02 GMT, Weijun Wang wrote: > The S4U2proxy extension requires that the service ticket to the first service > has the forwardable flag set, but some versions of Windows Server do not set > the forwardable flag in a S4U2self response and accept it in a S4U2proxy > request. > > There are 2 commits now. The 1st is a refactoring that sends more info into > the methods (Ex: `KdcComm::send(byte[])` -> `KdcComm::send(KrbKdcReq)`, and > `Ticket` -> `Credentials` in multiple places) so that inside `KdcComm::send` > there is enough info to decide how to deal with various errors. The 2nd is > the actual fix to this issue, i.e. ignore the flag and retry another KDC. Oops, I introduced a bug. At the end of the constructor of `KrbTgsReq`, `options` could be changed. Since I'm now calculating the encoding on-demand, the encoding will also change. I'll fix this in another commit. First, my latest commit contains a mechanism to tell if a ticket is from S4U2self. Is it significant enough to change your mind? Even if there is a system property for this purpose (suppose it's named `jdk.security.krb5.accept.non.forwardble.s4u2self.response.and.retry`), I wonder if it's worth turning it off. As you said, a legitimate S4U2self should always be FORWARDABLE, and in this case the system property is useless. But, if a service really receives such a ticket, I assume it would prefer to try it in a S4U2proxy request instead of just failing early. After all, when it impersonates someone, its purpose should be accessing a backend service. We usually introduce a system property for a compatibility reason so that existing normal cases will not behave differently, but here, we are simply trying to resurrect a former failure. The main problem I see with the current approach is still about whether a "tolerant" KDC can be accessed in time. If this can be optimized by adjusting the orders of KDC either in krb5.conf or in the DNS response, then I would be greatly relieved. That `ccreds` in `acquireS4U2selfCreds`? I'll fix it. Other comments accepted. I'll add a system property. All suggestions accepted, except that I still write out the full name for S4U2proxy in `java.security`. For the `KrbKdcReq` method name. It's now `encoding` because it's returning a byte array. `getMessage` was used to return a message type. - PR: https://git.openjdk.java.net/jdk/pull/6082
Re: RFR: 8272162: S4U2Self ticket without forwardable flag
On Mon, 1 Nov 2021 14:42:32 GMT, Martin Balao wrote: > But the question that concerns me most is if we really want to make such a > tight check, or we are willing to forward everything. Alexey said their customer has at least 50 KDCs. It will be quite a waste of time if we go through each of them and get a KDC_ERR_BADOPTION all the time. Therefore I would like this retry to be as restricted as possible. > `additionalTickets` is a term introduced in the RFC. Even when it does not > have defined semantics (i.e.: what are these attached/additional tickets > for?), I'd keep it for everything related to message formatting. My comment > was more about 'second', which is undefined in RFC/docs and not a very > meaningful name. I prefer `clientCreds` over `proxyCreds` because 'proxy' > makes me think about the middle-service. What about `userCreds`? (the reason > I like 'user' is because it's more about the actor, while client might be a > role that the middle-service is playing in a communication with a KDC or a > back-end) Unfortunately we cannot call them `additionalTickets` anymore, first it's no longer just a message, second it's not plural. Yes, `userCreds` is better. One place `proxyCreds` is used is because it's a `Krb5ProxyCredential`. As for `second`, I think it might be from the "second ticket" inside a ccache. I've pushed a new commit for everything I've tried on. - PR: https://git.openjdk.java.net/jdk/pull/6082
RFR: 8272162: S4U2Self ticket without forwardable flag
The S4U2proxy extension requires that the service ticket to the first service has the forwardable flag set, but some versions of Windows Server do not set the forwardable flag in a S4U2self response and accept it in a S4U2proxy request. There are 2 commits now. The 1st is a refactoring that sends more info into the methods (Ex: `KdcComm::send(byte[])` -> `KdcComm::send(KrbKdcReq)`, and `Ticket` -> `Credentials` in multiple places) so that inside `KdcComm::send` there is enough info to decide how to deal with various errors. The 2nd is the actual fix to this issue, i.e. ignore the flag and retry another KDC. - Commit messages: - TGT needs not to be forwardable in S4U2self request - address martin's comments - Merge - also a security property - a system property, do not care where ticket is from, more renames - move KDCReq::encoding to KrbKdcReq::obuf, no more ibuf in KrbTgsReq - a type label for credentials, encoding in KrbKdcReq, and some renames - implement the change - 8272162: S4U2Self ticket without forwardable flag Changes: https://git.openjdk.java.net/jdk/pull/6082/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6082&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272162 Stats: 413 lines in 17 files changed: 218 ins; 38 del; 157 mod Patch: https://git.openjdk.java.net/jdk/pull/6082.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6082/head:pull/6082 PR: https://git.openjdk.java.net/jdk/pull/6082
Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled [v2]
On Thu, 18 Nov 2021 15:03:33 GMT, Sean Mullan wrote: >> We should, but the problem is that jarsigner needs to individually test each >> algorithm, so it can properly display which algorithm is restricted. So, I >> think it will need to parse the RSSASSA params itself, and then call the >> constraints code to check each algorithm. Let me see if I can code up >> something that does that. > > I would like to defer the checking of AlgorithmParameters as part of another > bug. There are some major restructuring changes that would need to be made to > jarsigner to support this. And for RSASSA-PSS, there should not be any risk > for a while since by default jarsigner uses at least SHA-256 for the digest > algorithms in the PSS parameters. Looks so. - PR: https://git.openjdk.java.net/jdk/pull/6296
Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled [v2]
On Tue, 16 Nov 2021 18:10:04 GMT, Sean Mullan wrote: >> When a signature/digest algorithm was being checked, the algorithm >> constraints checked both the signature/digest algorithm and the key to see >> if they were restricted. This caused duplicate checks and was also >> problematic for `jarsigner` (and `keytool`) which need to distinguish these >> two cases, so that the output can properly indicate when the key is disabled >> but the signature or digest alg is ok. >> >> To address this issue, a new `checkKey` parameter is added to the >> `DisabledAlgorithmConstraints.permits` methods. When `true` the key (alg and >> size) is also checked, otherwise it is not. This flag is always set to >> `false` by `jarsigner` when checking algs and by the JDK when checking >> digest algorithms. Other small changes include changes in `SignerInfo` to >> use a record to store info about the algorithms to be checked, and removing >> an unnecessary CRL checking method from `AlgorithmChecker`. >> >> `keytool` will be enhanced in a subsequent CR to call the new methods. > > Sean Mullan has updated the pull request incrementally with one additional > commit since the last revision: > > Use var in for loop in SignerInfo. > In TimestampCheck test, combine/simplify what messages should not be emitted > when jar is signed with 512-bit RSA key. Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6296
Integrated: 8277246: Check for NonRepudiation as well when validating a TSA certificate
On Tue, 16 Nov 2021 19:36:11 GMT, Weijun Wang wrote: > There is no need to check for the KeyUsage extension when validating a TSA > certificate. > > A test is modified where a TSA cert has a KeyUsage but without the > DigitalSignature bit. This pull request has now been integrated. Changeset: 262d0700 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/262d07001babcbe7f9acd2053aa3b7bac304cf85 Stats: 6 lines in 2 files changed: 3 ins; 0 del; 3 mod 8277246: Check for NonRepudiation as well when validating a TSA certificate Reviewed-by: xuelei, mullan - PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8277224: sun.security.pkcs.PKCS9Attributes.toString() throws NPE [v2]
On Wed, 17 Nov 2021 17:16:38 GMT, Sean Coffey wrote: >> Some elements of the PKCS9Attribute.PKCS9_OIDS array may have null value. >> The PKCS9Attributes.toString() and PKCS9Attributes.getAttributes() methods >> need to account for that. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > test update Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6433
Re: RFR: 8277224: sun.security.pkcs.PKCS9Attributes.toString() throws NPE
On Wed, 17 Nov 2021 16:00:04 GMT, Sean Coffey wrote: > Some elements of the PKCS9Attribute.PKCS9_OIDS array may have null value. The > PKCS9Attributes.toString() and PKCS9Attributes.getAttributes() methods need > to account for that. test/jdk/sun/security/x509/AlgorithmId/NonStandardNames.java line 67: > 65: // test PKCS9Attributes.toString(), > PKCS9Attributes.getAttributes() > 66: System.out.println(authed); > 67: authed.getAttributes(); Looks like the old `getAttributes()` would only throw NPE if one of the attribute is of a type after `PKCS9_OIDS[10]`. - PR: https://git.openjdk.java.net/jdk/pull/6433
Re: RFR: 8277246: Check for NonRepudiation as well when validating a TSA certificate [v3]
On Wed, 17 Nov 2021 14:16:26 GMT, Sean Mullan wrote: > Can you change the synopsis of the bug to more accurately reflect the current > fix? Updated. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8277246: No need to check about KeyUsage when validating a TSA certificate [v2]
On Tue, 16 Nov 2021 21:00:12 GMT, Weijun Wang wrote: >> There is no need to check for the KeyUsage extension when validating a TSA >> certificate. >> >> A test is modified where a TSA cert has a KeyUsage but without the >> DigitalSignature bit. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > clarify RFC requirement Revert the removal and add check on nonRepudiation as well. And the test still works. :-) - PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8277246: No need to check about KeyUsage when validating a TSA certificate [v3]
> There is no need to check for the KeyUsage extension when validating a TSA > certificate. > > A test is modified where a TSA cert has a KeyUsage but without the > DigitalSignature bit. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: check either KU_SIGNATURE or KU_NON_REPUDIATION - Changes: - all: https://git.openjdk.java.net/jdk/pull/6416/files - new: https://git.openjdk.java.net/jdk/pull/6416/files/a5b3bc86..2bd3c546 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6416&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6416&range=01-02 Stats: 10 lines in 1 file changed: 9 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6416/head:pull/6416 PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8277246: No need to check about KeyUsage when validating a TSA certificate [v2]
On Tue, 16 Nov 2021 21:00:12 GMT, Weijun Wang wrote: >> There is no need to check for the KeyUsage extension when validating a TSA >> certificate. >> >> A test is modified where a TSA cert has a KeyUsage but without the >> DigitalSignature bit. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > clarify RFC requirement Really? The TSA is http://timestamp.digicert.com and the cert chain is CN=DigiCert Timestamp 2021, O="DigiCert, Inc.", C=US KeyUsage: DigitalSignature ExtendedKeyUsages: timeStamping CN=DigiCert SHA2 Assured ID Timestamping CA, OU=www.digicert.com, O=DigiCert Inc, C=US KeyUsage: DigitalSignature, Key_CertSign, Crl_Sign ExtendedKeyUsages: timeStamping You mean this CA can be used for time stamping as well? I understand that when KU is using you can find out its usage in EKU (vice versa), but here it's a CA that can sign cert and CRLs. Does it really need to act as the end entity cert of a TSA server? - PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8277246: No need to check about KeyUsage when validating a TSA certificate [v2]
On Tue, 16 Nov 2021 21:00:12 GMT, Weijun Wang wrote: >> There is no need to check for the KeyUsage extension when validating a TSA >> certificate. >> >> A test is modified where a TSA cert has a KeyUsage but without the >> DigitalSignature bit. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > clarify RFC requirement I did see an issuer of TSA certs whose own certificate has EKU with id-kp-timeStamping and KU with both DigitialSignature and keyCertsign. This cert should be rejected if it signed a timestamp response. - PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8277246: No need to check about KeyUsage when validating a TSA certificate [v2]
On Tue, 16 Nov 2021 21:00:12 GMT, Weijun Wang wrote:
>> There is no need to check for the KeyUsage extension when validating a TSA
>> certificate.
>>
>> A test is modified where a TSA cert has a KeyUsage but without the
>> DigitalSignature bit.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> clarify RFC requirement
Sean found RFC 5280, 4.2.1.12:
id-kp-timeStampingOBJECT IDENTIFIER ::= { id-kp 8 }
-- Binding the hash of an object to a time
-- Key usage bits that may be consistent: digitalSignature
-- and/or nonRepudiation
so it seems either is OK. That said, it's just a "may", and to me it reads more
like that others are NOT consistent and should be rejected.
-
PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8277246: No need to check about KeyUsage when validating a TSA certificate [v2]
On Tue, 16 Nov 2021 21:00:12 GMT, Weijun Wang wrote: >> There is no need to check for the KeyUsage extension when validating a TSA >> certificate. >> >> A test is modified where a TSA cert has a KeyUsage but without the >> DigitalSignature bit. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > clarify RFC requirement Hi Michael. Thanks for the comment. That was also our previous understanding but we are seeing timestamp returning by sigstore.dev (see the `rekor timestamp` command at https://github.com/sigstore/rekor) whose cert does not have the DigitialSignature bit set. -BEGIN CERTIFICATE- MIIBvzCCAWWgAwIBAgICBnowCgYIKoZIzj0EAwIwHzEdMBsGA1UEChMUaHR0cHM6 Ly9zaWdzdG9yZS5kZXYwHhcNMjExMTAyMTgxODI5WhcNMzExMTAyMTgxODI5WjAi MSAwHgYDVQQKExdSZWtvciBUaW1lc3RhbXBpbmcgQ2VydDBZMBMGByqGSM49AgEG CCqGSM49AwEHA0IABJIsOXOZUdgXhnNmvue9AAsxSYWdp+1HvEQQMYuZUsU0Ylf2 bKqIp3zVrc0a58pGJZvwGjyOHgY5lRevPP1huuOjgY0wgYowDgYDVR0PAQH/BAQD AgZAMAwGA1UdEwEB/wQCMAAwDgYDVR0OBAcEBQECAwQGMB8GA1UdIwQYMBaAFIiV AzEbE/rHgP3CA3x7tofqTtIcMCEGA1UdEQQaMBiHBH8AAAGHEAAA AAEwFgYDVR0lAQH/BAwwCgYIKwYBBQUHAwgwCgYIKoZIzj0EAwIDSAAwRQIg XdDBMPMTrtXiHmhFJOgQW8DDz/IaHkNZ+hXNL19dmuICIQCw3lE5+52F41kpY3B/ sJaPjuKmeIuEyYZDnMnlhHSusg== -END CERTIFICATE- - PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8277246: No need to check about KeyUsage when validating a TSA certificate [v2]
> There is no need to check for the KeyUsage extension when validating a TSA > certificate. > > A test is modified where a TSA cert has a KeyUsage but without the > DigitalSignature bit. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: clarify RFC requirement - Changes: - all: https://git.openjdk.java.net/jdk/pull/6416/files - new: https://git.openjdk.java.net/jdk/pull/6416/files/e7db147e..a5b3bc86 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6416&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6416&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6416/head:pull/6416 PR: https://git.openjdk.java.net/jdk/pull/6416
RFR: 8277246: No need to check about KeyUsage when validating a TSA certificate
There is no need to check for the KeyUsage extension when validating a TSA certificate. A test is modified where a TSA cert has a KeyUsage but without the DigitalSignature bit. - Commit messages: - 8277246: No need to check about KeyUsage when validating a TSA certificate Changes: https://git.openjdk.java.net/jdk/pull/6416/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6416&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277246 Stats: 7 lines in 2 files changed: 0 ins; 6 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6416/head:pull/6416 PR: https://git.openjdk.java.net/jdk/pull/6416
Re: RFR: 8275887: jarsigner prints invalid digest/signature algorithm warnings if keysize is weak/disabled
On Mon, 8 Nov 2021 14:04:15 GMT, Sean Mullan wrote:
> When a signature/digest algorithm was being checked, the algorithm
> constraints checked both the signature/digest algorithm and the key to see if
> they were restricted. This caused duplicate checks and was also problematic
> for `jarsigner` (and `keytool`) which need to distinguish these two cases, so
> that the output can properly indicate when the key is disabled but the
> signature or digest alg is ok.
>
> To address this issue, a new `checkKey` parameter is added to the
> `DisabledAlgorithmConstraints.permits` methods. When `true` the key (alg and
> size) is also checked, otherwise it is not. This flag is always set to
> `false` by `jarsigner` when checking algs and by the JDK when checking digest
> algorithms. Other small changes include changes in `SignerInfo` to use a
> record to store info about the algorithms to be checked, and removing an
> unnecessary CRL checking method from `AlgorithmChecker`.
>
> `keytool` will be enhanced in a subsequent CR to call the new methods.
I'm feeling we should completely dump checking for algorithms and switch to
checking algorithmIds. Even if currently it's only RSASSA-PSS, but suppose one
day we support the SHAKE256-LEN MessageDigest algorithm and I suppose that LEN
cannot be any number.
src/java.base/share/classes/sun/security/pkcs/SignerInfo.java line 749:
> 747: Set enabledAlgorithms = new HashSet<>();
> 748: try {
> 749: for (Map.Entry algorithm :
You can use `var`.
src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java line 1491:
> 1489: private static String checkWeakAlg(String alg,
> CertPathConstraintsParameters cpcp) {
> 1490: try {
> 1491: CERTPATH_DISABLED_CHECK.permits(alg, cpcp, false);
Do we need to check AlgorithmParamters as well? Ex: if `alg` is RSASSA-PSS.
test/jdk/sun/security/tools/jarsigner/TimestampCheck.java line 368:
> 366: .shouldNotContain("The SHA-256 algorithm
> specified " +
> 367: "for the -tsadigestalg option is considered
> a " +
> 368: "security risk and is disabled")
Maybe just check `.shouldNotContain("option is considered a security risk and
is disabled")`?
-
PR: https://git.openjdk.java.net/jdk/pull/6296
Integrated: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs
On Thu, 5 Aug 2021 20:10:44 GMT, Weijun Wang wrote: > New `Subject` APIs `current()` and `callAs()` are created to be replacements > of `getSubject()` and `doAs()` since the latter two methods are now > deprecated for removal. > > In this implementation, by default, `current()` returns the same value as > `getSubject(AccessController.getCurrent())` and `callAs()` is implemented > based on `doAs()`. This behavior is subject to change in the future once > `SecurityManager` is removed. > > User can experiment a possible future mechanism by setting the system > property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` > method stores the subject into a `ThreadLocal` object and the `current()` > method returns it (Note: this mechanism does not work with principal-based > permissions). > > Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and > user can start switching to `callAs()` in their applications. Users can also > switch to `current()` but please note that if you used to call > `getSubject(acc)` in a `doPrivileged` call you might need to try calling > `current()` in a `doPrivilegedWithCombiner` call to see if the > `AccessControlContext` inside the call inherits the subject from the outer > one. This pull request has now been integrated. Changeset: a5c160c7 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/a5c160c711a3f66db18c75973f4efdea63332863 Stats: 665 lines in 20 files changed: 449 ins; 96 del; 120 mod 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/5024
Integrated: 8276863: Remove test/jdk/sun/security/ec/ECDSAJavaVerify.java
On Tue, 9 Nov 2021 14:23:54 GMT, Weijun Wang wrote: > The test was added in JDK-8237218 to confirm that Java impl is used when > verifying a signature. It is useless now since the native implementation is > completely removed. This pull request has now been integrated. Changeset: c27afb31 Author: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/c27afb313b77d19e7ace7101c6f21aa5b2c56505 Stats: 247 lines in 1 file changed: 0 ins; 247 del; 0 mod 8276863: Remove test/jdk/sun/security/ec/ECDSAJavaVerify.java Reviewed-by: ascarpino - PR: https://git.openjdk.java.net/jdk/pull/6313
RFR: 8276863: Remove test/jdk/sun/security/ec/ECDSAJavaVerify.java
The test was added in JDK-8237218 to confirm that Java impl is used when verifying a signature. It is useless now since the native implementation is completely removed. - Commit messages: - 8276863: Remove test/jdk/sun/security/ec/ECDSAJavaVerify.java Changes: https://git.openjdk.java.net/jdk/pull/6313/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6313&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276863 Stats: 247 lines in 1 file changed: 0 ins; 247 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6313.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6313/head:pull/6313 PR: https://git.openjdk.java.net/jdk/pull/6313
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v5]
On Thu, 4 Nov 2021 22:11:41 GMT, Weijun Wang wrote: >> New `Subject` APIs `current()` and `callAs()` are created to be replacements >> of `getSubject()` and `doAs()` since the latter two methods are now >> deprecated for removal. >> >> In this implementation, by default, `current()` returns the same value as >> `getSubject(AccessController.getCurrent())` and `callAs()` is implemented >> based on `doAs()`. This behavior is subject to change in the future once >> `SecurityManager` is removed. >> >> User can experiment a possible future mechanism by setting the system >> property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` >> method stores the subject into a `ThreadLocal` object and the `current()` >> method returns it (Note: this mechanism does not work with principal-based >> permissions). >> >> Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and >> user can start switching to `callAs()` in their applications. Users can also >> switch to `current()` but please note that if you used to call >> `getSubject(acc)` in a `doPrivileged` call you might need to try calling >> `current()` in a `doPrivilegedWithCombiner` call to see if the >> `AccessControlContext` inside the call inherits the subject from the outer >> one. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > tiny spec change to be the same as CSR Your suggestion is more low-level. Anyway, I think this is an enhancement and not a blocker to the current PR. I’ve created a new JBS issue at https://bugs.openjdk.java.net/browse/JDK-8276807. I do think it’s debatable whether such an API is worth adding and how it fits into the structure of JAAS. - PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v5]
On Thu, 4 Nov 2021 22:11:41 GMT, Weijun Wang wrote: >> New `Subject` APIs `current()` and `callAs()` are created to be replacements >> of `getSubject()` and `doAs()` since the latter two methods are now >> deprecated for removal. >> >> In this implementation, by default, `current()` returns the same value as >> `getSubject(AccessController.getCurrent())` and `callAs()` is implemented >> based on `doAs()`. This behavior is subject to change in the future once >> `SecurityManager` is removed. >> >> User can experiment a possible future mechanism by setting the system >> property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` >> method stores the subject into a `ThreadLocal` object and the `current()` >> method returns it (Note: this mechanism does not work with principal-based >> permissions). >> >> Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and >> user can start switching to `callAs()` in their applications. Users can also >> switch to `current()` but please note that if you used to call >> `getSubject(acc)` in a `doPrivileged` call you might need to try calling >> `current()` in a `doPrivilegedWithCombiner` call to see if the >> `AccessControlContext` inside the call inherits the subject from the outer >> one. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > tiny spec change to be the same as CSR When we introduce a new API, we hope it always works as specified. In this case, it should work with both the `ThreadLocal` and `AccessContolContext` mechanisms. In my understanding, to work with the `AccessContolContext` mechanism, this `Supplier` must be wrapped into some kind of `Subject`. This could be weird. Now, if we do this inside JDK, then `callAs(Supplier,Callable)` will create this weird subject and then call `callAs(Subject,Callable)`, and then `current()` will inspect if the subject is in this style and if so fetch the real inner subject and return it. This will be transparent to users. Otherwise, users would need to apply this trick themselves. They need to create this weird subject and call the usual `callAs`. Inside the action, they do the same inspection and fetch the real subject. They can either use this subject directly, or, if the final consumer of the subject is something they can not control (For example, the JGSS-API `GSSContext::initSecContext` searches for Kerberos tickets in subject), they would have to call `callAs` again with this real subject. That's what I meant `callAs` in `callAs`. That's all I can think of now. - PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v5]
> New `Subject` APIs `current()` and `callAs()` are created to be replacements > of `getSubject()` and `doAs()` since the latter two methods are now > deprecated for removal. > > In this implementation, by default, `current()` returns the same value as > `getSubject(AccessController.getCurrent())` and `callAs()` is implemented > based on `doAs()`. This behavior is subject to change in the future once > `SecurityManager` is removed. > > User can experiment a possible future mechanism by setting the system > property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` > method stores the subject into a `ThreadLocal` object and the `current()` > method returns it (Note: this mechanism does not work with principal-based > permissions). > > Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and > user can start switching to `callAs()` in their applications. Users can also > switch to `current()` but please note that if you used to call > `getSubject(acc)` in a `doPrivileged` call you might need to try calling > `current()` in a `doPrivilegedWithCombiner` call to see if the > `AccessControlContext` inside the call inherits the subject from the outer > one. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: tiny spec change to be the same as CSR - Changes: - all: https://git.openjdk.java.net/jdk/pull/5024/files - new: https://git.openjdk.java.net/jdk/pull/5024/files/a9380c0b..8d547bbd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5024&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5024&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5024.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5024/head:pull/5024 PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8225181: KeyStore should have a getAttributes method [v3]
On Wed, 3 Nov 2021 14:18:38 GMT, Weijun Wang wrote: >> Add `KeyStore::getAttributes` so that one can get the attributes of an entry >> without retrieving the entry first. This is especially useful for a private >> key entry which can only be retrieved with a password. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > more clear and precise spec New commit pushed. Typo, I meant "refine". - PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8225181: KeyStore should have a getAttributes method [v4]
> Add `KeyStore::getAttributes` so that one can get the attributes of an entry > without retrieving the entry first. This is especially useful for a private > key entry which can only be retrieved with a password. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: redine spec - Changes: - all: https://git.openjdk.java.net/jdk/pull/6026/files - new: https://git.openjdk.java.net/jdk/pull/6026/files/6145..901bdf83 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6026&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6026&range=02-03 Stats: 8 lines in 1 file changed: 2 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6026.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6026/head:pull/6026 PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8225181: KeyStore should have a getAttributes method [v3]
On Thu, 4 Nov 2021 13:21:19 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> more clear and precise spec
>
> src/java.base/share/classes/java/security/KeyStore.java line 1027:
>
>> 1025: *
>> 1026: * @implSpec
>> 1027: * The default implementation returns an empty {@code Set}.
>
> Would it make more sense for the default impl to throw
> `UnsupportedOperationException` or maybe call `getEntry(alias, null)`?
> Otherwise, an application cannot know the difference between an alias that
> has no attributes and an alias that has attributes but is from a `KeyStore`
> impl that has not overridden the corresponding Spi method.
The one benefit I can think of to throw a UOE is that the caller can fallback
to `getEntry(...).getAttributes()` when an exception is thrown. However, as far
as I know, our PKCS12KeyStore is the only KeyStore implementation that has made
use of attributes. Therefore it's still not late for another implementation to
start supporting both at the same time. For most of the KeyStore
implementations, both `ks.getAttributes()` and
`ks.getEntry(...).getAttributes()` returning empty seems more consistent.
-
PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8231107: Allow store password to be null when saving a PKCS12 KeyStore [v4]
> You can create a password-less PKCS12 KeyStore file now by calling > `ks.store(outStream, null)` no matter what the default cert protection > algorithm and Mac algorithm are defined in `java.security`. > > Note: the system properties set in `ToolsJDK.gmk` to generate `cacerts` must > be retained (at the moment) because the tool is launched with BOOT_JDK. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: same spec in SPI - Changes: - all: https://git.openjdk.java.net/jdk/pull/5950/files - new: https://git.openjdk.java.net/jdk/pull/5950/files/3670f992..266cf6de Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5950&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5950&range=02-03 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5950.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5950/head:pull/5950 PR: https://git.openjdk.java.net/jdk/pull/5950
Re: RFR: 8225181: KeyStore should have a getAttributes method [v2]
On Tue, 2 Nov 2021 15:18:10 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> clarification on protected attributes
>
> src/java.base/share/classes/java/security/KeyStore.java line 1030:
>
>> 1028: *
>> 1029: * @param alias the alias name
>> 1030: * @return an unmodifiable {@code Set} of attributes, possibly
>> empty
>
> If the alias does not exist, then is it definitely always empty? I think the
> word "possibly" here may be a bit misleading. Maybe reword this as:
> "an unmodifiable {@code Set} of attributes." This set is empty if the given
> alias does not exist or there are no attributes associated with the alias.
> This set may also be empty for PrivateKeyEntry or SecretKeyEntry entries that
> contain protected attributes and are only available through the ... "
Updated as suggested. CSR updated as well.
> src/java.base/share/classes/java/security/KeyStore.java line 1031:
>
>> 1029: * @param alias the alias name
>> 1030: * @return an unmodifiable {@code Set} of attributes, possibly
>> empty
>> 1031: * if the given alias does not exist, or there is no
>
> s/is no/are no/
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8225181: KeyStore should have a getAttributes method [v3]
> Add `KeyStore::getAttributes` so that one can get the attributes of an entry > without retrieving the entry first. This is especially useful for a private > key entry which can only be retrieved with a password. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: more clear and precise spec - Changes: - all: https://git.openjdk.java.net/jdk/pull/6026/files - new: https://git.openjdk.java.net/jdk/pull/6026/files/7238e784..6145 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6026&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6026&range=01-02 Stats: 16 lines in 2 files changed: 4 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/6026.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6026/head:pull/6026 PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8273026: Slow LoginContext.login() on multi threading application [v7]
On Tue, 2 Nov 2021 20:39:47 GMT, Florent Guillaume wrote: >> Larry-N has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address review notes > > Could the original JDK-8230297 be closed as a duplicate please? @efge Closed. Thanks for reminding. - PR: https://git.openjdk.java.net/jdk/pull/5748
Re: RFR: 8257722: Improve "keytool -printcert -jarfile" output [v4]
On Thu, 28 Oct 2021 21:13:40 GMT, Hai-May Chao wrote: >> This change does a few improvements to the output of `keytool -printcert >> -jarfile` command to help readability and diagnosis. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Update while block code Looks fine now. I wonder if the order of signers in the set matters. It's possible that one signer appears first in the result of `getCodeSigners()` but because `HashSet` maintains no order it becomes the second. If you are also worried about this, you can make `ss` a `LinkedHashSet`. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6126
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v4]
On Thu, 28 Oct 2021 17:21:42 GMT, Weijun Wang wrote: >> New `Subject` APIs `current()` and `callAs()` are created to be replacements >> of `getSubject()` and `doAs()` since the latter two methods are now >> deprecated for removal. >> >> In this implementation, by default, `current()` returns the same value as >> `getSubject(AccessController.getCurrent())` and `callAs()` is implemented >> based on `doAs()`. This behavior is subject to change in the future once >> `SecurityManager` is removed. >> >> User can experiment a possible future mechanism by setting the system >> property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` >> method stores the subject into a `ThreadLocal` object and the `current()` >> method returns it (Note: this mechanism does not work with principal-based >> permissions). >> >> Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and >> user can start switching to `callAs()` in their applications. Users can also >> switch to `current()` but please note that if you used to call >> `getSubject(acc)` in a `doPrivileged` call you might need to try calling >> `current()` in a `doPrivilegedWithCombiner` call to see if the >> `AccessControlContext` inside the call inherits the subject from the outer >> one. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > more spec tweaks Hi, Chap. Thanks for the comment. Is it possible you run `callAs` with a "higher level" subject that contains a link to another more dynamic subject? For example, just make your `Supplier` a public credential inside. - PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8273026: Slow LoginContext.login() on multi threading application [v7]
On Thu, 28 Oct 2021 18:55:32 GMT, Larry-N wrote: >> This fix adds a cache of service provider classes to LoginContext (in >> particular, it's a cache of LoginModules classes). The approach helps to >> increase the performance of the LoginContext.login() method significantly, >> especially in a multi-threading environment. Service Loader is used for >> polling on Service Provider classes, without instantiating LoginModules >> object if Service Provider name doesn't match record in .config file. The >> set of service providers is cached for each Context Loader separately. >> This code passed successfully tier1 and tier2 tests on mac os. > > Larry-N has updated the pull request incrementally with one additional commit > since the last revision: > > Address review notes Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5748
Re: RFR: 8273026: Slow LoginContext.login() on multi threading application [v5]
On Wed, 27 Oct 2021 20:08:34 GMT, Larry-N wrote: >> This fix adds a cache of service provider classes to LoginContext (in >> particular, it's a cache of LoginModules classes). The approach helps to >> increase the performance of the LoginContext.login() method significantly, >> especially in a multi-threading environment. Service Loader is used for >> polling on Service Provider classes, without instantiating LoginModules >> object if Service Provider name doesn't match record in .config file. The >> set of service providers is cached for each Context Loader separately. >> This code passed successfully tier1 and tier2 tests on mac os. > > Larry-N has updated the pull request incrementally with one additional commit > since the last revision: > > Trailing spaces You might want to see if the "clean" action is useful here. See https://openjdk.java.net/jtreg/tag-spec.html. - PR: https://git.openjdk.java.net/jdk/pull/5748
Re: RFR: 8273026: Slow LoginContext.login() on multi threading application [v5]
On Thu, 28 Oct 2021 17:31:26 GMT, Larry-N wrote: > Thank you for the explanations. When I cleaned up the working directory all > pass ok. ( And fails when I submitted the test a second time) Let's hope the directory is always clean when the test is actually launched. I have no other comments. Thanks for the patience. - PR: https://git.openjdk.java.net/jdk/pull/5748
Re: RFR: 8273026: Slow LoginContext.login() on multi threading application [v6]
On Thu, 28 Oct 2021 17:42:28 GMT, Larry-N wrote: >> This fix adds a cache of service provider classes to LoginContext (in >> particular, it's a cache of LoginModules classes). The approach helps to >> increase the performance of the LoginContext.login() method significantly, >> especially in a multi-threading environment. Service Loader is used for >> polling on Service Provider classes, without instantiating LoginModules >> object if Service Provider name doesn't match record in .config file. The >> set of service providers is cached for each Context Loader separately. >> This code passed successfully tier1 and tier2 tests on mac os. > > Larry-N has updated the pull request incrementally with one additional commit > since the last revision: > > A Corrected the Loader.java testcase Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5748
Re: RFR: 8257722: Improve "keytool -printcert -jarfile" output [v3]
On Thu, 28 Oct 2021 16:17:44 GMT, Hai-May Chao wrote: >> This change does a few improvements to the output of `keytool -printcert >> -jarfile` command to help readability and diagnosis. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Update output per review comment src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2895: > 2893: ss.add(signer); > 2894: } > 2895: } I think the `while (entries.hasMoreElements())` block should end here, so that you collect all signers of all entries before printing them out. Also, there is no need to count `CodeSigner`, you can simply get it from `ss.size()`. - PR: https://git.openjdk.java.net/jdk/pull/6126
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v4]
On Thu, 28 Oct 2021 17:21:42 GMT, Weijun Wang wrote: >> New `Subject` APIs `current()` and `callAs()` are created to be replacements >> of `getSubject()` and `doAs()` since the latter two methods are now >> deprecated for removal. >> >> In this implementation, by default, `current()` returns the same value as >> `getSubject(AccessController.getCurrent())` and `callAs()` is implemented >> based on `doAs()`. This behavior is subject to change in the future once >> `SecurityManager` is removed. >> >> User can experiment a possible future mechanism by setting the system >> property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` >> method stores the subject into a `ThreadLocal` object and the `current()` >> method returns it (Note: this mechanism does not work with principal-based >> permissions). >> >> Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and >> user can start switching to `callAs()` in their applications. Users can also >> switch to `current()` but please note that if you used to call >> `getSubject(acc)` in a `doPrivileged` call you might need to try calling >> `current()` in a `doPrivilegedWithCombiner` call to see if the >> `AccessControlContext` inside the call inherits the subject from the outer >> one. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > more spec tweaks New commit pushed. CSR updated as well. - PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v4]
> New `Subject` APIs `current()` and `callAs()` are created to be replacements > of `getSubject()` and `doAs()` since the latter two methods are now > deprecated for removal. > > In this implementation, by default, `current()` returns the same value as > `getSubject(AccessController.getCurrent())` and `callAs()` is implemented > based on `doAs()`. This behavior is subject to change in the future once > `SecurityManager` is removed. > > User can experiment a possible future mechanism by setting the system > property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` > method stores the subject into a `ThreadLocal` object and the `current()` > method returns it (Note: this mechanism does not work with principal-based > permissions). > > Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and > user can start switching to `callAs()` in their applications. Users can also > switch to `current()` but please note that if you used to call > `getSubject(acc)` in a `doPrivileged` call you might need to try calling > `current()` in a `doPrivilegedWithCombiner` call to see if the > `AccessControlContext` inside the call inherits the subject from the outer > one. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: more spec tweaks - Changes: - all: https://git.openjdk.java.net/jdk/pull/5024/files - new: https://git.openjdk.java.net/jdk/pull/5024/files/a29c8d93..a9380c0b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5024&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5024&range=02-03 Stats: 14 lines in 1 file changed: 4 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/5024.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5024/head:pull/5024 PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8225181: KeyStore should have a getAttributes method [v2]
On Thu, 28 Oct 2021 13:51:01 GMT, Sean Mullan wrote:
>> I wonder if someone will interpret this as "after I've called `getEntry` on
>> a private key, I can get the encrypted attributes through
>> `KeyStore::getAttributes`". How about something like "and only available
>> through the {@link KeyEntry.getAttributres} method after the entry is
>> extracted"?
>
> Ok.
New commit pushed. The words are sightly different between in `KeyStore` and
`KeyStoreSpi`. CSR updated as well.
-
PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8225181: KeyStore should have a getAttributes method [v2]
> Add `KeyStore::getAttributes` so that one can get the attributes of an entry > without retrieving the entry first. This is especially useful for a private > key entry which can only be retrieved with a password. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: clarification on protected attributes - Changes: - all: https://git.openjdk.java.net/jdk/pull/6026/files - new: https://git.openjdk.java.net/jdk/pull/6026/files/86bbe87b..7238e784 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6026&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6026&range=00-01 Stats: 18 lines in 2 files changed: 13 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/6026.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6026/head:pull/6026 PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8273026: Slow LoginContext.login() on multi threading application [v5]
On Wed, 27 Oct 2021 20:08:34 GMT, Larry-N wrote: >> This fix adds a cache of service provider classes to LoginContext (in >> particular, it's a cache of LoginModules classes). The approach helps to >> increase the performance of the LoginContext.login() method significantly, >> especially in a multi-threading environment. Service Loader is used for >> polling on Service Provider classes, without instantiating LoginModules >> object if Service Provider name doesn't match record in .config file. The >> set of service providers is cached for each Context Loader separately. >> This code passed successfully tier1 and tier2 tests on mac os. > > Larry-N has updated the pull request incrementally with one additional commit > since the last revision: > > Trailing spaces On my machine, the 1st `@run` fails as expected with "java.util.ServiceConfigurationError: javax.security.auth.spi.LoginModule: Provider SecondLoginModule not found". When it passed on your machine, can you find SecondLoginModule.class in jtreg's working directory? If I stop at the first `@run` I only see Loader.class and FirstLoginModule.class. Maybe you need to clean up the working directory first? - PR: https://git.openjdk.java.net/jdk/pull/5748
Re: RFR: 8225181: KeyStore should have a getAttributes method
On Wed, 27 Oct 2021 19:40:16 GMT, Sean Mullan wrote:
>> This is complicated. Theoretically a KeyStore implementation can store some
>> attributes in clear text and some encrypted, and it's probably not possible
>> to know if there exist any encrypted ones before actually decrypting the
>> entry. Maybe I should say "For a PrivateKeyEntry or SecretKeyEntry, some
>> attributes might only be available after the entry is extracted by the
>> getEntry() method. Try calling the entry's getAttributes() method to see if
>> there are any".
>
> Yes, a sentence like that would help. Some suggested tweaks: "For a
> PrivateKeyEntry or SecretKeyEntry, some attributes may be protected and not
> available unless the entry is first extracted by the getEntry() method."
>
> I don't think you need the last sentence.
I wonder if someone will interpret this as "after I've called `getEntry` on a
private key, I can get the encrypted attributes through
`KeyStore::getAttributes`". How about something like "and only available
through the {@link KeyEntry.getAttributres} method after the entry is
extracted"?
-
PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8273026: Slow LoginContext.login() on multi threading application [v4]
On Wed, 27 Oct 2021 18:43:41 GMT, Larry-N wrote: >> This fix adds a cache of service provider classes to LoginContext (in >> particular, it's a cache of LoginModules classes). The approach helps to >> increase the performance of the LoginContext.login() method significantly, >> especially in a multi-threading environment. Service Loader is used for >> polling on Service Provider classes, without instantiating LoginModules >> object if Service Provider name doesn't match record in .config file. The >> set of service providers is cached for each Context Loader separately. >> This code passed successfully tier1 and tier2 tests on mac os. > > Larry-N has updated the pull request incrementally with one additional commit > since the last revision: > > Address review notices Have you removed the `isLoaded` flags? If they are still referenced in `Loader`, the two files will always be compiled before running the test. - PR: https://git.openjdk.java.net/jdk/pull/5748
Re: RFR: 8231107: Allow store password to be null when saving a PKCS12 KeyStore [v3]
> You can create a password-less PKCS12 KeyStore file now by calling > `ks.store(outStream, null)` no matter what the default cert protection > algorithm and Mac algorithm are defined in `java.security`. > > Note: the system properties set in `ToolsJDK.gmk` to generate `cacerts` must > be retained (at the moment) because the tool is launched with BOOT_JDK. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: spec change only in patch2: unchanged: - Changes: - all: https://git.openjdk.java.net/jdk/pull/5950/files - new: https://git.openjdk.java.net/jdk/pull/5950/files/aae1b0c1..3670f992 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5950&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5950&range=01-02 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5950.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5950/head:pull/5950 PR: https://git.openjdk.java.net/jdk/pull/5950
Re: RFR: 8257722: Improve "keytool -printcert -jarfile" output [v2]
On Wed, 27 Oct 2021 16:32:48 GMT, Hai-May Chao wrote: >> This change does a few improvements to the output of `keytool -printcert >> -jarfile` command to help readability and diagnosis. > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > Updated TimestampCheck test and removed its unused methods One new comment: Now that if there is only one cert, there is no need to write `Certificate #n of m`. If there's only one signer, we also don't need to write `of signer #n`. - PR: https://git.openjdk.java.net/jdk/pull/6126
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v2]
On Sat, 23 Oct 2021 00:40:39 GMT, Weijun Wang wrote: >> New `Subject` APIs `current()` and `callAs()` are created to be replacements >> of `getSubject()` and `doAs()` since the latter two methods are now >> deprecated for removal. >> >> In this implementation, by default, `current()` returns the same value as >> `getSubject(AccessController.getCurrent())` and `callAs()` is implemented >> based on `doAs()`. This behavior is subject to change in the future once >> `SecurityManager` is removed. >> >> User can experiment a possible future mechanism by setting the system >> property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` >> method stores the subject into a `ThreadLocal` object and the `current()` >> method returns it (Note: this mechanism does not work with principal-based >> permissions). >> >> Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and >> user can start switching to `callAs()` in their applications. Users can also >> switch to `current()` but please note that if you used to call >> `getSubject(acc)` in a `doPrivileged` call you might need to try calling >> `current()` in a `doPrivilegedWithCombiner` call to see if the >> `AccessControlContext` inside the call inherits the subject from the outer >> one. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > renames New commit pushed. The only unresolved is about whether `callAs` can take a null subject. - PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v3]
> New `Subject` APIs `current()` and `callAs()` are created to be replacements > of `getSubject()` and `doAs()` since the latter two methods are now > deprecated for removal. > > In this implementation, by default, `current()` returns the same value as > `getSubject(AccessController.getCurrent())` and `callAs()` is implemented > based on `doAs()`. This behavior is subject to change in the future once > `SecurityManager` is removed. > > User can experiment a possible future mechanism by setting the system > property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` > method stores the subject into a `ThreadLocal` object and the `current()` > method returns it (Note: this mechanism does not work with principal-based > permissions). > > Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and > user can start switching to `callAs()` in their applications. Users can also > switch to `current()` but please note that if you used to call > `getSubject(acc)` in a `doPrivileged` call you might need to try calling > `current()` in a `doPrivilegedWithCombiner` call to see if the > `AccessControlContext` inside the call inherits the subject from the outer > one. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: address comments from Sean and Bernd - Changes: - all: https://git.openjdk.java.net/jdk/pull/5024/files - new: https://git.openjdk.java.net/jdk/pull/5024/files/2f862e02..a29c8d93 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5024&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5024&range=01-02 Stats: 39 lines in 5 files changed: 0 ins; 11 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/5024.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5024/head:pull/5024 PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v2]
On Wed, 27 Oct 2021 12:46:57 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> renames
>
> src/java.base/share/classes/javax/security/auth/Subject.java line 324:
>
>> 322: }
>> 323:
>> 324: // Store the current subject to a ThreadLocal when a system
>> property is set.
>
> nit: "... in a ThreadLocal ..."
Oops.
> src/java.base/share/classes/javax/security/auth/Subject.java line 360:
>
>> 358: *
>> 359: * No matter what storage is chosen, the current subject will
>> 360: * always be installed by the {@link #callAs} method.
>
> I'm not really sure if this sentence is necessary. It seems like it doesn't
> need to be in the specification to me. The first sentence is clear enough to
> me: "The current subject is installed by the {@link #callAs} method."
Removed.
> src/java.base/share/classes/javax/security/auth/Subject.java line 362:
>
>> 360: * always be installed by the {@link #callAs} method.
>> 361: *
>> 362: * @return the current subject. The return value can be
>
> Suggest to combine this into one sentence: the current subject, or {@code
> null} if a current subject is not installed or the current subject is set to
> {@code null}."
OK.
> src/java.base/share/classes/javax/security/auth/Subject.java line 394:
>
>> 392: *
>> 393: * @param subject the intended current subject for {@code action}.
>> 394: *Can be {@code null}.
>
> Is it necessary to allow `null` in the new `callAs` method? Is there a use
> case where this is useful? I know the `doAs` methods allowed it, but it looks
> like they simply call `AccessController.doPrivileged` in that case w/o a
> subject. Seems it would be simpler to not allow it and throw an NPE.
Maybe in a nested call that wants to clear out the existing subject
temporarily? It could be useful.
-
PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v2]
On Mon, 25 Oct 2021 20:02:14 GMT, Bernd wrote:
>> Oh, it's needed. Otherwise the `AccessController.getContext()` call (which
>> is inside `current()`) will also be called in a clean privileged context and
>> there is no subject associated with it.
>>
>> On the other hand, it still needs to in some sort of doPriv. I don't want to
>> ignore `AuthPermission("getSubject")`.
>
> Hm yes i see why „withacombiner“ would be needed, just not sure why
> doPriveledged() is needed (especially since current() does it as well). But I
> admit i don’t see through that part, so ignore me if I don’t make sense.
Application code calling `Krb5Context::initSecContext` does not require a
`AuthPermission("getSubject")` because `getSubject` was called in a doPriv.
After switching to `current`, it still does not require this permission if we
call the new method in a doPrivWithCombiner.
-
PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v2]
On Wed, 27 Oct 2021 13:49:18 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/javax/security/auth/Subject.java line 296:
>>
>>> 294: * which is equivalent to
>>> 295: * {@code Subject.getSubject(AccessController.getContext())}
>>> 296: * by default in this implementation.
>>
>> I don't think you need the words "by default".
>
> I suggest changing the last sentence to two sentences: "However, obtaining a
> Subject is useful independent of the Security Manager. Thus, a replacement
> API named {@link #current()} has been added which can be used to obtain the
> current subject."
>
> I don't think you need to describe the default implementation here because it
> is hard to explain that succinctly -- just keep that info in `current`.
OK.
-
PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v2]
On Mon, 25 Oct 2021 19:52:06 GMT, Bernd wrote:
>> Sure, I can. I was testing the default value to "false" at some time and
>> found `"true".equals` and `!"false".equals` symmetric and good-looking. :-)
>
> It probably doesn’t matter to have the Logic centralized, just looked a bit
> hardcoded (who knows if yes/no/1/0 is wanted as well ,).
OK, switch to GetBooleanAction.
>> I said "After {@code action} is finished, the current subject is reset to
>> its previous value". Is that what you meant?
>
> My question was more along the line of Post-Security-manager. Will you always
> be able to call Subject.doAs - especially with null? And if this can already
> be speced if it might be rejected or not… (the „after finished“ is well
> documented)
Before the method is removed (It's deprecated from removal), you should always
be able to call it with whatever subject including null. If there's no more
AccessControlContext we might even have to implement it with callAs.
-
PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8273026: Slow LoginContext.login() on multi threading application [v3]
On Wed, 27 Oct 2021 13:06:54 GMT, Larry-N wrote:
>> This fix adds a cache of service provider classes to LoginContext (in
>> particular, it's a cache of LoginModules classes). The approach helps to
>> increase the performance of the LoginContext.login() method significantly,
>> especially in a multi-threading environment. Service Loader is used for
>> polling on Service Provider classes, without instantiating LoginModules
>> object if Service Provider name doesn't match record in .config file. The
>> set of service providers is cached for each Context Loader separately.
>> This code passed successfully tier1 and tier2 tests on mac os.
>
> Larry-N has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Corrected trailing whitespaces
Source code change looks mostly fine to me. I'm running some tests now. Will
get back once they finish.
As for the test, IMO, it was meant to show that `SecondLoginModule` is still
needed even if it's not in the JAAS login config file. Your updated test only
prove it's not really instantiated. How about this:
Change the test directives to
* @build FirstLoginModule
* @run main/othervm/fail Loader
* @build SecondLoginModule
* @run main/othervm Loader
so that the login succeeds only after there exists a `SecondLoginModule`. All
those `isLoaded` flags should be removed so their enclosing classes are not
compiled automatically.
src/java.base/share/classes/javax/security/auth/login/LoginContext.java line
229:
> 227: private static final sun.security.util.Debug debug =
> 228: sun.security.util.Debug.getInstance("logincontext",
> "\t[LoginContext]");
> 229: private static final WeakHashMap Set>> cacheServiceProviders = new WeakHashMap<>();
This line can be a little shorter. There are some other lines below which is
also quite long. The original limit was 80 chars but please be at most 90.
src/java.base/share/classes/javax/security/auth/login/LoginContext.java line
704:
> 702: lmProviders =
> cacheServiceProviders.get(contextClassLoader);
> 703: if (lmProviders == null){
> 704: if (debug != null)
Please enclose the following line in a pair of braces.
src/java.base/share/classes/javax/security/auth/login/LoginContext.java line
717:
> 715: }
> 716:
> cacheServiceProviders.put(contextClassLoader,lmProviders);
> 717: }
Is it necessary the lines below still be inside the synchronized block?
src/java.base/share/classes/javax/security/auth/login/LoginContext.java line
723:
> 721: if (debug != null) {
> 722: debug.println(name + " loaded as a
> service");
> 723: }
Please de-indent the if block above.
-
PR: https://git.openjdk.java.net/jdk/pull/5748
Re: RFR: 8257722: Improve "keytool -printcert -jarfile" output
On Tue, 26 Oct 2021 22:37:02 GMT, Hai-May Chao wrote:
> This change does a few improvements to the output of `keytool -printcert
> -jarfile` command to help readability and diagnosis.
src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2878:
> 2876: out.println();
> 2877: out.println();
> 2878: out.println(rb.getString("Signature."));
Please remove a newline as well. There needn't be 3 `println()` calls.
src/java.base/share/classes/sun/security/tools/keytool/Resources.java line 469:
> 467: {"one.in.many", "%1$s #%2$d of %3$d"},
> 468: {"one.in.many1", "%1$s of signer #%2$d"},
> 469: {"one.in.many2", "%1$s #%2$d of %3$d of signer #%4$d"},
`certificate #1 of 2 of signer #1` is a little too complicated. The second
number is not really necessary. I think `certificate #1 of signer #1` is enough.
-
PR: https://git.openjdk.java.net/jdk/pull/6126
Re: RFR: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable [v3]
On Tue, 26 Oct 2021 19:35:42 GMT, Sean Mullan wrote: >> The JDK implementation (as supplied by the "SUN" provider) of >> `X509Certificate::getSubjectAlternativeNames` and >> `X509Certificate::getIssuerAlternativeNames` returns `null` instead of >> throwing a `CertificateParsingException` when the extension is unparseable. >> >> This fix changes the behavior to comply with the specification. >> >> CSR: https://bugs.openjdk.java.net/browse/JDK-8275822 > > Sean Mullan has updated the pull request incrementally with one additional > commit since the last revision: > > Fix NPE. Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6106
Re: RFR: 8231107: Allow store password to be null when saving a PKCS12 KeyStore [v2]
> You can create a password-less PKCS12 KeyStore file now by calling > `ks.store(outStream, null)` no matter what the default cert protection > algorithm and Mac algorithm are defined in `java.security`. > > Note: the system properties set in `ToolsJDK.gmk` to generate `cacerts` must > be retained (at the moment) because the tool is launched with BOOT_JDK. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: test update - Changes: - all: https://git.openjdk.java.net/jdk/pull/5950/files - new: https://git.openjdk.java.net/jdk/pull/5950/files/7d789775..aae1b0c1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5950&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5950&range=00-01 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5950.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5950/head:pull/5950 PR: https://git.openjdk.java.net/jdk/pull/5950
Re: RFR: 8251468: X509Certificate.get{Subject, Issuer}AlternativeNames does not throw CertificateParsingException if extension is unparseable
On Tue, 26 Oct 2021 15:28:51 GMT, Sean Mullan wrote: >> I was asking if `getIssuerAlternativeNameExtension` can throw the exception >> if IAE exists but not parseable. > > Ok, I understand your comment now. I'm hesitant to change those methods to > throw an exception because to be consistent all the `get*Extension()` methods > should then throw an Exception. That might be the right thing to do, but it > is a bigger change and more risky. The code that calls these internal methods > is used for building certification paths, and if null is returned, it is as > if the certificate did not contain the extension. That might be a more > reasonable behavior than throwing an Exception, since it allows the code to > find other potential certificates or certification paths. Adding certpath > debug can always be used to find out more about why certain certificates were > not selected. OK. - PR: https://git.openjdk.java.net/jdk/pull/6106
Re: RFR: 8225181: KeyStore should have a getAttributes method
On Mon, 25 Oct 2021 14:36:58 GMT, Sean Mullan wrote:
>> Add `KeyStore::getAttributes` so that one can get the attributes of an entry
>> without retrieving the entry first. This is especially useful for a private
>> key entry which can only be retrieved with a password.
>
> src/java.base/share/classes/java/security/KeyStore.java line 1038:
>
>> 1036: * @throwsKeyStoreException if the keystore has not been
>> initialized
>> 1037: * (loaded).
>> 1038: *
>
> throw NPE if alias is null?
OK.
> src/java.base/share/classes/java/security/KeyStoreSpi.java line 457:
>
>> 455: */
>> 456: public Set engineGetAttributes(String alias) {
>> 457: return Collections.emptySet();
>
> Would `Set.of()` be better here?
Both returns an internal constant object and I think they are similar. I was
copying the default implementation of `Entry::getAttributes`.
-
PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8225181: KeyStore should have a getAttributes method
On Mon, 25 Oct 2021 14:34:57 GMT, Sean Mullan wrote:
>> Add `KeyStore::getAttributes` so that one can get the attributes of an entry
>> without retrieving the entry first. This is especially useful for a private
>> key entry which can only be retrieved with a password.
>
> src/java.base/share/classes/java/security/KeyStore.java line 1035:
>
>> 1033: * not extractable (For example, if the attributes is
>> encrypted
>> 1034: * in a private key entry or a secret key entry).
>> 1035: *
>
> I think this would read better if you broke it up into multiple sentences,
> ex: "an unmodifiable {@code Set} of attributes. The set may be empty if the
> given alias does not exist, or the alias does exist but there are no
> attributes associated with it or the attributes are not extractable (for
> example, the attributes may not be extractable if they are encrypted in a
> private key or secret key entry)."
>
> You may also want to add a sentence to try the
> `KeyStore$Entry::getAttributes` method if there are no attributes.
>
> Did you consider throwing a KeyStoreException if they are not extractable? It
> would be useful to distinguish that case from an alias that has no attributes.
This is complicated. Theoretically a KeyStore implementation can store some
attributes in clear text and some encrypted, and it's probably not possible to
know if there exist any encrypted ones before actually decrypting the entry.
Maybe I should say "For a PrivateKeyEntry or SecretKeyEntry, some attributes
might only be available after the entry is extracted by the getEntry() method.
Try calling the entry's getAttributes() method to see if there are any".
> src/java.base/share/classes/java/security/KeyStoreSpi.java line 450:
>
>> 448: /**
>> 449: * Retrieves the attributes associated with the given alias.
>> 450: *
>
> You should also document the default implementation.
OK.
-
PR: https://git.openjdk.java.net/jdk/pull/6026
Re: RFR: 8275918: Remove unused local variables in java.base security code
On Sat, 23 Oct 2021 14:04:07 GMT, Andrey Turbanov wrote:
> Cleanup unused local variables. Looks like they are leftovers after
> refactoring.
src/java.base/share/classes/sun/security/rsa/RSAPSSSignature.java line 211:
> 209: AlgorithmParameterSpec keyParams = rsaKey.getParams();
> 210: // validate key parameters
> 211: if (!isCompatible(keyParams, this.sigParams)) {
Why not remove the assignment on line 209? This is more consistent with your
fix this time.
-
PR: https://git.openjdk.java.net/jdk/pull/6092
Integrated: 8251134: Unwrapping a key with a Private Key generated by Microsoft CNG fails
On Wed, 20 Oct 2021 18:06:39 GMT, Weijun Wang wrote: > Support Cipher operations on CNG keys. This pull request has now been integrated. Changeset: 10e1610f Author: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/10e1610f7b99f42f834478528df7ecfb4320aec1 Stats: 210 lines in 4 files changed: 201 ins; 0 del; 9 mod 8251134: Unwrapping a key with a Private Key generated by Microsoft CNG fails Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk/pull/6049
Re: RFR: 8251134: Unwrapping a key with a Private Key generated by Microsoft CNG fails [v2]
> Support Cipher operations on CNG keys. 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. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6049/files - new: https://git.openjdk.java.net/jdk/pull/6049/files/2612797b..2612797b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=6049&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=6049&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6049.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6049/head:pull/6049 PR: https://git.openjdk.java.net/jdk/pull/6049
Integrated: 8185844: MSCAPI doesn't list aliases correctly
On Wed, 20 Oct 2021 17:54:50 GMT, Weijun Wang wrote: > If a entry is overwritten by another one using the same alias, make sure the > old one is removed. This pull request has now been integrated. Changeset: 43619458 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/43619458d183bbbaec745887314ddcf7a8aa4136 Stats: 174 lines in 2 files changed: 123 ins; 34 del; 17 mod 8185844: MSCAPI doesn't list aliases correctly Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk/pull/6047
Re: RFR: 8251468: X509Certificate.get{Subject, Issuer}AlternativeNames does not throw CertificateParsingException if extension is unparseable
On Mon, 25 Oct 2021 20:17:17 GMT, Sean Mullan wrote: >> That's probably a little deeper and changing it will have a mass effect. >> What about at the `getIssuerAlternativeNameExtension` level? > > Unless I am misunderstanding your comment, I don't think this is an issue in > practice. The code inside the `X509CertImpl.getExtension` method only throws > an Exception if invalid OIDs or attribute names are passed to the internal > `get` methods of `X509CertInfo` and `CertificateExtensions`, which isn't > possible when you are passing in known values/attributes. I think this is why > the code swallows the exceptions and returns null, but it would be nice to > have a comment explaining that. I was asking if `getIssuerAlternativeNameExtension` can throw the exception is IAE exists but not parseable. - PR: https://git.openjdk.java.net/jdk/pull/6106
Re: RFR: 8231107: Allow store password to be null when saving a PKCS12 KeyStore
On Mon, 25 Oct 2021 17:05:58 GMT, Sean Mullan wrote: >> You can create a password-less PKCS12 KeyStore file now by calling >> `ks.store(outStream, null)` no matter what the default cert protection >> algorithm and Mac algorithm are defined in `java.security`. >> >> Note: the system properties set in `ToolsJDK.gmk` to generate `cacerts` must >> be retained (at the moment) because the tool is launched with BOOT_JDK. > > test/jdk/sun/security/pkcs12/EmptyPassword.java line 27: > >> 25: * @test >> 26: * @bug 8202299 8231107 >> 27: * @modules java.base/sun.security.tools.keytool > > Can you add an @summary? I'll update the existing summary to `@summary Testing empty (null, "", "\0") password behaviors`. - PR: https://git.openjdk.java.net/jdk/pull/5950
Re: RFR: 8231107: Allow store password to be null when saving a PKCS12 KeyStore
On Mon, 25 Oct 2021 17:02:10 GMT, Sean Mullan wrote: >> You can create a password-less PKCS12 KeyStore file now by calling >> `ks.store(outStream, null)` no matter what the default cert protection >> algorithm and Mac algorithm are defined in `java.security`. >> >> Note: the system properties set in `ToolsJDK.gmk` to generate `cacerts` must >> be retained (at the moment) because the tool is launched with BOOT_JDK. > > test/jdk/sun/security/pkcs12/EmptyPassword.java line 57: > >> 55: }); >> 56: >> 57: // 8202299: interop before new char[0] and new char[1] > > Can you make this comment more descriptive? Not sure what "before" means. Is > this just making sure you can store a keystore and key entry with "\0" as the > password and load it back with ""? Should you also try to load it back with > "\0" too? Typo: s/before/between/. I'll add one with "\0". That should always work since it's the same password used in load() and store(). - PR: https://git.openjdk.java.net/jdk/pull/5950
Re: RFR: 8251468: X509Certificate.get{Subject, Issuer}AlternativeNames does not throw CertificateParsingException if extension is unparseable
On Mon, 25 Oct 2021 15:13:25 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 1618:
>>
>>> 1616: }
>>> 1617: SubjectAlternativeNameExtension subjectAltNameExt =
>>> 1618: getSubjectAlternativeNameExtension();
>>
>> Does it make sense to let the line above throwing an exception? I see the
>> method is called in several places (`X509CertSelector`, `Builder`, etc).
>> What is the correct behavior in those places?
>
> To clarify, do you mean this code in `getExtension(ObjectIdentifier)` that
> swallows the exception?:
>
>
> } catch (IOException ioe) {
> return null;
> }
That's probably a little deeper and changing it will have a mass effect. What
about at the `getIssuerAlternativeNameExtension` level?
-
PR: https://git.openjdk.java.net/jdk/pull/6106
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v2]
On Mon, 25 Oct 2021 18:24:36 GMT, Weijun Wang wrote: >> test/jdk/sun/security/krb5/KrbCredSubKey.java line 34: >> >>> 32: >>> 33: import java.io.FileOutputStream; >>> 34: import java.util.concurrent.Callable; >> >> Should those tests run with both permutations of the system property? > > I did ran it but not sure if it's good to add an extra `@run` to all tests. > I'll select a few. Hi Bernd, thanks a huge lot for all your comments. I'm really glad you care about the next steps of JEP 411 in such details. There are quite some other things we need to do in the next few releases. - PR: https://git.openjdk.java.net/jdk/pull/5024
