Code Review Request, JDK-8185576 : New handshake implementation
Xuelei Fan
xuelei.fan at oracle.com
Thu Jan 4 20:03:49 UTC 2018
Nice catches! The updated will present in the next webrev.
For the comment out ClientKeyExchangeService blocks, they're placeholder
for KRB5 cipher suites. If no objections to remove KRB5 cipher suites,
I will clear up these blocks in a future webrev.
Thanks,
Xuelei
On 1/3/2018 4:04 PM, Jamil Nimeh wrote:
> 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
>
More information about the security-dev
mailing list