We could have some enhancement in the current TLS 1.3 implementation. This is a collection of them.
1. sun/security/ssl/SignatureScheme.java
Valerie:
Do we need a constructor with both NamedGroup and SigAlgParamSpec? NamedGroup is for EC and SigAlgParamSpec is for RSASSA-PSS, they shouldn't co-exist in the same scheme, right?
There are currently 5 constructors in SignatureScheme class, I find the ordering of arguments somewhat confusing. The required arguments should appear first and we can add optional ones after them. Easier to read this way, at least for me...
2. cipher suites preference
Brad: Since we're down in this code anyway, this is probably a good time to revisit our default ciphersuite ordering scheme.
3. Performance impact of CipherSuite name look up
Brad: I'm concerned of the performance impact of repeatedly iterating over 300+ ciphersuite ids/names using values(). You should benchmark and see if it makes sense to switch to using a HashMap (or even TreeMap) here. For the limited number of Protocols (< 10 for TLSv1.x), this approach is fine, but this has quite a bit larger search space.
4. Remove the temporary tls13VN (done 2018/08/15)
5. Combing SSLSocketImpl contructors
Brad: Any chance of combining the commonalities of these different constructors into 1, and then do the constructor-specific things at the end? It will help with future maintenance to not have to make the same changes in multiple places.
6. SSLLogger
Weijun: Since SSLConsoleLogger is a Logger child, its log(..., params) method should follow the Logger::log spec, which has
/**
* Logs a message with resource bundle and an optional list of
* parameters.
* ...
* @param format the string message format in {@link
* java.text.MessageFormat} format, (or a key in the message
* catalog if {@code bundle} is not {@code null}); can be {@code null}.
* @param params an optional list of parameters to the message (may be
* none).
* ...
*/
public void log(Level level, ResourceBundle bundle, String format,
Object... params);
Especially, the String parameter is in MessageFormat format (that's why it's named "format" but not "message").
Daniel Fuchs: Just a note that it might be a better idea to rework the implementation of SSLLogger/SSLConsoleLogger a bit in order to have SSLLogger implement System.Logger. This would ensure that the SSLLogger class is skipped when looking for the caller, when the underlying logger is a logger returned by System.getogger() and the backend is java.util.logging. Otherwise the caller will appear to be the static methods defined on the SSLLogger class itself.
7. nameOf or ofName, or of().
Weijun: nameOf() sounds like it would return a name. Given the return type is a value, maybe simply of()?
Brad:
SSLEngineImpl.java
------------------
757: At line 735, you use
CipherSuite.validValuesOf(suites);
but here you use:
ProtocolVersion.namesOf(protocols);
Seems like it's basically the same kind of method, do you want to make the names consistent.
8. SSLConfiguration, 'noSniExtension/Matcher' is confusing.
Valerie: I have some doubt on line 191 and 198, does "noSniExtension/Matcher" means "no SNI Extension/Matcher"?
9. Move setUseClientMode(boolean) implementation details to SSLConfiguration.
Brad: (SSLServerSocketImpl.java) Can we move this sslConfig code (157-167) to a method in SSLConfiguration instead? The logic here in SSLServerSocketImpl uses sslContext.* 8 times (e.g. sslContext.getDefaultProtocolVersions(!useClientMode)). This code is very implementation-dependent on SSLConfiguration's internals, and thus a good encapsulation candidate.
10. typos
Brad:
CertSignAlgsExtension.java: List<SignatureScheme> shemes =
->
CertSignAlgsExtension.java: List<SignatureScheme> schemes =
SignatureScheme.java: "Ignore disabled signature sheme: " + ss.name);
SignatureScheme.java: "Ignore inactive signature sheme: " + ss.name);
->
SignatureScheme.java: "Ignore disabled signature scheme: " + ss.name);
SignatureScheme.java: "Ignore inactive signature scheme: " + ss.name);
Missing "c"
11. Brad
I've noticed lots of enhanced for looping where the less common or preferred things like enums are listed first. If you're going to do this coding style, you might put the common ones up front, and the less common at the end. e.g.
for (Map.Entry<HandshakeProducer,
ProtocolVersion[]> phe : handshakeProducers) {
for (ProtocolVersion pv : phe.getValue()) {
if (protocolVersion == pv) {
return phe.getKey();
This (protocolVersion == pv) goes through the producers list, which has 6 elements first. So it cycles through TLSv1.2/1.1/1/SSLv3/DTLS12/DTLS10, then finally back to the single element TLSv1.3. I noticed this in several places and seems like it could be improved.
12. Brad
We don't implement the certificate_authorities extension, i.e. we only look at the signature types. Thus our calls to any X509KM currently don't have a way to choose a name based on server's CA preference. This seems pretty significant. (This is likely covered by JDK-8206925)
13. Brad
Moved to JDK-8210474.
14. supported_versions extensions consumed twice. First during extension processing to determine which protocol to use, then again during actual T13 server_hello processing.
SSLExtension.consumeOnLoad:542
SSLExtensions.consumeOnLoad:186
ServerHello$ServerHelloConsumer.onServerHello:941
ServerHello$ServerHelloConsumer.consume:877
SSLHandhsake.consume:392
HandshakeContext.dispatch:441
HandshakeContext.dispatch:419
TransportContext.dispatch:177
SSLTransport.decode:164
SSLSocketImpl.decode:1180
SSLSocketImpl.readHandshakeRecord:1091
SSLExtension.consumeOnLoad:542
SSLExtensions.consumeOnLoad:186
ServerHello$T13ServerHelloConsumer.consume:1207
ServerHello$ServerHelloConsumer.onServerHello:989
ServerHello$ServerHelloConsumer.consume:877
SSLHandhsake.consume:392
HandshakeContext.dispatch:441
HandshakeContext.dispatch:419
TransportContext.dispatch:177
SSLTransport.decode:164
SSLSocketImpl.decode:1180
SSLSocketImpl.readHandshakeRecord:1091
We end up with two identical debug output messages.