Re: Code Review Request: TLS 1.3 Implementation

2018-06-25 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/32a737f51e37 Xuelei On 6/18/2018 8:48 AM, Xuelei Fan wrote: CertificateMessage.java X509TrustManagerImpl.java X509KeyManagerImpl.java - These implementation has not consider the impact of RSASSA-PSS key type. Xuelei O

Re: Code Review Request: TLS 1.3 Implementation

2018-06-22 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/63ab0dfe3dbb Except the issues in this thread, this update also include changes to set signature parameters after key initialization, so that the provider of Signature instance is the same as its key provider if needed. We have fixed the SunM

Re: Code Review Request: TLS 1.3 Implementation

2018-06-22 Thread Xuelei Fan
All good catches! I will push the changeset soon after the testing. On 6/22/2018 11:18 AM, Jamil Nimeh wrote: * DHKeyExchange.java o 177-192: Am I missing something or does isRecovering get defined as false and never gets set to true within the lifetime of the variable? 

Re: Code Review Request: TLS 1.3 Implementation

2018-06-22 Thread Jamil Nimeh
* DHKeyExchange.java o 177-192: Am I missing something or does isRecovering get defined as false and never gets set to true within the lifetime of the variable?  Do we still need this? o 178: Nit/typo: "recove" --> "recover" o 207-210: Catching Exception seems really

Re: Code Review Request: TLS 1.3 Implementation

2018-06-21 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/76025c6c6e29 On 6/21/2018 1:10 PM, Valerie Peng wrote: > - line 175, the exception message seems not right, clientAlias is a > private key entry with no cert chain stored. Good catch! > - line 56, instead of returning null, I wonder if we should

Re: Code Review Request: TLS 1.3 Implementation

2018-06-21 Thread Valerie Peng
- line 175, the exception message seems not right, clientAlias is a private key entry with no cert chain stored. - line 56, instead of returning null, I wonder if we should throw exception to catch unsupported type early. That's all. Valerie On 5/25/2018 4:45 PM, Xuelei Fan wrote: Hi, I

Re: Code Review Request: TLS 1.3 Implementation

2018-06-20 Thread Valerie Peng
Hi Xuelei, - line 2, why is the copyright year changed from 2015 only to 2003 and 2018? Don't we normally preserve the first year and only update/add the second year? - line 110, instead of erroring out, I wonder if it's better to call createPossessions(handshakeContext) and only error out if

Re: Code Review Request: TLS 1.3 Implementation

2018-06-20 Thread Valerie Peng
On 6/20/2018 9:58 AM, Xuelei Fan wrote: On 6/19/2018 7:07 PM, Valerie Peng wrote: Hi Xuelei, These look fine. - kind of strange to see SSLKeyAgreement extends SSLKeyAgreementGenerator... Normally, the naming convention implies one generates the other. The name should be more

Re: Code Review Request: TLS 1.3 Implementation

2018-06-20 Thread Xuelei Fan
On 6/19/2018 7:07 PM, Valerie Peng wrote: Hi Xuelei, These look fine. - kind of strange to see SSLKeyAgreement extends SSLKeyAgreementGenerator... Normally, the naming convention implies one generates the other. The name should be more like SSLKeyAgreementKeyDerivationGenerator. I

Re: Code Review Request: TLS 1.3 Implementation

2018-06-20 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/1cc2f6afa943 On 6/19/2018 12:19 PM, Valerie Peng wrote: Hi Xuelei, Just questions and some nits. looks good looks good - line 98: for unsupported digest, maybe we should consider throw exceptions. It's easier to find out where the suppor

Re: Code Review Request: TLS 1.3 Implementation

2018-06-19 Thread Valerie Peng
Hi Xuelei, These look fine. - kind of strange to see SSLKeyAgreement extends SSLKeyAgreementGenerator... Normally, the naming convention implies one generates the other. - same method name as in SSLKeyDerivationGenerator. I assume that this is intentional as both are meant to derive

Re: Code Review Request: TLS 1.3 Implementation

2018-06-19 Thread Valerie Peng
Hi Xuelei, Just questions and some nits. looks good looks good - line 98: for unsupported digest, maybe we should consider throw exceptions. It's easier to find out where the support needs to be added if we fail early. - Just wondering why we need difference classes of SecretKeyDeriva

Re: Code Review Request: TLS 1.3 Implementation

2018-06-19 Thread Xuelei Fan
ECPointFormatsExtension.java PredefinedDHParameterSpecs.java SSLAlgorithmConstraints.java SSLAlgorithmDecomposer.java --- Looks fine to me, except the copyright year of PredefinedDHParameterSpecs and SSLAlgorithmDecomposer. Updated in the following changeset. http:

Re: Code Review Request: TLS 1.3 Implementation

2018-06-18 Thread Xuelei Fan
On 6/18/2018 12:28 PM, Valerie Peng wrote: Hi Xuelei, Looks fine. -line 39, I didn't find info on extended master secret extension in RFC 5746? It looks like the original comment which refers to RFC7627 "Transport Layer Security (TLS) Session Hash anf Extended Master Secret Extension" ma

Re: Code Review Request: TLS 1.3 Implementation

2018-06-18 Thread Valerie Peng
Hi Xuelei, Looks fine. -line 39, I didn't find info on extended master secret extension in RFC 5746? It looks like the original comment which refers to RFC7627 "Transport Layer Security (TLS) Session Hash anf Extended Master Secret Extension" matches better. Looks fine. Valerie ** On 5/

Re: Code Review Request: TLS 1.3 Implementation

2018-06-18 Thread Xuelei Fan
CertificateMessage.java X509TrustManagerImpl.java X509KeyManagerImpl.java - These implementation has not consider the impact of RSASSA-PSS key type. Xuelei On 6/8/2018 10:21 AM, Xuelei Fan wrote: Here is the 3rd full webrev:    http://cr.openjdk.java.net/~xuelei/819658

Re: Code Review Request: TLS 1.3 Implementation

2018-06-15 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/125890684a60 Supports supported_versions extension for TLS 1.2 and prior versions. Xuelei On 6/15/2018 7:56 AM, Xuelei Fan wrote: SSLExtension.java - The "supported_versions" can be used in TLS 1.2 ClientHello, per the specifi

Re: Code Review Request: TLS 1.3 Implementation

2018-06-15 Thread Xuelei Fan
On 6/15/2018 9:27 AM, Anthony Scarpino wrote: On 06/15/2018 06:53 AM, Xuelei Fan wrote: SSLCipher.java -- In the implementation, the key usage impacts the write side only.   I think the read side should also limit the key usage. I remember it being discussed many many months ago w

Re: Code Review Request: TLS 1.3 Implementation

2018-06-15 Thread Anthony Scarpino
On 06/15/2018 06:53 AM, Xuelei Fan wrote: SSLCipher.java -- In the implementation, the key usage impacts the write side only.   I think the read side should also limit the key usage. I remember it being discussed many many months ago we at first planned to do the read side, but th

Re: Code Review Request: TLS 1.3 Implementation

2018-06-15 Thread Xuelei Fan
SSLExtension.java - The "supported_versions" can be used in TLS 1.2 ClientHello, per the specification: Implementations of TLS 1.3 which choose to support prior versions of TLS SHOULD support TLS 1.2. Servers MUST be prepared to receive ClientHellos that include this e

Re: Code Review Request: TLS 1.3 Implementation

2018-06-15 Thread Xuelei Fan
SSLCipher.java -- In the implementation, the key usage impacts the write side only. I think the read side should also limit the key usage. The crypto limit issues applies to TLS 1.2 and prior versions as well. Xuelei On 6/8/2018 10:21 AM, Xuelei Fan wrote: Here is the 3rd full

Re: Code Review Request: TLS 1.3 Implementation

2018-06-13 Thread Xuelei Fan
AlpnExtension.java -- line 145-153, 354-365 If the extension is not available, the applicationProtocol is not set to empty. Update: http://hg.openjdk.java.net/jdk/sandbox/rev/d0728b0f98f9 Xuelei On 6/8/2018 10:21 AM, Xuelei Fan wrote: Here is the 3rd full webrev:    http://

Re: Code Review Request: TLS 1.3 Implementation

2018-06-12 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/2b4ae319412b This update enable RSASSA-PSS signature algorithm for TLS 1.2, and improve the SSLSocket close implementation. Xuelei On 6/8/2018 10:21 AM, Xuelei Fan wrote: Here is the 3rd full webrev:    http://cr.openjdk.java.net/~xuelei/81

Re: Code Review Request: TLS 1.3 Implementation

2018-06-12 Thread Xuelei Fan
On 6/12/2018 5:43 PM, Bradford Wetmore wrote: I really don't have much to say about SSLEngineImpl.  I like the refactoring you've done.  Just some nits. Ignore my previous suggestion about conContext, after seeing it a bit more often, it makes sense now. SSLSocketImpl.java --

Re: Code Review Request: TLS 1.3 Implementation

2018-06-12 Thread Bradford Wetmore
I really don't have much to say about SSLEngineImpl. I like the refactoring you've done. Just some nits. Ignore my previous suggestion about conContext, after seeing it a bit more often, it makes sense now. SSLSocketImpl.java -- 72: I noticed you removed the public here fro

Re: Code Review Request: TLS 1.3 Implementation

2018-06-12 Thread Xuelei Fan
On 6/12/2018 3:29 PM, Bradford Wetmore wrote: On 6/11/2018 6:36 PM, Xuelei Fan wrote: SSLServerSocketImpl.java 160:  You should allow multiple changes between server to client, and switch enabled protocols/ciphersuites accordingly. Yes, multiple changes are allowed

Re: Code Review Request: TLS 1.3 Implementation

2018-06-12 Thread Bradford Wetmore
On 6/11/2018 6:36 PM, Xuelei Fan wrote: SSLServerSocketImpl.java 160:  You should allow multiple changes between server to client, and switch enabled protocols/ciphersuites accordingly. Yes, multiple changes are allowed. I didn't see a change here.  If you go fro

Re: Code Review Request: TLS 1.3 Implementation

2018-06-12 Thread Bradford Wetmore
I see what you're doing, but I'm not understanding why we're not checking the other direction also? e.g.  if (p < SSLv3 && not SSLv2) || (p > TLSv1.3 ) { Per TLS 1.2 version negotiation specification, the higher number is not limited.  For example, if client request for TLS 1.9, the server

Re: Code Review Request: TLS 1.3 Implementation

2018-06-12 Thread Valerie Peng
I see. Maybe document what you explained for these flags would help. Update looks good. Thanks, Valerie On 6/11/2018 5:23 PM, Xuelei Fan wrote: Update: http://hg.openjdk.java.net/jdk/sandbox/rev/0811eaea3cd4 On 6/11/2018 3:44 PM, Valerie Peng wrote: Hi Xuelei, - If no child class intended

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/ae0cd8b2e2c2 Xuelei On 6/11/2018 6:36 PM, Xuelei Fan wrote: On 6/11/2018 5:56 PM, Bradford Wetmore wrote: <...skipped...> 262:  What is the point of the aliases argument in the constructor? Was the idea to provide a mapping between suites we

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Xuelei Fan
On 6/11/2018 6:43 PM, Weijun Wang wrote: I was also thinking about the name. Why don't we always make the enum field identical to the name (including the unsupported ones)? Then we don't need a name property and valueOf() automagically works. Hm, looks like we can do this way as the names are

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Weijun Wang
I was also thinking about the name. Why don't we always make the enum field identical to the name (including the unsupported ones)? Then we don't need a name property and valueOf() automagically works. --Max > On Jun 12, 2018, at 9:36 AM, Xuelei Fan wrote: > > > > On 6/11/2018 5:56 PM, Brad

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Xuelei Fan
On 6/11/2018 5:56 PM, Bradford Wetmore wrote: <...skipped...> 262:  What is the point of the aliases argument in the constructor? Was the idea to provide a mapping between suites we originally created with the SSL_ prefix vs the more current TLS_ prefix we used in the later TLS protocols?  T

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Bradford Wetmore
CipherSuite.java 74:  There is a mix of ciphersuite initialization styles, which I find confusing. >>> These two suites may not be the best comparison, since you have a non-AEAD stream cipher up against AES-GCM.  For the latter, I think M_MULL in the MacAlg field is the r

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Xuelei Fan
On 6/11/2018 5:17 PM, Bradford Wetmore wrote: On 6/8/2018 7:45 PM, Xuelei Fan wrote:  > BaseSSLSocketImpl.java  > -- Update: http://hg.openjdk.java.net/jdk/sandbox/rev/bad9f4c7eeec Update to handle EOFException and requireCloseNotify property. Couple new typo nits: 588/

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Xuelei Fan
On 6/11/2018 5:14 PM, Bradford Wetmore wrote: ProtocolVersion.java Later down, you mention DTLS 1.3 quite a bit (e.g. PROTOCOLS_TO_13), is this going to cause any adverse reactions since there is no DTLS 1.3 implementation yet? I removed DTLS 1.3 as we don't actually

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/0811eaea3cd4 On 6/11/2018 3:44 PM, Valerie Peng wrote: Hi Xuelei, - If no child class intended, class may be made final. Updated. - line 99-101 and 120-133: delete as the comments said so? It is used for interop testing right now. I will

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Bradford Wetmore
On 6/8/2018 7:45 PM, Xuelei Fan wrote: > BaseSSLSocketImpl.java > -- Update: http://hg.openjdk.java.net/jdk/sandbox/rev/bad9f4c7eeec Update to handle EOFException and requireCloseNotify property. Couple new typo nits: 588/843/: // if no exceptio thrown -> // if no exc

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Bradford Wetmore
ProtocolVersion.java Later down, you mention DTLS 1.3 quite a bit (e.g. PROTOCOLS_TO_13), is this going to cause any adverse reactions since there is no DTLS 1.3 implementation yet? I removed DTLS 1.3 as we don't actually support it right now. Ok. Too late now, bu

Re: Code Review Request: TLS 1.3 Implementation

2018-06-11 Thread Valerie Peng
Hi Xuelei, - If no child class intended, class may be made final. - line 99-101 and 120-133: delete as the comments said so? - I have some doubt on line 191 and 198, does "noSniExtension/Matcher" means "no SNI Extension/Matcher"? If yes, it seems that the condition should be    if (serverNa

Re: SSLContextImpl.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-11 Thread Xuelei Fan
On 6/11/2018 7:59 AM, Weijun Wang wrote: On Jun 11, 2018, at 10:32 PM, Xuelei Fan wrote: The protocols (for example, SSLParameters::getProtocols) are now from new to old, which is opposite from the previous order. Why make this change? You didn't answer this. I missed this one. The

Re: SSLContextImpl.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-11 Thread Weijun Wang
> On Jun 11, 2018, at 10:32 PM, Xuelei Fan wrote: > >> The protocols (for example, SSLParameters::getProtocols) are now from new to >> old, which is opposite from the previous order. Why make this change? You didn't answer this. >> 41 * Instances of this class are immutable after the con

Re: SSLContextImpl.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-11 Thread Xuelei Fan
On 6/11/2018 7:36 AM, Weijun Wang wrote: On Jun 11, 2018, at 10:32 PM, Xuelei Fan wrote: Another thing in CipherSuite.java: 803 static CipherSuite nameOf(String ciperSuiteName) { Why isn't this method named valueOf()? There is a default builtin valueOf(String) method for enum type.

Re: SSLContextImpl.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-11 Thread Weijun Wang
> On Jun 11, 2018, at 10:32 PM, Xuelei Fan wrote: > >> >> Another thing in CipherSuite.java: >> 803 static CipherSuite nameOf(String ciperSuiteName) { >> Why isn't this method named valueOf()? > There is a default builtin valueOf(String) method for enum type. Ah, I see. But given the r

Re: SSLContextImpl.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-11 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/12e20a7d6e26 On 6/10/2018 10:37 PM, Weijun Wang wrote: The protocols (for example, SSLParameters::getProtocols) are now from new to old, which is opposite from the previous order. Why make this change? 41 * Instances of this class are immu

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-11 Thread Xuelei Fan
Hi Daniel, It's a good idea. I added your comment to the TLS 1.3 implementation issues tracking enhancement: https://bugs.openjdk.java.net/browse/JDK-8204636 Thanks, Xuelei On 6/11/2018 2:28 AM, Daniel Fuchs wrote: Hi Xuelei, Just a note that it might be a better idea to rework the imp

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-11 Thread Daniel Fuchs
Hi Xuelei, Just a note that it might be a better idea to rework the implementation of SSLLogger/SSLConsoleLogger a bit in order to have SSLLogger implement System.Logger. This would ensure that the SSLLogger class is skipped when looking for the caller, when the underlying logger is a logger ret

SSLContextImpl.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-10 Thread Weijun Wang
The protocols (for example, SSLParameters::getProtocols) are now from new to old, which is opposite from the previous order. Why make this change? 41 * Instances of this class are immutable after the context is initialized. You mean instances of child of this class? It looks like this class i

Re: Code Review Request: TLS 1.3 Implementation

2018-06-10 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/da9979451b7a On 6/8/2018 11:00 PM, Jamil Nimeh wrote: Hi Brad, just one follow-up to your CipherSuite comment, please see below. --Jamil On 06/08/2018 07:55 PM, Bradford Wetmore wrote: CipherSuite.java 74:  There is a mix o

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-10 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/e4fe7c97b1de On 6/9/2018 2:42 AM, Seán Coffey wrote: Some comments on SSLLogger also : formatCaller() uses getStackTrace() to walk the stack. It's probably more expensive than using the newer Stackwalker class. Could it be replaced with somet

Re: Code Review Request: TLS 1.3 Implementation

2018-06-09 Thread Xuelei Fan
Hi Valerie, All good catches! I made a code clean up in the update: http://hg.openjdk.java.net/jdk/sandbox/rev/38c2a4078033 Thanks, Xuelei On 6/8/2018 5:16 PM, Valerie Peng wrote: Hi Xuelei, Will this.requestedServerNames ever be null? This field is initialized in constructors with Col

Re: Code Review Request: TLS 1.3 Implementation

2018-06-09 Thread Anthony Scarpino
On 06/08/2018 07:55 PM, Bradford Wetmore wrote: [...] TransportContext.java - 90:  Any chance of combining these 3 constructors into 1?  Almost identical duplicate code here. I have some code to change this already. Tony

Re: Code Review Request: TLS 1.3 Implementation

2018-06-09 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/2d7e08d730b6 On 6/7/2018 7:36 PM, Bradford Wetmore wrote: I'm working my way through CipherSuite.java, slow going as I'm making sure all of the ciphersuites got updated to the new format. SSLServerSocketFactoryImpl.java -

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-09 Thread Seán Coffey
Some comments on SSLLogger also : formatCaller() uses getStackTrace() to walk the stack. It's probably more expensive than using the newer Stackwalker class. Could it be replaced with something like : return StackWalker.getInstance().walk(s -> s.dropWhile((f -> f.g

Re: Code Review Request: TLS 1.3 Implementation

2018-06-08 Thread Jamil Nimeh
Hi Brad, just one follow-up to your CipherSuite comment, please see below. --Jamil On 06/08/2018 07:55 PM, Bradford Wetmore wrote: CipherSuite.java 74:  There is a mix of ciphersuite initialization styles, which I find confusing.  In the *_OF_12, you pass the MAC of M_NULL,

Re: Code Review Request: TLS 1.3 Implementation

2018-06-08 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/a02692615745 Code clean up for SSLSessionContextImpl.java On 6/7/2018 10:28 AM, Bradford Wetmore wrote: SSLSessionContextImpl.java -- The only change here was to reorder the import of Vector (obsolete) and update the c

Re: Code Review Request: TLS 1.3 Implementation

2018-06-08 Thread Bradford Wetmore
CipherSuite.java 74: There is a mix of ciphersuite initialization styles, which I find confusing. In the *_OF_12, you pass the MAC of M_NULL, with H_* being used for PRF/HASH, which is also used for the MAC value for new suites, IIUC. Would you consider specifying both in

Re: Code Review Request: TLS 1.3 Implementation

2018-06-08 Thread Xuelei Fan
> BaseSSLSocketImpl.java > -- Update: http://hg.openjdk.java.net/jdk/sandbox/rev/bad9f4c7eeec Update to handle EOFException and requireCloseNotify property. Xuelei On 6/7/2018 10:28 AM, Bradford Wetmore wrote: Also reviewed SSLSocketFactoryImpl.java BaseSSLSocketImpl.java

Re: Code Review Request: TLS 1.3 Implementation

2018-06-08 Thread Valerie Peng
Hi Xuelei, Will this.requestedServerNames ever be null? This field is initialized in constructors with Collections.xxxList(...) method. Are the two private fields "peerPrincipal" and "localPrincipal" really used? There are two pkg private methods, setPeer/LocalPrincipal(...) which sets the

Re: SSLStringize.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-08 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/25178bb3e8f5 On 6/6/2018 3:58 AM, Weijun Wang wrote: SSLStringize.java: 1. I assume the toString() method does not change the internal status of a ByteBuffer? Is it worth mentioning this in the spec? 2. Should this interface and all its imple

Re: Code Review Request: TLS 1.3 Implementation

2018-06-08 Thread Xuelei Fan
On 6/8/2018 10:21 AM, Xuelei Fan wrote: Here is the 3rd full webrev:    http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.02 and the delta update to the 1st webrev: typo: 1st -> 2nd    http://cr.openjdk.java.net/~xuelei/8196584/webrev-delta.01 Xuelei On 6/3/2018 9:43 PM, Xuelei Fan w

Re: Code Review Request: TLS 1.3 Implementation

2018-06-08 Thread Xuelei Fan
Here is the 3rd full webrev: http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.02 and the delta update to the 1st webrev: http://cr.openjdk.java.net/~xuelei/8196584/webrev-delta.01 Xuelei On 6/3/2018 9:43 PM, Xuelei Fan wrote: Hi, Here it the 2nd full webrev:   http://cr.openjdk.j

Re: Code Review Request: TLS 1.3 Implementation

2018-06-07 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/a82a96b62d22 This update fix the signature algorithm selection issue for ecdsa_secp256r1_sha256/ecdsa_secp384r1_sha384/ecdsa_secp512r1_sha512. The update code will be included in the next webrev for further review. Xuelei On 6/3/2018 9:43 PM

Re: Code Review Request: TLS 1.3 Implementation

2018-06-07 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/33a2451070d3 This update add debug log for pre_shared_key extension. The update code will be included in the next webrev for further review if needed. Xuelei On 6/3/2018 9:43 PM, Xuelei Fan wrote: Hi, Here it the 2nd full webrev:   http:/

Re: Other files assigned to me (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
On 6/7/2018 9:25 PM, Weijun Wang wrote: I've finished reviewing of other files assigned to me. Turns out most changes are on Debug -> SSLLogger. Thanks! Well, would you mind to pick up a few more P1 file code review? We are trying to catch the June 13 deadline. It still looks like a chall

Other files assigned to me (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
I've finished reviewing of other files assigned to me. Turns out most changes are on Debug -> SSLLogger. There are only 2 concerns left: 1. SSLLogger: I talked about in another mail, SSLConsoleLogger::log does not use format. 2. SSLStringize: I think this is unnecessarily complex. Also, you h

Re: Code Review Request: TLS 1.3 Implementation

2018-06-07 Thread Xuelei Fan
Update: http://hg.openjdk.java.net/jdk/sandbox/rev/75527e40bdfd This update fixed the session resumption and related issues. The update code will be included in the next webrev for further review. Xuelei On 6/3/2018 12:58 PM, Xuelei Fan wrote: > http://cr.openjdk.java.net/~xuelei/8196584/

Re: Code Review Request: TLS 1.3 Implementation

2018-06-07 Thread Bradford Wetmore
I'm working my way through CipherSuite.java, slow going as I'm making sure all of the ciphersuites got updated to the new format. SSLServerSocketFactoryImpl.java --- This comment is actually an overall comment in other files in your review. Typically we don't m

Re: HelloCookieManager.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
On 6/7/2018 6:29 PM, Weijun Wang wrote: Have you updated it yourself? I did not. I have a proposal at http://cr.openjdk.java.net/~weijun/999/webrev.hello-cookie-manager/ Looks fine to me. Minor nit: Would you mind limit the maximum line character to 80 characters (line 250)? Th

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
I got your ideas. Let's see if we can make a change, a little bit later. Thanks, Xuelei On 6/7/2018 6:23 PM, Weijun Wang wrote: My final comment on this class: Since SSLConsoleLogger is a Logger child, its log(..., params) method should follow the Logger::log spec, which has /**

Re: HelloCookieManager.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
Have you updated it yourself? I have a proposal at http://cr.openjdk.java.net/~weijun/999/webrev.hello-cookie-manager/ --Max > On Jun 6, 2018, at 10:40 PM, Xuelei Fan wrote: > > On 6/5/2018 8:48 PM, Weijun Wang wrote: >>> On 6/5/2018 6:54 AM, Weijun Wang wrote: HelloCookieManager.j

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
My final comment on this class: Since SSLConsoleLogger is a Logger child, its log(..., params) method should follow the Logger::log spec, which has /** * Logs a message with resource bundle and an optional list of * parameters. * ... * @param format th

Re: Code Review Request: TLS 1.3 Implementation

2018-06-07 Thread Adam Petcher
Updates to address these comments: http://hg.openjdk.java.net/jdk/sandbox/rev/2dc6efcdeb11 http://hg.openjdk.java.net/jdk/sandbox/rev/97447478b7da On 6/3/2018 8:24 PM, Xuelei Fan wrote: > http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00 PskKeyExchangeModesExtension.java -

Re: Code Review Request: TLS 1.3 Implementation

2018-06-07 Thread Bradford Wetmore
Also reviewed SSLSocketFactoryImpl.java BaseSSLSocketImpl.java -- I don't see where requireCloseNotify is actually being used in the remaining code. If you are removing this functionality, you should probably be calling that out in your CSR. SSLSessionContextImpl.java --

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
Looks good to me. Xuelei On 6/7/2018 8:56 AM, Weijun Wang wrote: Oops, another place needs change http://hg.openjdk.java.net/jdk/sandbox/rev/64aa781522be --Max On Jun 7, 2018, at 11:03 PM, Xuelei Fan wrote: Looks fine to me. Thanks! Xuelei On 6/7/2018 8:01 AM, Weijun Wang wrote: St

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
Oops, another place needs change http://hg.openjdk.java.net/jdk/sandbox/rev/64aa781522be --Max > On Jun 7, 2018, at 11:03 PM, Xuelei Fan wrote: > > Looks fine to me. Thanks! > > Xuelei > > On 6/7/2018 8:01 AM, Weijun Wang wrote: >> Still at >>http://cr.openjdk.java.net/~weijun/999

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
Looks fine to me. Thanks! Xuelei On 6/7/2018 8:01 AM, Weijun Wang wrote: Still at http://cr.openjdk.java.net/~weijun/999/webrev.more-krb5-cleanup/ I keep the existing unsupported KRB5 ciphersuites and move the 10 previously supported ones there. --Max On Jun 7, 2018, at 10:49 PM

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
Still at http://cr.openjdk.java.net/~weijun/999/webrev.more-krb5-cleanup/ I keep the existing unsupported KRB5 ciphersuites and move the 10 previously supported ones there. --Max > On Jun 7, 2018, at 10:49 PM, Weijun Wang wrote: > > > >> On Jun 7, 2018, at 10:47 PM, Xuelei Fan wro

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
On 6/7/2018 7:49 AM, Weijun Wang wrote: On Jun 7, 2018, at 10:47 PM, Xuelei Fan wrote: CipherSuite.java I think we may still want to have KRB5 cipher suite in the unsupported list, as we did for from line 455. -TLS_KRB5_WITH_3DES_EDE_CBC_SHA( - 0x001F, false,

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
> On Jun 7, 2018, at 10:47 PM, Xuelei Fan wrote: > > CipherSuite.java > > I think we may still want to have KRB5 cipher suite in the unsupported list, > as we did for from line 455. > > -TLS_KRB5_WITH_3DES_EDE_CBC_SHA( > - 0x001F, false, "TLS_KRB5_WITH_3DES_EDE_

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
On 6/7/2018 7:43 AM, Weijun Wang wrote: Or you mean keep the definitions but move them to "known but unsupported"? Yes. Xuelei On Jun 7, 2018, at 10:41 PM, Weijun Wang wrote: Please take a review http://cr.openjdk.java.net/~weijun/999/webrev.more-krb5-cleanup/ --Max On Jun 7, 20

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
CipherSuite.java I think we may still want to have KRB5 cipher suite in the unsupported list, as we did for from line 455. -TLS_KRB5_WITH_3DES_EDE_CBC_SHA( - 0x001F, false, "TLS_KRB5_WITH_3DES_EDE_CBC_SHA", "", - ProtocolVersion.PROTOCOLS_TO_T12, -

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
Or you mean keep the definitions but move them to "known but unsupported"? > On Jun 7, 2018, at 10:41 PM, Weijun Wang wrote: > > Please take a review > > http://cr.openjdk.java.net/~weijun/999/webrev.more-krb5-cleanup/ > > --Max > > >> On Jun 7, 2018, at 10:24 PM, Xuelei Fan wrote: >>

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
Please take a review http://cr.openjdk.java.net/~weijun/999/webrev.more-krb5-cleanup/ --Max > On Jun 7, 2018, at 10:24 PM, Xuelei Fan wrote: > > > Yes, please KRB5 cipher suite from the supported list. > Typo: Yes, please remove KRB5 cipher suite from the supported list. > > On 6/7/201

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
> Yes, please KRB5 cipher suite from the supported list. Typo: Yes, please remove KRB5 cipher suite from the supported list. On 6/7/2018 7:23 AM, Xuelei Fan wrote: Yes, please KRB5 cipher suite from the supported list. For the public APIs part, please leave it as it is before we deprecate the

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Xuelei Fan
Yes, please KRB5 cipher suite from the supported list. For the public APIs part, please leave it as it is before we deprecate the specification. Some other JSSE provider might still support KRB5 cipher suites. Xuelei On 6/7/2018 1:45 AM, Weijun Wang wrote: And there are the Kerberos word i

Re: KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
And there are the Kerberos word in public APIs: share/classes/javax/net/ssl/SSLContext.java 336: * Some cipher suites (such as Kerberos) require remote hostname 366: * Some cipher suites (such as Kerberos) require remote hostname share/classes/javax/net/ssl/HttpsURLConnection.java 106:

KRB5 (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-07 Thread Weijun Wang
I still see K_KRB5 KeyExchange and TLS_KRB5_WITH_3DES_EDE_CBC_SHA etc in CipherSuite.java. Shall I also remove them. Thanks Max

Re: Utilities.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Weijun Wang
> On Jun 7, 2018, at 12:40 PM, Xuelei Fan wrote: > > On 6/6/2018 8:11 PM, Weijun Wang wrote: >> Utilities.java: >> 39 static final char[] hexDigits = "0123456789ABCDEF".toCharArray(); >> 40 private static final String indent = " "; >> 41 private static final Pattern lineBrea

Re: Utilities.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Xuelei Fan
On 6/6/2018 8:11 PM, Weijun Wang wrote: Utilities.java: 39 static final char[] hexDigits = "0123456789ABCDEF".toCharArray(); 40 private static final String indent = " "; 41 private static final Pattern lineBreakPatern = 42 Pattern.compile("\\r\\n|\\n|\\r"

Re: RandomCookie.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Xuelei Fan
On 6/6/2018 8:17 PM, Weijun Wang wrote: On Jun 7, 2018, at 12:19 AM, Xuelei Fan wrote: On 6/5/2018 10:37 PM, Weijun Wang wrote: RandomCookie.java: +private boolean isT12Downgrade() { +return Arrays.equals(randomBytes, 24, 31, t12Protection, 0, 7); +} + +private boolea

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Weijun Wang
I think I understand. So if a user sets it to empty, they will also need to create a customized logger/formatter like SSLLogger that is able to output a parameter without a placeholder in msg. --Max > On Jun 7, 2018, at 9:51 AM, Xuelei Fan wrote: > > In this implementation, the SunJSSE loggin

Re: RandomCookie.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Weijun Wang
> On Jun 7, 2018, at 12:19 AM, Xuelei Fan wrote: > > On 6/5/2018 10:37 PM, Weijun Wang wrote: >> RandomCookie.java: >> +private boolean isT12Downgrade() { >> +return Arrays.equals(randomBytes, 24, 31, t12Protection, 0, 7); >> +} >> + >> +private boolean isT11Downgrade() { >

Utilities.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Weijun Wang
Utilities.java: 39 static final char[] hexDigits = "0123456789ABCDEF".toCharArray(); 40 private static final String indent = " "; 41 private static final Pattern lineBreakPatern = 42 Pattern.compile("\\r\\n|\\n|\\r"); Use UPPERCASE letters for final static fie

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Xuelei Fan
In this implementation, the SunJSSE logging is updated to (the class comment lines): /** * Implementation of SSL logger. * * If the system property "javax.net.debug" is not defined, the debug logging * is turned off. If the system property "javax.net.debug" is defined as * empty, the deb

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Weijun Wang
But will any application use the logger named "javax.net.debug"? I think that's only for JSSE. > On Jun 7, 2018, at 9:35 AM, Xuelei Fan wrote: > > I see your concerns now. Currently, we don't fine the format if using System > logger. Applications can define the format and output style for th

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Xuelei Fan
I see your concerns now. Currently, we don't fine the format if using System logger. Applications can define the format and output style for themselves if needed. That's also the purpose why we introduce the System Logger in the provider. Xuelei On 6/6/2018 6:10 PM, Weijun Wang wrote: I a

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Weijun Wang
I assume this output is for the internal SSLLogger. I was asking what would be printed if someone only set "-Djavax.net.debug" and a System logger is used. --Max > On Jun 7, 2018, at 8:54 AM, Xuelei Fan wrote: > > > > On 6/6/2018 5:46 PM, Weijun Wang wrote: >>> On Jun 7, 2018, at 8:41 AM, Xu

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Xuelei Fan
On 6/6/2018 5:46 PM, Weijun Wang wrote: On Jun 7, 2018, at 8:41 AM, Xuelei Fan wrote: On 6/6/2018 4:21 PM, Weijun Wang wrote: On Jun 7, 2018, at 12:27 AM, Xuelei Fan wrote: On 6/6/2018 5:41 AM, Weijun Wang wrote: There are lots of calls like RSAClientKeyExchangeMessage ckem =

Re: SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

2018-06-06 Thread Weijun Wang
> On Jun 7, 2018, at 8:41 AM, Xuelei Fan wrote: > > > > On 6/6/2018 4:21 PM, Weijun Wang wrote: >>> On Jun 7, 2018, at 12:27 AM, Xuelei Fan wrote: >>> >>> On 6/6/2018 5:41 AM, Weijun Wang wrote: There are lots of calls like RSAClientKeyExchangeMessage ckem = ne

  1   2   >