You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by GitBox <gi...@apache.org> on 2021/04/09 12:36:11 UTC
[GitHub] [httpcomponents-client] peterdettman opened a new pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
peterdettman opened a new pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302
https://issues.apache.org/jira/browse/HTTPCLIENT-2149
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] peterdettman commented on a change in pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
peterdettman commented on a change in pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#discussion_r610667052
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
##########
@@ -114,18 +114,15 @@ public void verify(
matchIPv6Address(host, subjectAlts);
break;
default:
- matchDNSName(host, subjectAlts, this.publicSuffixMatcher);
+ // In case there are no SubjectName.DNS entries, fallback to CN matching
Review comment:
For comparison, SunJSSE uses [HostnameChecker](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/util/HostnameChecker.java) for the equivalent checks when endpoint identification is set to HTTPS. Likewise our (BCJSSE provider) uses [HostnameUtil](https://github.com/bcgit/bc-java/blob/master/tls/src/main/java/org/bouncycastle/jsse/provider/HostnameUtil.java).
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] peterdettman commented on pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
peterdettman commented on pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#issuecomment-817095363
I've removed the redundancy. Note that `#getSubjectAltNames(X509Certificate)` now will return all names (that have String values), not just DNS, IP. For the only existing caller (`TlsSessionValidator` failure logging), that seems OK.
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#discussion_r610618689
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
##########
@@ -114,18 +114,15 @@ public void verify(
matchIPv6Address(host, subjectAlts);
break;
default:
- matchDNSName(host, subjectAlts, this.publicSuffixMatcher);
+ // In case there are no SubjectName.DNS entries, fallback to CN matching
Review comment:
@peterdettman I only expected changes in handling of URIs specified as an IP address rather than a hostname.
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#discussion_r610636445
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
##########
@@ -114,18 +114,15 @@ public void verify(
matchIPv6Address(host, subjectAlts);
break;
default:
- matchDNSName(host, subjectAlts, this.publicSuffixMatcher);
+ // In case there are no SubjectName.DNS entries, fallback to CN matching
Review comment:
@peterdettman Then what is the point in calling `#matchCN` here?
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
ok2c commented on pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#issuecomment-817085033
@peterdettman This is so much better. One last thing. Is there a chance we could avoid having two almost identical `#getSubjectAltNames` methods?
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] peterdettman commented on pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
peterdettman commented on pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#issuecomment-817076218
Have updated so that matchCN is no longer called when host is an IP address, and to clarify the handling for the DNS case.
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c merged pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
ok2c merged pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] peterdettman commented on a change in pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
peterdettman commented on a change in pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#discussion_r610634373
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
##########
@@ -114,18 +114,15 @@ public void verify(
matchIPv6Address(host, subjectAlts);
break;
default:
- matchDNSName(host, subjectAlts, this.publicSuffixMatcher);
+ // In case there are no SubjectName.DNS entries, fallback to CN matching
Review comment:
Yes, matchDNSNames is only returning false if there were no dNSName at all. If there were any present that didn’t match, it would have already thrown an exception.
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#discussion_r610844241
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
##########
@@ -114,18 +114,15 @@ public void verify(
matchIPv6Address(host, subjectAlts);
break;
default:
- matchDNSName(host, subjectAlts, this.publicSuffixMatcher);
+ // In case there are no SubjectName.DNS entries, fallback to CN matching
Review comment:
> Perhaps the code could be clearer if we only collected either IP or DNS entries depending on the HostNameType?
@peterdettman This is what I was trying to hint at. I really do not understand why there should be multiple invocations of `#matchCN`. Please refactor the code to make the execution flow as well as the intent of your changes a bit easier to understand.
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] peterdettman commented on a change in pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
peterdettman commented on a change in pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#discussion_r610655455
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
##########
@@ -114,18 +114,15 @@ public void verify(
matchIPv6Address(host, subjectAlts);
break;
default:
- matchDNSName(host, subjectAlts, this.publicSuffixMatcher);
+ // In case there are no SubjectName.DNS entries, fallback to CN matching
Review comment:
Perhaps the code could be clearer if we only collected either IP or DNS entries depending on the HostNameType?
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] peterdettman commented on a change in pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
peterdettman commented on a change in pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#discussion_r610651915
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
##########
@@ -114,18 +114,15 @@ public void verify(
matchIPv6Address(host, subjectAlts);
break;
default:
- matchDNSName(host, subjectAlts, this.publicSuffixMatcher);
+ // In case there are no SubjectName.DNS entries, fallback to CN matching
Review comment:
That is the fallback if there were no dNSName entries to even check against. RFC 2818 3.1: “If a subjectAltName extension of type dNSName is present, that MUST be used as the identity. Otherwise, the (most specific) Common Name field in the Subject field of the certificate MUST be used.”. Note that IP addresses are singled out further down: “In some cases, the URI is specified as an IP address rather than a hostname. In this case, the iPAddress subjectAltName must be present in the certificate and must exactly match the IP in the URI.”.
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-client] ok2c commented on a change in pull request #302: HTTPCLIENT-2149: When no dNSName, match against CN
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #302:
URL: https://github.com/apache/httpcomponents-client/pull/302#discussion_r610616621
##########
File path: httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultHostnameVerifier.java
##########
@@ -114,18 +114,15 @@ public void verify(
matchIPv6Address(host, subjectAlts);
break;
default:
- matchDNSName(host, subjectAlts, this.publicSuffixMatcher);
+ // In case there are no SubjectName.DNS entries, fallback to CN matching
Review comment:
@peterdettman I am not sure this is what the RFC says. I believe that CN matching is to be used _only_ if there are no `dNSName` entries present.
--
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@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org