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
