Withdrawn: 6859027: Duplicate communications to KDC in GSSManager.createCredential(usage)
On Fri, 27 Aug 2021 19:47:30 GMT, Weijun Wang wrote: > A one slot ccache is added to avoid multiple authentication from the same > principal. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/5286
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v2]
On Fri, 22 Oct 2021 22:00:57 GMT, Bernd wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> renames
>
> src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5Util.java
> line 107:
>
>> 105: */
>> 106: public static ServiceCreds getServiceCreds(GSSCaller caller,
>> 107: String serverPrincipal) throws LoginException {
>
> What would be the new way to pass an authentication context on, passing the
> subject directly? (In case of Krb5AcceptCredential acc is actually the
> current one)
What about the Kerberos cipher suite callsite mentioned in the comment? If no
longer used, can this be made not Public (and remove the comment)
-
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 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
Hello,
overall I think the new api is really need. Just wonder if it needs a way to
restrict subject changes in called sections (to replace security manager) and
also if the test cases should all run for both environment settings for
SubjectTL.
src/java.base/share/classes/javax/security/auth/Subject.java line 325:
> 323:
> 324: // Store the current subject to a ThreadLocal when a system property
> is set.
> 325: private static final boolean USE_TL = "true".equalsIgnoreCase(
Can you use GetBooleanAction.privilegedGetProperty instead?
src/java.base/share/classes/javax/security/auth/Subject.java line 349:
> 347: * the one of its parent thread, and will not change even if
> 348: * its parent thread's current subject is changed to another value.
> 349: *
Should it say something about installing or unsettling the subject in a nested
execution (if it can be restricted)?
src/java.base/share/classes/javax/security/auth/Subject.java line 393:
> 391: * always be retrievable by the {@link #current} method.
> 392: *
> 393: * @param subject the intended current subject for {@code action}.
The „current“ could be removed to make it less complex to read? (Especially if
the next parameter still uses the „current“ term.
src/java.base/share/classes/javax/security/auth/Subject.java line 475:
> 473: * call {@link #callAs} to perform the same work, which is
> based on
> 474: * {@link #doAs(Subject, PrivilegedExceptionAction)}
> 475: * by default in this implementation.
Should it also mention that unless you define the TL system property it will
still affect the new current() call? (Just to introduce the concept by
repetition).
src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5Context.java
line 708:
> 706: @SuppressWarnings("removal")
> 707: final Subject subject =
> 708:
> AccessController.doPrivilegedWithCombiner(
Is this actually needed and correct to wrap this into a priveledged action?
src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5Util.java line
107:
> 105: */
> 106: public static ServiceCreds getServiceCreds(GSSCaller caller,
> 107: String serverPrincipal) throws LoginException {
What would be the new way to pass an authentication context on, passing the
subject directly? (In case of Krb5AcceptCredential acc is actually the current
one)
test/jdk/javax/security/auth/Subject/DoAs.java line 44:
> 42: final int index = i;
> 43: Subject.callAs(subject, () -> {
> 44: Subject s = Subject.current();
Should it Test old and new method to retrieve subject?
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?
-
PR: https://git.openjdk.java.net/jdk/pull/5024
Re: RFR: 8158689: java.security.KeyPair should implement Destroyable
On Fri, 22 Oct 2021 23:50:38 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this change. It makes KeyPair implement Destroyable and > implements the methods to call the underlying privateKey. It also sets the > public and private key to 'final'. > > The bug includes a CSR and Release Notes > CSR: https://bugs.openjdk.java.net/browse/JDK-8275823 > RN: https://bugs.openjdk.java.net/browse/JDK-8275826 I just think this is not worth doing. Plus, there are conventions for `Destroyable` that need to be followed. For example, after one call `destroy()` on a `KeyPair`, can `getPrivateKey()` still be called? Should it throw an `IllegalStateException`? Also, `PrivateKey` has implemented `Destroyable `for quite some time and it looks like none of its implementations inside JDK has actually implemented this method. This seems a more valuable thing to do. There are other interfaces like `KeySpec` (or some of its children) that should also implement `Destroyable`. - PR: https://git.openjdk.java.net/jdk/pull/6089
Re: RFR: 8158689: java.security.KeyPair should implement Destroyable
On Fri, 22 Oct 2021 23:50:38 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this change. It makes KeyPair implement Destroyable and > implements the methods to call the underlying privateKey. It also sets the > public and private key to 'final'. > > The bug includes a CSR and Release Notes > CSR: https://bugs.openjdk.java.net/browse/JDK-8275823 > RN: https://bugs.openjdk.java.net/browse/JDK-8275826 Even though it has been the only option for a long time, calling `keyPair.getPrivateKey().destory()` is not clean and one should not have had to go into a class's objects to cleanup. It's reminiscent of free memory in C. - PR: https://git.openjdk.java.net/jdk/pull/6089
Re: RFR: 8158689: java.security.KeyPair should implement Destroyable
On Fri, 22 Oct 2021 23:50:38 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this change. It makes KeyPair implement Destroyable and > implements the methods to call the underlying privateKey. It also sets the > public and private key to 'final'. > > The bug includes a CSR and Release Notes > CSR: https://bugs.openjdk.java.net/browse/JDK-8275823 > RN: https://bugs.openjdk.java.net/browse/JDK-8275826 You can, it's just a multi-step process that should have been available from KeyPair in a single step process - PR: https://git.openjdk.java.net/jdk/pull/6089
Re: RFR: 8158689: java.security.KeyPair should implement Destroyable
On Fri, 22 Oct 2021 23:50:38 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this change. It makes KeyPair implement Destroyable and > implements the methods to call the underlying privateKey. It also sets the > public and private key to 'final'. > > The bug includes a CSR and Release Notes > CSR: https://bugs.openjdk.java.net/browse/JDK-8275823 > RN: https://bugs.openjdk.java.net/browse/JDK-8275826 `KeyPair::getPrivateKey` returns the private key inside (i.e. not a clone). Why can't we just destroy it? - PR: https://git.openjdk.java.net/jdk/pull/6089
Re: RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/5024/files - new: https://git.openjdk.java.net/jdk/pull/5024/files/7c99d9b7..2f862e02 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5024=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5024=00-01 Stats: 28 lines in 6 files changed: 0 ins; 0 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
RFR: 8158689: java.security.KeyPair should implement Destroyable
Hi, I need a review of this change. It makes KeyPair implement Destroyable and implements the methods to call the underlying privateKey. It also sets the public and private key to 'final'. The bug includes a CSR and Release Notes CSR: https://bugs.openjdk.java.net/browse/JDK-8275823 RN: https://bugs.openjdk.java.net/browse/JDK-8275826 - Commit messages: - initial fix Changes: https://git.openjdk.java.net/jdk/pull/6089/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6089=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8158689 Stats: 62 lines in 2 files changed: 55 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/6089.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6089/head:pull/6089 PR: https://git.openjdk.java.net/jdk/pull/6089
Re: RFR: 8275811 Incorrect instance to dispose [v2]
On Fri, 22 Oct 2021 19:25:38 GMT, Daniel Jeliński wrote: >> The current code that changes cipher suites disposes the new suite instead >> of the old one, which usually silently fails. This patch fixes the code to >> dispose the old instance instead. >> >> DTLS appears to be unaffected: DTLSOutputRecord keeps 2 ciphers and >> correctly [disposes the old >> one](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Fix one more case The problem is with SSLEngine; we create the ChangeCipherSpec message in a delegated task, then encrypt it later using memoized cipher: at java.base/sun.security.ssl.SSLCipher$T10BlockWriteCipherGenerator$BlockWriteCipher.encrypt(SSLCipher.java:1206) at java.base/sun.security.ssl.OutputRecord.t10Encrypt(OutputRecord.java:441) at java.base/sun.security.ssl.OutputRecord.encrypt(OutputRecord.java:345) at java.base/sun.security.ssl.SSLEngineOutputRecord$HandshakeFragment.acquireCiphertext(SSLEngineOutputRecord.java:518) at java.base/sun.security.ssl.SSLEngineOutputRecord.acquireCiphertext(SSLEngineOutputRecord.java:346) at java.base/sun.security.ssl.SSLEngineOutputRecord.encode(SSLEngineOutputRecord.java:198) (line numbers may be off a bit) If the memoized cipher is disposed, the operation produces incorrect output. This happens only when ChangeCipherSpec or KeyUpdate is encrypted and we're using SSLEngine. Looks like we can move writeCipher disposal to the relevant OutputRecord subclasses. Will look into it. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 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.
Still reviewing but here are some initial minor comments.
src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5Util.java line
67:
> 65:
> 66: @SuppressWarnings("removal")
> 67: Subject accSubj = Subject.current();
Change variable name to `currentSubj` or `currSubj`?
src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5Util.java line
68:
> 66: @SuppressWarnings("removal") AccessControlContext acc) throws
> LoginException {
> 67:
> 68: // Try to get ticket from acc's Subject
Is this comment still helpful? Maybe just change it to `// Try to get ticket
from current Subject`
test/jdk/java/security/AccessController/PreserveCombiner.java line 46:
> 44:
> 45: // get subject from current ACC - this always worked
> 46: Subject doAsSubject = Subject.current();
Nit: change variable name to `callAsSubject`.
test/jdk/sun/security/krb5/auto/Context.java line 114:
> 112: }
> 113: });
> 114: } catch (CompletionException pae) {
Maybe rename the variable to `ce`. Same comment below.
-
PR: https://git.openjdk.java.net/jdk/pull/5024
Integrated: 8272163: Add -version option to keytool and jarsigner
On Thu, 14 Oct 2021 16:04:08 GMT, Hai-May Chao wrote: > It'd be useful to have a -version option for keytool and jarsigner. Many > other JDK tools already have a -version option. This is to add -version > option to keytool and jarsigner like jar command does. > > CSR review: > https://bugs.openjdk.java.net/browse/JDK-8275174 This pull request has now been integrated. Changeset: fec470f2 Author:Hai-May Chao URL: https://git.openjdk.java.net/jdk/commit/fec470f262d1df581f2a5fc2f0bdfea66757a8ad Stats: 213 lines in 6 files changed: 212 ins; 0 del; 1 mod 8272163: Add -version option to keytool and jarsigner Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/5954
Re: RFR: 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key [v4]
On Fri, 22 Oct 2021 18:45:31 GMT, Alexey Bakhtin wrote: >> Hello, >> >> Could you please review the small patch for the issue described in >> JDK-8271199: Mutual TLS handshake fails signing client certificate with >> custom sensitive PKCS11 key >> >> I suggest updating the RSAPSSSignature.isValid() method to verify if >> provided key components can be applied to SunRSASign implementation. >> If not applied, implementation can try to select signer from other providers >> >> Regards >> Alexey > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Simplified isPrivateKeyValid Thank you all. :jdk_security tests passed without regression - PR: https://git.openjdk.java.net/jdk/pull/4887
Re: RFR: 8275811 Incorrect instance to dispose
On Fri, 22 Oct 2021 18:45:31 GMT, Xue-Lei Andrew Fan wrote: >> The current code that changes cipher suites disposes the new suite instead >> of the old one, which usually silently fails. This patch fixes the code to >> dispose the old instance instead. >> >> DTLS appears to be unaffected: DTLSOutputRecord keeps 2 ciphers and >> correctly [disposes the old >> one](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Did you want to cover the update for line 222 at OutputRecord.java as well? Thanks @XueleiFan , but I guess this needs a bit more love. Just finished running jdk_security tests, and a few tests failed, apparently related: javax/net/ssl/SSLEngine/NoAuthClientAuth.java javax/net/ssl/TLSv1/TLSRehandshakeTest.java javax/net/ssl/TLSv1/TLSRehandshakeWithCipherChangeTest.java javax/net/ssl/TLSv1/TLSRehandshakeWithDataExTest.java javax/net/ssl/TLSv11/TLSRehandshakeTest.java javax/net/ssl/TLSv11/TLSRehandshakeWithDataExTest.java I'll see if I can figure this out. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v2]
On Fri, 22 Oct 2021 19:25:38 GMT, Daniel Jeliński wrote: >> The current code that changes cipher suites disposes the new suite instead >> of the old one, which usually silently fails. This patch fixes the code to >> dispose the old instance instead. >> >> DTLS appears to be unaffected: DTLSOutputRecord keeps 2 ciphers and >> correctly [disposes the old >> one](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), >> and DTLSInputRecord [doesn't dispose >> anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Fix one more case Looks good to me. Thank you! - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose [v2]
> The current code that changes cipher suites disposes the new suite instead of > the old one, which usually silently fails. This patch fixes the code to > dispose the old instance instead. > > DTLS appears to be unaffected: DTLSOutputRecord keeps 2 ciphers and correctly > [disposes the old > one](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Fix one more case - Changes: - all: https://git.openjdk.java.net/jdk/pull/6084/files - new: https://git.openjdk.java.net/jdk/pull/6084/files/773c073c..1ee99ae4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6084=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6084=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6084.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6084/head:pull/6084 PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8275811 Incorrect instance to dispose
On Fri, 22 Oct 2021 18:24:22 GMT, Daniel Jeliński wrote: > The current code that changes cipher suites disposes the new suite instead of > the old one, which usually silently fails. This patch fixes the code to > dispose the old instance instead. > > DTLS appears to be unaffected: DTLSOutputRecord keeps 2 ciphers and correctly > [disposes the old > one](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Ah, missed that one. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key [v3]
On Fri, 22 Oct 2021 17:24:36 GMT, Valerie Peng wrote: > Changes look fine. Have you run through all security regression tests besides > those under sun/security/rsa? Hi @valeriepeng, Yes, I've run this code with all security tests. Passed successfully. - PR: https://git.openjdk.java.net/jdk/pull/4887
Re: RFR: 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key [v4]
On Fri, 22 Oct 2021 18:45:31 GMT, Alexey Bakhtin wrote: >> Hello, >> >> Could you please review the small patch for the issue described in >> JDK-8271199: Mutual TLS handshake fails signing client certificate with >> custom sensitive PKCS11 key >> >> I suggest updating the RSAPSSSignature.isValid() method to verify if >> provided key components can be applied to SunRSASign implementation. >> If not applied, implementation can try to select signer from other providers >> >> Regards >> Alexey > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Simplified isPrivateKeyValid It looks good to me. Please make sure to run all security regression tests. Thank you! - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4887
Re: RFR: 8275811 Incorrect instance to dispose
On Fri, 22 Oct 2021 18:24:22 GMT, Daniel Jeliński wrote: > The current code that changes cipher suites disposes the new suite instead of > the old one, which usually silently fails. This patch fixes the code to > dispose the old instance instead. > > DTLS appears to be unaffected: DTLSOutputRecord keeps 2 ciphers and correctly > [disposes the old > one](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), > and DTLSInputRecord [doesn't dispose > anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) Did you want to cover the update for line 222 at OutputRecord.java as well? - PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key [v3]
On Thu, 21 Oct 2021 19:16:34 GMT, Alexey Bakhtin wrote: >> Hello, >> >> Could you please review the small patch for the issue described in >> JDK-8271199: Mutual TLS handshake fails signing client certificate with >> custom sensitive PKCS11 key >> >> I suggest updating the RSAPSSSignature.isValid() method to verify if >> provided key components can be applied to SunRSASign implementation. >> If not applied, implementation can try to select signer from other providers >> >> Regards >> Alexey > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Change exception handling Agree, It looks less readable. Just tried to avoid rethrowing the exception. Reverted back to the original version. - PR: https://git.openjdk.java.net/jdk/pull/4887
Re: RFR: 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key [v4]
> Hello, > > Could you please review the small patch for the issue described in > JDK-8271199: Mutual TLS handshake fails signing client certificate with > custom sensitive PKCS11 key > > I suggest updating the RSAPSSSignature.isValid() method to verify if provided > key components can be applied to SunRSASign implementation. > If not applied, implementation can try to select signer from other providers > > Regards > Alexey Alexey Bakhtin has updated the pull request incrementally with one additional commit since the last revision: Simplified isPrivateKeyValid - Changes: - all: https://git.openjdk.java.net/jdk/pull/4887/files - new: https://git.openjdk.java.net/jdk/pull/4887/files/df6f212d..ede6436f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4887=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4887=02-03 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4887.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4887/head:pull/4887 PR: https://git.openjdk.java.net/jdk/pull/4887
Re: Bug / performance problem in changeCipherSuite
Thanks Xuelei for the JBS ticket and the hint. PR is on its way. Regards, Daniel pt., 22 paź 2021 o 19:28 Xuelei Fan napisał(a): > > Hi Daniel, > > Thank you for the nice catch! I filed a JBS bug: > https://bugs.openjdk.java.net/browse/JDK-8275811 > > It would be nice if you could also update similar issues in (DTLS)OutRecord > files. > > Thanks, > Xuelei > > On Oct 22, 2021, at 8:14 AM, Daniel Jeliński wrote: > > Hi all, > During routine examination of thread dumps I noticed a stack trace you > may find interesting. Relevant part: > > java.lang.Thread.State: RUNNABLE > ... > at java.lang.IllegalStateException.(java.base@11.0.11/Unknown Source) > at javax.crypto.Cipher.checkCipherState(java.base@11.0.11/Unknown Source) > at javax.crypto.Cipher.doFinal(java.base@11.0.11/Unknown Source) > at > sun.security.ssl.SSLCipher$T12GcmReadCipherGenerator$GcmReadCipher.dispose(java.base@11.0.11/Unknown > Source) > at sun.security.ssl.InputRecord.changeReadCiphers(java.base@11.0.11/Unknown > Source) > at > sun.security.ssl.ChangeCipherSpec$T10ChangeCipherSpecConsumer.consume(java.base@11.0.11/Unknown > Source) > ... > > All handshakes that negotiate GCM ciphers throw and catch an > exception, because the newly created cipher is disposed before use. > > I believe this is caused by this line of code: > https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/InputRecord.java#L125 > > I think it should read as follows: > this.readCipher.dispose(); > > I can file a PR, just need help with JBS ID. > Regards, > Daniel > >
RFR: 8275811 Incorrect instance to dispose
The current code that changes cipher suites disposes the new suite instead of the old one, which usually silently fails. This patch fixes the code to dispose the old instance instead. DTLS appears to be unaffected: DTLSOutputRecord keeps 2 ciphers and correctly [disposes the old one](https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java#L106), and DTLSInputRecord [doesn't dispose anything](https://github.com/openjdk/jdk/blob/4b9303b77b43d890ebacbec38b4ac5db7e171886/src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java#L57) - Commit messages: - Dispose correct cipher Changes: https://git.openjdk.java.net/jdk/pull/6084/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6084=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275811 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6084.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6084/head:pull/6084 PR: https://git.openjdk.java.net/jdk/pull/6084
Re: RFR: 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key [v3]
On Thu, 21 Oct 2021 19:16:34 GMT, Alexey Bakhtin wrote:
>> Hello,
>>
>> Could you please review the small patch for the issue described in
>> JDK-8271199: Mutual TLS handshake fails signing client certificate with
>> custom sensitive PKCS11 key
>>
>> I suggest updating the RSAPSSSignature.isValid() method to verify if
>> provided key components can be applied to SunRSASign implementation.
>> If not applied, implementation can try to select signer from other providers
>>
>> Regards
>> Alexey
>
> Alexey Bakhtin has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Change exception handling
Changes requested by xuelei (Reviewer).
src/java.base/share/classes/sun/security/rsa/RSAPSSSignature.java line 212:
> 210: */
> 211: private void isPrivateKeyValid(RSAPrivateKey prKey) throws
> InvalidKeyException {
> 212: InvalidKeyException ikException = null;
If I read the code correct, by define a local variable, it looks like you are
trying to avoid to re-throw the exception in the catch clause. But this style
adds additional effort to read the code. Exception re-throw should be fine as
if the exception has been generated and will be thrown in the end of the method.
src/java.base/share/classes/sun/security/rsa/RSAPSSSignature.java line 220:
> 218: crtKey.getPublicExponent());
> 219: } else {
> 220: ikException = new InvalidKeyException(
See above comment, I will just throw the exception, rather than cache it.
-
PR: https://git.openjdk.java.net/jdk/pull/4887
Openjdk build failure in slowdebug mode
I would like to point your attention to this issue https://github.com/openjdk/jdk/pull/5123 Simple 2 compiler warnings / errors because -Werror when building openjdk in slowdebug mode and a simple fix. -- Marián Konček
Re: Bug / performance problem in changeCipherSuite
Hi Daniel, Thank you for the nice catch! I filed a JBS bug: https://bugs.openjdk.java.net/browse/JDK-8275811 It would be nice if you could also update similar issues in (DTLS)OutRecord files. Thanks, Xuelei On Oct 22, 2021, at 8:14 AM, Daniel Jeliński mailto:djelins...@gmail.com>> wrote: Hi all, During routine examination of thread dumps I noticed a stack trace you may find interesting. Relevant part: java.lang.Thread.State: RUNNABLE ... at java.lang.IllegalStateException.(java.base@11.0.11/Unknown Source) at javax.crypto.Cipher.checkCipherState(java.base@11.0.11/Unknown Source) at javax.crypto.Cipher.doFinal(java.base@11.0.11/Unknown Source) at sun.security.ssl.SSLCipher$T12GcmReadCipherGenerator$GcmReadCipher.dispose(java.base@11.0.11/Unknown Source) at sun.security.ssl.InputRecord.changeReadCiphers(java.base@11.0.11/Unknown Source) at sun.security.ssl.ChangeCipherSpec$T10ChangeCipherSpecConsumer.consume(java.base@11.0.11/Unknown Source) ... All handshakes that negotiate GCM ciphers throw and catch an exception, because the newly created cipher is disposed before use. I believe this is caused by this line of code: https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/InputRecord.java#L125 I think it should read as follows: this.readCipher.dispose(); I can file a PR, just need help with JBS ID. Regards, Daniel
Re: RFR: 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key [v3]
On Thu, 21 Oct 2021 19:16:34 GMT, Alexey Bakhtin wrote: >> Hello, >> >> Could you please review the small patch for the issue described in >> JDK-8271199: Mutual TLS handshake fails signing client certificate with >> custom sensitive PKCS11 key >> >> I suggest updating the RSAPSSSignature.isValid() method to verify if >> provided key components can be applied to SunRSASign implementation. >> If not applied, implementation can try to select signer from other providers >> >> Regards >> Alexey > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > Change exception handling Changes look fine. Have you run through all security regression tests besides those under sun/security/rsa? - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4887
Re: Java ignores/errors canonicalized principals (NT-PRINCIPAL) from Active Directory
Apoligies, the Java kinit run was not performed with the correct
krb5.conf. I have now repeated with the proper one and the stacktrace
fits the code and my analysis now:
C:\Users\osipovmi>kinit osipo...@ad001.siemens.net
Picked up _JAVA_OPTIONS: -Dsun.security.krb5.debug=false
-Djava.security.krb5.conf=C:\Config\Kerberos\krb5.conf
Password for osipo...@ad001.siemens.net:
Exception: krb_error 41 Message stream modified (41) Message stream modified
KrbException: Message stream modified (41)
at sun.security.krb5.KrbKdcRep.check(KrbKdcRep.java:69)
at sun.security.krb5.KrbAsRep.decrypt(KrbAsRep.java:159)
at sun.security.krb5.KrbAsRep.decryptUsingPassword(KrbAsRep.java:139)
at sun.security.krb5.KrbAsReqBuilder.resolve(KrbAsReqBuilder.java:312)
at sun.security.krb5.KrbAsReqBuilder.action(KrbAsReqBuilder.java:498)
at sun.security.krb5.internal.tools.Kinit.acquire(Kinit.java:248)
at sun.security.krb5.internal.tools.Kinit.(Kinit.java:134)
at sun.security.krb5.internal.tools.Kinit.main(Kinit.java:96)
I am pretty confident that the sname realm string is the cause here.
Michael
Am 2021-10-20 um 10:03 schrieb Osipov, Michael (LDA IT PLM):
Hi folks,
we have recently noticed the following with Java's kinit (tested with
Zulu 8 and 13, code is identical in 18 as well):
C:\Users\osipovmi>kinit osipo...@ad001.siemens.net
I have intentionally written the realm in lowercase to rely on
canonicalization of the AD KDC. krb5.conf contains "canonicalize = true"
as well.
Wireshark shows me in the AS-REQ:
as-req/req-body/sname: name-type KRB5-NT-SRV-INST (2), sname-string (2):
krbtgt and ad001.siemens.net
AS-REP:
as-rep/ticket/sname: name-type KRB5-NT-SRV-INST (2), sname-string (2):
krbtgt and AD001.SIEMENS.NET
Hence, the KDC has properly canonicalized the sname. Java gives me:
Exception: krb_error 41 Message stream modified (41) Message stream
modified
KrbException: Message stream modified (41)
at sun.security.krb5.KrbKdcRep.check(KrbKdcRep.java:55)
at sun.security.krb5.KrbAsRep.decrypt(KrbAsRep.java:159)
at
sun.security.krb5.KrbAsRep.decryptUsingPassword(KrbAsRep.java:139)
at
sun.security.krb5.KrbAsReqBuilder.resolve(KrbAsReqBuilder.java:312)
at
sun.security.krb5.KrbAsReqBuilder.action(KrbAsReqBuilder.java:498)
at sun.security.krb5.internal.tools.Kinit.acquire(Kinit.java:248)
at sun.security.krb5.internal.tools.Kinit.(Kinit.java:134)
at sun.security.krb5.internal.tools.Kinit.main(Kinit.java:96)
Referrals aren't involved here, same realm.
The failing code block is from:
*
https://github.com/AdoptOpenJDK/openjdk-jdk8u/blob/9a751dc19fae78ce58fb0eb176522070c992fb6f/jdk/src/share/classes/sun/security/krb5/KrbKdcRep.java#L58-L71
*
https://github.com/AdoptOpenJDK/openjdk-jdk/blob/ff4997014fe5462dca2b313f3f483400ffee5b62/src/java.security.jgss/share/classes/sun/security/krb5/KrbKdcRep.java#L58-L71
// sname change in TGS-REP is allowed only if client
// sent CANONICALIZE and new sname is a referral of
// the form krbtgt/to-realm@from-realm.com.
if (!req.reqBody.sname.equals(rep.encKDCRepPart.sname)) {
String[] snameStrings =
rep.encKDCRepPart.sname.getNameStrings();
if (isAsReq ||
!req.reqBody.kdcOptions.get(KDCOptions.CANONICALIZE) ||
snameStrings == null || snameStrings.length != 2 ||
!snameStrings[0].equals(PrincipalName.TGS_DEFAULT_SRV_NAME) ||
!rep.encKDCRepPart.sname.getRealmString().equals(
req.reqBody.sname.getRealmString())) {
rep.encKDCRepPart.key.destroy();
throw new KrbApErrException(Krb5.KRB_AP_ERR_MODIFIED);
}
}
So it either fails for
isAsReq
or
!rep.encKDCRepPart.sname.getRealmString().equals(
req.reqBody.sname.getRealmString())
Here is MIT Kerberos:
$ kinit osipo...@ad001.siemens.net
Passwort für osipo...@ad001.siemens.net:
$ klist
Ticketzwischenspeicher: FILE:/tmp/krb5cc_722
Standard-Principal: osipo...@ad001.siemens.net
Valid starting Expires Service principal
20.10.2021 10:02:14 20.10.2021 20:02:14
krbtgt/ad001.siemens@ad001.siemens.net
erneuern bis 21.10.2021 10:02:10
Works as expected. Can someone explain? Shall I create a bug for this?
Michael
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v5]
On Wed, 20 Oct 2021 15:41:35 GMT, Daniel Fuchs wrote: >> Aleksei Efimov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Change InetAddressResolver method names > > Marked as reviewed by dfuchs (Reviewer). @dfuch @AlanBateman fa655be2bb0a402b70916567d34cc29a7898f938 and 2a554c91864e3b42a0ea315b0a671782fe341927 contain reworked `InetAddress`/`InetAddressResolverProvider`/`InetAddressResolver` specs with the following changes: - `InetAddress Resolver Providers` InetAddress section was modified and moved to `InetAddressResolverProvider`. - `Host Name Resolution` InetAddress section was updated to reference new InetAddress resolver SPI. - Changes for previous review comments. - javadoc formatting clean-up - PR: https://git.openjdk.java.net/jdk/pull/5822
Bug / performance problem in changeCipherSuite
Hi all, During routine examination of thread dumps I noticed a stack trace you may find interesting. Relevant part: java.lang.Thread.State: RUNNABLE ... at java.lang.IllegalStateException.(java.base@11.0.11/Unknown Source) at javax.crypto.Cipher.checkCipherState(java.base@11.0.11/Unknown Source) at javax.crypto.Cipher.doFinal(java.base@11.0.11/Unknown Source) at sun.security.ssl.SSLCipher$T12GcmReadCipherGenerator$GcmReadCipher.dispose(java.base@11.0.11/Unknown Source) at sun.security.ssl.InputRecord.changeReadCiphers(java.base@11.0.11/Unknown Source) at sun.security.ssl.ChangeCipherSpec$T10ChangeCipherSpecConsumer.consume(java.base@11.0.11/Unknown Source) ... All handshakes that negotiate GCM ciphers throw and catch an exception, because the newly created cipher is disposed before use. I believe this is caused by this line of code: https://github.com/openjdk/jdk/blob/739769c8fc4b496f08a92225a12d07414537b6c0/src/java.base/share/classes/sun/security/ssl/InputRecord.java#L125 I think it should read as follows: this.readCipher.dispose(); I can file a PR, just need help with JBS ID. Regards, Daniel
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v7]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with one additional commit since the last revision: More javadoc updates to API classes - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/2a554c91..fa655be2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=05-06 Stats: 88 lines in 3 files changed: 22 ins; 8 del; 58 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: Java ignores/errors canonicalized principals (NT-PRINCIPAL) from Active Directory
Am 2021-10-21 um 21:38 schrieb Wei-Jun Wang:
KrbKdcReq throws the exception on line 55, so it is the previous check
if (isAsReq && !req.reqBody.cname.equals(rep.cname) &&
((!req.reqBody.kdcOptions.get(KDCOptions.CANONICALIZE) &&
req.reqBody.cname.getNameType() !=
PrincipalName.KRB_NT_ENTERPRISE) ||
!rep.encKDCRepPart.flags.get(Krb5.TKT_OPTS_ENC_PA_REP))) {
rep.encKDCRepPart.key.destroy();
throw new KrbApErrException(Krb5.KRB_AP_ERR_MODIFIED);
}
So maybe it's the cname was changed, but I'm not sure about the flags.
Can you send me some packets? Hopefully with a key tab or password so I can
look into rep.encKDCRepPart.
I misread the block, of course it is this one. the crealm is changing an
I am not providing an enterprise principal.
Sent you the pcap file. If this isn't enough, will prepare with a keytab.
Michael
