Code Review Request, JDK-8185576 : New handshake implementation

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Code Review Request, JDK-8185576 : New handshake implementation

Xuelei Fan-2
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
Reply | Threaded
Open this post in threaded view
|

Re: Code Review Request, JDK-8185576 : New handshake implementation

Jamil Nimeh
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
    • 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
    • 130: Remove @Override comment, doesn't look like you need it.
  • DTLSOutpuTRecord.java
    • 58: Remove comment line for this.protocolVersion
  • HandshakeHash.java
    • 44: Nit: trhe -> the
    • 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.
    • 509 and others: Nit: ClonableHash -> CloneableHash
    • 537 and others: Nit: NoneClonableHash -> NonCloneableHash
  • JsseJce.java
    • 55-61: Does the static initializer for the ClientKeyExchangeService still need to be there?  It's commented out right now.
  • MAC.java
    • 190: Typo in comment "the the"
  • ProtocolVersion
    • 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.
    • 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.
    • 267: Nit: "pv .id" (note the extra space).  Not sure if that's webrev rendering or you really have that extra space in there.
    • 267: For the first clause in the ternary expression (DTLS) shouldn't the comparison be <= instead of >=?
  • SSLAlgorithmDecomposer.java
    • 132-138: Do you still want to hang onto this commented-out block of code?
  • SSLContextImpl.java
    • 889 & 896: Will these commented-out lines be temporary and be uncommented once the 1.3 handshake is done?
  • SSLEngineInputRecord.java
    • 287: Would this be a good place to use Record.getInt24()?
    • 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.
    • 338: Another spot for Record.getInt24() maybe.
  • SSLSessionImpl.java
    • 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
    • Similar to the comment on SSLEngineInputRecord.java:319: you could do ByteBuffer.allocate() and bypass the explicit byte[] constructor call.
    • 333: Another spot you could use Record.getInt24().
  • SignatureAlgorithmsExtension.java
    • 276: The hashAlgorithms array appears to be missing "sha384" at index 5.
  • SupportedGroupsExtension.java
    • 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

Reply | Threaded
Open this post in threaded view
|

Re: Code Review Request, JDK-8185576 : New handshake implementation

Xuelei Fan-2
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
>
Reply | Threaded
Open this post in threaded view
|

Re: Code Review Request, JDK-8185576 : New handshake implementation

Jamil Nimeh
In reply to this post by Jamil Nimeh
Hello Xuelei,

Here are a few more comments from some of the new files, maybe one or two existing ones that I missed the first time around.  I still need to do the final two big ones (SSLSocketImpl and SSLEngineImpl), but I think I've gone through every other file in the webrev at this point.  Like before, it's mostly small stuff.
  • Utilities.java
    • 143 & 147: Is the name of the method supposed to be "intent" or "indent"?  If the latter then maybe correct the method name?  If not, it seems like an odd name to give to a method that breaks up multi-line strings and adds a prefix to each line after the first.  Also you'll have both a class variable and method named "indent" so maybe one of them changes?  But that's only if the method is really "indent".
  • CertStatusExtension.java
    • 765: For the empty status_request_v2, I would suggest a two-entry extension_data containing both OCSP_MULTI and OCSP types.  That's what the current code does at least.  The extData would be { 0x00, 0x0e, 0x02, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00 }.
    • 907 onward: I think the StatusResponseManager class in the CertStatusExtension.java file should have its own file as it was originally.  The SRM is really a functional unit of its own.  It is instantiated and managed by SSLContext and the output it provides is more for the CertificateStatus message or the Certificate message in TLS 1.3.  While it is related to the status_request[_v2] extensions, it doesn't have a direct hand in encoding and decoding them.
  • ClientHandshakeContext.java
    • 89: Similar to the explanation for the reserved server cert chain, should there be a comment explaining deferred certs?  If you think it prudent, I would suggest, "Hang onto the peer certificate chain until after the CertificateStatus message is received and processed."
  • ClientHello.java
    • 1155-1158: Masking with 0xFF is not necessary for these primitive narrowing conversions.
  • ContentType.java
    • 40: After APPLICATION_DATA should we add an entry for HEARTBEAT (24), just for completeness?  We don't support heartbeat, but it might allow us to log the record type by name if we were to receive one rather than call it "unknown".
  • KeyUpdate.java
    • I know this doesn't cover the guts of the 1.3 handshake yet, just a note that you'll probably want an enum for KeyUpdateRequest, similar to what you've done for other constant/enumerated values elsewhere in the code.
  • RSAServerKeyExchange.java
    • 301: Comment nit: Should be RSA public key, not EC.
  • SSLExtension.java
    • 284, and others, including other files (e.g. SSLExtensions.java): Spelling nit, Change "onLoadConcumer" to "onLoadConsumer"
    • Is this intended to be a registry of all known extensions?  There are gaps in this list that are in the IANA extensions registry, but they're all extension types we don't support yet or have no plans to support.  Since it's an enum, should this list be as complete as possible given the registered IDs that are out there?
  • SSLExtensions.java
    • 59: Should there be a check to make sure there is sufficient remaining space in the buffer after the extensions length is retrieved?  You do something similar to that down on line 62 after you get each individual extension length, perhaps a top-level one would be good too.
  • SSLHandshake.java
    • 128, possibly used in other files: Spelling nit, CERTIFICARE_REQUEST --> CERTIFICATE_REQUEST
    • 253: Maybe add a known-but-not-supported entry for MESSAGE_HASH ((byte)0xFE, "message_hash");
  • TransportContext.java
    • 288 & 343: On 288 you comment about shutting down the transport, but the transport object doesn't really get used until way down at 343, and there's only logging up around 288.  Maybe this comment should get moved down there?
    • 448: This is more curiosity on my part, I see isBroken checked in a few different places throughout the code, but nowhere in the webrev do I see a place where isBroken is ever set to true.  Is this something you expect to set when more of the handshake framework is implemented?  What is the criteria for setting isBroken to true?

Thanks,

--Jamil

Reply | Threaded
Open this post in threaded view
|

Re: Code Review Request, JDK-8185576 : New handshake implementation

Adam Petcher
In reply to this post by Xuelei Fan-2
Disclaimer: I am not a Reviewer. This is not a Review.

I looked through all of the code and I didn't find any significant
issues. In general, I think this is a nice improvement, and it makes the
code easier to understand. Below are a few minor comments:

SupportedGroups.java: There are (draft) OIDs for X25519 and X448, in
case you want to include them here:
https://tools.ietf.org/html/draft-ietf-curdle-pkix-01. Probably no need
to worry about this now, though---we will fix all this when we add
support for X25519/X448 later.

Utilities.java: intent -> indent

I found some "TODO" strings in several of the files. I don't know if you
intended to leave them in for now. If you are going to leave them in, it
would be more clear if you included more information saying why those
parts are not done. E.g. "TODO: TLS 1.3" or "TODO: JDK-8888888".

Multiple files: concumer -> consumer

PredefinedDHParameterSpecs.java: Can we remove all the primes less than
1024 bits? They are all obsolete since the last update release. Also,
some of these values are duplicated in sun.security.provider.ParameterCache.


On 12/15/2017 11: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