Skip to content

Stop using commonName to validate certificates. #238

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+280 −56
Open

Stop using commonName to validate certificates. #238

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 15 additions & 12 deletions Sources/NIOSSL/IdentityVerification.swift
View file
Original file line number Diff line number Diff line change
@@ -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<UInt8>?,
socketAddress: socketAddress,
leafCertificate: leafCertificate)
leafCertificate: leafCertificate,
includeCommonName: includeCommonName)
}
}

private func validIdentityForService(serverHostname: Array<UInt8>?,
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<UInt8>? = nil
@@ -150,16 +154,15 @@ private func validIdentityForService(serverHostname: Array<UInt8>?,
}

// 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)
}


3 changes: 2 additions & 1 deletion Sources/NIOSSL/SSLConnection.swift
View file
Original file line number Diff line number Diff line change
@@ -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 ?? "<none>")
}
}
118 changes: 107 additions & 11 deletions Sources/NIOSSL/TLSConfiguration.swift
View file
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we (assuming it's possible) mark the setter for this as deprecated right from the start?


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)
}
}
2 changes: 2 additions & 0 deletions Tests/NIOSSLTests/IdentityVerificationTest+XCTest.swift
View file
Original file line number Diff line number Diff line change
@@ -51,6 +51,8 @@ extension IdentityVerificationTest {
("testDoesNotMatchSANWithEmbeddedNULL", testDoesNotMatchSANWithEmbeddedNULL),
("testFallsBackToCommonName", testFallsBackToCommonName),
("testLowercasesForCommonName", testLowercasesForCommonName),
("testDoesNotFallBackToCommonName", testDoesNotFallBackToCommonName),
("testNoLowercasesForCommonName", testNoLowercasesForCommonName),
("testRejectsUnicodeCommonNameWithUnencodedIDNALabel", testRejectsUnicodeCommonNameWithUnencodedIDNALabel),
("testRejectsUnicodeCommonNameWithEncodedIDNALabel", testRejectsUnicodeCommonNameWithEncodedIDNALabel),
("testHandlesMissingCommonName", testHandlesMissingCommonName),
102 changes: 74 additions & 28 deletions Tests/NIOSSLTests/IdentityVerificationTest.swift
View file

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions Tests/NIOSSLTests/NIOSSLIntegrationTest+XCTest.swift
View file
Original file line number Diff line number Diff line change
@@ -64,6 +64,8 @@ extension NIOSSLIntegrationTest {
("testLoadsOfCloses", testLoadsOfCloses),
("testWriteFromFailureOfWrite", testWriteFromFailureOfWrite),
("testTrustedFirst", testTrustedFirst),
("testCommonNameFails", testCommonNameFails),
("testCommonNameSucceedsIfOverrideApplied", testCommonNameSucceedsIfOverrideApplied),
]
}
}
71 changes: 71 additions & 0 deletions Tests/NIOSSLTests/NIOSSLIntegrationTest.swift
View file
Original file line number Diff line number Diff line change
@@ -407,6 +407,8 @@ fileprivate func _clientTLSChannel<TLS: NIOClientTLSProvider>(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())
}
}
9 changes: 6 additions & 3 deletions Tests/NIOSSLTests/NIOSSLTestHelpers.swift
View file
Original file line number Diff line number Diff line change
@@ -432,7 +432,7 @@ func addExtension(x509: UnsafeMutablePointer<X509>, 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<Int8>) -> Void in
pointer.withMemoryRebound(to: UInt8.self, capacity: commonName.lengthOfBytes(using: .utf8)) { (pointer: UnsafePointer<UInt8>) -> 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())

2 changes: 1 addition & 1 deletion Tests/NIOSSLTests/SSLCertificateTest.swift
View file
Original file line number Diff line number Diff line change
@@ -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 {