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.
* CipherBox.java
o 146: Were you intending to leave that commented protocol version
line in there in the short term? If not then maybe remove it.
* DTLSInputRecord.java
o 130: Remove @Override comment, doesn't look like you need it.
* DTLSOutpuTRecord.java
o 58: Remove comment line for this.protocolVersion
* HandshakeHash.java
o 44: Nit: trhe -> the
o 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.
o 509 and others: Nit: ClonableHash -> CloneableHash
o 537 and others: Nit: NoneClonableHash -> NonCloneableHash
* JsseJce.java
o 55-61: Does the static initializer for the
ClientKeyExchangeService still need to be there? It's commented
out right now.
* MAC.java
o 190: Typo in comment "the the"
* ProtocolVersion
o 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.
o 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.
o 267: Nit: "pv .id" (note the extra space). Not sure if that's
webrev rendering or you really have that extra space in there.
o 267: For the first clause in the ternary expression (DTLS)
shouldn't the comparison be <= instead of >=?
* SSLAlgorithmDecomposer.java
o 132-138: Do you still want to hang onto this commented-out block
of code?
* SSLContextImpl.java
o 889 & 896: Will these commented-out lines be temporary and be
uncommented once the 1.3 handshake is done?
* SSLEngineInputRecord.java
o 287: Would this be a good place to use Record.getInt24()?
o 319: This is just a nit, you don't need to do it, but it would
look a bit cleaner to do
ByteBuffer.allocate(handshakeBuffer.remaining() +
fragment.remaining()). You'll still have a backing array
according to the docs.
o 338: Another spot for Record.getInt24() maybe.
* SSLSessionImpl.java
o 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?
* SSLSocketInputRecord.java
o Similar to the comment on SSLEngineInputRecord.java:319: you
could do ByteBuffer.allocate() and bypass the explicit byte[]
constructor call.
o 333: Another spot you could use Record.getInt24().
* SignatureAlgorithmsExtension.java
o 276: The hashAlgorithms array appears to be missing "sha384" at
index 5.
* SupportedGroupsExtension.java
o 309: Cut-n-paste bug: x448's ID should be 0x001E and the fourth
field should be "x448"
--Jamil
On 12/15/2017 08:20 PM, Xuelei Fan wrote:
Hi,
Please review the change:
http://cr.openjdk.java.net/~xuelei/8185576/webrev.00/
This fix is trying to re-org the TLS handshaking implementation in the
SunJSSE provider.
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.
We are consider to simplify the handshake implementation by:
1. Use difference handshake handler for different TLS versions.
2. Improved message driven handler for different handshake messages.
The basic ideas for the simplification look like:
1. message/even driving handshake processing model.
An actor is a stateless service that is used to consume an input
message, or produce output message, or both.
message A message B
--------------> Actor A ------------> Actor B
2. immutable actor and centralized states.
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.
message A message B
--------------> Actor A --------------------------> Actor B
Context Context [Actor A updated]
3. dynamical actor hierarchical structure
The actor hierarchical structure is flexible and can be dynamically
updated if needed.
message A message B
--------------> Actor A ----------------------------> ...
(Context) | (Context [Actor A updated]) ^
| |
| Add Actor C between A and B |
| [Optional] |
+----------------------------------+
<continue>
... <optional> Message C
[ Actor C --------------------------> ] Actor B
(Context [Actor B updated]
4. consumer and producer
The consumer/producer pattern is used to consume input messages, and
produce output messages.
message A message B
------------> Consumer A Producer B ------------------->
(Context) | ^ (Context [Consumer A updated])
| |
| Add Producer B |
+------------------+
message A
Producer A -----------------------> Consumer A -------------------->
| (Context) ^ (Context [Producer A updated])
| |
| Add consumer B |
+-------------------------------+
In this implementation, I removed:
1. the extended master secret extension
I will add it back soon.
2. the KRB5 cipher suite implementation.
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.
3. OCSP stapling
This feature will be added back later.
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.
Please don't be hesitate if you have any questions.
Regards,
Xuelei
