<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hello Xuelei,<br>
<br>
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.<br>
<ul>
<li>Utilities.java</li>
<ul>
<li>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".<br>
</li>
</ul>
<li>CertStatusExtension.java</li>
<ul>
<li>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 }.<br>
</li>
<li>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.<br>
</li>
</ul>
<li>ClientHandshakeContext.java</li>
<ul>
<li>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."</li>
</ul>
<li>ClientHello.java</li>
<ul>
<li>1155-1158: Masking with 0xFF is not necessary for these
primitive narrowing conversions.</li>
</ul>
<li>ContentType.java</li>
<ul>
<li>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".<br>
</li>
</ul>
<li>KeyUpdate.java</li>
<ul>
<li>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.</li>
</ul>
<li>RSAServerKeyExchange.java</li>
<ul>
<li>301: Comment nit: Should be RSA public key, not EC.</li>
</ul>
<li>SSLExtension.java</li>
<ul>
<li>284, and others, including other files (e.g.
SSLExtensions.java): Spelling nit, Change "onLoadConcumer" to
"onLoadConsumer"<br>
</li>
<li>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?</li>
</ul>
<li>SSLExtensions.java</li>
<ul>
<li>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.</li>
</ul>
<li>SSLHandshake.java</li>
<ul>
<li>128, possibly used in other files: Spelling nit,
CERTIFICARE_REQUEST --> CERTIFICATE_REQUEST</li>
<li>253: Maybe add a known-but-not-supported entry for
MESSAGE_HASH ((byte)0xFE, "message_hash");</li>
</ul>
<li>TransportContext.java</li>
<ul>
<li>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?</li>
<li>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?</li>
</ul>
</ul>
<p>Thanks,</p>
<p>--Jamil<br>
</p>
</body>
</html>