Re: RFR: JDK-6725221 Standardize obtaining boolean properties with defaults [v2]

[Bradford Wetmore]

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: 8284191: Replace usages of 'a the' in hotspot and java.base

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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

[Bradford Wetmore]

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-8285504 Minor cleanup could be done in javax.net [v5]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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

Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

[Bradford Wetmore]

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: JDK-8285504 Minor cleanup could be done in javax.net [v2]

[Bradford Wetmore]

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: JDK-8285504 Minor cleanup could be done in javax.net [v2]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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: 8284892: java/net/httpclient/http2/TLSConnection.java fails intermittently [v2]

[Bradford Wetmore]

On Thu, 14 Apr 2022 18:45:10 GMT, Daniel Fuchs  wrote:

>> java/net/httpclient/http2/TLSConnection.java has been observed failing (even 
>> though rarely) in test jobs.
>> 
>> The issue is that the handler used on the the server sides maintains a 
>> volatile `sslSession` field which it sets when receiving a request, so that 
>> the client can check which SSLParameters were negotiated after receiving the 
>> response.
>> However that field is set a little too late: after writing the request body 
>> bytes. This means that sometimes the client is able to receive the last byte 
>> before the server has updated the field, and can observe the previous value, 
>> which was set by the previous request. Checking of SSL parameters on the 
>> client side then usually fails, as it is looking at the wrong session.
>> 
>> The proposed fix is very simple: just update the `sslSession` field a little 
>> earlier - before writing the response.
>
> Daniel Fuchs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Copyright update

LGTM.

-

PR: https://git.openjdk.java.net/jdk/pull/8249

Re: RFR: 8284893: Fix typos in java.base

[Bradford Wetmore]

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]

[Bradford Wetmore]

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: 8284567: Collapse identical catch branches in java.base

[Bradford Wetmore]

On Sat, 2 Apr 2022 16:05:06 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

LGTM

-

Marked as reviewed by wetmore (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8081

Re: RFR: 8284567: Collapse identical catch branches in java.base

[Bradford Wetmore]

On Sat, 2 Apr 2022 16:05:06 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

src/java.base/share/classes/java/net/URI.java line 47:

> 45: import sun.nio.cs.UTF_8;
> 46: 
> 47: import java.lang.Character; // for javadoc

Looks like the javadoc comment no longer applies, so should be ok to remove.

-

PR: https://git.openjdk.java.net/jdk/pull/8081

Re: RFR: 8282723: Add constructors taking a cause to JSSE exceptions [v5]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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

Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v2]

[Bradford Wetmore]

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: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v4]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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

Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v2]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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]

[Bradford Wetmore]

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

[Bradford Wetmore]

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: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket [v2]

[Bradford Wetmore]

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

[Bradford Wetmore]

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

Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket

[Bradford Wetmore]

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

Re: RFR: 8282529: Fix API Note in javadoc for javax.net.ssl.SSLSocket

[Bradford Wetmore]

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 update the copyright to include 2022.  Thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/7648

Integrated: 8276677: Malformed Javadoc inline tags in JDK source in javax/net/ssl

[Bradford Wetmore]

On Mon, 8 Nov 2021 22:59:30 GMT, Bradford Wetmore  wrote:

> Minor typos.

This pull request has now been integrated.

Changeset: 38e6d5d6
Author:    Bradford Wetmore 
URL:   
https://git.openjdk.java.net/jdk/commit/38e6d5d6ed967f68e6ac1bfaa285efa16577c790
Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod

8276677: Malformed Javadoc inline tags in JDK source in javax/net/ssl

Reviewed-by: jnimeh

-

PR: https://git.openjdk.java.net/jdk/pull/6301

RFR: 8276677: Malformed Javadoc inline tags in JDK source in javax/net/ssl

[Bradford Wetmore]

Minor typos.

-

Commit messages:
 - Forgot copyright date
 - 8276677: Malformed Javadoc inline tags in JDK source in javax/net/ssl

Changes: https://git.openjdk.java.net/jdk/pull/6301/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6301=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8276677
  Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6301.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6301/head:pull/6301

PR: https://git.openjdk.java.net/jdk/pull/6301

Re: RFR: 8274809: Update java.base classes to use try-with-resources [v3]

[Bradford Wetmore]

On Wed, 6 Oct 2021 18:47:26 GMT, Andrey Turbanov 
 wrote:

>> 8274809: Update java.base classes to use try-with-resources
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8274809: Update java.base classes to use try-with-resources
>   update copyright year

src/java.base/share/classes/sun/net/NetProperties.java line 72:

> 70: fname = f.getCanonicalPath();
> 71: try (FileInputStream in = new FileInputStream(fname)) {
> 72: BufferedInputStream bin = new BufferedInputStream(in);

Shouldn't this have a multiple resource block in the try-with block here:  

try (FileInputStream in = new FileInputStream(fname);
BufferedInputStream bin = new BufferedInputStream(in)) {
props.land(bin);
}

-

PR: https://git.openjdk.java.net/jdk/pull/5818

Re: RFR: 8274809: Update java.base classes to use try-with-resources [v3]

[Bradford Wetmore]

On Wed, 6 Oct 2021 18:47:26 GMT, Andrey Turbanov 
 wrote:

>> 8274809: Update java.base classes to use try-with-resources
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8274809: Update java.base classes to use try-with-resources
>   update copyright year

Greater than 80 chars comment missing in latest changeset.

-

PR: https://git.openjdk.java.net/jdk/pull/5818

Re: RFR: 8274809: Update java.base classes to use try-with-resources [v3]

[Bradford Wetmore]

On Wed, 6 Oct 2021 16:07:12 GMT, Bradford Wetmore  wrote:

>> Andrey Turbanov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8274809: Update java.base classes to use try-with-resources
>>   update copyright year
>
> src/java.base/share/classes/sun/security/util/PolicyUtil.java line 156:
> 
>> 154: }
>> 155: 
>> 156: try (InputStream inStream = new 
>> BufferedInputStream(getInputStream(keyStoreUrl))) {
> 
> Same comment as above.

Looks like it's still > 80 chars

-

PR: https://git.openjdk.java.net/jdk/pull/5818

Re: RFR: 8274809: Update java.base classes to use try-with-resources

[Bradford Wetmore]

On Tue, 5 Oct 2021 09:36:23 GMT, Andrey Turbanov 
 wrote:

> 8274809: Update java.base classes to use try-with-resources

I checked the rest.  The one BufferedInputStream change is puzzling.  Please 
explain or address.

Files like HttpTimestamper need the copyright dates updated to 2021.

src/java.base/share/classes/sun/net/NetProperties.java line 71:

> 69: f = new File(f, "net.properties");
> 70: fname = f.getCanonicalPath();
> 71: try (FileInputStream fis = new FileInputStream(fname)) {

Why did the BufferedInputStream get pulled out here?

src/java.base/share/classes/sun/security/util/PolicyUtil.java line 156:

> 154: }
> 155: 
> 156: try (InputStream inStream = new 
> BufferedInputStream(getInputStream(keyStoreUrl))) {

Same comment as above.

-

Marked as reviewed by wetmore (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5818

Re: RFR: 8274809: Update java.base classes to use try-with-resources

[Bradford Wetmore]

On Tue, 5 Oct 2021 09:36:23 GMT, Andrey Turbanov 
 wrote:

> 8274809: Update java.base classes to use try-with-resources

Reviewed the crypto/security files.

src/java.base/share/classes/sun/security/timestamp/HttpTimestamper.java line 
115:

> 113: 
> 114: // Send the request
> 115: try (DataOutputStream output = new 
> DataOutputStream(connection.getOutputStream())) {

Please break this up so it doesn't have lines > 80.  e.g.

try (DataOutputStream output =
new DataOutputStream(connection.getOutputStream())) {

This makes side-by-side viewing very hard without a wider monitor.  

Thank you.

src/java.base/share/classes/sun/security/timestamp/HttpTimestamper.java line 
127:

> 125: // Receive the reply
> 126: byte[] replyBuffer = null;
> 127: try (BufferedInputStream input = new 
> BufferedInputStream(connection.getInputStream())) {

Same comment as above.

-

Marked as reviewed by wetmore (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5818

Integrated: 8273045: Fix misc javadoc bugs in the java.security and javax.net.ssl code

[Bradford Wetmore]

On Fri, 27 Aug 2021 01:35:17 GMT, Bradford Wetmore  wrote:

> Did a quick sweep of some minor non-standard javadoc issues.  This silences 
> 3rd party tooling warnings and fixes some linkage issues.

This pull request has now been integrated.

Changeset: 76baace2
Author:Bradford Wetmore 
URL:   
https://git.openjdk.java.net/jdk/commit/76baace2f07cb7b5d5fd20abd1612085bdba4292
Stats: 43 lines in 10 files changed: 8 ins; 3 del; 32 mod

8273045: Fix misc javadoc bugs in the java.security and javax.net.ssl code

Reviewed-by: xuelei

-

PR: https://git.openjdk.java.net/jdk/pull/5271

Re: RFR: 8273045: Fix misc javadoc bugs in the java.security and javax.net.ssl code [v2]

[Bradford Wetmore]

> Did a quick sweep of some minor non-standard javadoc issues.  This silences 
> 3rd party tooling warnings and fixes some linkage issues.

Bradford Wetmore has updated the pull request incrementally with one additional 
commit since the last revision:

  Codereview Comment

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/5271/files
  - new: https://git.openjdk.java.net/jdk/pull/5271/files/82fd00a4..705104a7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=5271=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5271=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5271.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5271/head:pull/5271

PR: https://git.openjdk.java.net/jdk/pull/5271

Re: RFR: 8273045: Fix misc javadoc bugs in the java.security and javax.net.ssl code [v2]

[Bradford Wetmore]

On Fri, 27 Aug 2021 03:38:55 GMT, Xue-Lei Andrew Fan  wrote:

>> Bradford Wetmore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Codereview Comment
>
> src/java.base/share/classes/javax/net/ssl/SNIHostName.java line 37:
> 
>> 35: import java.util.Objects;
>> 36: import java.util.regex.Pattern;
>> 37: import java.util.regex.PatternSyntaxException;
> 
> Does it mean that the classes introduced in java doc should also be imported? 
>  Maybe, the path package names could be removed in the createSNIMatcher() 
> method spec.
> 
> - * @throws java.util.regex.PatternSyntaxException if the regular 
> expression's
> - * syntax is invalid
> + * @throws PatternSyntaxException if the regular expression's syntax is 
> invalid

Thanks for the catch.

It doesn't absolutely have to be fixed to get javadoc to run (it apparently is 
a good guesser), but 3rd party tools like IntelliJ Idea aren't as robust, and 
treat this as an error.  

In the second update, I removed the extra package name in setSNIMatcher.  

Thanks for the review.  I'll go ahead and push.

-

PR: https://git.openjdk.java.net/jdk/pull/5271

RFR: 8273045: Fix misc javadoc bugs in the java.security and javax.net.ssl code

[Bradford Wetmore]

Did a quick sweep of some minor non-standard javadoc issues.  This silences 3rd 
party tooling warnings and fixes some linkage issues.

-

Commit messages:
 - 8273045: Fix misc javadoc bugs in the java.security and javax.net.ssl code

Changes: https://git.openjdk.java.net/jdk/pull/5271/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5271=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8273045
  Stats: 42 lines in 10 files changed: 8 ins; 3 del; 31 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5271.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5271/head:pull/5271

PR: https://git.openjdk.java.net/jdk/pull/5271

Re: RFR: 8263531: Remove unused buffer int

[Bradford Wetmore]

On Mon, 12 Jul 2021 20:39:53 GMT, Christoph Langer  wrote:

> The change for JDK-8257001 left an obsolete int field. Remove it.

Marked as reviewed by wetmore (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4759

Integrated: 8267750: Incomplete fix for JDK-8267683

[Bradford Wetmore]

On Wed, 26 May 2021 01:12:14 GMT, Bradford Wetmore  wrote:

> Missed updating today's changeset with the new variable name.
> 
> It's a "one character fix."

This pull request has now been integrated.

Changeset: b33b8bc8
Author:    Bradford Wetmore 
URL:   
https://git.openjdk.java.net/jdk/commit/b33b8bc88da3afe4f9f6321673df061ea4196962
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8267750: Incomplete fix for JDK-8267683

Reviewed-by: jnimeh

-

PR: https://git.openjdk.java.net/jdk/pull/4196

RFR: 8267750: Incomplete fix for JDK-8267683

[Bradford Wetmore]

Missed updating today's changeset with the new variable name.

It's a "one character fix."

-

Commit messages:
 - 8267750: Incomplete fix for JDK-8267683

Changes: https://git.openjdk.java.net/jdk/pull/4196/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4196=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267750
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4196.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4196/head:pull/4196

PR: https://git.openjdk.java.net/jdk/pull/4196

RFR: 8267683: rfc7301Grease8F value not displayed correctly in SSLParameters javadoc

[Bradford Wetmore]

Simple typo fix.  Somehow the trailing "u" got omitted, so the code won't parse 
when fed into the compiler.

Resulting javadoc output now compiles.

-

Commit messages:
 - Codereview Comments.
 - 8267683: rfc7301Grease8F value not displayed correctly in SSLParameters 
javadoc

Changes: https://git.openjdk.java.net/jdk/pull/4193/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4193=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267683
  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4193.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4193/head:pull/4193

PR: https://git.openjdk.java.net/jdk/pull/4193

Integrated: 8267683: rfc7301Grease8F value not displayed correctly in SSLParameters javadoc

[Bradford Wetmore]

On Tue, 25 May 2021 18:03:51 GMT, Bradford Wetmore  wrote:

> Simple typo fix.  Somehow the trailing "u" got omitted, so the code won't 
> parse when fed into the compiler.
> 
> Resulting javadoc output now compiles.

This pull request has now been integrated.

Changeset: e751b7b1
Author:Bradford Wetmore 
URL:   
https://git.openjdk.java.net/jdk/commit/e751b7b1b6f7269a1fe20c07748c726536388f6d
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod

8267683: rfc7301Grease8F value not displayed correctly in SSLParameters javadoc

Reviewed-by: coffeys

-

PR: https://git.openjdk.java.net/jdk/pull/4193

Integrated: 8254631: Better support ALPN byte wire values in SunJSSE

[Bradford Wetmore]

On Wed, 25 Nov 2020 20:03:01 GMT, Bradford Wetmore  wrote:

> Certain TLS ALPN values can't be properly read or written by the SunJSSE 
> provider. This is due to the choice of Strings as the API interface and the 
> undocumented internal use of the UTF-8 Character Set which converts 
> characters larger than U+7F into multi-byte arrays that may not be 
> expected by a peer.
> 
> Full details are available in:
> 
> - Bug:  https://bugs.openjdk.java.net/browse/JDK-8254631
> - CSR:  https://bugs.openjdk.java.net/browse/JDK-8256817

This pull request has now been integrated.

Changeset: fe5cccc1
Author:Bradford Wetmore 
URL:   https://git.openjdk.java.net/jdk/commit/fe51
Stats: 451 lines in 6 files changed: 440 ins; 1 del; 10 mod

8254631: Better support ALPN byte wire values in SunJSSE

Reviewed-by: xuelei, dfuchs

-

PR: https://git.openjdk.java.net/jdk/pull/1440

Re: RFR: 8254631: Better support ALPN byte wire values in SunJSSE

[Bradford Wetmore]

On Thu, 26 Nov 2020 20:26:36 GMT, Xue-Lei Andrew Fan  wrote:

>> Certain TLS ALPN values can't be properly read or written by the SunJSSE 
>> provider. This is due to the choice of Strings as the API interface and the 
>> undocumented internal use of the UTF-8 Character Set which converts 
>> characters larger than U+7F into multi-byte arrays that may not be 
>> expected by a peer.
>> 
>> Full details are available in:
>> 
>> - Bug:  https://bugs.openjdk.java.net/browse/JDK-8254631
>> - CSR:  https://bugs.openjdk.java.net/browse/JDK-8256817
>
> src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 341:
> 
>> 339:  * The ApplicationProtocol {@code String} values returned by the 
>> methods in
>> 340:  * this class are in the network byte representation sent by the peer 
>> and
>> 341:  * may need to be converted to its Unicode format before use.  For 
>> example,
> 
> I think there are two possible directions to use the bytes, concerting 
> application ALPN representation to network byte representation, or concerting 
> network bytes to application representation.  The 1st sentence, I may focus 
> on the the network byte representation description.
> 
> "The ApplicationProtocol {@code String} values returned by the methods in 
> this class are in the network byte representation sent by the peer.  If an 
> ALPN value is not encoded in network byte ..., an conversion may be required. 
>  For example, ..."

Discussed with Xuelei, the concern was it wasn't clear that you could compare 
using byte[] or Strings, and possible byte encodings that might be received.  I 
believe these have been addressed in the current version.

-

PR: https://git.openjdk.java.net/jdk/pull/1440

Re: RFR: 8254631: Better support ALPN byte wire values in SunJSSE

[Bradford Wetmore]

On Wed, 25 Nov 2020 20:03:01 GMT, Bradford Wetmore  wrote:

> Certain TLS ALPN values can't be properly read or written by the SunJSSE 
> provider. This is due to the choice of Strings as the API interface and the 
> undocumented internal use of the UTF-8 Character Set which converts 
> characters larger than U+7F into multi-byte arrays that may not be 
> expected by a peer.
> 
> Full details are available in:
> 
> - Bug:  https://bugs.openjdk.java.net/browse/JDK-8254631
> - CSR:  https://bugs.openjdk.java.net/browse/JDK-8256817

I am not able to see the comment here in github due to the Terms of Use issue 
flagged by github, but did get email with the issue so I'm hoping/assuming that 
the terms have been accepted.

For this test, I just took the standard JSSE test template and added the few 
lines to check for the proper ALPN exchange.

I'm 99% sure that for the JDK testsuite, the default Charset is UTF-8 which 
will accept ASCII values like this.  If not, there's going to be a lot more 
problems.  :)

Consulted with the Sustaining Team, they requested that this note not be 
included, as the versions are not yet known.

-

PR: https://git.openjdk.java.net/jdk/pull/1440

Re: RFR: 8254631: Better support ALPN byte wire values in SunJSSE

[Bradford Wetmore]

On Thu, 26 Nov 2020 10:33:26 GMT, Daniel Fuchs  wrote:

>> Certain TLS ALPN values can't be properly read or written by the SunJSSE 
>> provider. This is due to the choice of Strings as the API interface and the 
>> undocumented internal use of the UTF-8 Character Set which converts 
>> characters larger than U+7F into multi-byte arrays that may not be 
>> expected by a peer.
>> 
>> Full details are available in:
>> 
>> - Bug:  https://bugs.openjdk.java.net/browse/JDK-8254631
>> - CSR:  https://bugs.openjdk.java.net/browse/JDK-8256817
>
> src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 353:
> 
>> 351:  * // MEETEI MAYEK LETTERS HUK UN I (Unicode 0xabcd->0xabcf)
>> 352:  * if (unicodeString.equals("\uabcd\uabce\uabcf") {
>> 353:  * ...
> 
> Hi Brad,
> 
> There's a missing closing parenthesis here on line 352. 
> 
> Additionally - the unicode characters in the string above will be substituted 
> by the compiler before the API documentation is generated. I am suspecting 
> that this is not what you want. If you want to see the literal unicode escape 
> in the generated documentation, you will need to employ some tricks. One of 
> them could be to use the unicode escape of \ instead of \ to prevent the 
> compiler from interpreting \uabcd as a unicode escape.
> 
> Something like:
> 
>  * // MEETEI MAYEK LETTERS HUK UN I (Unicode 0xabcd->0xabcf)
>  * if (unicodeString.equals("\u005cuabcd\u005cuabce\u005cuabcf")) {
> 
> would do the trick. Alternatively - this would probably work too:
> 
>  * // MEETEI MAYEK LETTERS HUK UN I (Unicode 0xabcd->0xabcf)
>  * {@code if (unicodeString.equals("}{@code uabcd}{@code uabce}{@code 
> uabcf"))} {
> 
> I realize none of these alternatives are ideal - maybe someone knows a better 
> trick...

I made this change in SSLParameters, and forgot that I had a similar change to 
make in SSLEngine/SSLSocket.

> src/java.base/share/classes/javax/net/ssl/SSLSocket.java line 146:
> 
>> 144:  *
>> 145:  * // MEETEI MAYEK LETTERS HUK UN I (Unicode 0xabcd->0xabcf)
>> 146:  * if (unicodeString.equals("\uabcd\uabce\uabcf") {
> 
> Same remark here

Also fixed using \u005c.

-

PR: https://git.openjdk.java.net/jdk/pull/1440

RFR: 8254631: Better support ALPN byte wire values in SunJSSE

[Bradford Wetmore]

Certain TLS ALPN values can't be properly read or written by the SunJSSE 
provider. This is due to the choice of Strings as the API interface and the 
undocumented internal use of the UTF-8 Character Set which converts characters 
larger than U+7F into multi-byte arrays that may not be expected by a peer.

Full details are available in:

- Bug:  https://bugs.openjdk.java.net/browse/JDK-8254631
- CSR:  https://bugs.openjdk.java.net/browse/JDK-8256817

-

Commit messages:
 - Code Review Comments 4
 - Code Review Comments #3
 - Code Review Comments #2
 - Code Review Comments
 - Seventh iteration
 - Sixth iteration
 - Fifth iteration
 - Fourth iteration
 - Third iteration
 - Second iteration
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/b52f6c05...9e6f0a4f

Changes: https://git.openjdk.java.net/jdk/pull/1440/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1440=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8254631
  Stats: 451 lines in 6 files changed: 440 ins; 1 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1440.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1440/head:pull/1440

PR: https://git.openjdk.java.net/jdk/pull/1440

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[Bradford Wetmore]

On Fri, 11 Sep 2020 07:15:26 GMT, Dmitriy Dumanskiy 
 wrote:

>> 1) This is un-necessary churn.
>> 2) I can't even be sure I am finding the ones in my area because there's so 
>> much here
>> 3) The ones I can find have no need of whatever performance improvement this 
>> might bring.
>> I think this whole PR should be withdrawn, and there may an attempt at 
>> measuring the benefits of this for the various
>> cases before submitting in appropriate smaller chunks. But I'll tell you now 
>> I don't see a need for the test updates
>> you are making.
>
> Ok, sorry for the distraction.

Our local Santuario maintainer says:

In general, changes to Apache Santuario should also be made at Apache so we 
stay in sync.

-

PR: https://git.openjdk.java.net/jdk/pull/29

Re: RFR [13] JDK-8215430: Remove the internal package com.sun.net.ssl

[Bradford Wetmore]

AbstractDelegateHttpsURLConnection.java
---
74/88/etc.  I haven't checked for sure, but IIRC we made setNewClient 
public from protected and other such changes in order for these classes 
to use the delegation model and allow calls to both sun/net/www/protocol 
and com/sun/net/ssl/internal/www/protocol...  Should the delegation code 
be stripped out, and these methods be set back to protected?


ProviderConfig.java
---
I think we're over-rotating on this one, I'd be very surprised if anyone 
is still using com.sun.net.ssl.internal.ssl.Provider.  My personal 
preference is to kill it now, but if you really want to keep it, please 
file a bug and commit to a near release so we can finally kill this 
thing off.


test/jdk/com/sun/net/ssl/SSLSecurity/ComTrustManagerFactoryImpl.java
test/jdk/com/sun/net/ssl/SSLSecurity/JavaxTrustManagerFactoryImpl.java

When we opensourced the JSSE code, we left some of the javax and com 
tests in the com/sun/net/ssl test directory.  The test names are very 
similar, with the exception of the Com/Javax prefix.  The tests were 
identical, with the exception of whether they called into the com or 
javax APIs.


Here you are deleting the ComTrustManagerFactoryImpl test which is 
probably correct, but should you also be deleting the 
JavaxTrustManagerFactoryImpl test as well?  Please check that we didn't 
get too carried away and are deleting still valid tests.


Thanks,

Brad


On 2/26/2019 7:33 AM, Chris Hegarty wrote:



On 26 Feb 2019, at 03:53, Xuelei Fan  wrote:



It makes sense to me to leave the AbstractDelegateHttpsURLConnection methods as 
public.  We may need to re-org the code later, but it is out of the scope of 
this update.

Here is the new webrev (only AbstractDelegateHttpsURLConnection updated):
http://cr.openjdk.java.net/~xuelei/8215430/webrev.01/


I think that this is probably ok ( since the package is not exported ),
but it seems a little distasteful to not have the API move through
the terminal deprecation route before being removed.

-Chris.

Re: [jdk10] (XXS) RFR 8189631 : Missing space in the javadoc for InetAddress.createNameService()

[Bradford Wetmore]

+1.

Brad


On 11/14/2017 11:47 AM, Roger Riggs wrote:

+1, Reviewed

Roger


On 11/14/2017 2:45 PM, Ivan Gerasimov wrote:

Hello!

A typo in the javadoc:

--- a/src/java.base/share/classes/java/net/InetAddress.java
+++ b/src/java.base/share/classes/java/net/InetAddress.java
@@ -1133,7 +1133,7 @@
 /**
  * Create an instance of the NameService interface based on
- * the setting of the {@codejdk.net.hosts.file} system property.
+ * the setting of the {@code jdk.net.hosts.file} system property.
  *
  * The default NameService is the PlatformNameService, which 
typically

  * delegates name and address resolution calls to the underlying


Would you approve the fix?

Re: RFR of JDK-8158881: Doc typo in src/../java/net/URI.java

[Bradford Wetmore]

Looks good, thanks.

brad


On 6/8/2016 1:16 AM, Chris Hegarty wrote:

On 8 Jun 2016, at 03:36, Hamlin Li <huaming...@oracle.com> wrote:


On 2016/6/8 4:27, Bradford Wetmore wrote:

May I suggest using example.com [1] and an arbitrary path instead on these two 
examples, so that there's no chance of it being mistaken for a valid URL.  This 
example uses old hostnames that might change yet again (docs.oracle.com) and an 
ancient JDK platform (1.3), let's just remove that confusion now.

Thank you Brad.
Sure, your suggestion sounds reasonable, I should have made it more clear that 
the changed URLs in doc are not valid URLs, they just sample.
Please check webrev: http://cr.openjdk.java.net/~mli/8158881/webrev.01/


Thank you Hamlin ( and Brad ), this looks good.

-Chris.


Thank you
-Hamlin


Thanks,

Brad

[1] RFC 2606:  https://tools.ietf.org/html/rfc2606

On 6/7/2016 2:34 AM, Chris Hegarty wrote:



On 7 Jun 2016, at 05:50, Hamlin Li <huaming...@oracle.com> wrote:

Would you please review the following simple doc patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8158881
webrev: http://cr.openjdk.java.net/~mli/8158881/webrev.00/


I’m not sure why the URL’s were ever changed from java.sun.com, since they are 
not
hyperlinks. This was a bad mistake, and badly broke the documentation, for an
unseeingly related change.

What about the scheme, shouldn’t that be updated to ‘https' too ?

-Chris.


Thank you
-Hamlin


--- a/src/java.base/share/classes/java/net/URI.javaTue Jun 07 10:33:38 2016 
+0800
+++ b/src/java.base/share/classes/java/net/URI.javaMon Jun 06 21:47:44 2016 
-0700
@@ -183,7 +183,7 @@
* (1)
* 
*
- * against the base URI {@code http://java.sun.com/j2se/1.3/} is the result
+ * against the base URI {@code http://docs.oracle.com/javase/1.3/} is the 
result
* URI
*
* 
@@ -193,13 +193,13 @@
* Resolving the relative URI
*
* 
- * {@code 
../../../demo/jfc/SwingSet2/src/SwingSet2.java}(2)
+ * {@code 
../../demo/jfc/SwingSet2/src/SwingSet2.java}(2)
* 
*
* against this result yields, in turn,
*
* 
- * {@code http://java.sun.com/j2se/1.3/demo/jfc/SwingSet2/src/SwingSet2.java}
+ * {@code http://docs.oracle.com/demo/jfc/SwingSet2/src/SwingSet2.java}
* 
*

Re: RFR of JDK-8158881: Doc typo in src/../java/net/URI.java

[Bradford Wetmore]

May I suggest using example.com [1] and an arbitrary path instead on 
these two examples, so that there's no chance of it being mistaken for a 
valid URL.  This example uses old hostnames that might change yet again 
(docs.oracle.com) and an ancient JDK platform (1.3), let's just remove 
that confusion now.


Thanks,

Brad

[1] RFC 2606:  https://tools.ietf.org/html/rfc2606

On 6/7/2016 2:34 AM, Chris Hegarty wrote:



On 7 Jun 2016, at 05:50, Hamlin Li  wrote:

Would you please review the following simple doc patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8158881
webrev: http://cr.openjdk.java.net/~mli/8158881/webrev.00/


I’m not sure why the URL’s were ever changed from java.sun.com, since they are 
not
hyperlinks. This was a bad mistake, and badly broke the documentation, for an
unseeingly related change.

What about the scheme, shouldn’t that be updated to ‘https' too ?

-Chris.


Thank you
-Hamlin


--- a/src/java.base/share/classes/java/net/URI.javaTue Jun 07 10:33:38 2016 
+0800
+++ b/src/java.base/share/classes/java/net/URI.javaMon Jun 06 21:47:44 2016 
-0700
@@ -183,7 +183,7 @@
 * (1)
 * 
 *
- * against the base URI {@code http://java.sun.com/j2se/1.3/} is the result
+ * against the base URI {@code http://docs.oracle.com/javase/1.3/} is the 
result
 * URI
 *
 * 
@@ -193,13 +193,13 @@
 * Resolving the relative URI
 *
 * 
- * {@code 
../../../demo/jfc/SwingSet2/src/SwingSet2.java}(2)
+ * {@code 
../../demo/jfc/SwingSet2/src/SwingSet2.java}(2)
 * 
 *
 * against this result yields, in turn,
 *
 * 
- * {@code http://java.sun.com/j2se/1.3/demo/jfc/SwingSet2/src/SwingSet2.java}
+ * {@code http://docs.oracle.com/demo/jfc/SwingSet2/src/SwingSet2.java}
 * 
 *

Re: Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension

[Bradford Wetmore]

I just would like to remind that session resumption is a very
important use case to support for ALPN.


Understood.  The ALPN value is tied to a handshake, either already 
completed and active (getApplicationProtocol()) or still in progress 
(getHandshakeApplicationProtocol()).  Each handshake results in a 
complete ALPN negotiation.  So a session resumption will always do a 
ALPN negotiation from scratch.



I have not seen anything related to this review for session resumption.
Has it been tested ?


The current unit tests don't have resumption, but there should be a RFE 
(if not already) for resumption testing along with some other cases. 
Vinnie is working on these.



It is still not clear to me what a client and a server have to do to
support ALPN.


If you want to use the default behavior (server-prioritized list looking 
for a common element), just call SSLParameters.setApplicationProtocols() 
on both sides.


If you want to do very specific protocol/ciphersuite/alpn configuration 
before handshaking, then do the configuration before starting the 
handshake (more details and similar to what you suggested below).  This 
is what was decided back in October when we pulled out the Matcher 
mechanism in v6/v7.  [1]



 From my understanding a server has to:

* read encrypted bytes into a ByteBuffer


BTW, during the initial handshake on a connection, these are not encrypted.


* parse the TLS ClientHello frame on its own
* extract from the ClientHello the TLS Protocol version, the ALPN
extension, and the ciphers
* run some logic to determine the AP
** if no AP can be chosen, generate a TLS Alert frame on its own to
close the connection and bail out


I would create a regular SSLContext/SSLEngine/configureALPN and start 
the handshake as usual.  The JSSE implementation will create the TLS 
ALPN Alert when it realizes it doesn't have any ALPN values in common. 
The server then should obtain those bytes from the wrap() and send to 
the peer, as usual.



* otherwise, an AP/cipher combo has been chosen


Filling in a few more details:

SSLContext.getInstance("protocol"); // returns a context with
// "protocol" and possibly
// other protocols enabled.
SSLEngine ssle = SSLContext.createSSLEngine(...);

Read ClientHello from Socket/SocketChannel/AsynchronousSocketChannel/etc.

Parse ClientHello for requested protocol/alpn/ciphersuites

choose protocol/alpn/ciphersuite value(s)

SSLParameters sslParameters = ssle.getSSLParameters();
sslParameters.setProtocols(protocols);// possibly just one
// You could use some non-matching value like
// "no_application_protocol" if you want to generate the
// no_application_protocol alert.
sslParameters.setApplicationProtocols(APs);   // possibly just one
sslParameters.setCipherSuites(ciphers)// possibly just one

sslEngine.setSSLParameters(sslParameters)

reset the ByteBuffer position to the beginning

sslEngine.unwrap()

JDK implementation will re-parse the ClientHello, and use the
sslParameters data during handshake and when generating the
ServerHello.  Any error alerts will be output by the SSLEngine as usual.

sslEngine.wrap()/unwrap() as usual.


A client has to:

* read encrypted bytes into a ByteBuffer
* parse a ServerHello frame on its own
* extract the ALPN extension and the cipher


You could parse this if you want, but wasn't expecting client apps 
would.  See below.



* run some logic to verify that the AP and the cipher can be used together
** if AP and cipher don't match, generate a TLS Alert frame on its own
to close the connection and bail out.


There is nothing in the ALPN RFC about this situation.  In the HTTP 
spec, if such a situation is negotiated, the application layer waits for 
the handshake completion, examines the state, sends a HTTP2 connection 
error of type INADEQUATE_SECURITY, then closes.


BTW, we would lose the ability to parse plaintext ServerHellos in 
TLSv1.3, as ServerHello is now encrypted.



* otherwise the AP/cipher combo is ok
* reset the ByteBuffer position to the beginning
* pass the ByteBuffer to sslEngine.unwrap()
* JDK implementation will re-parse the ServerHello and complete the
TLS handshake

Is that right ?


If you really want to parse it, yes.


In that scenario, what is the use of
SSLEngine.getHandshakeApplicationProtocol() ?


When either side needs to determine the selected ALPN value before the 
completion of the handshake, say during X509KeyManager selection or 
X509TrustManager verification.  On the server side, let's say we have 
two possible keys/certs, and we receive a ClientHello.


ClientHello  ->
  JSSE chooses protocol/ALPN
  JSSE iterates on ciphersuite
  iter calls X509KeyManager to find a key
X509KM calls setHandshakeApplicationProtocol()
to see which protocol/ALPN value was negotiated,
uses that 

Re: Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension

[Bradford Wetmore]

298:  This test is not actually calling into checkResult on the server side.  
Ooops!  You need to check the output of the wrap() before calling unwrap() as 
it overwrites the serverResult.  You need to put in a similar checkResult() 
before doing the flip()s.


So checks are required before and after the buffer flips, right?


Yes.  In a full handshake, the Handshake complete message could be the 
result of an unwrap (client) or wrap (server).


Brad

Re: Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension

[Bradford Wetmore]

On 11/29/2015 4:08 PM, Vincent Ryan wrote:

> Following on from Brad’s recent email, here is the full webrev of the
> API and the implementation classes for ALPN:
> http://cr.openjdk.java.net/~vinnie/8144093/webrev.00/
>
> In adds the implementation classes (sun/security/ssl) to the public API
> classes (javax/net/ssl) which have already been agreed.
> Some basic tests (test/javax/net/ssl) are also included.

SSLEngineAlpnTest.java
==

333:  A minor comment here. I think you should test NONE like this:

if (ap == null) {
throw new Exception(
"Handshake was completed but null was received");
}
if (expectedAP.equals("NONE")) {
if (!ap.isEmpty()) {
throw new Exception();
} else {
System.out.println("No ALPN value negotiated, as expected");
}
} else if (!expectedAP.equals(ap)) {
throw new Exception(expectedAP +
" ALPN value not available on negotiated connection");
}

BTW, the indentions here at 331/332/334 are off.  The rest are all ok.

We had also talked privately about testing 
getHandshakeApplicationProtocol(), but I don't see it here.  Let me know 
if you didn't understand my comments about adding a 
X509ExtendedKeyManager (or X509ExtendedTrustManager), or else please 
file a RFE for it if it won't be ready for the initial integration.


298:  This test is not actually calling into checkResult on the server 
side.  Ooops!  You need to check the output of the wrap() before calling 
unwrap() as it overwrites the serverResult.  You need to put in a 
similar checkResult() before doing the flip()s.



SSLSocketAlpnTest.java
==

Same concern here as above.


On 11/29/2015 10:30 PM, Xuelei Fan wrote:
> line 538-546
> 
> String negotiatedValue = null;
> List protocols = clientHelloALPN.getPeerAPs();
>
> for (String ap : localApl) {
> if (protocols.contains(ap)) {
> negotiatedValue = ap;
> break;
> }
> }
>
> I think the above order prefer the server preference order of
> application protocols.  It's OK per Section 3.2 of RFC 7301.

Correct:

   In that case, the server SHOULD select the most
   highly preferred protocol that it supports and that is also
   advertised by the client.

> However RFC 7301 also defines the client preference order.  Looks like
> the client preference order is not necessary.  It's a little bit
> confusing, and may result in different behaviors for different TLS 
vendors.

>
> Maybe, we can add an option to honor server preference order in the 
future.


That was the plan if needed.

Vincent wrote:
> If necessary I think we can add support for controlling this via a
> property, later.

Or a new API (when possible):  a new "preferPeerOrder" method and an 
update to the setApplicationProtocols() text:


Unless configured by the preferPeerOrder() method, the first
matched value becomes the negotiated value.

On 11/30/2015 3:08 PM, Vincent Ryan wrote:
>> SSLEngineImpl.java
>> >==
>> >
>> >line 1411-1412:
>> >private Ciphertext writeRecord(ByteBuffer[] appData,
>> >int offset, int length, ByteBuffer netData) throws IOException {
>> >...
>> >  if (...) {
>> >...
>> >// set connection ALPN value
>> >applicationProtocol =
>> >handshaker.getHandshakeApplicationProtocol();
>> >...
>> >  }
>> >}
>> >
>> >Is it redundant to set applicationProtocol here.  I was wondering, the
>> >value should set in the processInputRecord() method.
>
> My understanding is the the wrapping is performed here and the unwrapping
> performed in processInputRecord(). Isn’t the assignment required in 
both places?


Apparently it's not, I thought it was.  When unwrapping the final 
inbound Finished message around line 1069, it advances the internal 
state and writes its own Finished message into the outbound queue, but 
doesn't actually return hs=FINISHED until the outbound queue is drained.


I had to step through it to confirm.  So you can remove it.

Thanks, Vinnie, looks good.

Brad

TLS ALPN Update

[Bradford Wetmore]

On 10/6/2015 7:42 AM, David M. Lloyd wrote:

I didn't reply on this last week, but this looks workable to me.  Thanks!


Thanks to you guys for all the discussion!

During the architectural review, it was pointed out that the 
SSLParameters.setApplicationProtocols() API proposal with the 
server-side single array element restriction (i.e. a pre-chosen ALPN 
value) was unnecessarily limiting, and that we could achieve exactly the 
same result by removing the single element restriction.


For the advanced application cases like those that David/Simone/H2 are 
concerned about, you still have full control over your environment 
configuration and eventual ALPN selection.  Nothing has changed from 
earlier:


1.  "SSLExplore" the ClientHello

2.  obtain specialized SSLContext (if desired)

3.  set the enabled protocols, enable/order the ciphersuites, etc.

4.  select/set the *single* ALPN value

5.  start the handshake

6.  X509KeyManagers can look at the selected ALPN value
mid-handshake using
SSLSocket/SSLEngine.getHandshakeApplicationProtocol().

However, for less-restrictive situations or cases that don't require any 
connection setup, we follow the ordering language in RFC 7301/3.2:


   It is expected that a server will have a list of protocols that it
   supports, in preference order, and will only select a protocol if the
   client supports it.  In that case, the server SHOULD select the most
   highly preferred protocol that it supports and that is also
   advertised by the client.

The JSSE implementation is just a simple loop in the server side to 
select the ALPN value if both sides have values set.  Meaning:


select TLS protocol
select alpn value  (loop using server's order)
select ciphersuite

Changes to the APIs:

SSLParameters.setApplicationProtocols():  Removed 
"no_application_protocol" as a reserved string, and removed the first 
element restriction.


Updated ServerHandshaker as per above.

Return value on 
SSLSocket/SSLEngine.getApplicationProtocol()/getHandshakeApplicationProtocol() 
is a tri-state:  null (not set yet), empty string ("" if no ALPN value 
is set), or non-empty string (if ALPN was negotiated).


wordsmithing/cleanups


We have done successful testing with our primary internal consumer:

https://bugs.openjdk.java.net/browse/JDK-8042950
JEP 110: HTTP/2.

Vinnie will be publishing a full webrev of the API and implementation in 
the next day or so.


Thanks again for everyone's comments and feedback during this process. 
Except for any bugs found in this webrev, this is what we expect for the 
initial JEP integration for Feature Complete next week.


Cheers,

Brad

Re: RFR: 5108778 Too many instances of java.lang.Boolean created in Java application(net)

[Bradford Wetmore]

On 11/7/2015 1:02 AM, Sebastian Sickelmann wrote:

Hi Brad,

The bug is for the complete codebase, where the webrev is for the
net-dev part only. I have already created
a subtask for the macos-port-dev part, and it is fixed already.


Ok.


Now I would create a net-dev part sub-task as well.
For the net-dev part of this JBS-Ticket, it is javadoc only. But for the
rest (jaxp, corba, etc.) it is a little bit more.
Do you want to update your comment in JBS, to limit your analysis to the
net-dev part?


Done.


What do you mean by "Did you make the javadocs target to test?".
Do you think it is worth to write/update tests for the new suggestion
from the javadoc?


No, just that you made the javadoc target and it built ok.  I've seen 
javadoc typos which caused the build to fail.  Just making sure that 
didn't happen here.


Brad






-- Sebastian

On 11/06/2015 11:00 PM, Bradford Wetmore wrote:


This bug talks about not only javadoc, but the actual code as well. It
looks like someone has already hit all of that, so this is the only
thing left.

What you've proposed looks ok.  Did you make the javadocs target to test?




Brad



On 11/5/2015 7:42 PM, Sebastian Sickelmann wrote:

Hi, i wanted to start an discussion/review-process some time ago, see
second-try below.

Is there someone who wants to discuss/review this javadoc-only change?

Else, should i link my result as reference into the JBS?

-- Sebastian

On 10/27/2015 05:28 AM, Sebastian Sickelmann wrote:

Hello,

Actually I am searching through the JBS for low hanging fruits.
Right now i am looking through the openjdk-sources and try to evaluate
if i can make something about JDK-5108778.

Please find my webrevs for the jdk(net) repos at:

http://cr.openjdk.java.net/~sebastian/5108778/net/webrev.00/

The changes are javadoc only.

-- Sebastian

Re: RFR: 5108778 Too many instances of java.lang.Boolean created in Java application(net)

[Bradford Wetmore]

This bug talks about not only javadoc, but the actual code as well. It 
looks like someone has already hit all of that, so this is the only 
thing left.


What you've proposed looks ok.  Did you make the javadocs target to test?

Brad



On 11/5/2015 7:42 PM, Sebastian Sickelmann wrote:

Hi, i wanted to start an discussion/review-process some time ago, see
second-try below.

Is there someone who wants to discuss/review this javadoc-only change?

Else, should i link my result as reference into the JBS?

-- Sebastian

On 10/27/2015 05:28 AM, Sebastian Sickelmann wrote:

Hello,

Actually I am searching through the JBS for low hanging fruits.
Right now i am looking through the openjdk-sources and try to evaluate
if i can make something about JDK-5108778.

Please find my webrevs for the jdk(net) repos at:

http://cr.openjdk.java.net/~sebastian/5108778/net/webrev.00/

The changes are javadoc only.

-- Sebastian

Re: TLS ALPN Proposal v7

[Bradford Wetmore]

On Sat, Oct 3, 2015 at 2:19 AM, Bradford Wetmore
<bradford.wetm...@oracle.com> wrote:

Thanks for the comments everyone.  I'm submitting the following to the CCC
(internal review board):

 http://cr.openjdk.java.net/~wetmore/8051498/webrev.17/

Changes:

1.  No H2 Blacklist/Comparator

2.  set/getApplicationProtocols() back to SSLParameters.


Have you implemented this solution already ?


It is underway.  The guts was already done based on previous API 
versions.  The new API is less involved, so it should be simpler to do 
as it's just cutting out the existing ciphersuite/ALPN selection stuff.



Also for clients ?


Client/server are both externally(API)/internally essentially the same. 
 The big difference is that for clients you send all the values passed 
in, for servers you only consult the first which is the selected ALPN value.



Do you have feedback on actually implementing ALPN in this way ?


Based on what I saw previously, it should be pretty straighforward.

Brad

TLS ALPN Proposal v7

[Bradford Wetmore]

On 10/1/2015 7:35 PM, Xuelei Fan wrote:

On 10/2/2015 9:03 AM, Bradford Wetmore wrote:

Major changes:

1.  ApplicationProtocols is gone.  The H2 black list and comparator were
moved to StandardConstants.

2.  StandardConstants.  Strings for "h2" and "http/1.1" are back.  And
now that you are parsing the raw network bytes, I added a convenience
mapping between the two byte ciphersuite IANA-assigned value and the
Java Standard Name.


There is no SSLExplorer in OpenJDK. I think, maybe, the map is not
belong to OpenJDK either.

I think, the constants for HTTP2 is also belong to application protocol
(HTTP2) layer.  Application (HTTP2) implementation would take care of
them.  Maybe, they are not a part of JSSE framework either.


Ok, I've removed all of the H2 constants from StandardConstants.  I've 
mentioned to the the H2/network team that this is something they will 
need to handle/include (e.g. Blacklist/Comparator) in their code, and 
they might want to consider APIs similar to what I had.


I personally found it very useful to have a proper mapping to get the 
SSL_ vs. TLS_ prefix correct in the blacklist.



I would like to have "h2" and "http/1.1" defined as Standard Algorithms
Docs as we usually did for other standard constants.


Of course, they will be added as Standard Algorithm names.


3.  SSLParameter (set/get) are moved to SSLSocket/SSLEngine.  Even
though these could go into SSLParameters, this change makes backporting
much easier.  The helper code simply has to reflectively look for the
four methods in the implementation classes, and call if they are there.

Otherwise, there would have to be reflection both in the user code
(above) and implementation (to see if the passed SSLParameters had the
new methods via a subclass).

But, looking forward, per JSSE framework, SSLParameters should be the
central place to define SSL/TLS configuration parameters. We'd better
follow the conventions so that application developers won't get confused
about where SSL/TLS parameters should be configured.


I went back and forth on this many, many times yesterday.


Maybe, we cannot add public APIs for backporting.


I figured  we'd have to use reflection on user-derived classes to see if 
the methods were there, but apparently implementation-specifc APIs can 
be added in an update release, just not Java APIs. So if we really can 
do that, then we can add a jdk.SSLParameters/ 
org.openjdk.jsse.SSLParameters extends SSLParameters, and in the 
implementation, look for instanceof.


> I think backporting is

another history, and would better not impact too much of the design for
JDK 9 and future releases.


Ok, so get/setApplicationProtocols() is back in SSLParameters.

Thanks for the comments everyone.  I'm submitting the following to the 
CCC (internal review board):


http://cr.openjdk.java.net/~wetmore/8051498/webrev.17/

Changes:

1.  No H2 Blacklist/Comparator

2.  set/getApplicationProtocols() back to SSLParameters.

Thanks,

Brad

TLS ALPN Proposal v6

[Bradford Wetmore]

You guys (David/Simone/Bernd/Jason) are more on the front lines in 
server development and how functional this API will be, so I'll trust 
your judgement here.  If you are ok with:


1. potentially being blind during renegotiations in the existing 
TLSv1/v1.1/v1.2, and,


2. not having an SSLExplorer as part of the JDK (i.e. you parsing the 
ClientHellos ala SSLExplorer),


3. requiring all ALPN logic be in the application and none in the JDK,

I'm willing to go with this approach. It doesn't seem optimal for what I 
would call casual users, but it does solve the ugly issues with the 
matches() API.


(BTW, I did consider adding a 
ClientHelloCallback/ClientHello/ServerHelloCallback/ServerHello class 
that would handle callbacks, but that was getting complicated also.)


For current renegotiations, the big use case is adding client 
authentication, so it seems likely that the same ciphersuite will be 
offered/chosen, so it's likely moot.



For the existing URL code, do you think we need:

1. to provide a "https.alpn" System Property for the existing 
URLConnections?


2. a getApplicationProtocol() for HttpsURLConnection?

Michael (net-dev) says H2 will not be backported into the URL mechanism 
and doesn't see a need for it yet, so I'm inclined to say no.


More inline:

On 9/29/2015 8:07 AM, David M. Lloyd wrote:

Hi Brad, thanks for replying; comments are inline:
On 09/28/2015 08:40 PM, Bradford Wetmore wrote:

1. Only the initial ClientHellos are parsable.
===
The biggest problem I have with an Explorer-based design is that only
the initial ClientHello on a connection is passed in the clear.
Subsequent negotiations on this connection will be completely missed, as
the ClientHellos are now encrypted.

This seems like a deal breaker for me.


You are right, I cannot come up with a good solution for this, so that
might mean the idea is shot - *however* - I would point out that the
latest draft of TLS 1.3 [1] completely kills off the capability of the
client to renegotiate a connection, meaning that this will no longer be
possible anyway, and given it's a 1% kind of use case, that might be
enough to let it slide.  Combine this with what I consider to be the
unlikelihood of this working with HTTP/2.0, and I would feel very safe
assuming that nobody will ever actually do this.


Thanks for pointing this out, I thought PSK+tickets were a replacement 
for a renegotiation (Section 6.2.3), but it's apparently only for 
session resumption.


BTW, the WG is up to a Sept 29, 2015 version (draft-09).

[1] https://tlswg.github.io/tls13-spec/


I would also note that, as you state later on, it would be possible to
combine this solution with any other solution (including the proposed
one) to cover both cases.  And given that this is still (in my
estimation) a "99%" solution, in my opinion it is still a viable
candidate for adding this functionality to Java 8 as a first pass or
stopgap as I described in my emails, particularly if the method(s) to
establish/query the protocol names are a strict subset of the proposed
Java 9 API (given that we cannot really overhaul the Java SE 8 API at
this point).


[...]
2.  "SSLExplorer" or something similar is needed.
=
This approach depends on "examining SSLClientHello"s, but there isn't a
class for this other than some sample code from a previous attempt.  I
am assuming that this approach would make such an external API a
necessity?  Being able to parse possible ClientHello formats is not a
straightforward/easy job.  This will add a fair amount of complexity,
and likely not an easy job in the remaining few weeks.  It could be
added later for JDK 10 but that means apps would likely need to roll
their own for 9.


And 8, yes, you definitely would need to roll your own, though Xuelei
Fan already has a nice example up on his blog that was built for SNI
(but uses the same principle).


If you are referring to:


http://simsmi.blogspot.com/2014/01/jep-114-tls-sni-extension-virtual.html

This is just describing the general approach for the sample 
SSLExplorer/SSLCapabilities code in the JSSE Reference Guide.  The 
actual code can be found here:



http://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html#CodeExamples

My hope is to expand it to include parsing the ciphersuites and ALPN 
extensions.  I've moved/added some helper functions from 
ApplicationProtocol to StandardConstants.



 If it were me, I wouldn't even bother
adding it even in JDK 10, since (a) it applies only to the server side
and (b) there are a plethora of third-party server-side network I/O and
security libraries which are natural candidates to host this type of logic.


Ok.


3.  "no_application_protocol"
=
If the server doesn't support the protocols that the client advertises,
the "no_application_protocol&quo

Re: TLS ALPN Proposal v5

[Bradford Wetmore]

Several comments about David's proposal:


1. Only the initial ClientHellos are parsable.
===
The biggest problem I have with an Explorer-based design is that only 
the initial ClientHello on a connection is passed in the clear. 
Subsequent negotiations on this connection will be completely missed, as 
the ClientHellos are now encrypted.


This seems like a deal breaker for me.

Also, and this is moot if the ClientHellos are encrypted, unless you 
wrap the underlying raw socket in your own "observer" socket wrapper, 
you won't be able to view the raw data once the SSLSocket starts 
processing.  SSLEngine doesn't have this problem since it's just 
ByteBuffers.


One other comments while on the topic of renegotiations/resumptions as 
there were some incorrect statements in previous emails.  H2 (RFC 
7540-Sec 9.2.1) allows for renegotiations/resumptions to occur before 
the initial connection preface to protect *CLIENT CREDENTIALS* only, but 
prohibits it once data has started passing. But there is no such 
restriction on HTTP/1.1, nor on ALPN in general. So in such a case, 
there's no way you can change it with this proposal, and the ALPN RFC 
specifically allows for it.



2.  "SSLExplorer" or something similar is needed.
=
This approach depends on "examining SSLClientHello"s, but there isn't a 
class for this other than some sample code from a previous attempt.  I 
am assuming that this approach would make such an external API a 
necessity?  Being able to parse possible ClientHello formats is not a 
straightforward/easy job.  This will add a fair amount of complexity, 
and likely not an easy job in the remaining few weeks.  It could be 
added later for JDK 10 but that means apps would likely need to roll 
their own for 9.



3.  "no_application_protocol"
=
If the server doesn't support the protocols that the client advertises, 
the "no_application_protocol" must be thrown.   We could add a 
"no_application_protocol" protocol String that would flag such a 
condition internally.



4.  Much of this is already possible.
=
If we were to go with the current API/internal and apps provided their 
own ClientHello scanner, many of the benefits of what was proposed are 
already available.  Apps can ask for the desired SSLContext, get the 
SSLSocket/SSLengine, check the SNI/ALPN values, order/set the enabled 
protocols/ciphers/etc + single ALPN value, then wrap the raw socket 
using SSLSocketFactory.createSocket(Socket s, InputStream consumed, 
boolean autoClose), and start the handshake.  The internal code would 
still call matches() but only once.  If you want to be sure the 
internals select the ApplicationProtocol, just put in a permissive 
ApplicationProtocol.


The API is still more complicated unfortunately as ApplicationProtocol 
is still present, but the overall behavior is quite similar.



5.  Other failure mode/fallback.

In the new proposal, suppose you do set a single ALPN value in the 
application level, and the ServerHandshaker finds some other aspect of 
the handshake wasn't appropriate (creds were mentioned several times, 
but maybe a ciphersuite went dark due to new AlgorithmConstraints). 
This would cause the ServerHandshaker to fail and there's no way to go 
back to a different version unless you add a "for ALPN" loop into 
application.



6.  "Only one new method on SSLSocket/SSLEngine to set the protocol list 
(client) or selected single protocol (server)"

==
I think you would need two, "use this value on the next handshake" and 
"this was last negotiated/currently in effect."



One other comment:

Simone wrote:

> I don't know if all the blacklisted ciphers are actually lower
> strength of all the remaining ciphers,

Mainly it's about removing support for various algorithms.  e.g. static 
RSA/DH Key Exchange, DSA (draft-09), non-AEAD ciphers, customer DHE groups.


https://tlswg.github.io/tls13-spec/#rfc.section.1.2


Unfortunately, this is a big change in direction and is coming very 
late. I'm getting very significant pushback and schedule pressure from 
management now, so without an "Aha" moment, we may not be able to go 
this route.


Thanks,

Brad



On 9/28/2015 11:06 AM, Jason Greene wrote:



On Sep 25, 2015, at 3:22 PM, David M. Lloyd  wrote:

On 09/25/2015 02:11 PM, Simone Bordet wrote:

Hi,

On Fri, Sep 25, 2015 at 7:23 PM, David M. Lloyd  wrote:

The application protocol implementation chooses only valid cipher suites for
the protocol.  Why would it choose one that is not valid, considering that
the protocol implementation itself is the only thing that "knows" what is
valid or not?


The cipher could fail for the number of reasons it fails in
trySetCipherSuite(), even if the 

Re: TLS ALPN Proposal v5

[Bradford Wetmore]

You guys certainly were prolific in your discussions last night.  ;) 
Many comments to touch on, and I definitely won't have time today to 
respond to everything.


Xuelei wrote:
> I don't think we really need to re-order the cipher suites.

Simone wrote:
> Of course you need to re-order in this case.

In an iterative implementation like SunJSSE is currently, if you want to 
have the preference order of H2/H1, you need to try all of the 
H2-compatible ciphersuites first. Once you try a non-H2-compatible 
suite, the H2 matcher will fail, and it will then go to the H1 matcher, 
which will succeed.  This particular situation was discussed in RFC 7540.


> It might be not customers expected behavior to re-order/sort their
> preference of cipher suites or preference.

Are we are clear that the intention was never for the JDK to internally 
resort the ciphersuites, but rather to provide an external helper 
function (H2BLACKLISTCOMPARATOR) with which applications can do their 
own sorting and pass the results to setEnabledCiphersuite()?  I think 
maybe the confusion came from the 3 roles you describe later.


> If there are three roles, OpenJDK, application, customers, there are
> three result:

For JDK developers, the line between application/customer is quite 
blurry.  You/I are concerned about the interface between the OpenJDK and 
(application+customers).  The application folks will mainly be handling 
the customer configuration.


> {TLS_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384}

BTW, in case anyone was wondering, both of these suites are on the RFC 
7540 blacklist.


Simone wrote:

> for each cipher
>   for each application protocol
>   end
> end
>
> All the rest being equal, ciphers dominate application protocol
> selection.

Correct.  That's the current proposal.  It's a chicken/egg problem.  The 
KeyManager is part of the ciphersuite selection mechanims, and should 
have a proposed ALPN value value to use (ala RFC 7301), but the ALPN 
mechanism needs to have a ciphersuite in mind for it's calculation.



I will think over David's proposal over the weekend.  Much of the I/O 
required is already there using the JDK 1.8 method 
SSLSocketFactory.createSocket(Socket s, InputStream consumed, boolean 
autoClose), which was added for this very purpose.  There's nothing to 
do for SSLEngine.


Just a bit of history, we did consider a ClientHello parser when working 
on SNI. I don't remember the details as to why we didn't add it, but the 
SSLCapabilities/SSLExplorer classes in the JSSE sample code came from 
that attempt.  I have a vague recollection that the API was getting too 
complicated in the time we had.


More next week.

Have a good weekend, all.

Brad

Re: TLS ALPN Proposal v5

[Bradford Wetmore]

On 9/23/2015 2:33 AM, Simone Bordet wrote:

Hi,

On Wed, Sep 23, 2015 at 7:04 AM, Bradford Wetmore
<bradford.wetm...@oracle.com> wrote:



This new proposal still requires that ciphers are sorted in a way that
matches the ApplicationProtocol order.
Would be nice if, along with the HTTP/2 blacklist, there is a HTTP/2
comparator that sorts ciphers putting the blacklisted ones at the end.


Hm...is the sample code at the end of the initial class description
insufficient?  Adding a comparator seems a little heavyweight in that it
could require access to the ciphersuite internals and would add a lot of
complexity for this one known use case.  When TLSv1.3 is done, the blacklist
stuff in HTTP/2 goes away.


Sure, but until TLS 1.3 widely deployed, applications will have to
sort the ciphers to put HTTP/2 ones before the blacklisted ones.
Providing this comparator is as trivial as providing
ApplicationProtocol.HTTP2BLACKLIST, so I thought to mention it.


I learned something today: Collections/Arrays.sort()/others are stable. 
 (i.e. equal elements will not be reordered as a result of the sort) 
My expectation of "equals" was .equals(), not return value == 0.


I was concerned that such a Comparator might reorder valid H2 suites 
from what was passed in.  Thankfully, that's not the case, so I've added 
this Comparator.  There is a warning now about "consistency with 
equals()", as the Strings obvioulsy won't be equals() and thus sorted 
sets/maps just won't work.  (See the Comparator pages for further 
discussion.)



I also don't understand why there are 2 methods for the protocol name
? What value does it bring to have 2 methods for the same thing ?


Please see the IANA registry:

http://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids

for RFC 7301:

 http://www.rfc-editor.org/info/rfc7301

getProtocolName() is the IANA/IETF textual representation of the protocol
name (i.e. "Protocol" column), for example "HTTP/1.1", "SPDY/3", and "HTTP/2
over TLS".  I suppose toString() could be used instead, but thought it might
eventually output additional ALPN value state.  I don't have any concrete
plans at this point.

getNetworkSequence() is the identification sequence for the protocol (i.e.
"Identification Sequence" column), and represents the actual byte
identifiers that will travel the network in an ALPN extension.

 0x68 0x74 0x74 0x70 0x2f 0x31 0x2e 0x31 ("http/1.1")
 0x73 0x70 0x64 0x79 0x2f 0x33 ("spdy/3")
 0x68 0x32 ("h2")

When client wants to send the extension over the network, it grabs the
ApplicationProtocols values from the SSLParameters, then calls
getNetworkSequence() on each ApplicationProtocol to obtain the actual opaque
ProtocolName(1..2^8-1) to send.  Likewise on the server side, we match the
incoming active ALPN opaque values with the list of mutually agreeable ALPN
values.  And of course, send back the final selected value.


Sure, but application will have to implement two methods instead of
one, and AFAIU the JDK implementation is never calling
getProtocolName() since it's just a description for humans.


I think that a textual name will be better than:

// Output:  javax.net.ssl.ApplicationProtocol$1@1b9e1916

System.out.println(ApplicationProtocol.H2);

and there's no UTF-8 ambiguity.


I've updated the webrev to include an SSLSocket test variant, and added a
few more comments.

 http://cr.openjdk.java.net/~wetmore/8051498/webrev.14/

Hopefully things are more clear now.  Thanks for your review/comments.


I see now, thanks for the pointers !

Indulge me a bit more below on the Map passed as parameter to
ApplicationProtocol :)

IIUC, by the time we are executing the code that calls
ApplicationProtocol.match(), the TLS protocol is already chosen and
it's available in SSLSession.


Not necessarily.  This could also be called before the connection has 
even been attempted, say if the client wants to determine if the 
proposed protocols or protocol/ciphersuites it wants to use are even 
allowed by an ApplicationProtocol.



When remains is the transient value of cipher that is being chosen.
Because we already have modified the API to support the application
protocol transient value (by adding
SSLEngine.getHandshakeApplicationProtocol()) to be used by
KeyManagers, I was wondering if we cannot either:


CipherSuite is more of a Session value, so it should probably be part of 
the handshakeSSLSession.  We could set 
handshakeSSLSession.getCipherSuite() before calling the ALPN selector, 
or pass it in as part of the Map.


> A) add: String SSLEngine.getHandshakeCipherSuite(), to be used by
> ApplicationProtocol

That's kind of what we are doing already, but just in the 
ApplicationProtocol matcher instead so that it doesn't add extra methods 
to SSLSocket/SSLEngine.


And this doesn't really help the pre-connect

Re: TLS ALPN Proposal v5

[Bradford Wetmore]

> This new proposal still requires that ciphers are sorted in a way that
> matches the ApplicationProtocol order.
> Would be nice if, along with the HTTP/2 blacklist, there is a HTTP/2
> comparator that sorts ciphers putting the blacklisted ones at the end.

Hm...is the sample code at the end of the initial class description 
insufficient?  Adding a comparator seems a little heavyweight in that it 
could require access to the ciphersuite internals and would add a lot of 
complexity for this one known use case.  When TLSv1.3 is done, the 
blacklist stuff in HTTP/2 goes away.


> I don't like the first parameter of ApplicationProtocol.match() to be
> a Map;

I personally don't like the Map either, but for 
situations where we don't have a connection yet (e.g. trying to reduce 
the set of protocols and/or ciphersuites for setEnabledCiphersuites()), 
this worked.


Xuelei suggested the String->String map for future expansion, making it 
easier to add new parameters in between major releases if new ALPN 
protocols are introduced.  (For those who aren't aware, adding 
classes/methods/variables/etc. is pretty much forbidden in update 
(minor) releases, but simply adding a new string to a Map is generally OK.)


> I would prefer a full blown class at this
> point, that contains all the parameters, for example:
>
> inner class ApplicationProtocol.Info
> {
>  tlsProtocol
>  cipher
>  sslEngine
>  sessionIsResuming
>  etc.
> }

The other problem here is having a way to get at all the parameters of a 
connection.  Enumerating them all in such a class is essentially 
duplicating methods that are already currently available in the 
SSLSession (handshakeSession) + SSLSocket/SSLEngines classes.  Any 
future enhancements to SSLSocket/SSLEngine would have to be added here 
again in such a class.


> I also don't understand why there are 2 methods for the protocol name
> ? What value does it bring to have 2 methods for the same thing ?

Please see the IANA registry:


http://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids

for RFC 7301:

http://www.rfc-editor.org/info/rfc7301

getProtocolName() is the IANA/IETF textual representation of the 
protocol name (i.e. "Protocol" column), for example "HTTP/1.1", 
"SPDY/3", and "HTTP/2 over TLS".  I suppose toString() could be used 
instead, but thought it might eventually output additional ALPN value 
state.  I don't have any concrete plans at this point.


getNetworkSequence() is the identification sequence for the protocol 
(i.e. "Identification Sequence" column), and represents the actual byte 
identifiers that will travel the network in an ALPN extension.


0x68 0x74 0x74 0x70 0x2f 0x31 0x2e 0x31 ("http/1.1")
0x73 0x70 0x64 0x79 0x2f 0x33 ("spdy/3")
0x68 0x32 ("h2")

When client wants to send the extension over the network, it grabs the 
ApplicationProtocols values from the SSLParameters, then calls 
getNetworkSequence() on each ApplicationProtocol to obtain the actual 
opaque ProtocolName(1..2^8-1) to send.  Likewise on the server side, we 
match the incoming active ALPN opaque values with the list of mutually 
agreeable ALPN values.  And of course, send back the final selected value.


> RFC 7301 hints that the bytes to send over the network are the UTF-8
> bytes from that string.

Intentional, this ApplicationProtocol class and these two contained 
values eliminate that problem.  There is no UTF-8 to network byte 
conversion needed.  The network sequence is hard-coded into the 
ApplicationProtocol and is used directly as the network bytes.  The 
protocolName string is for user clarity.


> There are still a lot of details missing, such as where is the chosen
> ALPN value stored

In SSLSocket/SSLEngine.

If the connection is in the middle of a handshake and you need the 
handshake value:


getHandshakeApplicationProtocol()

Once the connection has finished handshaking, you can get the 
final/active value:


getApplicationProtocol()

The ALPN value is no longer in SSLSession, since it needs to be 
per-connection, not per session.


>  (and how it can be retrieved by the KeyManager, for
> example),

JSSE makes calls to X509KeyManagers (for SSLSockets) and 
X509ExtendedKeyManager (for SSLEngines).  The chooseServerAlias methods 
are passed SSLSocket/SSLEngines for the connection being negotiated.


As above, SSLSocket/SSLEngine now have:

getHandshakeApplicationProtocol();  // if handshaking
getApplicationProtocol();   // if not handshaking

So if chooseServerAlias is called, we're in the middle of a handshake, 
and can get the ALPN value thusly:


chooseClientAlias(String[] keyType,
Principal[] issuers, Socket socket) {

...deleted...

ApplicationProtocol alpn =
socket.getHandshakeApplicationProtocol();

...deleted...

> as well as the webrev not showing any real implementation,

Yes, 

TLS ALPN Proposal v5

[Bradford Wetmore]

Hi all,

On 9/7/2015 7:18 AM, Simone Bordet wrote:

On Mon, Sep 7, 2015 at 5:54 AM, Bradford Wetmore
<bradford.wetm...@oracle.com> wrote:



In general, if the ciphersuites aren't ordered properly, that should be
considered a configuration error on the part of the application(s). However,
this dynamic ALPN selection approach still allows for appropriate ALPN
values to be selected for each possible ciphersuite. That is, if the suites
were ordered H1/H2, the ALPN value should be H1.

For an example of such a selector, please see the new H2/H2+H1/H1
ApplicationProtocolSelector implementions in
ApplicationProtocolSelector.java, and how they are called in
ServerHandshaker.java.


Here is the latest:

http://cr.openjdk.java.net/~wetmore/8051498/webrev.09


Are you sure this is the latest code ?
The relevant changes in ServerHandshaker are commented out.


Yes, sorry for not making this clear.  We've had a lot of internal 
discussions about the API and how to implement it:  the comments in the 
implementation classes (sun.security.ssl) are primarily for Vinnie who 
has been working on the implementation.  But I thought they provide some 
explanation as to how things might be implemented internally so I left 
them in the webrev.



I have the feeling that ApplicationProtocolSelector (APS) is not
really an application protocol selector, but a cipher suite selector.


It was *(note past tense)* primarily an application protocol selector. 
The fact that it could also influence ciphersuite selection was an 
optimization to avoid using suites known not to work with a particular 
ALPN value, but were going to be attempted anyway (say due to a server 
misconfiguration).


However, that has changed as the API has taken a different direction.

You mentioned "session resumption" in your email, which reminded me that 
ALPN values should be attached to connections (i.e. 
SSLSocket/SSLEngine), not SSLSessions.  That simplified things in 
several areas, especially in the Protocol Selector.


We no longer have an ApplicationProtocolSelector, but rather an 
interface called ApplicationProtocol.  These are concrete 
implementations of each ALPN/NPN value, and are responsible for 
examining the state of the connection so far and to return true if the 
conditions are right.   These instances are passed as 
List to SSLParameters and then to 
SSLSocket/SSLEngines.  We have a similar but somewhat simpler server 
selection algorithm than what was described earlier:


Select TLS version

for each ciphersuite

load ALPN query with:
proposed TLS version + proposed ciphersuite
SSLSocket/SSLEngine + handshaking state

for each ALPN value
if (ALPN.matches())
temporarily assign ALPN to the
handshaking SSLSocket/SSLEngine

approve ciphersuite
calls KeyManager for alias
(can use ALPN value above)
checks cipher/MAC restrictions, etc
break;

if no errors
assign ciphersuite to handshakeSession
assign ALPN to SSLSocket/SSLEngine
assign handshakeSesion as the actual Session
to SSLSocket/SSLEngine


APS seem to conflate cipher suite selection with application protocol
selection: the return value of select() actually is a 2-tuple that
means (ciphers[0], protocol) so that returning the empty string or
null does not have any effect on protocol selection, but only on
cipher selection.


To restate the point I think you raising, "Should the ALPN selection 
mechanism influence the TLS protocol version selection mechanism?"


We had a similar discussion internally.

Why would we want to encourage/enable the selection of downrev'd TLS 
protocol versions based on ALPN values?  If we actually support the 
higher/stronger TLS version, IMHO we should be using them.


1.  I think the stronger security benefits from the later protocols 
should always take priority over ALPN values and that we shouldn't be 
encouraging downrev'ing at all. For example, if there is a 
TLSv1.2/1.1/1.0 server, and we negotiated down to 1.0 just to support 
some ALPN value (or due to a bug in the selector), we lose GCM (v1.2) 
and the explicit IV (v1.1). ALPN values are application values, which I 
would expect to not be as security sensitive as the protocol values.


2.  The server hello's protocol version is supposed to be the lower of 
that suggested by the client in the client hello and the highest 
supported by the server.  Returning a lower value just to satisfy an 
ALPN selection would indicate to anyone listening (proxy, other 
connection from same VM?) that the higher TLS values are not supported 
on this server, which I think is incorrect. Future connections to this 
server might cache this lower protocol value and lose the stronger 
protocols.


3. I would expect that "older" ALPN values will likely still be 
supported in later versions of th

TLS ALPN Proposal v4

[Bradford Wetmore]

Hi all,

Simone/Xuelei/I wrote:

Per my understanding, application protocol should be negotiated before
>>>cipher suite and protocol version negotiated.

>>
>>This is not possible for HTTP/2.
>>Application protocol negotiation MUST happen*after*  the TLS protocol
>>and the TLS cipher are negotiated.

>
>Yes, that's my understanding as well.

What are the behaviors of other vendors?  Can we ask for a clarification
from both HTTP/2 and TLS WG?


and then Simone wrote:

I support Xuelei in that you should ask confirmation to the HTTP/2
editor.


Thanks for the encouragement, Simone.  As you've probably heard, I did 
contact the IETF HTTP/2 working group and posed several questions about 
expected usage[1].  I'm combining those responses plus several other 
points that came up since the last discussion.  This may be repeating 
some things already said, but serves as useful background info for those 
here that might not be following closely:


1.  HTTP/2 (aka H2) and TLSv1.3 were developed in parallel.  The H2 
designers wanted the ciphersuite restrictions in the proposed TLSv1.3. 
But since TLSv1.3 wasn't ready, they compromised by allowing TLSv1.2 to 
be used but with the restriction that only those ciphersuites that were 
expected/allowed for TLSv1.3 could be used.  That is, there is a 
blacklist of ciphersuites for TLSv1.2:  if a suite is present, it can't 
be used in TLSv1.2.  (e.g. no RSA KeyExchange, no CBC-based block 
ciphers, no stream ciphers, etc.)


2. RFC 7301 says in a couple places that it would be advantageous to 
have the chosen ALPN value used by the certificate selection mechanism 
(See sections 1 & 4).  Without a radical rewrite of the current 
selection mechanism (n-tuple), that means ALPN selection should be done 
before the ciphersuite is selected.  We could also check after, but I 
have a different approach (see below).


3. From the H2 working group discussion, a server instance will very 
likely support both H2 and legacy HTTP/1.1. This means for servers that 
prefer H2, any iterative cipher selection mechanisms needs to try the 
H2-specific ciphersuites first, then legacy non-H2 suites.  That is, the 
suites must be ordered appropriately so that the ciphersuite selection 
mechanism won't attempt a blacklisted suite before exhausting all 
H2-acceptable suites.  This ordering can be requested today in JSSE by 
the server calling sslParameters.setUseCipherSuitesOrder(true).  This 
particular point won't matter when TLSv1.3 is in play, as we wouldn't 
try those suites at all.


4. Clients may not know whether a server will be H2 or HTTP/1.1, so they 
should also appropriately sort ciphersuites based on their ALPN 
preferences.  (H2 first, H1 second.)


5. For our SunJSSE, while I think our current enabled list order is 
generally ok, we should probably reorder the ciphersuite priorities so 
that the TLSv1.3 acceptable suites are up front, with the others 
following. This prefers forward secrecy ciphersuites to our current 
ordering. I am thinking we should probably do this for JDK 9, and maybe 
backport as well. The current webrev (link below) doesn't have this yet.


6.  To avoid downgrade attacks, applications should not provide for a 
fallback mechanism.  This includes ALPN selection.


Connection#1:  {"h2", "http/1.1"} // Don't make two connections.
Connection#2:  {"http/1.1"}

POODLE was a good example where allowing fallbacks bit hard.

Of course, we can't control this at the JSSE layer, it's the application 
layer responsibility.



Tradeoff between A) change radically the OpenJDK implementation to
support an iterative processing of TLS protocols, extensions (all of
them), ciphers, aliases, etc. that would be future proof (if that is
even possible) and B) hack in just enough of what is needed to support
H2 today (for which we already have working examples) ?


Given where we are now schedule wise (integration currently due at the 
end of September), and that SunJSSE is such an iterative implementation, 
coming up with a multi-selector API is likely beyond what we can do at 
this point.


(webrev link below).

IMHO, the following works well.  I've added a new method that contains 
the ordered list of ciphersuites still to be tried which is a hint for 
ALPN selection, but we delay the actual ciphersuite selection until 
after the ALPN value is chosen.  So the algorithm is now:


0.  Applications (especially server) might order suites with h2
first for TLSv1.2. sslParameters.setUseCipherSuitesOrder(true)
should be called on the server to ensure those suites are
tried first.

1.  Start Handshake.

2.  Internal server handshaker chooses the TLS version.

3.  The internal server handshaker finds the client/server/protocol
version intersection of the suites, loads the initial ordered
list into a new method on a SSLSession (obtained by the
getHandshakeSSLSession()), then iterates through the
ordered list of 

Re: Code review request, JDK-8040062 Need to add new methods in BaseSSLSocketImpl

[Bradford Wetmore]

There is a similar one for HttpsURLConnection, as we had problems in the 
past where the networking changes didn't update the SSL code.


jdk/sun/net/www/protocol/https/HttpsURLConnection/CheckMethods.java

Just a reminder to please include the JSSE reg tests when making 
Socket/HttpsURLConnection/etc. API changes or when changing code that is 
common to these methods.  Thankfully we were running it anyway and was 
caught quickly.


Thanks,

Brad





On 4/14/2014 6:45 AM, Chris Hegarty wrote:

+1 . Thanks for updating this Xuelei. Nice test that caught this.

-Chris.

On 14/04/14 13:59, Sean Mullan wrote:

Looks fine to me. Can you add a relates to link to JDK-8036979 and an
appropriate noreg label?

--Sean

On 04/14/2014 06:04 AM, Xuelei Fan wrote:

Hi,

Please review the fix for JDK-8040062:

 http://cr.openjdk.java.net/~xuelei/8040062/webrev.00/

In JDK-8036979, there are news methods are added to java.net.Socket.
These methods need to be overridden in SSL socket implementation,
sun/security/ssl/BaseSSLSocketImpl.java.

No new regression test.  The existing test:

sun/security/ssl/SSLSocketImpl/CheckMethods.java

can be used to verify this fix.

Thanks,
Xuelei

Re: RFR [9] 8038821: Fix typo; consructed to constructed

[Bradford Wetmore]

Chris/Ivan's changes look good to me.

Brad


On 3/31/2014 7:59 AM, Chris Hegarty wrote:

Thanks Ivan, I'll add them.

-Chris.

On 31/03/14 15:48, Ivan Gerasimov wrote:

Hi Chris!

Would it be good to include a couple more typo fixes in this change?

diff --git a/src/share/classes/java/net/DatagramSocket.java
b/src/share/classes/java/net/DatagramSocket.java
--- a/src/share/classes/java/net/DatagramSocket.java
+++ b/src/share/classes/java/net/DatagramSocket.java
@@ -445,7 +445,7 @@
   *
   * p If given an {@link InetSocketAddress InetSocketAddress},
this method
   * behaves as if invoking {@link #connect(InetAddress,int)
connect(InetAddress,int)}
- * with the the given socket addresses IP address and port number.
+ * with the given socket addresses IP address and port number.
   *
   * @param   addrThe remote address.
   *
diff --git a/src/share/classes/java/net/InetAddress.java
b/src/share/classes/java/net/InetAddress.java
--- a/src/share/classes/java/net/InetAddress.java
+++ b/src/share/classes/java/net/InetAddress.java
@@ -159,7 +159,7 @@
   * dl
   * dtbnetworkaddress.cache.ttl/b/dt
   * ddIndicates the caching policy for successful name lookups from
- * the name service. The value is specified as as integer to indicate
+ * the name service. The value is specified as an integer to indicate
   * the number of seconds to cache the successful lookup. The default
   * setting is to cache for an implementation specific period of time.
   * p
@@ -167,7 +167,7 @@
   * /dd
   * dtbnetworkaddress.cache.negative.ttl/b (default: 10)/dt
   * ddIndicates the caching policy for un-successful name lookups
- * from the name service. The value is specified as as integer to
+ * from the name service. The value is specified as an integer to
   * indicate the number of seconds to cache the failure for
   * un-successful lookups.
   * p

Sincerely yours,
Ivan


On 31.03.2014 17:42, Chris Hegarty wrote:

Trivial typo fix.

diff --git a/src/share/classes/java/net/HttpCookie.java
b/src/share/classes/java/net/HttpCookie.java
--- a/src/share/classes/java/net/HttpCookie.java
+++ b/src/share/classes/java/net/HttpCookie.java
@@ -74,7 +74,7 @@
 private boolean httpOnly;   // HttpOnly ... i.e. not accessible
to scripts
 private int version = 1;// Version=1 ... RFC 2965 style

-// The original header this cookie was consructed from, if it was
+// The original header this cookie was constructed from, if it was
 // constructed by parsing a header, otherwise null.
 private final String header;

@@ -985,7 +985,7 @@
 }

 /*
- * Returns the original header this cookie was consructed from,
if it was
+ * Returns the original header this cookie was constructed from,
if it was
  * constructed by parsing a header, otherwise null.
  */
 private String header() {
diff --git a/src/share/classes/sun/misc/JavaNetHttpCookieAccess.java
b/src/share/classes/sun/misc/JavaNetHttpCookieAccess.java
--- a/src/share/classes/sun/misc/JavaNetHttpCookieAccess.java
+++ b/src/share/classes/sun/misc/JavaNetHttpCookieAccess.java
@@ -36,7 +36,7 @@
 public ListHttpCookie parse(String header);

 /*
- * Returns the original header this cookie was consructed from,
if it was
+ * Returns the original header this cookie was constructed from,
if it was
  * constructed by parsing a header, otherwise null.
  */
 public String header(HttpCookie cookie);

hg: jdk8/tl/jdk: 8027526: CheckTipsAndVersions.java failing occasionally

[bradford . wetmore]

Changeset: f731d096530f
Author:wetmore
Date:  2013-10-30 16:49 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f731d096530f

8027526: CheckTipsAndVersions.java failing occasionally
Reviewed-by: mullan, mchung

! test/java/security/Signature/SignatureGetAlgorithm.java

hg: jdk8/tl/jdk: 8026762: jdk8-tl builds windows builds failing in corba - javac: no source files

[bradford . wetmore]

Changeset: a45acc8de0f3
Author:wetmore
Date:  2013-10-16 23:32 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a45acc8de0f3

8026762: jdk8-tl builds windows builds failing in corba - javac: no source files
Reviewed-by: katleman, dholmes

! makefiles/GenerateClasses.gmk

hg: jdk8/tl/corba: 8026762: jdk8-tl builds windows builds failing in corba - javac: no source files

[bradford . wetmore]

Changeset: 1a71d800b032
Author:wetmore
Date:  2013-10-16 23:31 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/corba/rev/1a71d800b032

8026762: jdk8-tl builds windows builds failing in corba - javac: no source files
Reviewed-by: katleman, dholmes

! makefiles/BuildCorba.gmk

hg: jdk8/tl/jdk: 8025694: Rename getStrongSecureRandom based on feedback; ...

[bradford . wetmore]

Changeset: a6ac824b463d
Author:wetmore
Date:  2013-10-02 09:38 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a6ac824b463d

8025694: Rename getStrongSecureRandom based on feedback
8014838: getStrongSecureRandom() should require at least one implementation
Reviewed-by: mullan, darcy

! src/share/classes/java/security/SecureRandom.java
! src/share/lib/security/java.security-windows
! test/sun/security/provider/SecureRandom/StrongSecureRandom.java

hg: jdk8/tl/jdk: 8019341: Update CookieHttpsClientTest to use the newer framework.

[bradford . wetmore]

Changeset: 11c15607e43f
Author:wetmore
Date:  2013-07-05 18:22 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/11c15607e43f

8019341: Update CookieHttpsClientTest to use the newer framework.
Reviewed-by: xuelei, smarks, michaelm

! 
test/sun/security/ssl/sun/net/www/protocol/https/HttpsURLConnection/CookieHttpsClientTest.java
! test/sun/security/ssl/templates/SSLEngineTemplate.java
! test/sun/security/ssl/templates/SSLSocketSSLEngineTemplate.java
! test/sun/security/ssl/templates/SSLSocketTemplate.java

hg: jdk8/tl/jdk: 8012530: test/sun/security/provider/SecureRandom/StrongSeedReader.java failing

[bradford . wetmore]

Changeset: b600d637ef77
Author:wetmore
Date:  2013-04-25 17:10 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b600d637ef77

8012530: test/sun/security/provider/SecureRandom/StrongSeedReader.java failing
Reviewed-by: wetmore
Contributed-by: alan.bate...@oracle.com

! test/sun/security/provider/SecureRandom/StrongSeedReader.java

hg: jdk8/tl/jdk: 7197071: Makefiles for various security providers aren't including the default manifest

[bradford . wetmore]

Changeset: 940c8cc5a5c4
Author:wetmore
Date:  2012-10-23 12:36 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/940c8cc5a5c4

7197071: Makefiles for various security providers aren't including the default 
manifest
Reviewed-by: valeriep, mullan, katleman

! make/com/oracle/security/ucrypto/Makefile
! make/javax/crypto/Defs-jce.gmk
! make/sun/security/ec/Makefile
! make/sun/security/mscapi/Makefile
! make/sun/security/pkcs11/Makefile
+ test/javax/crypto/sanity/CheckManifestForRelease.java

hg: jdk8/tl/jdk: 7177556: Put TestProviderLeak.java on the ProblemList until test can be reworked

[bradford . wetmore]

Changeset: 8e5635ded425
Author:wetmore
Date:  2012-06-15 17:42 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8e5635ded425

7177556: Put TestProviderLeak.java on the ProblemList until test can be reworked
Reviewed-by: khazra

! test/ProblemList.txt

hg: jdk8/tl/jdk: 7167362: SecureRandom.init should be converted, amendment to 7084245

[bradford . wetmore]

Changeset: 6438f1277df6
Author:wetmore
Date:  2012-05-09 16:33 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/6438f1277df6

7167362: SecureRandom.init should be converted, amendment to 7084245
Reviewed-by: sherman

! src/share/classes/sun/security/provider/SecureRandom.java

hg: jdk8/tl/jdk: 7157903: JSSE client sockets are very slow

[bradford . wetmore]

Changeset: 10480cf00dcd
Author:wetmore
Date:  2012-04-11 17:12 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/10480cf00dcd

7157903: JSSE client sockets are very slow
Reviewed-by: xuelei

! src/share/classes/sun/security/ssl/AppOutputStream.java
! src/share/classes/sun/security/ssl/EngineOutputRecord.java
! src/share/classes/sun/security/ssl/OutputRecord.java
! src/share/classes/sun/security/ssl/SSLSocketImpl.java

hg: jdk8/tl/jdk: 7141910: Incorrect copyright dates on new test cases.

[bradford . wetmore]

Changeset: 55a82eba1986
Author:wetmore
Date:  2012-02-01 16:00 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/55a82eba1986

7141910: Incorrect copyright dates on new test cases.
Reviewed-by: mullan

! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/EngineArgs/DebugReportsOneExtraByte.java
! 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/EngineArgs/DebugReportsOneExtraByte.sh

hg: jdk8/tl/jdk: 7126889: Incorrect SSLEngine debug output

[bradford . wetmore]

Changeset: 5ee30ab905db
Author:wetmore
Date:  2012-01-26 17:16 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/5ee30ab905db

7126889: Incorrect SSLEngine debug output
Reviewed-by: xuelei

! src/share/classes/sun/security/ssl/EngineArgs.java
! src/share/classes/sun/security/ssl/SSLEngineImpl.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/EngineArgs/DebugReportsOneExtraByte.java
+ 
test/sun/security/ssl/com/sun/net/ssl/internal/ssl/EngineArgs/DebugReportsOneExtraByte.sh

hg: jdk7/tl/jdk: 7031343: Provide API changes to support future GCM AEAD ciphers

[bradford . wetmore]

Changeset: 13af7c12c62a
Author:wetmore
Date:  2011-04-13 11:59 -0700
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/13af7c12c62a

7031343: Provide API changes to support future GCM AEAD ciphers
Reviewed-by: valeriep, xuelei

+ src/share/classes/javax/crypto/AEADBadTagException.java
! src/share/classes/javax/crypto/Cipher.java
! src/share/classes/javax/crypto/CipherSpi.java
+ src/share/classes/javax/crypto/spec/GCMParameterSpec.java
+ test/javax/crypto/Cipher/GCMAPI.java
+ test/javax/crypto/spec/GCMParameterSpec/GCMParameterSpecTest.java

hg: jdk7/tl/jdk: 6844879: Source distribution should be more robustly built without the security source distribution

[bradford . wetmore]

Changeset: 0f0d6b8f98cc
Author:wetmore
Date:  2011-02-23 22:54 -0800
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/0f0d6b8f98cc

6844879: Source distribution should be more robustly built without the security 
source distribution
Reviewed-by: ohair

! make/common/shared/Defs-java.gmk

hg: jdk7/tl/jdk: 6945604: wrong error message in CardImpl.java

[bradford . wetmore]

Changeset: d8ad2da3ecf3
Author:wetmore
Date:  2010-04-20 14:24 -0700
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/d8ad2da3ecf3

6945604: wrong error message in CardImpl.java
Reviewed-by: mullan

! src/share/classes/sun/security/smartcardio/CardImpl.java

hg: jdk7/tl: 6872177: JCE framework and provider builds broken following -target 7 changes

[bradford . wetmore]

Changeset: d8b49b53d8cf
Author:wetmore
Date:  2009-08-14 17:29 -0700
URL:   http://hg.openjdk.java.net/jdk7/tl/rev/d8b49b53d8cf

6872177: JCE framework and provider builds broken following -target 7 changes
Reviewed-by: ohair

! make/Defs-internal.gmk

hg: jdk7/tl/jdk: 2 new changesets

[bradford . wetmore]

Changeset: 8c0c96a3f9f6
Author:wetmore
Date:  2009-08-13 12:36 -0700
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/8c0c96a3f9f6

6870335: Provider numbers need to be bumped to 1.7
Reviewed-by: mullan

! src/share/classes/com/sun/security/sasl/Provider.java
! src/share/classes/sun/security/jgss/SunProvider.java
! src/share/classes/sun/security/provider/Sun.java
! src/share/classes/sun/security/smartcardio/SunPCSC.java
! src/share/classes/sun/security/ssl/SunJSSE.java

Changeset: 6797a2407a50
Author:wetmore
Date:  2009-08-13 12:37 -0700
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/6797a2407a50

Merge