Skip to content

Commit fc028b4

Browse files
LukasaLukasa
committed
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.
1 parent 0ca62dd commit fc028b4

9 files changed

+280
-56
lines changed

‎Sources/NIOSSL/IdentityVerification.swift‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -87,23 +87,27 @@ extension UInt8 {
8787
/// follow along at home.
8888
internal func validIdentityForService(serverHostname: String?,
8989
socketAddress: SocketAddress,
90-
leafCertificate: NIOSSLCertificate) throws -> Bool {
90+
leafCertificate: NIOSSLCertificate,
91+
includeCommonName: Bool) throws -> Bool {
9192
if let serverHostname = serverHostname {
9293
return try serverHostname.withLowercaseASCIIBuffer {
9394
try validIdentityForService(serverHostname: $0,
9495
socketAddress: socketAddress,
95-
leafCertificate: leafCertificate)
96+
leafCertificate: leafCertificate,
97+
includeCommonName: includeCommonName)
9698
}
9799
} else {
98100
return try validIdentityForService(serverHostname: nil as Array<UInt8>?,
99101
socketAddress: socketAddress,
100-
leafCertificate: leafCertificate)
102+
leafCertificate: leafCertificate,
103+
includeCommonName: includeCommonName)
101104
}
102105
}
103106

104107
private func validIdentityForService(serverHostname: Array<UInt8>?,
105108
socketAddress: SocketAddress,
106-
leafCertificate: NIOSSLCertificate) throws -> Bool {
109+
leafCertificate: NIOSSLCertificate,
110+
includeCommonName: Bool) throws -> Bool {
107111
// Before we begin, we want to locate the first period in our own domain name. We need to do
108112
// this because we may need to match a wildcard label.
109113
var serverHostnameSlice: ArraySlice<UInt8>? = nil
@@ -150,16 +154,15 @@ private func validIdentityForService(serverHostname: Array<UInt8>?,
150154
}
151155

152156
// In the absence of any matchable subjectAlternativeNames, we can fall back to checking
153-
// the common name. This is a deprecated practice, and in a future release we should
154-
// stop doing this.
155-
guard let commonName = leafCertificate.commonName() else {
156-
// No CN, no match.
157+
// the common name. We only do this if the user has configured a system that trusts the commonName field.
158+
if includeCommonName, let commonName = leafCertificate.commonName() {
159+
// We have a common name. Let's check it against the provided hostname. We never check
160+
// the common name against the IP address.
161+
return matchHostname(ourHostname: serverHostnameSlice, firstPeriodIndex: firstPeriodIndex, dnsName: commonName)
162+
} else {
163+
// We don't trust the common name, or we do but don't have one to look at.
157164
return false
158165
}
159-
160-
// We have a common name. Let's check it against the provided hostname. We never check
161-
// the common name against the IP address.
162-
return matchHostname(ourHostname: serverHostnameSlice, firstPeriodIndex: firstPeriodIndex, dnsName: commonName)
163166
}
164167

165168

‎Sources/NIOSSL/SSLConnection.swift‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,8 @@ internal final class SSLConnection {
213213

214214
guard try validIdentityForService(serverHostname: self.expectedHostname,
215215
socketAddress: address,
216-
leafCertificate: peerCert) else {
216+
leafCertificate: peerCert,
217+
includeCommonName: self.parentContext.configuration.trustCommonName) else {
217218
throw NIOSSLExtraError.failedToValidateHostname(expectedName: self.expectedHostname ?? "<none>")
218219
}
219220
}

‎Sources/NIOSSL/TLSConfiguration.swift‎

Lines changed: 107 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,18 @@ public struct TLSConfiguration {
303303
/// Whether renegotiation is supported.
304304
public var renegotiationSupport: NIORenegotiationSupport
305305

306+
/// Whether to trust the commonName field when validating certificate hostnames.
307+
///
308+
/// The commonName field of certificates has been deprecated for use as a validation source
309+
/// for nearly two decades. As browsers and the WebPKI now forbid the usage of common names
310+
/// in certificates, swift-nio-ssl defaults to this strict level of enforcement as well.
311+
///
312+
/// However, some older infrastructure does not issue certificates with valid subject alternative
313+
/// name fields. For this reason, we currently allow an override for trusting the common name. This
314+
/// is a temporary measure to allow users to work around these limitations, but we will remove this
315+
/// functionality in future.
316+
public var trustCommonName: Bool
317+
306318
private init(cipherSuiteValues: [NIOTLSCipher] = [],
307319
cipherSuites: String = defaultCipherSuites,
308320
verifySignatureAlgorithms: [SignatureAlgorithm]?,
@@ -317,7 +329,8 @@ public struct TLSConfiguration {
317329
shutdownTimeout: TimeAmount,
318330
keyLogCallback: NIOSSLKeyLogCallback?,
319331
renegotiationSupport: NIORenegotiationSupport,
320-
additionalTrustRoots: [NIOSSLAdditionalTrustRoots]) {
332+
additionalTrustRoots: [NIOSSLAdditionalTrustRoots],
333+
trustCommonName: Bool) {
321334
self.cipherSuites = cipherSuites
322335
self.verifySignatureAlgorithms = verifySignatureAlgorithms
323336
self.signingSignatureAlgorithms = signingSignatureAlgorithms
@@ -331,6 +344,7 @@ public struct TLSConfiguration {
331344
self.encodedApplicationProtocols = []
332345
self.shutdownTimeout = shutdownTimeout
333346
self.renegotiationSupport = renegotiationSupport
347+
self.trustCommonName = trustCommonName
334348
self.applicationProtocols = applicationProtocols
335349
self.keyLogCallback = keyLogCallback
336350
if !cipherSuiteValues.isEmpty {
@@ -368,7 +382,8 @@ public struct TLSConfiguration {
368382
shutdownTimeout: shutdownTimeout,
369383
keyLogCallback: keyLogCallback,
370384
renegotiationSupport: .none, // Servers never support renegotiation: there's no point.
371-
additionalTrustRoots: additionalTrustRoots)
385+
additionalTrustRoots: additionalTrustRoots,
386+
trustCommonName: false)
372387
}
373388

374389
/// Create a TLS configuration for use with server-side contexts.
@@ -398,7 +413,8 @@ public struct TLSConfiguration {
398413
shutdownTimeout: shutdownTimeout,
399414
keyLogCallback: keyLogCallback,
400415
renegotiationSupport: .none, // Servers never support renegotiation: there's no point.
401-
additionalTrustRoots: [])
416+
additionalTrustRoots: [],
417+
trustCommonName: false)
402418
}
403419

404420
/// Create a TLS configuration for use with server-side contexts.
@@ -430,7 +446,8 @@ public struct TLSConfiguration {
430446
shutdownTimeout: shutdownTimeout,
431447
keyLogCallback: keyLogCallback,
432448
renegotiationSupport: .none, // Servers never support renegotiation: there's no point.
433-
additionalTrustRoots: [])
449+
additionalTrustRoots: [],
450+
trustCommonName: false)
434451
}
435452

436453
/// Create a TLS configuration for use with server-side contexts.
@@ -463,7 +480,43 @@ public struct TLSConfiguration {
463480
shutdownTimeout: shutdownTimeout,
464481
keyLogCallback: keyLogCallback,
465482
renegotiationSupport: .none, // Servers never support renegotiation: there's no point.
466-
additionalTrustRoots: additionalTrustRoots)
483+
additionalTrustRoots: additionalTrustRoots,
484+
trustCommonName: false)
485+
}
486+
487+
/// Create a TLS configuration for use with server-side contexts.
488+
///
489+
/// This provides sensible defaults while requiring that you provide any data that is necessary
490+
/// for server-side function. For client use, try `forClient` instead.
491+
public static func forServer(certificateChain: [NIOSSLCertificateSource],
492+
privateKey: NIOSSLPrivateKeySource,
493+
cipherSuites: String = defaultCipherSuites,
494+
verifySignatureAlgorithms: [SignatureAlgorithm]? = nil,
495+
signingSignatureAlgorithms: [SignatureAlgorithm]? = nil,
496+
minimumTLSVersion: TLSVersion = .tlsv1,
497+
maximumTLSVersion: TLSVersion? = nil,
498+
certificateVerification: CertificateVerification = .none,
499+
trustRoots: NIOSSLTrustRoots = .default,
500+
applicationProtocols: [String] = [],
501+
shutdownTimeout: TimeAmount = .seconds(5),
502+
keyLogCallback: NIOSSLKeyLogCallback? = nil,
503+
additionalTrustRoots: [NIOSSLAdditionalTrustRoots] = [],
504+
trustCommonName: Bool) -> TLSConfiguration {
505+
return TLSConfiguration(cipherSuites: cipherSuites,
506+
verifySignatureAlgorithms: verifySignatureAlgorithms,
507+
signingSignatureAlgorithms: signingSignatureAlgorithms,
508+
minimumTLSVersion: minimumTLSVersion,
509+
maximumTLSVersion: maximumTLSVersion,
510+
certificateVerification: certificateVerification,
511+
trustRoots: trustRoots,
512+
certificateChain: certificateChain,
513+
privateKey: privateKey,
514+
applicationProtocols: applicationProtocols,
515+
shutdownTimeout: shutdownTimeout,
516+
keyLogCallback: keyLogCallback,
517+
renegotiationSupport: .none, // Servers never support renegotiation: there's no point.
518+
additionalTrustRoots: additionalTrustRoots,
519+
trustCommonName: trustCommonName)
467520
}
468521

469522
/// Creates a TLS configuration for use with client-side contexts. This allows setting the `NIOTLSCipher` property specifically.
@@ -497,7 +550,8 @@ public struct TLSConfiguration {
497550
shutdownTimeout: shutdownTimeout,
498551
keyLogCallback: keyLogCallback,
499552
renegotiationSupport: renegotiationSupport,
500-
additionalTrustRoots: additionalTrustRoots)
553+
additionalTrustRoots: additionalTrustRoots,
554+
trustCommonName: false)
501555
}
502556

503557
/// Creates a TLS configuration for use with client-side contexts.
@@ -527,7 +581,8 @@ public struct TLSConfiguration {
527581
shutdownTimeout: shutdownTimeout,
528582
keyLogCallback: keyLogCallback,
529583
renegotiationSupport: .none, // Default value is here for backward-compatibility.
530-
additionalTrustRoots: [])
584+
additionalTrustRoots: [],
585+
trustCommonName: false)
531586
}
532587

533588

@@ -559,7 +614,8 @@ public struct TLSConfiguration {
559614
shutdownTimeout: shutdownTimeout,
560615
keyLogCallback: keyLogCallback,
561616
renegotiationSupport: renegotiationSupport,
562-
additionalTrustRoots: [])
617+
additionalTrustRoots: [],
618+
trustCommonName: false)
563619
}
564620

565621
/// Creates a TLS configuration for use with client-side contexts.
@@ -592,7 +648,8 @@ public struct TLSConfiguration {
592648
shutdownTimeout: shutdownTimeout,
593649
keyLogCallback: keyLogCallback,
594650
renegotiationSupport: renegotiationSupport,
595-
additionalTrustRoots: [])
651+
additionalTrustRoots: [],
652+
trustCommonName: false)
596653
}
597654

598655
/// Creates a TLS configuration for use with client-side contexts.
@@ -626,7 +683,44 @@ public struct TLSConfiguration {
626683
shutdownTimeout: shutdownTimeout,
627684
keyLogCallback: keyLogCallback,
628685
renegotiationSupport: renegotiationSupport,
629-
additionalTrustRoots: additionalTrustRoots)
686+
additionalTrustRoots: additionalTrustRoots,
687+
trustCommonName: false)
688+
}
689+
690+
/// Creates a TLS configuration for use with client-side contexts. This allows setting the `NIOTLSCipher` property specifically.
691+
///
692+
/// This provides sensible defaults, and can be used without customisation. For server-side
693+
/// contexts, you should use `forServer` instead.
694+
public static func forClient(cipherSuites: String = defaultCipherSuites,
695+
verifySignatureAlgorithms: [SignatureAlgorithm]? = nil,
696+
signingSignatureAlgorithms: [SignatureAlgorithm]? = nil,
697+
minimumTLSVersion: TLSVersion = .tlsv1,
698+
maximumTLSVersion: TLSVersion? = nil,
699+
certificateVerification: CertificateVerification = .fullVerification,
700+
trustRoots: NIOSSLTrustRoots = .default,
701+
certificateChain: [NIOSSLCertificateSource] = [],
702+
privateKey: NIOSSLPrivateKeySource? = nil,
703+
applicationProtocols: [String] = [],
704+
shutdownTimeout: TimeAmount = .seconds(5),
705+
keyLogCallback: NIOSSLKeyLogCallback? = nil,
706+
renegotiationSupport: NIORenegotiationSupport = .none,
707+
additionalTrustRoots: [NIOSSLAdditionalTrustRoots] = [],
708+
trustCommonName: Bool) -> TLSConfiguration {
709+
return TLSConfiguration(cipherSuites: cipherSuites,
710+
verifySignatureAlgorithms: verifySignatureAlgorithms,
711+
signingSignatureAlgorithms: signingSignatureAlgorithms,
712+
minimumTLSVersion: minimumTLSVersion,
713+
maximumTLSVersion: maximumTLSVersion,
714+
certificateVerification: certificateVerification,
715+
trustRoots: trustRoots,
716+
certificateChain: certificateChain,
717+
privateKey: privateKey,
718+
applicationProtocols: applicationProtocols,
719+
shutdownTimeout: shutdownTimeout,
720+
keyLogCallback: keyLogCallback,
721+
renegotiationSupport: renegotiationSupport,
722+
additionalTrustRoots: additionalTrustRoots,
723+
trustCommonName: trustCommonName)
630724
}
631725
}
632726

@@ -657,7 +751,8 @@ extension TLSConfiguration {
657751
self.encodedApplicationProtocols == comparing.encodedApplicationProtocols &&
658752
self.shutdownTimeout == comparing.shutdownTimeout &&
659753
isKeyLoggerCallbacksEqual &&
660-
self.renegotiationSupport == comparing.renegotiationSupport
754+
self.renegotiationSupport == comparing.renegotiationSupport &&
755+
self.trustCommonName == comparing.trustCommonName
661756
}
662757

663758
/// Returns a best effort hash of this TLS configuration.
@@ -682,5 +777,6 @@ extension TLSConfiguration {
682777
hasher.combine(bytes: closureBits)
683778
}
684779
hasher.combine(renegotiationSupport)
780+
hasher.combine(trustCommonName)
685781
}
686782
}

‎Tests/NIOSSLTests/IdentityVerificationTest+XCTest.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ extension IdentityVerificationTest {
5151
("testDoesNotMatchSANWithEmbeddedNULL", testDoesNotMatchSANWithEmbeddedNULL),
5252
("testFallsBackToCommonName", testFallsBackToCommonName),
5353
("testLowercasesForCommonName", testLowercasesForCommonName),
54+
("testDoesNotFallBackToCommonName", testDoesNotFallBackToCommonName),
55+
("testNoLowercasesForCommonName", testNoLowercasesForCommonName),
5456
("testRejectsUnicodeCommonNameWithUnencodedIDNALabel", testRejectsUnicodeCommonNameWithUnencodedIDNALabel),
5557
("testRejectsUnicodeCommonNameWithEncodedIDNALabel", testRejectsUnicodeCommonNameWithEncodedIDNALabel),
5658
("testHandlesMissingCommonName", testHandlesMissingCommonName),

0 commit comments

Comments
 (0)