From fc028b42655f6ae949ee7d9070acae88daee8315 Mon Sep 17 00:00:00 2001 From: Cory Benfield Date: Fri, 14 Aug 2020 08:00:04 +0100 Subject: [PATCH] Stop using commonName to validate certificates. Motivation: CommonName as part of certificate hostname verification has been deprecated for more than two decades. Browsers have finally stopped using the commonName as part of verification, and the WebPKI forbids putting names into the commonName that are not also part of the subjectAlternativeName extension. Continuing to trust the commonName as part of the certificate hostname will put us in a weaker position than most of the browser ecosystem. That's not a good place to be, so we should flip our default to not trusting the commonName any longer. For pragmatic reasons, we do allow an override, but the default configuration is off. Users using this override will likely find themselves disappointed when we remove it at some point in the future. Modifications: - Added TLSConfiguration options to enable trusting commonName fields for hostname verification. - Made investigating the commonName field optional during hostname verification. - Tests to validate the new behaviour. Result: No trusting common names by default anymore. --- Sources/NIOSSL/IdentityVerification.swift | 27 ++-- Sources/NIOSSL/SSLConnection.swift | 3 +- Sources/NIOSSL/TLSConfiguration.swift | 118 ++++++++++++++++-- .../IdentityVerificationTest+XCTest.swift | 2 + .../IdentityVerificationTest.swift | 102 ++++++++++----- .../NIOSSLIntegrationTest+XCTest.swift | 2 + Tests/NIOSSLTests/NIOSSLIntegrationTest.swift | 71 +++++++++++ Tests/NIOSSLTests/NIOSSLTestHelpers.swift | 9 +- Tests/NIOSSLTests/SSLCertificateTest.swift | 2 +- 9 files changed, 280 insertions(+), 56 deletions(-) diff --git a/Sources/NIOSSL/IdentityVerification.swift b/Sources/NIOSSL/IdentityVerification.swift index 4d57220f..7b27166b 100644 --- a/Sources/NIOSSL/IdentityVerification.swift +++ b/Sources/NIOSSL/IdentityVerification.swift @@ -87,23 +87,27 @@ extension UInt8 { /// follow along at home. internal func validIdentityForService(serverHostname: String?, socketAddress: SocketAddress, - leafCertificate: NIOSSLCertificate) throws -> Bool { + leafCertificate: NIOSSLCertificate, + includeCommonName: Bool) throws -> Bool { if let serverHostname = serverHostname { return try serverHostname.withLowercaseASCIIBuffer { try validIdentityForService(serverHostname: $0, socketAddress: socketAddress, - leafCertificate: leafCertificate) + leafCertificate: leafCertificate, + includeCommonName: includeCommonName) } } else { return try validIdentityForService(serverHostname: nil as Array?, socketAddress: socketAddress, - leafCertificate: leafCertificate) + leafCertificate: leafCertificate, + includeCommonName: includeCommonName) } } private func validIdentityForService(serverHostname: Array?, socketAddress: SocketAddress, - leafCertificate: NIOSSLCertificate) throws -> Bool { + leafCertificate: NIOSSLCertificate, + includeCommonName: Bool) throws -> Bool { // Before we begin, we want to locate the first period in our own domain name. We need to do // this because we may need to match a wildcard label. var serverHostnameSlice: ArraySlice? = nil @@ -150,16 +154,15 @@ private func validIdentityForService(serverHostname: Array?, } // In the absence of any matchable subjectAlternativeNames, we can fall back to checking - // the common name. This is a deprecated practice, and in a future release we should - // stop doing this. - guard let commonName = leafCertificate.commonName() else { - // No CN, no match. + // the common name. We only do this if the user has configured a system that trusts the commonName field. + if includeCommonName, let commonName = leafCertificate.commonName() { + // We have a common name. Let's check it against the provided hostname. We never check + // the common name against the IP address. + return matchHostname(ourHostname: serverHostnameSlice, firstPeriodIndex: firstPeriodIndex, dnsName: commonName) + } else { + // We don't trust the common name, or we do but don't have one to look at. return false } - - // We have a common name. Let's check it against the provided hostname. We never check - // the common name against the IP address. - return matchHostname(ourHostname: serverHostnameSlice, firstPeriodIndex: firstPeriodIndex, dnsName: commonName) } diff --git a/Sources/NIOSSL/SSLConnection.swift b/Sources/NIOSSL/SSLConnection.swift index 814debab..dac9c578 100644 --- a/Sources/NIOSSL/SSLConnection.swift +++ b/Sources/NIOSSL/SSLConnection.swift @@ -213,7 +213,8 @@ internal final class SSLConnection { guard try validIdentityForService(serverHostname: self.expectedHostname, socketAddress: address, - leafCertificate: peerCert) else { + leafCertificate: peerCert, + includeCommonName: self.parentContext.configuration.trustCommonName) else { throw NIOSSLExtraError.failedToValidateHostname(expectedName: self.expectedHostname ?? "") } } diff --git a/Sources/NIOSSL/TLSConfiguration.swift b/Sources/NIOSSL/TLSConfiguration.swift index abf5ec57..127a21ef 100644 --- a/Sources/NIOSSL/TLSConfiguration.swift +++ b/Sources/NIOSSL/TLSConfiguration.swift @@ -303,6 +303,18 @@ public struct TLSConfiguration { /// Whether renegotiation is supported. public var renegotiationSupport: NIORenegotiationSupport + /// Whether to trust the commonName field when validating certificate hostnames. + /// + /// The commonName field of certificates has been deprecated for use as a validation source + /// for nearly two decades. As browsers and the WebPKI now forbid the usage of common names + /// in certificates, swift-nio-ssl defaults to this strict level of enforcement as well. + /// + /// However, some older infrastructure does not issue certificates with valid subject alternative + /// name fields. For this reason, we currently allow an override for trusting the common name. This + /// is a temporary measure to allow users to work around these limitations, but we will remove this + /// functionality in future. + public var trustCommonName: Bool + private init(cipherSuiteValues: [NIOTLSCipher] = [], cipherSuites: String = defaultCipherSuites, verifySignatureAlgorithms: [SignatureAlgorithm]?, @@ -317,7 +329,8 @@ public struct TLSConfiguration { shutdownTimeout: TimeAmount, keyLogCallback: NIOSSLKeyLogCallback?, renegotiationSupport: NIORenegotiationSupport, - additionalTrustRoots: [NIOSSLAdditionalTrustRoots]) { + additionalTrustRoots: [NIOSSLAdditionalTrustRoots], + trustCommonName: Bool) { self.cipherSuites = cipherSuites self.verifySignatureAlgorithms = verifySignatureAlgorithms self.signingSignatureAlgorithms = signingSignatureAlgorithms @@ -331,6 +344,7 @@ public struct TLSConfiguration { self.encodedApplicationProtocols = [] self.shutdownTimeout = shutdownTimeout self.renegotiationSupport = renegotiationSupport + self.trustCommonName = trustCommonName self.applicationProtocols = applicationProtocols self.keyLogCallback = keyLogCallback if !cipherSuiteValues.isEmpty { @@ -368,7 +382,8 @@ public struct TLSConfiguration { shutdownTimeout: shutdownTimeout, keyLogCallback: keyLogCallback, renegotiationSupport: .none, // Servers never support renegotiation: there's no point. - additionalTrustRoots: additionalTrustRoots) + additionalTrustRoots: additionalTrustRoots, + trustCommonName: false) } /// Create a TLS configuration for use with server-side contexts. @@ -398,7 +413,8 @@ public struct TLSConfiguration { shutdownTimeout: shutdownTimeout, keyLogCallback: keyLogCallback, renegotiationSupport: .none, // Servers never support renegotiation: there's no point. - additionalTrustRoots: []) + additionalTrustRoots: [], + trustCommonName: false) } /// Create a TLS configuration for use with server-side contexts. @@ -430,7 +446,8 @@ public struct TLSConfiguration { shutdownTimeout: shutdownTimeout, keyLogCallback: keyLogCallback, renegotiationSupport: .none, // Servers never support renegotiation: there's no point. - additionalTrustRoots: []) + additionalTrustRoots: [], + trustCommonName: false) } /// Create a TLS configuration for use with server-side contexts. @@ -463,7 +480,43 @@ public struct TLSConfiguration { shutdownTimeout: shutdownTimeout, keyLogCallback: keyLogCallback, renegotiationSupport: .none, // Servers never support renegotiation: there's no point. - additionalTrustRoots: additionalTrustRoots) + additionalTrustRoots: additionalTrustRoots, + trustCommonName: false) + } + + /// Create a TLS configuration for use with server-side contexts. + /// + /// This provides sensible defaults while requiring that you provide any data that is necessary + /// for server-side function. For client use, try `forClient` instead. + public static func forServer(certificateChain: [NIOSSLCertificateSource], + privateKey: NIOSSLPrivateKeySource, + cipherSuites: String = defaultCipherSuites, + verifySignatureAlgorithms: [SignatureAlgorithm]? = nil, + signingSignatureAlgorithms: [SignatureAlgorithm]? = nil, + minimumTLSVersion: TLSVersion = .tlsv1, + maximumTLSVersion: TLSVersion? = nil, + certificateVerification: CertificateVerification = .none, + trustRoots: NIOSSLTrustRoots = .default, + applicationProtocols: [String] = [], + shutdownTimeout: TimeAmount = .seconds(5), + keyLogCallback: NIOSSLKeyLogCallback? = nil, + additionalTrustRoots: [NIOSSLAdditionalTrustRoots] = [], + trustCommonName: Bool) -> TLSConfiguration { + return TLSConfiguration(cipherSuites: cipherSuites, + verifySignatureAlgorithms: verifySignatureAlgorithms, + signingSignatureAlgorithms: signingSignatureAlgorithms, + minimumTLSVersion: minimumTLSVersion, + maximumTLSVersion: maximumTLSVersion, + certificateVerification: certificateVerification, + trustRoots: trustRoots, + certificateChain: certificateChain, + privateKey: privateKey, + applicationProtocols: applicationProtocols, + shutdownTimeout: shutdownTimeout, + keyLogCallback: keyLogCallback, + renegotiationSupport: .none, // Servers never support renegotiation: there's no point. + additionalTrustRoots: additionalTrustRoots, + trustCommonName: trustCommonName) } /// Creates a TLS configuration for use with client-side contexts. This allows setting the `NIOTLSCipher` property specifically. @@ -497,7 +550,8 @@ public struct TLSConfiguration { shutdownTimeout: shutdownTimeout, keyLogCallback: keyLogCallback, renegotiationSupport: renegotiationSupport, - additionalTrustRoots: additionalTrustRoots) + additionalTrustRoots: additionalTrustRoots, + trustCommonName: false) } /// Creates a TLS configuration for use with client-side contexts. @@ -527,7 +581,8 @@ public struct TLSConfiguration { shutdownTimeout: shutdownTimeout, keyLogCallback: keyLogCallback, renegotiationSupport: .none, // Default value is here for backward-compatibility. - additionalTrustRoots: []) + additionalTrustRoots: [], + trustCommonName: false) } @@ -559,7 +614,8 @@ public struct TLSConfiguration { shutdownTimeout: shutdownTimeout, keyLogCallback: keyLogCallback, renegotiationSupport: renegotiationSupport, - additionalTrustRoots: []) + additionalTrustRoots: [], + trustCommonName: false) } /// Creates a TLS configuration for use with client-side contexts. @@ -592,7 +648,8 @@ public struct TLSConfiguration { shutdownTimeout: shutdownTimeout, keyLogCallback: keyLogCallback, renegotiationSupport: renegotiationSupport, - additionalTrustRoots: []) + additionalTrustRoots: [], + trustCommonName: false) } /// Creates a TLS configuration for use with client-side contexts. @@ -626,7 +683,44 @@ public struct TLSConfiguration { shutdownTimeout: shutdownTimeout, keyLogCallback: keyLogCallback, renegotiationSupport: renegotiationSupport, - additionalTrustRoots: additionalTrustRoots) + additionalTrustRoots: additionalTrustRoots, + trustCommonName: false) + } + + /// Creates a TLS configuration for use with client-side contexts. This allows setting the `NIOTLSCipher` property specifically. + /// + /// This provides sensible defaults, and can be used without customisation. For server-side + /// contexts, you should use `forServer` instead. + public static func forClient(cipherSuites: String = defaultCipherSuites, + verifySignatureAlgorithms: [SignatureAlgorithm]? = nil, + signingSignatureAlgorithms: [SignatureAlgorithm]? = nil, + minimumTLSVersion: TLSVersion = .tlsv1, + maximumTLSVersion: TLSVersion? = nil, + certificateVerification: CertificateVerification = .fullVerification, + trustRoots: NIOSSLTrustRoots = .default, + certificateChain: [NIOSSLCertificateSource] = [], + privateKey: NIOSSLPrivateKeySource? = nil, + applicationProtocols: [String] = [], + shutdownTimeout: TimeAmount = .seconds(5), + keyLogCallback: NIOSSLKeyLogCallback? = nil, + renegotiationSupport: NIORenegotiationSupport = .none, + additionalTrustRoots: [NIOSSLAdditionalTrustRoots] = [], + trustCommonName: Bool) -> TLSConfiguration { + return TLSConfiguration(cipherSuites: cipherSuites, + verifySignatureAlgorithms: verifySignatureAlgorithms, + signingSignatureAlgorithms: signingSignatureAlgorithms, + minimumTLSVersion: minimumTLSVersion, + maximumTLSVersion: maximumTLSVersion, + certificateVerification: certificateVerification, + trustRoots: trustRoots, + certificateChain: certificateChain, + privateKey: privateKey, + applicationProtocols: applicationProtocols, + shutdownTimeout: shutdownTimeout, + keyLogCallback: keyLogCallback, + renegotiationSupport: renegotiationSupport, + additionalTrustRoots: additionalTrustRoots, + trustCommonName: trustCommonName) } } @@ -657,7 +751,8 @@ extension TLSConfiguration { self.encodedApplicationProtocols == comparing.encodedApplicationProtocols && self.shutdownTimeout == comparing.shutdownTimeout && isKeyLoggerCallbacksEqual && - self.renegotiationSupport == comparing.renegotiationSupport + self.renegotiationSupport == comparing.renegotiationSupport && + self.trustCommonName == comparing.trustCommonName } /// Returns a best effort hash of this TLS configuration. @@ -682,5 +777,6 @@ extension TLSConfiguration { hasher.combine(bytes: closureBits) } hasher.combine(renegotiationSupport) + hasher.combine(trustCommonName) } } diff --git a/Tests/NIOSSLTests/IdentityVerificationTest+XCTest.swift b/Tests/NIOSSLTests/IdentityVerificationTest+XCTest.swift index b8a8d11c..29592a29 100644 --- a/Tests/NIOSSLTests/IdentityVerificationTest+XCTest.swift +++ b/Tests/NIOSSLTests/IdentityVerificationTest+XCTest.swift @@ -51,6 +51,8 @@ extension IdentityVerificationTest { ("testDoesNotMatchSANWithEmbeddedNULL", testDoesNotMatchSANWithEmbeddedNULL), ("testFallsBackToCommonName", testFallsBackToCommonName), ("testLowercasesForCommonName", testLowercasesForCommonName), + ("testDoesNotFallBackToCommonName", testDoesNotFallBackToCommonName), + ("testNoLowercasesForCommonName", testNoLowercasesForCommonName), ("testRejectsUnicodeCommonNameWithUnencodedIDNALabel", testRejectsUnicodeCommonNameWithUnencodedIDNALabel), ("testRejectsUnicodeCommonNameWithEncodedIDNALabel", testRejectsUnicodeCommonNameWithEncodedIDNALabel), ("testHandlesMissingCommonName", testHandlesMissingCommonName), diff --git a/Tests/NIOSSLTests/IdentityVerificationTest.swift b/Tests/NIOSSLTests/IdentityVerificationTest.swift index 74aba136..fedddc2e 100644 --- a/Tests/NIOSSLTests/IdentityVerificationTest.swift +++ b/Tests/NIOSSLTests/IdentityVerificationTest.swift @@ -75,7 +75,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiSanCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "localhost", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -83,7 +84,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiSanCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -91,7 +93,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiSanCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "example.com.", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -99,7 +102,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiSanCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "LoCaLhOsT", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -107,7 +111,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiSanCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "httpbin.org", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertFalse(matched) } @@ -115,7 +120,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiSanCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: nil, socketAddress: try .makeAddressResolvingHost("192.168.0.1", port: 443), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -127,7 +133,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiSanCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: nil, socketAddress: ipv6Address, - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -135,7 +142,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiSanCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: nil, socketAddress: try .makeAddressResolvingHost("192.168.0.2", port: 443), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertFalse(matched) } @@ -146,7 +154,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiSanCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: nil, socketAddress: ipv6Address, - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertFalse(matched) } @@ -154,7 +163,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "this.wildcard.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -162,7 +172,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "foo.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -170,7 +181,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "bar.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -178,7 +190,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "baz.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -186,7 +199,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "trailing.period.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -195,7 +209,8 @@ class IdentityVerificationTest: XCTestCase { XCTAssertThrowsError(try validIdentityForService(serverHostname: "straße.unicode.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert)) { error in + leafCertificate: cert, + includeCommonName: false)) { error in XCTAssertEqual(error as? NIOSSLExtraError, .serverHostnameImpossibleToMatch) XCTAssertEqual(String(describing: error), "NIOSSLExtraError.serverHostnameImpossibleToMatch: The server hostname straße.unicode.example.com cannot be matched due to containing non-DNS characters") @@ -207,7 +222,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "xn--strae-oqa.unicode.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -215,7 +231,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "xn--xx-gia.unicode.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertFalse(matched) } @@ -223,7 +240,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "weirdwildcard.nomatch.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertFalse(matched) } @@ -231,7 +249,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "one.two.double.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertFalse(matched) } @@ -240,7 +259,8 @@ class IdentityVerificationTest: XCTestCase { XCTAssertThrowsError(try validIdentityForService(serverHostname: "foo.straße.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert)) { error in + leafCertificate: cert, + includeCommonName: false)) { error in XCTAssertEqual(error as? NIOSSLExtraError, .serverHostnameImpossibleToMatch) XCTAssertEqual(String(describing: error), "NIOSSLExtraError.serverHostnameImpossibleToMatch: The server hostname foo.straße.example.com cannot be matched due to containing non-DNS characters") @@ -251,7 +271,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "foo.xn--strae-oqa.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertTrue(matched) } @@ -260,7 +281,8 @@ class IdentityVerificationTest: XCTestCase { XCTAssertThrowsError(try validIdentityForService(serverHostname: "nul\u{0000}l.example.com", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert)) { error in + leafCertificate: cert, + includeCommonName: false)) { error in XCTAssertEqual(error as? NIOSSLExtraError, .serverHostnameImpossibleToMatch) XCTAssertEqual(String(describing: error), "NIOSSLExtraError.serverHostnameImpossibleToMatch: The server hostname nul\u{0000}l.example.com cannot be matched due to containing non-DNS characters") @@ -271,7 +293,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiCNCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "localhost", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: true) XCTAssertTrue(matched) } @@ -279,16 +302,36 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(multiCNCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "LoCaLhOsT", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: true) XCTAssertTrue(matched) } + func testDoesNotFallBackToCommonName() throws { + let cert = try NIOSSLCertificate(bytes: .init(multiCNCert.utf8), format: .pem) + let matched = try validIdentityForService(serverHostname: "localhost", + socketAddress: try .init(unixDomainSocketPath: "/path"), + leafCertificate: cert, + includeCommonName: false) + XCTAssertFalse(matched) + } + + func testNoLowercasesForCommonName() throws { + let cert = try NIOSSLCertificate(bytes: .init(multiCNCert.utf8), format: .pem) + let matched = try validIdentityForService(serverHostname: "LoCaLhOsT", + socketAddress: try .init(unixDomainSocketPath: "/path"), + leafCertificate: cert, + includeCommonName: false) + XCTAssertFalse(matched) + } + func testRejectsUnicodeCommonNameWithUnencodedIDNALabel() throws { let cert = try NIOSSLCertificate(bytes: .init(unicodeCNCert.utf8), format: .pem) XCTAssertThrowsError(try validIdentityForService(serverHostname: "straße.org", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert)) { error in + leafCertificate: cert, + includeCommonName: false)) { error in XCTAssertEqual(error as? NIOSSLExtraError, .serverHostnameImpossibleToMatch) XCTAssertEqual(String(describing: error), "NIOSSLExtraError.serverHostnameImpossibleToMatch: The server hostname straße.org cannot be matched due to containing non-DNS characters") @@ -299,7 +342,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(unicodeCNCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "xn--strae-oqa.org", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertFalse(matched) } @@ -307,7 +351,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(noCNCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "localhost", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertFalse(matched) } @@ -315,7 +360,8 @@ class IdentityVerificationTest: XCTestCase { let cert = try NIOSSLCertificate(bytes: .init(weirdoPEMCert.utf8), format: .pem) let matched = try validIdentityForService(serverHostname: "httpbin.org", socketAddress: try .init(unixDomainSocketPath: "/path"), - leafCertificate: cert) + leafCertificate: cert, + includeCommonName: false) XCTAssertFalse(matched) } } diff --git a/Tests/NIOSSLTests/NIOSSLIntegrationTest+XCTest.swift b/Tests/NIOSSLTests/NIOSSLIntegrationTest+XCTest.swift index 94a344b6..b4177c4f 100644 --- a/Tests/NIOSSLTests/NIOSSLIntegrationTest+XCTest.swift +++ b/Tests/NIOSSLTests/NIOSSLIntegrationTest+XCTest.swift @@ -64,6 +64,8 @@ extension NIOSSLIntegrationTest { ("testLoadsOfCloses", testLoadsOfCloses), ("testWriteFromFailureOfWrite", testWriteFromFailureOfWrite), ("testTrustedFirst", testTrustedFirst), + ("testCommonNameFails", testCommonNameFails), + ("testCommonNameSucceedsIfOverrideApplied", testCommonNameSucceedsIfOverrideApplied), ] } } diff --git a/Tests/NIOSSLTests/NIOSSLIntegrationTest.swift b/Tests/NIOSSLTests/NIOSSLIntegrationTest.swift index 3c167b41..e33a5213 100644 --- a/Tests/NIOSSLTests/NIOSSLIntegrationTest.swift +++ b/Tests/NIOSSLTests/NIOSSLIntegrationTest.swift @@ -407,6 +407,8 @@ fileprivate func _clientTLSChannel(context: NIOSSLCon class NIOSSLIntegrationTest: XCTestCase { static var cert: NIOSSLCertificate! static var key: NIOSSLPrivateKey! + static var noSANCert: NIOSSLCertificate! + static var noSANKey: NIOSSLPrivateKey! static var encryptedKeyPath: String! override class func setUp() { @@ -416,6 +418,9 @@ class NIOSSLIntegrationTest: XCTestCase { NIOSSLIntegrationTest.cert = cert NIOSSLIntegrationTest.key = key NIOSSLIntegrationTest.encryptedKeyPath = keyInFile(key: NIOSSLIntegrationTest.key, passphrase: "thisisagreatpassword") + let (noSANCert, noSANKey) = generateSelfSignedCert(withSAN: false) + NIOSSLIntegrationTest.noSANCert = noSANCert + NIOSSLIntegrationTest.noSANKey = noSANKey } override class func tearDown() { @@ -2079,4 +2084,70 @@ class NIOSSLIntegrationTest: XCTestCase { let newBuffer = try completionPromise.futureResult.wait() XCTAssertEqual(newBuffer, originalBuffer) } + + func testCommonNameFails() throws { + let config = TLSConfiguration.forServer(certificateChain: [.certificate(NIOSSLIntegrationTest.noSANCert)], + privateKey: .privateKey(NIOSSLIntegrationTest.noSANKey)) + let context = try assertNoThrowWithValue(NIOSSLContext(configuration: config)) + + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + defer { + XCTAssertNoThrow(try group.syncShutdownGracefully()) + } + + let serverChannel: Channel = try serverTLSChannel(context: context, handlers: [], group: group) + defer { + XCTAssertNoThrow(try serverChannel.close().wait()) + } + + let handshakeResultPromise = group.next().makePromise(of: Void.self) + let handshakeWatcher = WaitForHandshakeHandler(handshakeResultPromise: handshakeResultPromise) + let clientChannel = try clientTLSChannel(context: try configuredClientContext(), + preHandlers: [], + postHandlers: [handshakeWatcher], + group: group, + connectingTo: serverChannel.localAddress!, + serverHostname: "camembert") + defer { + // Ignore errors here, the channel should be closed already by the time this happens. + try? clientChannel.close().wait() + } + + XCTAssertThrowsError(try handshakeResultPromise.futureResult.wait()) + } + + func testCommonNameSucceedsIfOverrideApplied() throws { + let config = TLSConfiguration.forServer(certificateChain: [.certificate(NIOSSLIntegrationTest.noSANCert)], + privateKey: .privateKey(NIOSSLIntegrationTest.noSANKey)) + let context = try assertNoThrowWithValue(NIOSSLContext(configuration: config)) + + let group = MultiThreadedEventLoopGroup(numberOfThreads: 1) + defer { + XCTAssertNoThrow(try group.syncShutdownGracefully()) + } + + let serverChannel: Channel = try serverTLSChannel(context: context, handlers: [], group: group) + defer { + XCTAssertNoThrow(try serverChannel.close().wait()) + } + + let clientConfig = TLSConfiguration.forClient(trustRoots: .certificates([NIOSSLIntegrationTest.noSANCert]), + trustCommonName: true) + let clientContext = try assertNoThrowWithValue(NIOSSLContext(configuration: clientConfig)) + + let handshakeResultPromise = group.next().makePromise(of: Void.self) + let handshakeWatcher = WaitForHandshakeHandler(handshakeResultPromise: handshakeResultPromise) + let clientChannel = try clientTLSChannel(context: clientContext, + preHandlers: [], + postHandlers: [handshakeWatcher], + group: group, + connectingTo: serverChannel.localAddress!, + serverHostname: "camembert") + defer { + // Ignore errors here, the channel should be closed already by the time this happens. + try? clientChannel.close().wait() + } + + XCTAssertNoThrow(try handshakeResultPromise.futureResult.wait()) + } } diff --git a/Tests/NIOSSLTests/NIOSSLTestHelpers.swift b/Tests/NIOSSLTests/NIOSSLTestHelpers.swift index 2ae34c19..455107f6 100644 --- a/Tests/NIOSSLTests/NIOSSLTestHelpers.swift +++ b/Tests/NIOSSLTests/NIOSSLTestHelpers.swift @@ -432,7 +432,7 @@ func addExtension(x509: UnsafeMutablePointer, nid: CInt, value: String) { CNIOBoringSSL_X509_EXTENSION_free(ext) } -func generateSelfSignedCert() -> (NIOSSLCertificate, NIOSSLPrivateKey) { +func generateSelfSignedCert(withSAN: Bool = true) -> (NIOSSLCertificate, NIOSSLPrivateKey) { let pkey = generateRSAPrivateKey() let x = CNIOBoringSSL_X509_new()! CNIOBoringSSL_X509_set_version(x, 2) @@ -456,7 +456,7 @@ func generateSelfSignedCert() -> (NIOSSLCertificate, NIOSSLPrivateKey) { CNIOBoringSSL_X509_set_pubkey(x, pkey) - let commonName = "localhost" + let commonName = "camembert" let name = CNIOBoringSSL_X509_get_subject_name(x) commonName.withCString { (pointer: UnsafePointer) -> Void in pointer.withMemoryRebound(to: UInt8.self, capacity: commonName.lengthOfBytes(using: .utf8)) { (pointer: UnsafePointer) -> Void in @@ -473,8 +473,11 @@ func generateSelfSignedCert() -> (NIOSSLCertificate, NIOSSLPrivateKey) { addExtension(x509: x, nid: NID_basic_constraints, value: "critical,CA:FALSE") addExtension(x509: x, nid: NID_subject_key_identifier, value: "hash") - addExtension(x509: x, nid: NID_subject_alt_name, value: "DNS:localhost") addExtension(x509: x, nid: NID_ext_key_usage, value: "critical,serverAuth,clientAuth") + + if withSAN { + addExtension(x509: x, nid: NID_subject_alt_name, value: "DNS:localhost") + } CNIOBoringSSL_X509_sign(x, pkey, CNIOBoringSSL_EVP_sha256()) diff --git a/Tests/NIOSSLTests/SSLCertificateTest.swift b/Tests/NIOSSLTests/SSLCertificateTest.swift index 34ad0189..433dfb57 100644 --- a/Tests/NIOSSLTests/SSLCertificateTest.swift +++ b/Tests/NIOSSLTests/SSLCertificateTest.swift @@ -352,7 +352,7 @@ class SSLCertificateTest: XCTestCase { } func testCommonNameForGeneratedCert() throws { - XCTAssertEqual([UInt8]("localhost".utf8), SSLCertificateTest.dynamicallyGeneratedCert.commonName()!) + XCTAssertEqual([UInt8]("camembert".utf8), SSLCertificateTest.dynamicallyGeneratedCert.commonName()!) } func testMultipleCommonNames() throws {