<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Xuelei, I'm only part way through this but I wanted to give you
what I had so far. I'm working on the new files now so hopefully
I'll have the rest of them done tomorrow.<br>
<ul>
<br>
<li>CipherBox.java</li>
<ul>
<li>146: Were you intending to leave that commented protocol
version line in there in the short term? If not then maybe
remove it.</li>
</ul>
<li>DTLSInputRecord.java</li>
<ul>
<li>130: Remove @Override comment, doesn't look like you need
it.</li>
</ul>
<li>DTLSOutpuTRecord.java</li>
<ul>
<li>58: Remove comment line for this.protocolVersion<br>
</li>
</ul>
<li>HandshakeHash.java</li>
<ul>
<li>44: Nit: trhe -> the</li>
<li>83-87: Do you need to do Arrays.copyOf() on holder? It
seems like you could just add holder directly to the reserves
list rather than duplicate it.</li>
<li>509 and others: Nit: ClonableHash -> CloneableHash</li>
<li>537 and others: Nit: NoneClonableHash -> NonCloneableHash</li>
</ul>
<li>JsseJce.java</li>
<ul>
<li>55-61: Does the static initializer for the
ClientKeyExchangeService still need to be there? It's
commented out right now.</li>
</ul>
<li>MAC.java</li>
<ul>
<li>190: Typo in comment "the the"</li>
</ul>
<li>ProtocolVersion</li>
<ul>
<li>167: I don't think you need to mask with "& 0xFF" since
you're doing a primitive narrowing conversion to byte from
int. That already lops off the upper 24 bits. But it's fine
if you wish to keep it, too.</li>
<li>254: In order to be consistent with the unmodifiable empty
list return on 257 consider doing "return
Collections.unmodifiableList(pvs)" here. If you're expecting
to make changes to the list if it is non-empty, then disregard
this comment.</li>
<li>267: Nit: "pv .id" (note the extra space). Not sure if
that's webrev rendering or you really have that extra space in
there.<br>
</li>
<li>267: For the first clause in the ternary expression (DTLS)
shouldn't the comparison be <= instead of >=?</li>
</ul>
<li>SSLAlgorithmDecomposer.java</li>
<ul>
<li>132-138: Do you still want to hang onto this commented-out
block of code?</li>
</ul>
<li>SSLContextImpl.java</li>
<ul>
<li>889 & 896: Will these commented-out lines be temporary
and be uncommented once the 1.3 handshake is done?</li>
</ul>
<li>SSLEngineInputRecord.java</li>
<ul>
<li>287: Would this be a good place to use Record.getInt24()?</li>
<li>319: This is just a nit, you don't need to do it, but it
would look a bit cleaner to do ByteBuffer.allocate(<span
class="changed">handshakeBuffer.remaining() +
fragment.remaining()). You'll still have a backing array
according to the docs.</span></li>
<li><span class="changed">338: Another spot for
Record.getInt24() maybe.</span></li>
</ul>
<li><span class="changed">SSLSessionImpl.java</span></li>
<ul>
<li><span class="changed">435 & 440, 492 & 497, and
others: There are multiple places where
ClientKeyExchangeService.find() calls are commented out.
Similar to the other places where there are commented
blocks, are these needed for future integration with the 1.3
handshake code?</span></li>
</ul>
<li><span class="changed">SSLSocketInputRecord.java</span></li>
<ul>
<li><span class="changed"></span>Similar to the comment on
SSLEngineInputRecord.java:319: you could do
ByteBuffer.allocate() and bypass the explicit byte[]
constructor call.</li>
<li>333: Another spot you could use Record.getInt24().</li>
</ul>
<li>SignatureAlgorithmsExtension.java</li>
<ul>
<li>276: The hashAlgorithms array appears to be missing "sha384"
at index 5.</li>
</ul>
<li>SupportedGroupsExtension.java</li>
<ul>
<li>309: Cut-n-paste bug: x448's ID should be 0x001E and the
fourth field should be "x448"</li>
</ul>
</ul>
<p>--Jamil<br>
</p>
<ul>
<ul>
</ul>
</ul>
<br>
<div class="moz-cite-prefix">On 12/15/2017 08:20 PM, Xuelei Fan
wrote:<br>
</div>
<blockquote
cite="mid:f4eb1f46-5535-873b-5d92-661d60a2b425@oracle.com"
type="cite">Hi, <br>
<br>
Please review the change: <br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Exuelei/8185576/webrev.00/">http://cr.openjdk.java.net/~xuelei/8185576/webrev.00/</a>
<br>
<br>
This fix is trying to re-org the TLS handshaking implementation in
the SunJSSE provider. <br>
<br>
The handshaking process in TLS 1.3 is lot different from that of
TLS 1.2. For TLS 1.3 implementation, it is not straightforward
any more to use the existing state machines for TLS 1.2. For
example, in TLS specification, the handshake message must be
delivered in strict order. The handshake message order in TLS 1.3
protocol is re-designed and significant different from that of TLS
1.2. If we re-use the state machine for TLS 1.3 and 1.2 and
previous versions, the state machine becomes very complicated and
hard to maintain. <br>
<br>
We are consider to simplify the handshake implementation by: <br>
1. Use difference handshake handler for different TLS versions. <br>
2. Improved message driven handler for different handshake
messages. <br>
<br>
The basic ideas for the simplification look like: <br>
1. message/even driving handshake processing model. <br>
An actor is a stateless service that is used to consume an input
message, or produce output message, or both. <br>
message A message B <br>
--------------> Actor A ------------> Actor B <br>
<br>
2. immutable actor and centralized states. <br>
Handshake states are represent in context objects
(ConnectionContext), and the context object is passed to each
actor. An actor can update the state in the context object
accordingly. At the same time, an actor's behavior may be
different for different states. <br>
message A message B <br>
--------------> Actor A -------------------------->
Actor B <br>
Context Context [Actor A updated] <br>
<br>
3. dynamical actor hierarchical structure <br>
The actor hierarchical structure is flexible and can be
dynamically updated if needed. <br>
<br>
message A message B <br>
--------------> Actor A ----------------------------> ...
<br>
(Context) | (Context [Actor A updated]) ^ <br>
| | <br>
| Add Actor C between A and B | <br>
| [Optional] | <br>
+----------------------------------+ <br>
<br>
<continue> <br>
... <optional> Message C <br>
[ Actor C --------------------------> ] Actor B <br>
(Context [Actor B updated] <br>
<br>
4. consumer and producer <br>
The consumer/producer pattern is used to consume input messages,
and produce output messages. <br>
<br>
message A message B <br>
------------> Consumer A Producer B
-------------------> <br>
(Context) | ^ (Context [Consumer A
updated]) <br>
| | <br>
| Add Producer B | <br>
+------------------+ <br>
<br>
<br>
message A <br>
Producer A -----------------------> Consumer A
--------------------> <br>
| (Context) ^ (Context [Producer A
updated]) <br>
| | <br>
| Add consumer B | <br>
+-------------------------------+ <br>
<br>
In this implementation, I removed: <br>
1. the extended master secret extension <br>
I will add it back soon. <br>
<br>
2. the KRB5 cipher suite implementation. <br>
I'm not very sure KRB5 cipher suites are still used in practice.
Please let me know if you still using KRB5 cipher suite. I may
not add these cipher suite back unless it is necessary. <br>
<br>
3. OCSP stapling <br>
This feature will be added back later. <br>
<br>
As this re-org is for TLS 1.3, so it contains a few draft skeleton
for TLS 1.3. I will file these parts when doing the TLS 1.3
implementation. <br>
<br>
Please don't be hesitate if you have any questions. <br>
<br>
Regards, <br>
Xuelei <br>
</blockquote>
<br>
</body>
</html>