Re: RFR: 8267860: Off-by-one bug when searching arrays in AlpnGreaseTest
On Fri, 10 Jun 2022 17:12:30 GMT, Kevin Driver wrote:
> This PR resolves: https://bugs.openjdk.org/browse/JDK-8267860
LGTM.
test/jdk/sun/security/ssl/ALPN/AlpnGreaseTest.java line 86:
> 84:
> 85: private static void findGreaseInClientHello(byte[] bytes) throws
> Exception {
> 86: for (int i = 0; i < bytes.length - greaseBytes.length + 1; i++) {
LGTM.
-
Marked as reviewed by wetmore (Reviewer).
PR: https://git.openjdk.org/jdk/pull/9131
Re: RFR: 8277307: Pre shared key sent under both session_ticket and pre_shared_key extensions [v2]
On Thu, 2 Jun 2022 21:02:16 GMT, Daniel Jeliński wrote: >> Session ticket extension should only contain pre-TLS1.3 stateless session >> tickets; it should not be used for sending TLS1.3 pre-shared keys. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > different check for TLS13 A little late to this review as it's already been pushed, but I would have suggested leaving the `return new SessionTicketSpec().getEncoded();` as it keeps the encapsulation more clear. Otherwise, it looks good. - PR: https://git.openjdk.org/jdk/pull/8922
Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults [v2]
On Tue, 24 May 2022 16:20:10 GMT, Mark Powers wrote: > Mach5 tier1 and tier2 completed without any failures. I don't know what to > make of the pre-submit failures - lang and hotspot? IIUC, these are due to Loom failures in some of the other platforms supported by OpenJDK but not by Oracle. They are being resolved. - PR: https://git.openjdk.java.net/jdk/pull/8559
Re: RFR: 8287119: Add Distrust.java to ProblemList
On Sat, 21 May 2022 00:26:10 GMT, Rajan Halade wrote: > It will take me some time to figure out what to do with expired certificates. > We can either remove those test scenarios, perform backdated validation or > allow those expired scenarios to be treated as pass. LGTM. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8823
Re: RFR: 8284209: Replace remaining usages of 'a the' in source code
On Wed, 18 May 2022 14:46:42 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > It's the last issue in the series, and it still touches different areas of > the code. Looked at -java.security.jgss. LGTM - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8771
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. Looked at - java.base/.../sun/security - jdk.crypto.* - test/*/com/sun/crypto/provider - test/*/java/lang/SecurityManager - test/*/java/security - test/*/krb5 - test/*/jarsigner and those look fine. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 22:38:04 GMT, Anthony Scarpino wrote: >> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 162: >> >>> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING); >>> 161: >>> 162: // Read NST >> >> What is NST? > > New Session Ticket Duh, of course! Yay, TLAs!!! (Three Letter Acronyms...) Feel free to expand if you're so inclined. ;) - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 23:03:27 GMT, Anthony Scarpino wrote: >> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 172: >> >>> 170: out.clear(); >>> 171: String testString = "ASDF"; >>> 172: in.put(testString.getBytes()).flip(); >> >> If you're going to convert back from UTF_8 later, you should probably >> convert using getBytes(UTF_8) here. > > setting the input to UTF8 isn't a concern. The latter line to decode it > changes it from using the ByteBuffer.toString() to the contents of the > ByteBuffer in a String. You could use the default charsets for encoding and decoding. i.e. in.clear(); receive(server, out.asReadOnlyBuffer(), in); byte[] ba = new byte[in.remaining()]; in.get(ba); String testResult = new String(ba); - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 22:25:43 GMT, Anthony Scarpino
wrote:
>> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 1:
>>
>>> 1: /*
>>
>> Wondering why this is in javax/net/ssl/SSLSession instead of
>> sun/security/ssl/SSLCipher.
>
> I can move it.. I created it from another test which happen to be in that
> directory
Ah. If you can, please do. Thanks.
>> test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 157:
>>
>>> 155: // Do TLS handshake
>>> 156: do {
>>> 157: statusClient = doHandshake(client, out, in);
>>
>> It's potentially a little inefficient returning after each wrap/unwrap()
>> instead of doing the task right away, but it works. No need to change.
>>
>> Is this style something you copied from another test? The
>> SSLEngineTemplate in the templates directory is what I often use.
>
> I got it from somewhere that I don't remember off hand. I'm just trying to
> get through the handshake.
It was just an observation, no need to change.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of this fix to allow a read-only 'src' buffer to be used with > SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher > operation when a read-only buffer is passed. If the 'src' is read-write, > there is no effect on the current operation > > The PR also includes a CSR for an API implementation note to the > SSLEngine.unwrap. The 'src' buffer may be modified during the decryption > operation. 'unwrap()' has had this behavior forever, so there is no > compatibility issue with this note. Using the 'src' buffer for in-place > decryption was a performance decision. > > Tony All of the comments observed are minor. Please consider some of the test changes, but otherwise looks good. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino
wrote:
> Hi,
>
> I need a review of this fix to allow a read-only 'src' buffer to be used with
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher
> operation when a read-only buffer is passed. If the 'src' is read-write,
> there is no effect on the current operation
>
> The PR also includes a CSR for an API implementation note to the
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption
> operation. 'unwrap()' has had this behavior forever, so there is no
> compatibility issue with this note. Using the 'src' buffer for in-place
> decryption was a performance decision.
>
> Tony
Finished the SSLEngine/tests, starting on the SSLCipher. Here's the notes so
far.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 1:
> 1: /*
Wondering why this is in javax/net/ssl/SSLSession instead of
sun/security/ssl/SSLCipher.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 51:
> 49: public class ReadOnlyEngine {
> 50:
> 51: private static String pathToStores = "../etc";
These 6 can be final if you want.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 96:
> 94: status = engine.getHandshakeStatus();
> 95: break;
> 96: case FINISHED:
Can combine FINISHED/NOT_HANDSHAKING?
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 157:
> 155: // Do TLS handshake
> 156: do {
> 157: statusClient = doHandshake(client, out, in);
It's potentially a little inefficient returning after each wrap/unwrap()
instead of doing the task right away, but it works. No need to change.
Is this style something you copied from another test? The SSLEngineTemplate
in the templates directory is what I often use.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 160:
> 158: statusServer = doHandshake(server, in, out);
> 159: } while (statusClient != HandshakeStatus.NOT_HANDSHAKING ||
> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING);
Minor indent problem.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 162:
> 160: statusServer != HandshakeStatus.NOT_HANDSHAKING);
> 161:
> 162: // Read NST
What is NST?
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 172:
> 170: out.clear();
> 171: String testString = "ASDF";
> 172: in.put(testString.getBytes()).flip();
If you're going to convert back from UTF_8 later, you should probably convert
using getBytes(UTF_8) here.
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 188:
> 186: in.clear();
> 187: System.out.println("2: Server send: " + testString);
> 188: in.put(testString.getBytes()).flip();
Same
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 189:
> 187: System.out.println("2: Server send: " + testString);
> 188: in.put(testString.getBytes()).flip();
> 189: send(server, in, out);
Did you want to try asReadOnlyBuffer() here also?
test/jdk/javax/net/ssl/SSLSession/ReadOnlyEngine.java line 191:
> 189: send(server, in, out);
> 190: in.clear();
> 191: receive(client, out, in);
And here?
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults
On Thu, 5 May 2022 16:49:07 GMT, Mark Powers wrote: > JDK-6725221 Standardize obtaining boolean properties with defaults Sorry for the confusion. The original intent of this bug 14 years ago was to standardize the seclibs code where simple getProperty calls were needed in the context of an AccessController, without having to provide the doProvided calls. i.e. GetBooleanAction.privilegedGetProperty(). This was not intended not other parts of the JDK. For example: ChannelImpl.java provides a getBooleanProperty() method, which is very similar to GetBooleanAction. I noticed other places in the security where this was being done. Some of the classes like Debug have been rewritten (SSLLogger), so the issue does not appear to exist there any logger. The certpath code is working with Security.getProperty(), so that was a red herring. - PR: https://git.openjdk.java.net/jdk/pull/8559
Re: RFR: JDK-8284688 Minor cleanup could be done in java.security.jgss [v5]
On Thu, 5 May 2022 23:24:12 GMT, Mark Powers wrote: > The IBM files say this at the top: DO NOT ALTER OR REMOVE COPYRIGHT NOTICES > OR THIS FILE HEADER That's the standard copyright notice. Let's check with Oracle's copyright person... - PR: https://git.openjdk.java.net/jdk/pull/7746
Re: RFR: JDK-8284688 Minor cleanup could be done in java.security.jgss [v5]
On Thu, 5 May 2022 21:05:40 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8284688 >> >> [JDK-8273046](https://bugs.openjdk.java.net/browse/JDK-8273046) is the >> umbrella bug for this bug. The changes were too large for a single code >> review, so it was decided to split into smaller chunks. This is one such >> chunk: >> >> open/src/java.security.jgss/share/classes/javax/security >> open/src/java.security.jgss/share/classes/org/ietf >> open/src/java.security.jgss/share/classes/sun/security > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Max comments Do the various IBM files need a copyright date update? - PR: https://git.openjdk.java.net/jdk/pull/7746
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
On Tue, 3 May 2022 02:07:13 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More note update linux-x64 Infra + JCK passed. Looks good. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 Infra + JCK passed. Looks good. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 tier1/tier2 tests pass. Did not try infra or JCK yet. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
On Tue, 3 May 2022 02:07:13 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More note update tier1/tier2 passed. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 Thanks for the extra time to review this change. I'm still wondering if there is better pattern that doesn't use user_canceled, but that doesn't need to delay this fix from going in. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
On Tue, 3 May 2022 23:10:51 GMT, Xue-Lei Andrew Fan wrote: > Could someone in Oracle help to run the Mach5 testing for security and > network components (or just tier1 and tier2), and let me know if this update > causes any failures? Builds underway. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v3]
On Tue, 3 May 2022 02:02:58 GMT, Xue-Lei Andrew Fan wrote: >>> Thanks for the rewording. Updated. >> >> I made one more tweak that reads better. > > Yes, it looks better. Updated. Thanks! Looks good, thanks. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v3]
On Mon, 2 May 2022 17:45:44 GMT, Xue-Lei Andrew Fan wrote: > Thanks for the rewording. Updated. I made one more tweak that reads better. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v3]
On Mon, 2 May 2022 05:01:21 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > comment about remove finalize() method src/java.base/share/classes/sun/security/ssl/BaseSSLSocketImpl.java line 44: > 42: * overridden in SSLSocketImpl. > 43: * > 44: * There used to be a finalize() implementation, but decided that was Since you haven't pushed yet, maybe: There used to be a finalize() implementation that sent close_notify's, but decided that was not needed. An application should close its sockets and not let them get garbage collected without closing them. The underlying native resources are handled by the Socket Cleaner. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with two > additional commits since the last revision: > > - typo blank linke correction > - revise the update Marked as reviewed by wetmore (Reviewer). src/java.base/share/classes/sun/security/ssl/BaseSSLSocketImpl.java line 265: > 263: } > 264: > 265: /** Can you please add a quick couple lines here with the technical rationale for removing it, so we don't forget down the road. i.e. There used to be a finalize() here, but decided that was not needed. If users don't properly close the socket... The native resources are handled by the Socket Cleaner. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with two > additional commits since the last revision: > > - typo blank linke correction > - revise the update Ok, after much back and forth, I'm convinced this is ok. @dfuch commented in another thread: > An application should really close its sockets and not let them get garbage > collected without closing them: this is sloppy. > So brutally closing the underlying TCP connection in that case should be an > acceptable behaviour, and that would be achieved by just removing the > finalize. Thanks for allowing me to look more deeply into this. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8285890: Fix some @param tags
On Fri, 29 Apr 2022 09:03:58 GMT, Pavel Rappo wrote: > * Syntactically improves a few cases from 8285676 > (https://github.com/openjdk/jdk/pull/8410) > * Fixes a few misspelled `@param` tags for elements that, although are not > included in the API Documentation, are visible in and processed by IDEs LGTM also. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8465
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 14 Apr 2022 15:37:05 GMT, Daniel Jeliński wrote: > IMO we should not send close_notify in the finalizer. It's the application's > responsibility to send close_notify when it's done with the socket; we should > not pretend that it was closed normally when it was not. @djelinski makes an excellent point which I hadn't really considered. This is an error situation. As the native Socket resources are cleaned/released as needed, a simple removal might actually be appropriate. @XueleiFan I'm going to send you some internal discussion we've had in a minute. Let's both parse it and see if there is anything further we should consider, and circle back tomorrow and finalize the plan & push. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v5]
On Thu, 28 Apr 2022 18:29:35 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Max and Brad comments round two LGTM, Pt2... - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v5]
On Thu, 28 Apr 2022 18:29:35 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Max and Brad comments round two Final changes LGTM. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:37:35 GMT, Mark Powers wrote: >> `Security.getProperty()` does not specify the value will be `trim()`. > > My mistake. It's only the trim that you wanted removed, line 94. No, the API for Security.getProperty doesn't specify trimming, so suggest leaving the trim() part also. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 16:22:43 GMT, Bradford Wetmore wrote:
>> src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:
>>
>>> 90: static String getSecurityProperty(final String name) {
>>> 91: return AccessController.doPrivileged((PrivilegedAction)
>>> () -> {
>>> 92: return Security.getProperty(name);
>>
>> I assume we still need to do the if-empty-then-null conversion?
>
> Just found the same. This needs to be reverted. You can set a Security
> Property to an "empty" string which won't work here. Suggest you revert to
> previous code, possibly using a lambda if that was the original intent.
`Security.getProperty()` does not specify the value will be `trim()`.
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 15:45:58 GMT, Weijun Wang wrote:
>> Mark Powers has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains eight additional
>> commits since the last revision:
>>
>> - Merge
>> - Max and Brad comments
>> - jaikiran comments
>> - Alan Bateman comments
>> - second iteration
>> - Merge
>> - Merge
>> - first iteration
>
> src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:
>
>> 90: static String getSecurityProperty(final String name) {
>> 91: return AccessController.doPrivileged((PrivilegedAction)
>> () -> {
>> 92: return Security.getProperty(name);
>
> I assume we still need to do the if-empty-then-null conversion?
Just found the same. This needs to be reverted. You can set a Security
Property to an "empty" string which won't work here. Suggest you revert to
previous code, possibly using a lambda if that was the original intent.
> src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82:
>
>> 80: String type;
>> 81: type = GetPropertyAction.privilegedGetProperty(
>> 82: "ssl.TrustManagerFactory.algorithm");
>
> Sorry I got it wrong here, this is a security property.
Similar comment.
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 02:33:49 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - Merge > - Max and Brad comments > - jaikiran comments > - Alan Bateman comments > - second iteration > - Merge > - Merge > - first iteration These need to be addressed before integration. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]
On Thu, 28 Apr 2022 15:47:44 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains eight additional >> commits since the last revision: >> >> - Merge >> - Max and Brad comments >> - jaikiran comments >> - Alan Bateman comments >> - second iteration >> - Merge >> - Merge >> - first iteration > > src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70: > >> 68: String type; >> 69: type = GetPropertyAction.privilegedGetProperty( >> 70: "ssl.KeyManagerFactory.algorithm"); > > So sorry I got it wrong here, this is a security property. > `GetPropertyAction.privilegedGetProperty` is for system properties. I just noticed the same. - PR: https://git.openjdk.java.net/jdk/pull/8384
Integrated: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields
On Tue, 26 Apr 2022 22:55:29 GMT, Bradford Wetmore wrote: > Two new constant fields `MGF1ParameterSpec.SHA512_224` and > `MGF1ParameterSpec.SHA512_256` didn't have `@since 11` tag added as part of > [JDK-8146293](https://bugs.openjdk.java.net/browse/JDK-8146293). > > This bug addresses this issue. This pull request has now been integrated. Changeset: cf1b00a6 Author:Bradford Wetmore URL: https://git.openjdk.java.net/jdk/commit/cf1b00a60483c2c45b9465aa2bdb7072c92b7072 Stats: 21 lines in 1 file changed: 8 ins; 0 del; 13 mod 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields Reviewed-by: hchao, valeriep, xuelei, mullan - PR: https://git.openjdk.java.net/jdk/pull/8411
Re: RFR: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields [v4]
> Two new constant fields `MGF1ParameterSpec.SHA512_224` and > `MGF1ParameterSpec.SHA512_256` didn't have `@since 11` tag added as part of > [JDK-8146293](https://bugs.openjdk.java.net/browse/JDK-8146293). > > This bug addresses this issue. Bradford Wetmore has updated the pull request incrementally with one additional commit since the last revision: Use @ code tags instead of raw HTML - Changes: - all: https://git.openjdk.java.net/jdk/pull/8411/files - new: https://git.openjdk.java.net/jdk/pull/8411/files/00edca0d..e733085f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8411=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8411=02-03 Stats: 17 lines in 1 file changed: 0 ins; 6 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8411.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8411/head:pull/8411 PR: https://git.openjdk.java.net/jdk/pull/8411
Re: RFR: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields [v3]
On Wed, 27 Apr 2022 13:08:03 GMT, Sean Mullan wrote: >> Bradford Wetmore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Missed one minor codereview suggestion. > > src/java.base/share/classes/java/security/spec/MGF1ParameterSpec.java line 98: > >> 96: >> 97: /** >> 98: * The MGF1ParameterSpec which uses SHA-512/224 message digest > > Feel free to cover this as a separate issue, but I think these constant > descriptions should end with a period. Also, I would say "... uses _a_ > SHA-512/224 message digest." (change in italics). There is also some > inconsistency in the constants, some put the digest algorithm in > double-quotes while others don't - it would be good to be consistent. Updated for consistency. - PR: https://git.openjdk.java.net/jdk/pull/8411
Re: RFR: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields [v3]
> Two new constant fields `MGF1ParameterSpec.SHA512_224` and > `MGF1ParameterSpec.SHA512_256` didn't have `@since 11` tag added as part of > [JDK-8146293](https://bugs.openjdk.java.net/browse/JDK-8146293). > > This bug addresses this issue. Bradford Wetmore has updated the pull request incrementally with one additional commit since the last revision: Missed one minor codereview suggestion. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8411/files - new: https://git.openjdk.java.net/jdk/pull/8411/files/e79b5d87..00edca0d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8411=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8411=01-02 Stats: 16 lines in 1 file changed: 5 ins; 0 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8411.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8411/head:pull/8411 PR: https://git.openjdk.java.net/jdk/pull/8411
Re: RFR: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields [v2]
> Two new constant fields `MGF1ParameterSpec.SHA512_224` and > `MGF1ParameterSpec.SHA512_256` didn't have `@since 11` tag added as part of > [JDK-8146293](https://bugs.openjdk.java.net/browse/JDK-8146293). > > This bug addresses this issue. Bradford Wetmore has updated the pull request incrementally with two additional commits since the last revision: - Minor IJ suggestion cleanup. - Addressed codereview comments on javadoc style. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8411/files - new: https://git.openjdk.java.net/jdk/pull/8411/files/8f3981c1..e79b5d87 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8411=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8411=00-01 Stats: 17 lines in 1 file changed: 5 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/8411.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8411/head:pull/8411 PR: https://git.openjdk.java.net/jdk/pull/8411
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Wed, 27 Apr 2022 15:22:08 GMT, Mark Powers wrote:
>> src/java.base/share/classes/javax/net/ssl/SSLSessionBindingEvent.java line
>> 37:
>>
>>> 35: * {@link SSLSession#putValue(String, Object)}
>>> 36: * or {@link SSLSession#removeValue(String)}, objects which
>>> 37: * implement the SSLSessionBindingListener will receive an
>>
>> If you're up for it, you could fix the missing `` or `@link`
>> throughout this small file. Ignore otherwise.
>
> I'll ignore for now. Javadoc issues are already being tracked for
> javax.crypto with JDK-8284851. This bug could easily be expanded to include
> javax.net.
Ok. I'm going to do a little surgery in a java.security this morning, but
JDK-8284851 could probably be expanded to address both the JCA/JCE code.
(java/security, javax/crypto).
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields
On Wed, 27 Apr 2022 00:12:32 GMT, Valerie Peng wrote: > Is a CSR needed? @valeriepeng I would be surprised. I don't think so, but checking with CSR. - PR: https://git.openjdk.java.net/jdk/pull/8411
Re: zlib before 1.2.12 allows memory corruption (CVE-2018-25032)
On 4/20/2022 5:06 PM, Vitaly Provodin wrote: Recently we (at JetBrains) were faced with the vulnerability issue CVE-2018-25032 (zlib before 1.2.12 allows memory corruption…) It is known that Linux, macOS builds uses system’s zlib but Windows - bundled one (by default). On Linux and macOS users can work around the issue by installing proper zlib on their systems. Are there any ideas for Windows? - the way building (under Cygwin!) with system zlib looks unworkable in case if Cygwin is not installed on user's machines. It looks like after implementing https://bugs.openjdk.java.net/browse/JDK-8249963 (which also discussed here https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-July/067868.html) the resolution of such issues can be shifted to users but what can be done now Hi Vitaly, A better forum might be core-lib-dev[1], and build-dev as you already cc'd. Brad [1] https://mail.openjdk.java.net/mailman/listinfo/core-libs-dev
Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces
On Tue, 26 Apr 2022 22:24:26 GMT, Joe Darcy wrote: > To enable more complete doclint checking (courtesy @jonathan-gibbons), please > review this PR to add type-level @param tags where they are missing. > > To the maintainers of java.util.concurrent, those changes could be separated > out in another bug if that would ease maintenance of that code. > > Making these library fixes is a blocker for correcting and expanding the > doclint checks (JDK-8285496). > > I'll update copyright years before pushing. The two `java.security` ones LGTM. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8410
RFR: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields
Two new constant fields `MGF1ParameterSpec.SHA512_224` and `MGF1ParameterSpec.SHA512_256` didn't have `@since 11` tag added as part of [JDK-8146293](https://bugs.openjdk.java.net/browse/JDK-8146293). This bug addresses this issue. - Commit messages: - 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields Changes: https://git.openjdk.java.net/jdk/pull/8411/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8411=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285683 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8411.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8411/head:pull/8411 PR: https://git.openjdk.java.net/jdk/pull/8411
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 19:49:11 GMT, Bradford Wetmore wrote:
>> src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 69:
>>
>>> 67: String type;
>>> 68: type = AccessController.doPrivileged((PrivilegedAction)
>>> () ->
>>> 69: Security.getProperty("ssl.KeyManagerFactory.algorithm"));
>>
>> We can probably use
>> `sun.security.action.GetPropertyAction::privilegedGetProperty`. This is
>> inside `java.base` so that class is always available.
>
> Wasn't there another bug to address this?
Perhaps as part of
[JDK-6725221](https://bugs.openjdk.java.net/browse/JDK-6725221)?
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: 8285380: Fix typos in security [v2]
On Thu, 21 Apr 2022 17:32:32 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on modules owned by the security team (`java.security.jgss >> java.security.sasl java.smartcardio java.xml.crypto jdk.crypto.cryptoki >> jdk.crypto.ec jdk.crypto.mscapi jdk.security.auth jdk.security.jgss`), and >> accepted those changes where it indeed discovered real typos. >> >> I will update copyright years using a script before pushing (otherwise like >> every second change would be a copyright update, making reviewing much >> harder). >> >> The long term goal here is to make tooling support for running `codespell`. >> The trouble with automating this is of course all false positives. But >> before even trying to solve that issue, all true positives must be fixed. >> Hence this PR. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Revert changes to Apache code LGTM. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8340
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 19:05:05 GMT, Weijun Wang wrote:
>> Mark Powers has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Alan Bateman comments
>
> src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 69:
>
>> 67: String type;
>> 68: type = AccessController.doPrivileged((PrivilegedAction)
>> () ->
>> 69: Security.getProperty("ssl.KeyManagerFactory.algorithm"));
>
> We can probably use
> `sun.security.action.GetPropertyAction::privilegedGetProperty`. This is
> inside `java.base` so that class is always available.
Wasn't there another bug to address this?
-
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8285504
>>
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for
>> a single code review, so it was decided to split into smaller chunks. This
>> is one such chunk:
>>
>> open/src/java.base/share/classes/java/net
>
> Mark Powers has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Alan Bateman comments
Other than the comments mentioned, LGTM.
src/java.base/share/classes/javax/net/ssl/SSLSessionBindingEvent.java line 37:
> 35: * {@link SSLSession#putValue(String, Object)}
> 36: * or {@link SSLSession#removeValue(String)}, objects which
> 37: * implement the SSLSessionBindingListener will receive an
If you're up for it, you could fix the missing `` or `@link`
throughout this small file. Ignore otherwise.
-
Marked as reviewed by wetmore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers wrote: >> https://bugs.openjdk.java.net/browse/JDK-8285504 >> >> JDK-8273046 is the umbrella bug for this bug. The changes were too large for >> a single code review, so it was decided to split into smaller chunks. This >> is one such chunk: >> >> open/src/java.base/share/classes/java/net > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > Alan Bateman comments src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line 72: > 70: > 71: this.parameters = List.copyOf(parameters); > 72: } If you leave this as is, you can use `<>` - PR: https://git.openjdk.java.net/jdk/pull/8384
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto [v3]
On Fri, 15 Apr 2022 23:32:11 GMT, Mark Powers wrote: >> JDK-8284112 Minor cleanup could be done in javax.crypto > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > cleanup This looks ok to me. Go ahead and push. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8214
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto [v3]
On Fri, 15 Apr 2022 23:32:11 GMT, Mark Powers wrote: >> JDK-8284112 Minor cleanup could be done in javax.crypto > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > cleanup src/java.base/share/classes/javax/crypto/CryptoPolicyParser.java line 282: > 280: } > 281: > 282: private static AlgorithmParameterSpec getInstance(String type, Why are you removing final here? - PR: https://git.openjdk.java.net/jdk/pull/8214
Re: RFR: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. I checked over: java.base/macosx/classes/apple/security java.base/share/classes/com/sun/crypto java.base/share/classes/com/sun/security java.base/share/classes/java/security java.base/share/classes/javax/crypto java.base/share/classes/javax/net java.base/share/classes/sun/security The copyright dates need updating. src/java.base/share/classes/sun/security/provider/certpath/AdjacencyList.java line 128: > 126: // Each time this method is called, we're examining a new list > 127: // from the global list. So, we have to start by getting the list > 128: // that contains the set of Vertices we're considering. The class being affected is a Vertex, so either change to vertices, or leave as is... - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8186958: Need method to create pre-sized HashMap [v21]
On Thu, 14 Apr 2022 18:10:28 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > add `@LastModified: Apr 2022` to DocumentCache I learned something new about HashMap today... I looked at java.security.cert and sun.security.* and that part LGTM. That said, you need to check with @seanjmullan for the java.xml.crypto code. We try to keep the code in sync with the Apache code. As this is a new API, we probably can't push this kind of change to Apache as they need to support older releases. src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/DOMXPathFilter2Transform.java line 110: > 108: int length = attributes.getLength(); > 109: Map namespaceMap = > 110: HashMap.newHashMap(length); Please check these changes with @seanjmullan before integrating. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto
On Tue, 12 Apr 2022 21:59:09 GMT, Mark Powers wrote:
> JDK-8284112 Minor cleanup could be done in javax.crypto
src/java.base/share/classes/javax/crypto/CipherInputStream.java line 159:
> 157: try {
> 158: ofinish = cipher.update(ibuffer, 0, readin, obuffer, ostart);
> 159: } catch (IllegalStateException e) {
This is another one of those I would probably leave alone, just so it's clear
what should be done.
src/java.base/share/classes/javax/crypto/CipherSpi.java line 29:
> 27:
> 28: import java.nio.ByteBuffer;
> 29: import java.security.*;
This is another one that some people will complain about. I personally prefer
this style, others prefer everything written out as long as it's not "too many."
-
PR: https://git.openjdk.java.net/jdk/pull/8214
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto
On Tue, 12 Apr 2022 21:59:09 GMT, Mark Powers wrote: > JDK-8284112 Minor cleanup could be done in javax.crypto src/java.base/share/classes/javax/crypto/spec/DESKeySpec.java line 49: > 47: * Weak/semi-weak keys copied from FIPS 74. > 48: * > 49: * "...The first 6 keys have duals different from themselves, hence Suggest you leave this alone as this is a direct quote from FIPS 74 (page 10). - PR: https://git.openjdk.java.net/jdk/pull/8214
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto
On Tue, 12 Apr 2022 21:59:09 GMT, Mark Powers wrote:
> JDK-8284112 Minor cleanup could be done in javax.crypto
src/java.base/share/classes/javax/crypto/SealedObject.java line 449:
> 447: final class extObjectInputStream extends ObjectInputStream {
> 448: extObjectInputStream(InputStream in)
> 449: throws IOException {
This is another "I probably wouldn't do that..."
It's nice to know what kind of IOExceptions you might get, so leaving
StreamCorruptedException is ok.
-
PR: https://git.openjdk.java.net/jdk/pull/8214
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto
On Tue, 12 Apr 2022 21:59:09 GMT, Mark Powers wrote: > JDK-8284112 Minor cleanup could be done in javax.crypto src/java.base/share/classes/javax/crypto/ExemptionMechanism.java line 142: > 140: * @see java.security.Provider > 141: */ > 142: public static ExemptionMechanism getInstance(String algorithm) Others might disagree with this comment, but I would encourage you not to make these changes throughout your PR. (@jddarcy ?) Even though a static method is implicitly final and IJ is correct, the final keyword does prevent subclasses from inadvertently using the same method signature. - PR: https://git.openjdk.java.net/jdk/pull/8214
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto
On Tue, 12 Apr 2022 21:59:09 GMT, Mark Powers wrote: > JDK-8284112 Minor cleanup could be done in javax.crypto Can you file a bug to update the javax.crypto files to use proper javadoc for mentioned classes, e.g. < code> tags. src/java.base/share/classes/javax/crypto/CryptoPermissions.java line 158: > 156: > 157: /** > 158: * Checks if this object's PermissionCollection for permissions Just FYI and not for this review, but this class should really be updated to use proper javadoc comment style, which is to tag all objects with < code >PermissionCollection< /code > - PR: https://git.openjdk.java.net/jdk/pull/8214
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto
On Tue, 12 Apr 2022 21:59:09 GMT, Mark Powers wrote:
> JDK-8284112 Minor cleanup could be done in javax.crypto
src/java.base/share/classes/javax/crypto/CryptoPermission.java line 437:
> 435: // may be the best try.
> 436: return this.algParamSpec.equals(algParamSpec);
> 437: } else return !this.checkParam;
Please use the standard coding format.
} else {
return !this.checkParam;
}
or
}
return !this.checkParam;
-
PR: https://git.openjdk.java.net/jdk/pull/8214
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński wrote: > During TLS handshake, hundreds of constraints are evaluated to determine > which cipher suites are usable. Most of the evaluations are performed using > `HandshakeContext#algorithmConstraints` object. By default that object > contains a `SSLAlgorithmConstraints` instance wrapping another > `SSLAlgorithmConstraints` instance. As a result the constraints defined in > `SSLAlgorithmConstraints` are evaluated twice. > > This PR improves the default case; if the user-specified constraints are left > at defaults, we use a single `SSLAlgorithmConstraints` instance, and avoid > duplicate checks. Please make sure @XueleiFan has a chance to look at this before integrating. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 Resettting the clock. Sorry for the delay. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Sat, 9 Apr 2022 14:59:13 GMT, Xue-Lei Andrew Fan wrote: > The existing behavior is trying to close the socket and send the > close_notify. As the socket closure has been done in the SocketImpl, I think > BaseSSLSocketImpl could rely on to the SocketImpl cleaner. So the left for > the cleaning is about the close_notify. @AlanBateman, just to clarify, send the close_notify first, then close the socket. >From the Socket's point of view, the encrypted close_notify message is just a >few bytes of application data sent over the Socket's OutputStream. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Fri, 8 Apr 2022 06:52:57 GMT, Xue-Lei Andrew Fan wrote: > The Socket implementation will take care of the file description/native > memory release, I think. I've walked through the network code and understand it now. The Socket creation code calls into sun.nio.ch.NioSocketImpl.create():451, which then allocates the FileDescriptor fd and assigns it to the Socket, then registers a closer for that FileDescriptor which will be triggered by the Socket GC. When the Socket is reclaimed, the FileDescriptor is released, but not by referencing the Socket itself. > It is expected to send the close_notify at the TLS layer. But the current > code using finalizer, which is not reliable. The underlying socket may have > been closed when the SSLSocket finalizing action is triggered. Generally, > application should call close() method explicitly, otherwise the finalizer is > not expect to work reliable. I was wondering it may be safe to remove the > finalizer. Yeah, I'm just not sure yet. > I agree that adding a best effort cleanup may be better. I was wondering to > check if it is possible to clean the socket in the socket creation factories > so that the object reference issues could be resolved. But as socket is a > kind of resource, application layer may make the clean up as well. > I'm still looking for a solution to clean up resource by using a method of > 'this'. Please advice if anyone has experiences. @AlanBateman, @dfuch, any great ideas here? - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan wrote:
>> Please review the update to remove finalizer method in the SunJSSE provider
>> implementation. It is one of the efforts to clean up the use of finalizer
>> method in JDK.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - typo blank linke correction
> - revise the update
Thanks for the explanation: this is my first exposure to the
`java.lang.ref.Cleaner` API, so am getting up to speed. Sorry if these are
dumb comments/questions.
I see now what was being talked about in your other PR:
https://github.com/openjdk/jdk/pull/8136 and to not use a reference to `this`
which would keep it from being GC'd. I also see how you're keeping a cleaner
object that has outside ("static") references to the actual things that need to
be released, but don't we need to do the similar cleaning for the underlying
Socket somehow? What do Sockets do to make sure the underlying file
descriptors/native memory are properly released?
That said, we still need to send the close_notify at the TLS layer, right?
Simply removing the finalize() method is just going to silently shutdown the
connection, and then the Socket is going to do whatever it does for
finalization/Cleaning.
-
PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method
On Thu, 7 Apr 2022 20:11:25 GMT, Xue-Lei Andrew Fan wrote: > The socket close() call in the finalize() method may be blocked for the SSL > implementation, which is not good for garbage collection. It should be safe > by just removing the finalize() method. Can you provide more detail? I expected something more like your first attempt (`java.lang.ref.Cleaner`) that would properly close/send the close_notify message. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8284415: Collapse identical catch branches in security libs
On Fri, 1 Apr 2022 07:32:21 GMT, Andrey Turbanov wrote:
> Let's take advantage of Java 7 language feature - "Catching Multiple
> Exception Types".
> It simplifies code. Reduces duplication.
> Found by IntelliJ IDEA inspection `Identical 'catch' branches in 'try'
> statement`
Other that the 1 copyright issue, LGTM.
I didn't notice any expect behavioral changes between the new and old code.
src/jdk.security.auth/share/classes/com/sun/security/auth/module/Krb5LoginModule.java
line 788:
> 786:
> 787: }
> 788: } catch (KrbException | IOException e) {
Please update copyright date.
-
Marked as reviewed by wetmore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8068
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v3]
> JDK-8253368 changed the behavior of SSLSocket to no longer throw a fatal > internal_error (80) and invalidate existing sessions (either completed or > under construction) as described in (RFC 4346/TLSv1.1+) if a connection was > closed without receiving a close_notify alert from the peer. > > This change introduces similar behavior to SSLEngine. > > The unit test checks that closing the read(input) sides of the > SSLSocket/SSLEngine throws an SSLException, but doesn't invalidate their > respective sessions. > > Tier1/2 mach5 tests have been successfully run. Bradford Wetmore 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 13 additional commits since the last revision: - Merge branch 'master' into JDK-8273553 - Code review comment: enclose conContext.closeInbound() in a try/finally block. - Merge branch 'master' into JDK-8273553 - Merge branch 'master' into JDK-8273553 - Added SSLSocket bugid since we're actually checking both sides now. - I/O Issues, rewrite the I/O section so that early Socket closes don't kill our server-side reads. - Merge branch 'master' into JDK-8273553 - Merge branch 'master' into JDK-8273553 - Merge - Minor test tweaks. - ... and 3 more: https://git.openjdk.java.net/jdk/compare/38460839...08d22aee - Changes: - all: https://git.openjdk.java.net/jdk/pull/7796/files - new: https://git.openjdk.java.net/jdk/pull/7796/files/b2f64d92..08d22aee Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7796=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7796=01-02 Stats: 65793 lines in 503 files changed: 62965 ins; 887 del; 1941 mod Patch: https://git.openjdk.java.net/jdk/pull/7796.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7796/head:pull/7796 PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8283711: Remove redundant 'new String' calls after concatenation
On Sun, 20 Mar 2022 12:07:52 GMT, Andrey Turbanov wrote: > Result of string concatenation is a newly created String object. There is no > need it wrap it in another 'new String' call. > Such calls are confusing and produce warnings in IDE. Without them code is > easier to read. > Similar cleanup > [JDK-8273375](https://bugs.openjdk.java.net/browse/JDK-8273375) in > java.desktop LGTM. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7876
Re: RFR: 8283691: Classes in java.security still reference deprecated classes in spec
On Fri, 25 Mar 2022 15:34:23 GMT, Weijun Wang wrote: > Some spec cleanup. LGTM. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7961
Re: [External] : Re: SSLEngine.unwrap on read-only input ByteBuffer
Problem easily duplicated, thanks for the reproducer. I've updated the bug with the info. Brad On 3/24/2022 9:13 AM, Chris Vest wrote: On Wed, Mar 23, 2022 at 10:38 AM Bradford Wetmore mailto:bradford.wetm...@oracle.com>> wrote: Offhand, sounds like a bug to me. I've filed: https://bugs.openjdk.java.net/browse/JDK-8283577 <https://bugs.openjdk.java.net/browse/JDK-8283577> Thanks. The in-place use of the input buffer might also be unexpected even when the buffer is not read-only. By chance, do you have a simple reproducer handy? See https://github.com/netty/netty/pull/12213#issuecomment-1077796917 <https://urldefense.com/v3/__https://github.com/netty/netty/pull/12213*issuecomment-1077796917__;Iw!!ACWV5N9M2RV99hQ!fZx3LxRdafSPcHg6-4XPFumXYR6gTlOaQfC14ixjjjwlZK7IbHD4voW9gxXeHFbPRTToQg$> Brad On 3/23/2022 9:54 AM, Chris Vest wrote: > Hi, > > In Netty we've been trying to design some safer APIs, and attempted to > make more use of read-only ByteBuffers. > > We discovered that SSLEngine.unwrap does not like read-only input > buffers, even though the input buffers should in theory only be read > from. We obviously make sure that the output buffers are writable. > > By my reading of the javadoc, and the code, I believe this was intended > to work - or at least not intended to not work - but probably wasn't > tested directly. > > When we try we get this stack trace on adopt-openjdk-11.0.7: > > javax.net.ssl.SSLProtocolException: null > at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:129) > at > java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:326) > at > java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:269) > at > java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:264) > at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:118) > at > java.base/sun.security.ssl.SSLEngineImpl.decode(SSLEngineImpl.java:668) > at > java.base/sun.security.ssl.SSLEngineImpl.readRecord(SSLEngineImpl.java:623) > at > java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:441) > at > java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:420) > at java.base/javax.net.ssl.SSLEngine.unwrap(SSLEngine.java:674) > at io.netty5.handler.ssl.EngineWrapper.unwrap(EngineWrapper.java:100) > at io.netty5.handler.ssl.SslHandler.unwrap(SslHandler.java:1227) > at > io.netty5.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1105) > at io.netty5.handler.ssl.SslHandler.decode(SslHandler.java:1165) > at > io.netty5.handler.codec.ByteToMessageDecoderForBuffer.decodeRemovalReentryProtection(ByteToMessageDecoderForBuffer.java:384) > at > io.netty5.handler.codec.ByteToMessageDecoderForBuffer.callDecode(ByteToMessageDecoderForBuffer.java:327) > ... 20 common frames omitted > Caused by: java.nio.ReadOnlyBufferException: null > at java.base/javax.crypto.Cipher.doFinal(Cipher.java:2493) > at > java.base/sun.security.ssl.SSLCipher$T12GcmReadCipherGenerator$GcmReadCipher.decrypt(SSLCipher.java:1629) > at > java.base/sun.security.ssl.SSLEngineInputRecord.decodeInputRecord(SSLEngineInputRecord.java:240) > at > java.base/sun.security.ssl.SSLEngineInputRecord.decode(SSLEngineInputRecord.java:197) > at > java.base/sun.security.ssl.SSLEngineInputRecord.decode(SSLEngineInputRecord.java:160) > at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:108) > ... 31 common frames omitted > > > I also tried this on a panama-preview snapshot JDK I have, and got a > similar stack trace: > > % java -version > openjdk version "19-internal" 2022-09-20 > OpenJDK Runtime Environment (fastdebug build > 19-internal-adhoc.chris.panama-foreign) > OpenJDK 64-Bit Server VM (fastdebug build > 19-internal-adhoc.chris.panama-foreign, mixed mode) > > > % git show > commit 144af9f43cd2d6f88b675b8c85e4034e5b9d6695 (HEAD -> > foreign-preview, origin/foreign-preview) > > > javax.net.ssl.SSLProtocolException: null > at java.base/sun.security.ss
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]
On Thu, 24 Mar 2022 06:45:46 GMT, Xue-Lei Andrew Fan wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java line 802:
>>
>>> 800: } finally {
>>> 801: engineLock.unlock();
>>> 802: }
>>
>> I looked the update further. It would be nice if the try-statement could
>> support more than one finally blocks in the future so that the code could be
>> more clear.
>>
>> try {
>> ...
>> } finally {
>>// the 1st final block
>> } finally {
>>// the 2nd final block
>> }
>>
>>
>> For the block from 781 to 787, it may be more effective if moving the block
>> out of the synchronization lock (that's, moving 781-787 to line 779.)
>
>> @XueleiFan, I'm not following your first comment.
>
> Sorry for that. I was wondering something new in the future, which is not
> doable in the current Java language. Please just leave it alone.
Ah! Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]
On Wed, 23 Mar 2022 18:09:46 GMT, Xue-Lei Andrew Fan wrote:
>> Bradford Wetmore 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 12 additional
>> commits since the last revision:
>>
>> - Code review comment: enclose conContext.closeInbound() in a try/finally
>> block.
>> - Merge branch 'master' into JDK-8273553
>> - Merge branch 'master' into JDK-8273553
>> - Added SSLSocket bugid since we're actually checking both sides now.
>> - I/O Issues, rewrite the I/O section so that early Socket closes don't
>> kill our server-side reads.
>> - Merge branch 'master' into JDK-8273553
>> - Merge branch 'master' into JDK-8273553
>> - Merge
>> - Minor test tweaks.
>> - Remove inadvertent whitespace
>> - ... and 2 more:
>> https://git.openjdk.java.net/jdk/compare/fdf65be0...b2f64d92
>
> src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java line 802:
>
>> 800: } finally {
>> 801: engineLock.unlock();
>> 802: }
>
> I looked the update further. It would be nice if the try-statement could
> support more than one finally blocks in the future so that the code could be
> more clear.
>
> try {
> ...
> } finally {
>// the 1st final block
> } finally {
>// the 2nd final block
> }
>
>
> For the block from 781 to 787, it may be more effective if moving the block
> out of the synchronization lock (that's, moving 781-787 to line 779.)
I'm not following your first comment. AFAIK, double finally blocks aren't
currently an option, unless I'm missing a new language feature. Or is this
just a general "it would be nice if the Java language was able to do..."
comment?
try {
...
throw new SSLException(...);
} finally {
conContext.closeInbound();
} finally {
engineLock.unlock();
}
As to your second question, the model of:
method() {
engineLock.lock();
try {
action();
} finally {
engineUnlock.unlock();
}
}
is very simple to understand and thus maintain, and is used quite consistently
throughout SSLEngineImpl and catches any problems. IIUC, you are suggesting:
public void closeInbound() throws SSLException {
if (isInboundDone()) {
return;
}
if (SSLLogger.isOn && SSLLogger.isOn("ssl")) {
SSLLogger.finest("Closing inbound of SSLEngine");
}
engineLock.lock();
try {
if (!conContext.isInputCloseNotified && (conContext.isNegotiated
|| conContext.handshakeContext != null)) {
throw new SSLException(
"closing inbound before receiving peer's close_notify");
}
} finally {
try {
conContext.closeInbound();
} finally {
engineLock.unlock();
}
}
}
What is the advantage to changing to this style, other than a very small
performance win by not locking in the already closed case? Isn't the locking
code pretty optimized? SSLLogger might throw a NPE (unlikely), and
isInboundDone() just checks a variable, but the code could change down the
road. I'm just not seeing a reason to change the maintainability of this code.
-
PR: https://git.openjdk.java.net/jdk/pull/7796
Re: SSLEngine.unwrap on read-only input ByteBuffer
Offhand, sounds like a bug to me. I've filed: https://bugs.openjdk.java.net/browse/JDK-8283577 to track. It's possible an optimization might have been done using an in-place en/decryption, but it should work. By chance, do you have a simple reproducer handy? If not, we should be able to come up with one if it is what I think it is. Brad On 3/23/2022 9:54 AM, Chris Vest wrote: Hi, In Netty we've been trying to design some safer APIs, and attempted to make more use of read-only ByteBuffers. We discovered that SSLEngine.unwrap does not like read-only input buffers, even though the input buffers should in theory only be read from. We obviously make sure that the output buffers are writable. By my reading of the javadoc, and the code, I believe this was intended to work - or at least not intended to not work - but probably wasn't tested directly. When we try we get this stack trace on adopt-openjdk-11.0.7: javax.net.ssl.SSLProtocolException: null at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:129) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:326) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:269) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:264) at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:118) at java.base/sun.security.ssl.SSLEngineImpl.decode(SSLEngineImpl.java:668) at java.base/sun.security.ssl.SSLEngineImpl.readRecord(SSLEngineImpl.java:623) at java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:441) at java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:420) at java.base/javax.net.ssl.SSLEngine.unwrap(SSLEngine.java:674) at io.netty5.handler.ssl.EngineWrapper.unwrap(EngineWrapper.java:100) at io.netty5.handler.ssl.SslHandler.unwrap(SslHandler.java:1227) at io.netty5.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1105) at io.netty5.handler.ssl.SslHandler.decode(SslHandler.java:1165) at io.netty5.handler.codec.ByteToMessageDecoderForBuffer.decodeRemovalReentryProtection(ByteToMessageDecoderForBuffer.java:384) at io.netty5.handler.codec.ByteToMessageDecoderForBuffer.callDecode(ByteToMessageDecoderForBuffer.java:327) ... 20 common frames omitted Caused by: java.nio.ReadOnlyBufferException: null at java.base/javax.crypto.Cipher.doFinal(Cipher.java:2493) at java.base/sun.security.ssl.SSLCipher$T12GcmReadCipherGenerator$GcmReadCipher.decrypt(SSLCipher.java:1629) at java.base/sun.security.ssl.SSLEngineInputRecord.decodeInputRecord(SSLEngineInputRecord.java:240) at java.base/sun.security.ssl.SSLEngineInputRecord.decode(SSLEngineInputRecord.java:197) at java.base/sun.security.ssl.SSLEngineInputRecord.decode(SSLEngineInputRecord.java:160) at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:108) ... 31 common frames omitted I also tried this on a panama-preview snapshot JDK I have, and got a similar stack trace: % java -version openjdk version "19-internal" 2022-09-20 OpenJDK Runtime Environment (fastdebug build 19-internal-adhoc.chris.panama-foreign) OpenJDK 64-Bit Server VM (fastdebug build 19-internal-adhoc.chris.panama-foreign, mixed mode) % git show commit 144af9f43cd2d6f88b675b8c85e4034e5b9d6695 (HEAD -> foreign-preview, origin/foreign-preview) javax.net.ssl.SSLProtocolException: null at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:129) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:371) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:314) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:309) at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:121) at java.base/sun.security.ssl.SSLEngineImpl.decode(SSLEngineImpl.java:736) at java.base/sun.security.ssl.SSLEngineImpl.readRecord(SSLEngineImpl.java:691) at java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:506) at java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:482) at java.base/javax.net.ssl.SSLEngine.unwrap(SSLEngine.java:719) at io.netty5.handler.ssl.EngineWrapper.unwrap(EngineWrapper.java:100) at io.netty5.handler.ssl.SslHandler.unwrap(SslHandler.java:1227) at io.netty5.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1105) at io.netty5.handler.ssl.SslHandler.decode(SslHandler.java:1165) at io.netty5.handler.codec.ByteToMessageDecoderForBuffer.decodeRemovalReentryProtection(ByteToMessageDecoderForBuffer.java:384) at io.netty5.handler.codec.ByteToMessageDecoderForBuffer.callDecode(ByteToMessageDecoderForBuffer.java:327) ... 20 common frames omitted Caused by:
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 [v2]
> JDK-8253368 changed the behavior of SSLSocket to no longer throw a fatal > internal_error (80) and invalidate existing sessions (either completed or > under construction) as described in (RFC 4346/TLSv1.1+) if a connection was > closed without receiving a close_notify alert from the peer. > > This change introduces similar behavior to SSLEngine. > > The unit test checks that closing the read(input) sides of the > SSLSocket/SSLEngine throws an SSLException, but doesn't invalidate their > respective sessions. > > Tier1/2 mach5 tests have been successfully run. Bradford Wetmore 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 12 additional commits since the last revision: - Code review comment: enclose conContext.closeInbound() in a try/finally block. - Merge branch 'master' into JDK-8273553 - Merge branch 'master' into JDK-8273553 - Added SSLSocket bugid since we're actually checking both sides now. - I/O Issues, rewrite the I/O section so that early Socket closes don't kill our server-side reads. - Merge branch 'master' into JDK-8273553 - Merge branch 'master' into JDK-8273553 - Merge - Minor test tweaks. - Remove inadvertent whitespace - ... and 2 more: https://git.openjdk.java.net/jdk/compare/84577bde...b2f64d92 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7796/files - new: https://git.openjdk.java.net/jdk/pull/7796/files/43953018..b2f64d92 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7796=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7796=00-01 Stats: 4167 lines in 793 files changed: 2485 ins; 1099 del; 583 mod Patch: https://git.openjdk.java.net/jdk/pull/7796.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7796/head:pull/7796 PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368
On Wed, 23 Mar 2022 15:10:21 GMT, Xue-Lei Andrew Fan wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLEngineImpl.java line 799:
>>
>>> 797: } finally {
>>> 798: conContext.closeInbound();
>>> 799: engineLock.unlock();
>>
>> I see that `onContext.closeInbound()` might throw, which would leave the
>> `engineLock` locked and could cause deadlocks down the road. So maybe you
>> should have a nested `try { } finally { }` here to make sure the lock is
>> properly unlocked.
>
> +1.
Good catch. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368
On Tue, 22 Mar 2022 16:32:22 GMT, Rajan Halade wrote: >> In previous days, we used to include the dates from the templates, which >> this test was derived from. I could go either way. > > I am not knowledgable one in this area and thought was merely a typo. Fine > either way. I'll leave for now. - PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368
On Tue, 22 Mar 2022 12:28:44 GMT, Sean Coffey wrote: >> Sigh...this is a whole can of worms I wasn't expecting. Looks like one >> person did the SSLContextTemplate and updated with SSLEngineTemplate, then >> another person took a completely different takes with SSLSocketTemplate, and >> then SSLSocketSSLEngineTemplate didn't get touched at all. >> >> This should really be harmonized. :( > > ouch - maybe this should be fixed up in a separate bug then. Don't think it > should be a blocker for this fix Assessing how much work it will be. - PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368
On Mon, 21 Mar 2022 15:05:02 GMT, Sean Coffey wrote: >> JDK-8253368 changed the behavior of SSLSocket to no longer throw a fatal >> internal_error (80) and invalidate existing sessions (either completed or >> under construction) as described in (RFC 4346/TLSv1.1+) if a connection was >> closed without receiving a close_notify alert from the peer. >> >> This change introduces similar behavior to SSLEngine. >> >> The unit test checks that closing the read(input) sides of the >> SSLSocket/SSLEngine throws an SSLException, but doesn't invalidate their >> respective sessions. >> >> Tier1/2 mach5 tests have been successfully run. > > test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java > line 130: > >> 128: * The following is to set up the keystores/trust material. >> 129: */ >> 130: private static final String pathToStores = >> "../../../../javax/net/ssl/etc"; > > I think the advise is to move away from binary style keystores. i.e. to use > test/jdk/javax/net/ssl/templates/SSLContextTemplate.java for a replacement. > Is that possible here ? Sigh...this is a whole can of worms I wasn't expecting. Looks like one person did the SSLContextTemplate and updated with SSLEngineTemplate, then another person took a completely different takes with SSLSocketTemplate, and then SSLSocketSSLEngineTemplate didn't get touched at all. This should really be harmonized. :( - PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368
On Mon, 21 Mar 2022 17:00:53 GMT, Rajan Halade wrote: >> JDK-8253368 changed the behavior of SSLSocket to no longer throw a fatal >> internal_error (80) and invalidate existing sessions (either completed or >> under construction) as described in (RFC 4346/TLSv1.1+) if a connection was >> closed without receiving a close_notify alert from the peer. >> >> This change introduces similar behavior to SSLEngine. >> >> The unit test checks that closing the read(input) sides of the >> SSLSocket/SSLEngine throws an SSLException, but doesn't invalidate their >> respective sessions. >> >> Tier1/2 mach5 tests have been successfully run. > > test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java > line 2: > >> 1: /* >> 2: * Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights >> reserved. > > should this be updated to only list 2022? In previous days, we used to include the dates from the templates, which this test was derived from. I could go either way. - PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8283426: Fix 'exeption' typo
On Sun, 20 Mar 2022 13:30:01 GMT, Andrey Turbanov wrote: > Fix repeated typo `exeption` Good grief! I wouldn't have expected it to be so widespread. Thanks for noticing and fixing. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7879
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Fri, 18 Mar 2022 23:05:36 GMT, Iris Clark wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> more test case udpate > > Marked as reviewed by iris (Reviewer). Thanks, @irisclark! - PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Tue, 8 Mar 2022 08:02:16 GMT, Xue-Lei Andrew Fan wrote:
>> Please review this small API enhancement to add the usual constructors
>> taking a cause to javax.net.ssl exceptions. The use of initCause in the
>> JSSE implementation code is updated to use the new constructors accordingly.
>>
>> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> more test case udpate
Please see latest comments and let's verify the copyright is correct, but
otherwise looks good.
test/jdk/javax/net/ssl/SSLException/CheckSSLHandshakeException.java line 32:
> 30: import java.util.Objects;
> 31:
> 32: public class CheckSSLHandshakeException {
My personal preference would have been to combine all of these into a single
testfile to minimize clutter in the logs and directories, but no strong
objection.
test/jdk/javax/net/ssl/templates/SSLSocketSSLEngineTemplate.java line 349:
> 347: }
> 348: }
> 349: } finally {
Copyright update
test/jdk/javax/net/ssl/templates/SSLSocketTemplate.java line 544:
> 542: /*
> 543: * Check various exception conditions.
> 544: */
Copyright update
-
Marked as reviewed by wetmore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]
On Tue, 8 Mar 2022 08:02:16 GMT, Xue-Lei Andrew Fan wrote:
>> Please review this small API enhancement to add the usual constructors
>> taking a cause to javax.net.ssl exceptions. The use of initCause in the
>> JSSE implementation code is updated to use the new constructors accordingly.
>>
>> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> more test case udpate
test/jdk/javax/net/ssl/ALPN/SSLServerSocketAlpnTest.java line 487:
> 485: if ((local != null) && (remote != null)) {
> 486: // If both failed, return the curthread's exception.
> 487: local.addSuppressed(remote);
Copyright 2016->2022.
test/jdk/javax/net/ssl/ALPN/SSLSocketAlpnTest.java line 483:
> 481: if ((local != null) && (remote != null)) {
> 482: // If both failed, return the curthread's exception.
> 483: local.addSuppressed(remote);
Copyright 2016->2022.
test/jdk/javax/net/ssl/SSLException/CheckSSLHandshakeException.java line 2:
> 1: /*
> 2: * Copyright (C) 2022 THL A29 Limited, a Tencent company. All rights
> reserved.
I am unsure about the copyrights on these files. They are probably ok, but you
should get approval by someone more familiar.
-
PR: https://git.openjdk.java.net/jdk/pull/7722
RFR: 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368
JDK-8253368 changed the behavior of SSLSocket to no longer throw a fatal internal_error (80) and invalidate existing sessions (either completed or under construction) as described in (RFC 4346/TLSv1.1+) if a connection was closed without receiving a close_notify alert from the peer. This change introduces similar behavior to SSLEngine. The unit test checks that closing the read(input) sides of the SSLSocket/SSLEngine throws an SSLException, but doesn't invalidate their respective sessions. Tier1/2 mach5 tests have been successfully run. - Commit messages: - Merge branch 'master' into JDK-8273553 - Added SSLSocket bugid since we're actually checking both sides now. - I/O Issues, rewrite the I/O section so that early Socket closes don't kill our server-side reads. - Merge branch 'master' into JDK-8273553 - Merge branch 'master' into JDK-8273553 - Merge - Minor test tweaks. - Remove inadvertent whitespace - Forgot copyright Info - 8273553: sun.security.ssl.SSLEngineImpl.closeInbound also has similar error of JDK-8253368 Changes: https://git.openjdk.java.net/jdk/pull/7796/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7796=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273553 Stats: 506 lines in 3 files changed: 493 ins; 4 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/7796.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7796/head:pull/7796 PR: https://git.openjdk.java.net/jdk/pull/7796
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 Thanks, been working on another close issue. I should be finishing that shortly, - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v2]
On Tue, 8 Mar 2022 15:23:13 GMT, zzambers wrote: >>> Sure if more changes are desired I can pull your changes. When It comes to >>> CSR I am not fully familiar with the >> process. Is action expected from my side? >> >> One of us needs to get the CSR approved. Why don't you pull the changes >> described in: >> >> https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029209.html >> >> Assuming you are ok with the updated wording, I can create the CSR and >> submit. Once that it approved, then we can get this PR approved and you can >> integrate. >> >> Are you at contributor status? > > @bradfordwetmore Your changes look good to me. When it comes to wording, I'll > let that to native english speaker(s) to judge :) (As I am not native english > speaker myself). I built docs locally and result looks good (also links > work), there was just one problem with brace, but I have fixed that. @zzambers, the CSR was approved yesterday, and is just waiting your integration. I will sponsor it. - PR: https://git.openjdk.java.net/jdk/pull/7648
Re: RFR: 8282600: SSLSocketImpl should not use user_canceled workaround when not necessary
On Wed, 2 Mar 2022 19:04:26 GMT, zzambers wrote: > When testing compatibility of jdk TLS implementation with gnutls, I have > found a problem. The problem is, that gnutls does not like use of > user_canceled alert when closing TLS-1.3 connection from duplexCloseOutput() > (used by socket.close() unless shutdownOutput was called explicitly) and > considers it error. (For more details see: [1]) > > As I understand it, usage of user_canceled alert before close is workaround > for an issue of not being able to cleanly initialize full (duplex) close of > TLS-1.3 connection (other side is not required to immediately close the after > receiving close_notify, unlike in earlier TLS versions). Some legacy programs > could probably hang or something, expecting socket.close to perform immediate > duplex close. Problem is this is not what user_canceled alert is intended for > [2] and it is therefore undefined how the other side handles this. (JDK > itself replies to close_notify preceded by user_canceled alert by immediately > closing its output [3].) > > This fix disables this workaround when it is not necessary (connection is > already half-closed by the other side). This way it fixes my case (gnutls > client connected to jdk server initiates close) and it should be safe. (As > removing workaround altogether could probably reintroduce issues for legacy > apps... ) > > I also ran jdk_security tests locally, which passed for me. > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1918473 > [2] https://datatracker.ietf.org/doc/html/rfc8446#section-6.1 > [3] > https://github.com/openjdk/jdk/blob/b6c35ae44aeb31deb7a15ee2939156ed00b3df52/src/java.base/share/classes/sun/security/ssl/Alert.java#L243 I am looking at a few things in this PR. Please ping me before integrating. I believe this code is correct, but checking on the overall USER_CANCELED strategy. - PR: https://git.openjdk.java.net/jdk/pull/7664
Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v4]
On Tue, 8 Mar 2022 15:03:57 GMT, zzambers wrote: >> Fixed API Note in javadoc for javax.net.ssl.SSLSocket class. API Note was >> introduced by JDK-8208526 [1]. At that point both Socket.shutdownInput() / >> Socket.shutdownOutput() and InputStream.close() / OutputStream.close() >> performed half-close of TLS-1.3 connection. However this behaviour has >> changed as result of JDK-8216326 [2]. InputStream.close() / >> OutputStream.close() no longer perform half-close but full socket close, but >> API Note was never updated. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8208526 >> [2] https://bugs.openjdk.java.net/browse/JDK-8216326 > > zzambers has updated the pull request incrementally with one additional > commit since the last revision: > > small fixes LGTM. I can sponsor after CSR approvals granted. Please hold off on integrating until then. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7648
Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v2]
On Tue, 8 Mar 2022 15:23:13 GMT, zzambers wrote: >>> Sure if more changes are desired I can pull your changes. When It comes to >>> CSR I am not fully familiar with the >> process. Is action expected from my side? >> >> One of us needs to get the CSR approved. Why don't you pull the changes >> described in: >> >> https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029209.html >> >> Assuming you are ok with the updated wording, I can create the CSR and >> submit. Once that it approved, then we can get this PR approved and you can >> integrate. >> >> Are you at contributor status? > > @bradfordwetmore Your changes look good to me. When it comes to wording, I'll > let that to native english speaker(s) to judge :) (As I am not native english > speaker myself). I built docs locally and result looks good (also links > work), there was just one problem with brace, but I have fixed that. @zzambers Thanks for catching the two little issues with the closing brackets/missing space. CSR has been finalized, just need to wait for approval. - PR: https://git.openjdk.java.net/jdk/pull/7648
Need a reviewer for CSR: JDK-8282768
Hi, We (zzambers/I) need a reviewer for this CSR involving the close @apiNote of SSLSocket.java: https://bugs.openjdk.java.net/browse/JDK-8282768 Fix API Note in javadoc for javax.net.ssl.SSLSocket The original bug JDK-8282529[1] is/has been discussed in these threads [2][3], and has this pull request[4]. Brad [1] https://bugs.openjdk.java.net/browse/JDK-8282529 [2] https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029132.html [3] https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029163.html [4] https://github.com/openjdk/jdk/pull/7648
Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v2]
On Mon, 7 Mar 2022 16:34:08 GMT, zzambers wrote: > Sure if more changes are desired I can pull your changes. When It comes to > CSR I am not fully familiar with the process. Is action expected from my side? One of us needs to get the CSR approved. Why don't you pull the changes described in: https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029209.html Assuming you are ok with the updated wording, I can create the CSR and submit. Once that it approved, then we can get this PR approved and you can integrate. Are you at contributor status? - PR: https://git.openjdk.java.net/jdk/pull/7648
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 20:39:44 GMT, Xue-Lei Andrew Fan wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLTrafficKeyDerivation.java
>> line 158:
>>
>>> 156: } catch (GeneralSecurityException gse) {
>>> 157: throw new SSLHandshakeException(
>>> 158: "Could not generate secret", gse);
>>
>> I can't quite tell given the coloring, but did this get changed into a tab?
>
> Yes, I added 4 more whitespaces in line 158.
Ah, missed that. Good to be consistent.
-
PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 19:42:47 GMT, Xue-Lei Andrew Fan wrote:
>> Please review this small API enhancement to add the usual constructors
>> taking a cause to javax.net.ssl exceptions. The use of initCause in the
>> JSSE implementation code is updated to use the new constructors accordingly.
>>
>> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> typo correction
Didn't see any unit tests for the new methods. Can approve after they are
included.
src/java.base/share/classes/sun/security/ssl/ECDHKeyExchange.java line 204:
> 202: } catch (GeneralSecurityException | java.io.IOException e) {
> 203: throw new SSLHandshakeException(
> 204: "Could not generate ECPublicKey", e);
Nit: I think combining these lines would be < 80 chars
src/java.base/share/classes/sun/security/ssl/SSLSocketInputRecord.java line 263:
> 261: fragment = plaintext.fragment;
> 262: contentType = plaintext.contentType;
> 263: } catch (BadPaddingException bpe) {
Does the copyright need to get updated?
-
PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v2]
On Mon, 7 Mar 2022 19:42:47 GMT, Xue-Lei Andrew Fan wrote:
>> Please review this small API enhancement to add the usual constructors
>> taking a cause to javax.net.ssl exceptions. The use of initCause in the
>> JSSE implementation code is updated to use the new constructors accordingly.
>>
>> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> typo correction
src/java.base/share/classes/sun/security/ssl/SSLTrafficKeyDerivation.java line
158:
> 156: } catch (GeneralSecurityException gse) {
> 157: throw new SSLHandshakeException(
> 158: "Could not generate secret", gse);
I can't quite tell given the coloring, but did this get changed into a tab?
-
PR: https://git.openjdk.java.net/jdk/pull/7722
Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions
On Mon, 7 Mar 2022 07:52:29 GMT, Xue-Lei Andrew Fan wrote:
> Please review this small API enhancement to add the usual constructors taking
> a cause to javax.net.ssl exceptions. The use of initCause in the JSSE
> implementation code is updated to use the new constructors accordingly.
>
> Please review the CSR: https://bugs.openjdk.java.net/browse/JDK-8282724
src/java.base/share/classes/javax/net/ssl/SSLException.java line 52:
> 50: */
> 51: public SSLException(String reason)
> 52: {
Thanks for changing the style. I've always hated the extra vertical space. :)
-
PR: https://git.openjdk.java.net/jdk/pull/7722
Re: [External] : Re: [Internet]Recent SSLSocket close() @apiNote Changes.
On 3/2/2022 11:46 PM, xueleifan(XueleiFan) wrote:
I think you are right that this design is actually for TLSv1.3 half-close mode.
For TLS 1.3, there is no duplex closure design. The close() implementation
in JDK is actually a workaround for compatibility. Application can use either
the half-close mode
socket.shutdownOutput();
socket.shutdownInput();
or duplex close mode for compatibilty:
socket.close();
Unfortunately, in practice the half-close and duplex close are mixed with three
lines:
socket.shutdownOutput();
socket.shutdownInput();
socket.close();
Yeah, I've seen that in things like Apache HttpClient Core, which has
subsequently been removed.
How about something like this:
http://cr.openjdk.java.net/~wetmore/8282529/webrev.00/
Thanks,
Brad
It was not something I expected when I designed the spec for half-close mode.
It should be helpful if having another iteration for the @apiNote.
Thanks,
Xuelei
On Mar 2, 2022, at 5:14 PM, Bradford Wetmore
wrote:
Hi Xuelei,
I am working on some close code including the recent PR[1] for:
8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket
and ran into a change I hadn't noticed before.
* @apiNote
* When the connection is no longer needed, the client and server
* applications should each close both sides of their respective connection.
* For {@code SSLSocket} objects, for example, an application can call
* {@link Socket#shutdownOutput()} for output stream close and call
* {@link Socket#shutdownInput()} for input stream close.
It used to be that just a single SSLSocket.close() was sufficient to completely
shutdown the SSLSocket, and under the hood it closed the output/input in the
right order.
I believe this code still closes everything as before, but the updated @apiNote
encourages the user to do a three-part shutdown instead.
socket.shutdownOutput();
socket.shutdownInput();
socket.close();// mostly repeats of above.
This approach seems unnecessary unless the user is interested in the TLSv1.3
half-close mode.
What is the rationale for recommending this way of doing closes in general? Or
does this @apiNote need another iteration?
Thanks,
Brad
[1]
https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/7648__;!!ACWV5N9M2RV99hQ!cHw7i1wGs-eyCPUcrFXtAdFiUZL6aCPUGpGEQ9u56HHSuwew1j6YHapR8sSefEwr7TRXKQ$
Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v2]
On Thu, 3 Mar 2022 12:36:33 GMT, zzambers wrote: >> Fixed API Note in javadoc for javax.net.ssl.SSLSocket class. API Note was >> introduced by JDK-8208526 [1]. At that point both Socket.shutdownInput() / >> Socket.shutdownOutput() and InputStream.close() / OutputStream.close() >> performed half-close of TLS-1.3 connection. However this behaviour has >> changed as result of JDK-8216326 [2]. InputStream.close() / >> OutputStream.close() no longer perform half-close but full socket close, but >> API Note was never updated. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8208526 >> [2] https://bugs.openjdk.java.net/browse/JDK-8216326 > > zzambers has updated the pull request incrementally with one additional > commit since the last revision: > > Updated copyright for SSLSocket.java There are more changes needed, and should probably be done as part of this issue since we're already in this code anyway. The current code only talks about shutdownInput/shutdownOutput, but makes no mention of the duplex close(), which is the other way to shutdown the SSLSocket. It only needs a few words. https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029167.html These two changes will require a CSR to update the @apiNote. - PR: https://git.openjdk.java.net/jdk/pull/7648
Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines
On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan wrote: > Hi > > I have reviewed the code for removing double semicolons at the end of lines > > all the best > matteo LGTM also. Similar suggestion for updating copyrights. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7268
Recent SSLSocket close() @apiNote Changes.
Hi Xuelei,
I am working on some close code including the recent PR[1] for:
8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket
and ran into a change I hadn't noticed before.
* @apiNote
* When the connection is no longer needed, the client and server
* applications should each close both sides of their respective
connection.
* For {@code SSLSocket} objects, for example, an application can call
* {@link Socket#shutdownOutput()} for output stream close and call
* {@link Socket#shutdownInput()} for input stream close.
It used to be that just a single SSLSocket.close() was sufficient to
completely shutdown the SSLSocket, and under the hood it closed the
output/input in the right order.
I believe this code still closes everything as before, but the updated
@apiNote encourages the user to do a three-part shutdown instead.
socket.shutdownOutput();
socket.shutdownInput();
socket.close();// mostly repeats of above.
This approach seems unnecessary unless the user is interested in the
TLSv1.3 half-close mode.
What is the rationale for recommending this way of doing closes in
general? Or does this @apiNote need another iteration?
Thanks,
Brad
[1] https://github.com/openjdk/jdk/pull/7648
Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket
On Tue, 1 Mar 2022 17:09:57 GMT, zzambers wrote: > Fixed API Note in javadoc for javax.net.ssl.SSLSocket class. API Note was > introduced by JDK-8208526 [1]. At that point both Socket.shutdownInput() / > Socket.shutdownOutput() and InputStream.close() / OutputStream.close() > performed half-close of TLS-1.3 connection. However this behaviour has > changed as result of JDK-8216326 [2]. InputStream.close() / > OutputStream.close() no longer perform half-close but full socket close, but > API Note was never updated. > > [1] https://bugs.openjdk.java.net/browse/JDK-8208526 > [2] https://bugs.openjdk.java.net/browse/JDK-8216326 Please also update JDK-8282529 to include the description (i.e. above). IMHO, the bug should contain the information for the issue, rather than need to navigate 2 steps to the pull request. - PR: https://git.openjdk.java.net/jdk/pull/7648
