Code Review Request, JDK-8185576 : New handshake implementation

[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
      o 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
      o 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 }.
      o 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
      o 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
      o 1155-1158: Masking with 0xFF is not necessary for these
        primitive narrowing conversions.
  * ContentType.java
      o 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
      o 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
      o 301: Comment nit: Should be RSA public key, not EC.
  * SSLExtension.java
      o 284, and others, including other files (e.g.
        SSLExtensions.java): Spelling nit, Change "onLoadConcumer" to
        "onLoadConsumer"
      o 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
      o 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
      o 128, possibly used in other files: Spelling nit,
        CERTIFICARE_REQUEST --> CERTIFICATE_REQUEST
      o 253: Maybe add a known-but-not-supported entry for MESSAGE_HASH
        ((byte)0xFE, "message_hash");
  * TransportContext.java
      o 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?
      o 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180110/acf926a6/attachment.htm>