Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTPCLIENT-2047: fixed regression in DefaultHostnameVerifier #203

Merged
merged 2 commits into from
Jan 29, 2020
Merged

HTTPCLIENT-2047: fixed regression in DefaultHostnameVerifier #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a7fb16
Bug fix: fixed handling of private domains by PublicSuffixMatcher
ok2c Jan 25, 2020
67b5e22
HTTPCLIENT-2047: fixed regression in DefaultHostnameVerifier causing …
ok2c Jan 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions httpclient/src/main/java/org/apache/http/conn/ssl/DefaultHostnameVerifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ static void matchDNSName(final String host, final List<SubjectName> subjectAlts,
final SubjectName subjectAlt = subjectAlts.get(i);
if (subjectAlt.getType() == SubjectName.DNS) {
final String normalizedSubjectAlt = DnsUtils.normalize(subjectAlt.getValue());
if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt, publicSuffixMatcher, DomainType.ICANN)) {
if (matchIdentityStrict(normalizedHost, normalizedSubjectAlt, publicSuffixMatcher)) {
return;
}
}
Expand All @@ -182,7 +182,7 @@ static void matchCN(final String host, final String cn,
final PublicSuffixMatcher publicSuffixMatcher) throws SSLException {
final String normalizedHost = DnsUtils.normalize(host);
final String normalizedCn = DnsUtils.normalize(cn);
if (!matchIdentityStrict(normalizedHost, normalizedCn, publicSuffixMatcher, DomainType.ICANN)) {
if (!matchIdentityStrict(normalizedHost, normalizedCn, publicSuffixMatcher)) {
throw new SSLPeerUnverifiedException("Certificate for <" + host + "> doesn't match " +
"common name of the certificate subject: " + cn);
}
Expand Down
31 changes: 18 additions & 13 deletions httpclient/src/main/java/org/apache/http/conn/util/PublicSuffixMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,20 +97,15 @@ public PublicSuffixMatcher(final Collection<PublicSuffixList> lists) {
}
}

private static boolean hasEntry(final Map<String, DomainType> map, final String rule, final DomainType expectedType) {
private static DomainType findEntry(final Map<String, DomainType> map, final String rule) {
if (map == null) {
return false;
return null;
}
final DomainType domainType = map.get(rule);
return domainType == null ? false : expectedType == null || domainType.equals(expectedType);
}

private boolean hasRule(final String rule, final DomainType expectedType) {
return hasEntry(this.rules, rule, expectedType);
return map.get(rule);
}

private boolean hasException(final String exception, final DomainType expectedType) {
return hasEntry(this.exceptions, exception, expectedType);
private static boolean match(final DomainType domainType, final DomainType expectedType) {
return domainType != null && (expectedType == null || domainType.equals(expectedType));
}

/**
Expand Down Expand Up @@ -147,18 +142,27 @@ public String getDomainRoot(final String domain, final DomainType expectedType)
while (segment != null) {
// An exception rule takes priority over any other matching rule.
final String key = IDN.toUnicode(segment);
if (hasException(key, expectedType)) {
final DomainType exceptionRule = findEntry(exceptions, key);
if (match(exceptionRule, expectedType)) {
return segment;
}
if (hasRule(key, expectedType)) {
final DomainType domainRule = findEntry(rules, key);
if (match(domainRule, expectedType)) {
if (domainRule == DomainType.PRIVATE) {
return segment;
}
return result;
}

final int nextdot = segment.indexOf('.');
final String nextSegment = nextdot != -1 ? segment.substring(nextdot + 1) : null;

if (nextSegment != null) {
if (hasRule("*." + IDN.toUnicode(nextSegment), expectedType)) {
final DomainType wildcardDomainRule = findEntry(rules, "*." + IDN.toUnicode(nextSegment));
if (match(wildcardDomainRule, expectedType)) {
if (wildcardDomainRule == DomainType.PRIVATE) {
return segment;
}
return result;
}
}
Expand All @@ -174,6 +178,7 @@ public String getDomainRoot(final String domain, final DomainType expectedType)
// If we did have expectations apparently there was no match
return null;
}

/**
* Tests whether the given domain matches any of entry from the public suffix list.
*/
Expand Down
26 changes: 26 additions & 0 deletions httpclient/src/test/java/org/apache/http/conn/ssl/TestDefaultHostnameVerifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.security.cert.CertificateFactory;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;

import javax.net.ssl.SSLException;
Expand Down Expand Up @@ -375,6 +376,7 @@ public void testHTTPCLIENT_1997_UNKNOWN() { // Only True on all domains (same as
Assert.assertTrue(DefaultHostnameVerifier.matchIdentity( "service.apps." + domain, "*.apps." + domain, publicSuffixMatcher, DomainType.UNKNOWN));
Assert.assertTrue(DefaultHostnameVerifier.matchIdentityStrict( "service.apps." + domain, "*.apps." + domain, publicSuffixMatcher, DomainType.UNKNOWN));
}

@Test // Check compressed IPv6 hostname matching
public void testHTTPCLIENT_1316() throws Exception{
final String host1 = "2001:0db8:aaaa:bbbb:cccc:0:0:0001";
Expand Down Expand Up @@ -417,4 +419,28 @@ public void testExtractCN() throws Exception {
}
}

@Test
public void testMatchDNSName() throws Exception {
DefaultHostnameVerifier.matchDNSName(
"host.domain.com",
Collections.singletonList(SubjectName.DNS("*.domain.com")),
publicSuffixMatcher);
DefaultHostnameVerifier.matchDNSName(
"host.xx",
Collections.singletonList(SubjectName.DNS("*.xx")),
publicSuffixMatcher);
DefaultHostnameVerifier.matchDNSName(
"host.appspot.com",
Collections.singletonList(SubjectName.DNS("*.appspot.com")),
publicSuffixMatcher);
DefaultHostnameVerifier.matchDNSName(
"demo-s3-bucket.s3.eu-central-1.amazonaws.com",
Collections.singletonList(SubjectName.DNS("*.s3.eu-central-1.amazonaws.com")),
publicSuffixMatcher);
DefaultHostnameVerifier.matchDNSName(
"hostname-workspace-1.local",
Collections.singletonList(SubjectName.DNS("hostname-workspace-1.local")),
publicSuffixMatcher);
}

}
18 changes: 10 additions & 8 deletions httpclient/src/test/java/org/apache/http/conn/util/TestPublicSuffixMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ public void setUp() throws Exception {
@Test
public void testGetDomainRootAnyType() {
// Private
Assert.assertEquals("example.xx", matcher.getDomainRoot("example.XX"));
Assert.assertEquals("example.xx", matcher.getDomainRoot("www.example.XX"));
Assert.assertEquals("example.xx", matcher.getDomainRoot("www.blah.blah.example.XX"));
Assert.assertEquals("xx", matcher.getDomainRoot("example.XX"));
Assert.assertEquals("xx", matcher.getDomainRoot("www.example.XX"));
Assert.assertEquals("xx", matcher.getDomainRoot("www.blah.blah.example.XX"));
Assert.assertEquals("appspot.com", matcher.getDomainRoot("example.appspot.com"));
// Too short
Assert.assertEquals(null, matcher.getDomainRoot("xx"));
Assert.assertEquals(null, matcher.getDomainRoot("jp"));
Assert.assertEquals(null, matcher.getDomainRoot("ac.jp"));
Assert.assertEquals(null, matcher.getDomainRoot("any.tokyo.jp"));
Expand All @@ -79,11 +79,11 @@ public void testGetDomainRootAnyType() {
@Test
public void testGetDomainRootOnlyPRIVATE() {
// Private
Assert.assertEquals("example.xx", matcher.getDomainRoot("example.XX", DomainType.PRIVATE));
Assert.assertEquals("example.xx", matcher.getDomainRoot("www.example.XX", DomainType.PRIVATE));
Assert.assertEquals("example.xx", matcher.getDomainRoot("www.blah.blah.example.XX", DomainType.PRIVATE));
Assert.assertEquals("xx", matcher.getDomainRoot("example.XX", DomainType.PRIVATE));
Assert.assertEquals("xx", matcher.getDomainRoot("www.example.XX", DomainType.PRIVATE));
Assert.assertEquals("xx", matcher.getDomainRoot("www.blah.blah.example.XX", DomainType.PRIVATE));
Assert.assertEquals("appspot.com", matcher.getDomainRoot("example.appspot.com"));
// Too short
Assert.assertEquals(null, matcher.getDomainRoot("xx", DomainType.PRIVATE));
Assert.assertEquals(null, matcher.getDomainRoot("jp", DomainType.PRIVATE));
Assert.assertEquals(null, matcher.getDomainRoot("ac.jp", DomainType.PRIVATE));
Assert.assertEquals(null, matcher.getDomainRoot("any.tokyo.jp", DomainType.PRIVATE));
Expand Down Expand Up @@ -128,6 +128,8 @@ public void testMatch() {
Assert.assertTrue(matcher.matches(".any.tokyo.jp"));
// exception
Assert.assertFalse(matcher.matches(".metro.tokyo.jp"));
Assert.assertFalse(matcher.matches(".xx"));
Assert.assertFalse(matcher.matches(".appspot.com"));
}

@Test
Expand Down
2 changes: 2 additions & 0 deletions httpclient/src/test/resources/suffixlistmatcher.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
// ===BEGIN PRIVATE DOMAINS===
xx
lan
appspot.com
s3.eu-central-1.amazonaws.com
// ===END PRIVATE DOMAINS===

// ===BEGIN ICANN DOMAINS===
Expand Down