Code Review Request, JDK-8185576 : New handshake implementation

Jamil Nimeh jamil.j.nimeh at oracle.com
Thu Jan 4 00:04:43 UTC 2018


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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/security-dev/attachments/20180103/ff656cd3/attachment-0001.html>


More information about the security-dev mailing list