You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/11/26 14:40:20 UTC
[GitHub] [tomcat] markt-asf opened a new pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
markt-asf opened a new pull request #380:
URL: https://github.com/apache/tomcat/pull/380
https://bz.apache.org/bugzilla/show_bug.cgi?id=64110
I'm providing a PR for this one as the additional attributes to add some additional overhead. The overhead is limited to keeping two lists of strings: one of requested protocol names and one of requested cipher names. In both cases the names are already in memory so the footprint will be limited to the lists and array of object references. The exceptions are the relatively new GREASE values (see RFC 8701). I don't think they'll cause too much overhead (and there are ways to workaround that if they do - namely do exactly what RFC 8701 doesn't want us to do and enumerate them).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf merged pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
markt-asf merged pull request #380:
URL: https://github.com/apache/tomcat/pull/380
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on a change in pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on a change in pull request #380:
URL: https://github.com/apache/tomcat/pull/380#discussion_r535618925
##########
File path: java/org/apache/tomcat/util/net/SecureNio2Channel.java
##########
@@ -70,6 +73,8 @@
protected boolean closed;
protected boolean closing;
+ private Map<String,List<String>> additionalTlsAttributes = new HashMap<>();
Review comment:
👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on a change in pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on a change in pull request #380:
URL: https://github.com/apache/tomcat/pull/380#discussion_r535319757
##########
File path: java/org/apache/tomcat/util/net/TLSClientHelloExtractor.java
##########
@@ -193,6 +214,15 @@ public String getSNIValue() {
}
+ public List<String> getClientRequestedCipherNames() {
+ if (result == ExtractorResult.COMPLETE || result == ExtractorResult.NOT_PRESENT) {
+ return clientRequestedCipherNames;
+ } else {
+ throw new IllegalStateException();
Review comment:
-1 to having no message
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #380:
URL: https://github.com/apache/tomcat/pull/380#discussion_r535499442
##########
File path: java/org/apache/tomcat/util/net/SecureNioChannel.java
##########
@@ -68,6 +71,8 @@
protected boolean closed = false;
protected boolean closing = false;
+ private Map<String,List<String>> additionalTlsAttributes = new HashMap<>();
Review comment:
Will do
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #380:
URL: https://github.com/apache/tomcat/pull/380#discussion_r535501364
##########
File path: java/org/apache/tomcat/util/net/TLSClientHelloExtractor.java
##########
@@ -193,6 +214,15 @@ public String getSNIValue() {
}
+ public List<String> getClientRequestedCipherNames() {
+ if (result == ExtractorResult.COMPLETE || result == ExtractorResult.NOT_PRESENT) {
+ return clientRequestedCipherNames;
+ } else {
+ throw new IllegalStateException();
Review comment:
I'll add a message. Force push to follow shortly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on a change in pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on a change in pull request #380:
URL: https://github.com/apache/tomcat/pull/380#discussion_r535344046
##########
File path: java/org/apache/tomcat/util/net/TLSClientHelloExtractor.java
##########
@@ -202,6 +232,15 @@ public String getSNIValue() {
}
+ public List<String> getClientRequestedProtocols() {
+ if (result == ExtractorResult.COMPLETE || result == ExtractorResult.NOT_PRESENT) {
+ return clientRequestedProtocols;
+ } else {
+ throw new IllegalStateException();
Review comment:
-1 to having no detail message
##########
File path: java/org/apache/tomcat/util/net/TLSClientHelloExtractor.java
##########
@@ -72,7 +76,9 @@ public TLSClientHelloExtractor(ByteBuffer netInBuffer) throws IOException {
int limit = netInBuffer.limit();
ExtractorResult result = ExtractorResult.NOT_PRESENT;
List<Cipher> clientRequestedCiphers = new ArrayList<>();
+ List<String> clientRequestedCipherNames = new ArrayList<>();
List<String> clientRequestedApplicationProtocols = new ArrayList<>();
+ List<String> clientRequestedProtocols = new ArrayList<>();
Review comment:
Clients can only request a single protocol version. If there is a mismatch, the Server Hello response will propose another protocol. This will continue until the two agree or decide to terminate the connection. Is this list of protocols intended to capture that back-and-forth?
Oh, wait. `supported_versions` extension and TLSv1.3.
https://tools.ietf.org/html/rfc8446#section-4.2.1
LGTM!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] martin-g commented on a change in pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #380:
URL: https://github.com/apache/tomcat/pull/380#discussion_r532527179
##########
File path: java/org/apache/catalina/util/TLSUtil.java
##########
@@ -38,6 +38,8 @@ public static boolean isTLSRequestAttribute(String name) {
Globals.KEY_SIZE_ATTR.equals(name) ||
Globals.SSL_SESSION_ID_ATTR.equals(name) ||
Globals.SSL_SESSION_MGR_ATTR.equals(name) ||
- SSLSupport.PROTOCOL_VERSION_KEY.equals(name);
+ SSLSupport.PROTOCOL_VERSION_KEY.equals(name) ||
+ SSLSupport.REQUESTED_PROTOCOL_VERSIONS_KEY.equals(name) ||
+ SSLSupport.REQUESTED_CIPHERS_KEY.equals(name);
Review comment:
I think this would look and read nicer if we use `switch (String)`. Extra benefit: it will be faster!
##########
File path: java/org/apache/tomcat/util/net/SecureNioChannel.java
##########
@@ -68,6 +71,8 @@
protected boolean closed = false;
protected boolean closing = false;
+ private Map<String,List<String>> additionalTlsAttributes = new HashMap<>();
Review comment:
`final`
##########
File path: java/org/apache/tomcat/util/net/TLSClientHelloExtractor.java
##########
@@ -193,6 +214,15 @@ public String getSNIValue() {
}
+ public List<String> getClientRequestedCipherNames() {
+ if (result == ExtractorResult.COMPLETE || result == ExtractorResult.NOT_PRESENT) {
+ return clientRequestedCipherNames;
+ } else {
+ throw new IllegalStateException();
Review comment:
Intentionally without a message ?
##########
File path: java/org/apache/tomcat/util/net/SecureNio2Channel.java
##########
@@ -70,6 +73,8 @@
protected boolean closed;
protected boolean closing;
+ private Map<String,List<String>> additionalTlsAttributes = new HashMap<>();
Review comment:
I see there are some `volatile` member fields in this class. Should `sniComplete` be `volatile` as well ?
##########
File path: java/org/apache/tomcat/util/net/SecureNio2Channel.java
##########
@@ -70,6 +73,8 @@
protected boolean closed;
protected boolean closing;
+ private Map<String,List<String>> additionalTlsAttributes = new HashMap<>();
Review comment:
could be `final`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #380:
URL: https://github.com/apache/tomcat/pull/380#discussion_r535501143
##########
File path: java/org/apache/tomcat/util/net/SecureNio2Channel.java
##########
@@ -70,6 +73,8 @@
protected boolean closed;
protected boolean closing;
+ private Map<String,List<String>> additionalTlsAttributes = new HashMap<>();
Review comment:
Yes, sniComplete should be volatile.
The reason to use Map<String,List<String>> is that the attribute is exposed to user code so I don;t want it to be an internal Tomcat class. If we get this added to the spec then we could define a class for it in the spec.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on a change in pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on a change in pull request #380:
URL: https://github.com/apache/tomcat/pull/380#discussion_r535327550
##########
File path: java/org/apache/tomcat/util/net/SecureNio2Channel.java
##########
@@ -70,6 +73,8 @@
protected boolean closed;
protected boolean closing;
+ private Map<String,List<String>> additionalTlsAttributes = new HashMap<>();
Review comment:
Is there a reason to use a Map of String-Lists here, instead of having a more concrete object with well-defined members? Hash maps are quick, but since we know the "names" of these things in advance, why not use a custom class for this purpose?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #380: Fix BZ 64110 - request attr for requested ciphers and protocols
Posted by GitBox <gi...@apache.org>.
markt-asf commented on a change in pull request #380:
URL: https://github.com/apache/tomcat/pull/380#discussion_r535499222
##########
File path: java/org/apache/catalina/util/TLSUtil.java
##########
@@ -38,6 +38,8 @@ public static boolean isTLSRequestAttribute(String name) {
Globals.KEY_SIZE_ATTR.equals(name) ||
Globals.SSL_SESSION_ID_ATTR.equals(name) ||
Globals.SSL_SESSION_MGR_ATTR.equals(name) ||
- SSLSupport.PROTOCOL_VERSION_KEY.equals(name);
+ SSLSupport.PROTOCOL_VERSION_KEY.equals(name) ||
+ SSLSupport.REQUESTED_PROTOCOL_VERSIONS_KEY.equals(name) ||
+ SSLSupport.REQUESTED_CIPHERS_KEY.equals(name);
Review comment:
Will do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org