[Collections] Check for collections that are required to be signed (#3297)

yim-lee · web-flow · commit 8e919a04b54d · 2021-02-23T10:44:46.000-08:00
Motivation:
If a collection source has an entry in the certificate pinning configuration that it must be signed. Currently we are not checking for this.

Modifications:
Update `JSONPackageCollectionProvider` to check that if a collection is unsigned, then it shouldn't have an entry in the certificate pinning configuration. Otherwise, throw `missingSignature` error which cannot be bypassed.
diff --git a/Sources/Commands/SwiftPackageCollectionsTool.swift b/Sources/Commands/SwiftPackageCollectionsTool.swift
@@ -22,6 +22,7 @@ private enum CollectionsError: Swift.Error {
     case unsigned
     case cannotVerifySignature
     case invalidSignature
+    case missingSignature
 }
 
 // FIXME: add links to docs in error messages
@@ -38,6 +39,8 @@ extension CollectionsError: CustomStringConvertible {
             return "The collection's signature cannot be verified due to missing configuration. Please refer to documentations on how to set up trusted root certificates or rerun 'add' with '--skip-signature-check'."
         case .invalidSignature:
             return "The collection's signature is invalid. If you would still like to add it please rerun 'add' with '--skip-signature-check'."
+        case .missingSignature:
+            return "The collection is missing required signature, which means it might have been compromised. Please contact the collection's authors and alert them of the issue."
         }
     }
 }
@@ -139,6 +142,8 @@ public struct SwiftPackageCollectionsTool: ParsableCommand {
                     throw CollectionsError.cannotVerifySignature
                 } catch PackageCollectionError.invalidSignature {
                     throw CollectionsError.invalidSignature
+                } catch PackageCollectionError.missingSignature {
+                    throw CollectionsError.missingSignature
                 }
             }
 
diff --git a/Sources/PackageCollections/API.swift b/Sources/PackageCollections/API.swift
@@ -180,4 +180,6 @@ public enum PackageCollectionError: Equatable, Error {
     case cannotVerifySignature
 
     case invalidSignature
+
+    case missingSignature
 }
diff --git a/Sources/PackageCollections/PackageCollections+CertificatePolicy.swift b/Sources/PackageCollections/PackageCollections+CertificatePolicy.swift
@@ -17,20 +17,30 @@ import TSCBasic
 /// that are more restrictive. For example, a source may want to require that all their package
 /// collections be signed using certificate that belongs to certain subject user ID.
 internal struct PackageCollectionSourceCertificatePolicy {
-    static let sourceCertPolicies: [String: CertificatePolicyConfig] = [:]
+    private static let defaultSourceCertPolicies: [String: CertificatePolicyConfig] = [:]
 
-    static func certificatePolicyKey(for source: Model.CollectionSource) -> CertificatePolicyKey? {
-        // Certificate policy is associated to a collection host
-        source.url.host.flatMap { self.sourceCertPolicies[$0]?.certPolicyKey }
-    }
+    private let sourceCertPolicies: [String: CertificatePolicyConfig]
 
-    static var allRootCerts: [String]? {
-        let allRootCerts = Self.sourceCertPolicies.values
+    var allRootCerts: [String]? {
+        let allRootCerts = self.sourceCertPolicies.values
             .compactMap { $0.base64EncodedRootCerts }
             .flatMap { $0 }
         return allRootCerts.isEmpty ? nil : allRootCerts
     }
 
+    init(sourceCertPolicies: [String: CertificatePolicyConfig]? = nil) {
+        self.sourceCertPolicies = sourceCertPolicies ?? Self.defaultSourceCertPolicies
+    }
+
+    func mustBeSigned(source: Model.CollectionSource) -> Bool {
+        source.certPolicyConfigKey.map { self.sourceCertPolicies[$0] != nil } ?? false
+    }
+
+    func certificatePolicyKey(for source: Model.CollectionSource) -> CertificatePolicyKey? {
+        // Certificate policy is associated to a collection host
+        source.certPolicyConfigKey.flatMap { self.sourceCertPolicies[$0]?.certPolicyKey }
+    }
+
     struct CertificatePolicyConfig {
         let certPolicyKey: CertificatePolicyKey
 
@@ -39,3 +49,9 @@ internal struct PackageCollectionSourceCertificatePolicy {
         let base64EncodedRootCerts: [String]?
     }
 }
+
+private extension Model.CollectionSource {
+    var certPolicyConfigKey: String? {
+        self.url.host
+    }
+}
diff --git a/Sources/PackageCollections/Providers/JSONPackageCollectionProvider.swift b/Sources/PackageCollections/Providers/JSONPackageCollectionProvider.swift
@@ -34,10 +34,12 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
     private let decoder: JSONDecoder
     private let validator: JSONModel.Validator
     private let signatureValidator: PackageCollectionSignatureValidator
+    private let sourceCertPolicy: PackageCollectionSourceCertificatePolicy
 
     init(configuration: Configuration = .init(),
          httpClient: HTTPClient? = nil,
          signatureValidator: PackageCollectionSignatureValidator? = nil,
+         sourceCertPolicy: PackageCollectionSourceCertificatePolicy = PackageCollectionSourceCertificatePolicy(),
          fileSystem: FileSystem = localFileSystem,
          diagnosticsEngine: DiagnosticsEngine) {
         self.configuration = configuration
@@ -47,10 +49,11 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
         self.validator = JSONModel.Validator(configuration: configuration.validator)
         self.signatureValidator = signatureValidator ?? PackageCollectionSigning(
             trustedRootCertsDir: configuration.trustedRootCertsDir ?? fileSystem.dotSwiftPM.appending(components: "config", "trust-root-certs").asURL,
-            additionalTrustedRootCerts: PackageCollectionSourceCertificatePolicy.allRootCerts,
+            additionalTrustedRootCerts: sourceCertPolicy.allRootCerts,
             callbackQueue: DispatchQueue.global(),
             diagnosticsEngine: diagnosticsEngine
         )
+        self.sourceCertPolicy = sourceCertPolicy
     }
 
     func get(_ source: Model.CollectionSource, callback: @escaping (Result<Model.Collection, Error>) -> Void) {
@@ -109,7 +112,7 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
                             return callback(.failure(Errors.invalidResponse("Body is empty")))
                         }
 
-                        let certPolicyKey = PackageCollectionSourceCertificatePolicy.certificatePolicyKey(for: source) ?? .default
+                        let certPolicyKey = self.sourceCertPolicy.certificatePolicyKey(for: source) ?? .default
                         self.decodeAndRunSignatureCheck(source: source, data: body, certPolicyKey: certPolicyKey, callback: callback)
                     }
                 }
@@ -149,7 +152,11 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
                 }
             }
         } catch {
-            // Collection is not signed
+            // Bad: collection is supposed to be signed but it isn't
+            guard !self.sourceCertPolicy.mustBeSigned(source: source) else {
+                return callback(.failure(PackageCollectionError.missingSignature))
+            }
+            // Collection is unsigned
             guard let collection = try? self.decoder.decode(JSONModel.Collection.self, from: data) else {
                 return callback(.failure(Errors.invalidJSON(error)))
             }
diff --git a/Tests/PackageCollectionsSigningTests/CertificatePolicyTests.swift b/Tests/PackageCollectionsSigningTests/CertificatePolicyTests.swift
@@ -319,16 +319,46 @@ class CertificatePolicyTests: XCTestCase {
 
             #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
             // The Apple root certs come preinstalled on Apple platforms and they are automatically trusted
-            let policy = DefaultCertificatePolicy(trustedRootCertsDir: nil, additionalTrustedRootCerts: nil, expectedSubjectUserID: expectedSubjectUserID,
-                                                  callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
-            XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
+
+            // Subject user ID matches
+            do {
+                let policy = DefaultCertificatePolicy(trustedRootCertsDir: nil, additionalTrustedRootCerts: nil, expectedSubjectUserID: expectedSubjectUserID,
+                                                      callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
+                XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
+            }
+            // Subject user ID does not match
+            do {
+                let mismatchSubjectUserID = "\(expectedSubjectUserID)-2"
+                let policy = DefaultCertificatePolicy(trustedRootCertsDir: nil, additionalTrustedRootCerts: nil, expectedSubjectUserID: mismatchSubjectUserID,
+                                                      callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
+                XCTAssertThrowsError(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) }) { error in
+                    guard CertificatePolicyError.subjectUserIDMismatch == error as? CertificatePolicyError else {
+                        return XCTFail("Expected CertificatePolicyError.subjectUserIDMismatch")
+                    }
+                }
+            }
             #else
             // On other platforms we have to specify `trustedRootCertsDir` so the Apple root cert is trusted
             try withTemporaryDirectory { tmp in
                 try localFileSystem.copy(from: rootCAPath, to: tmp.appending(components: "AppleIncRoot.cer"))
-                let policy = DefaultCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: nil, expectedSubjectUserID: expectedSubjectUserID,
-                                                      callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
-                XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
+
+                // Subject user ID matches
+                do {
+                    let policy = DefaultCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: nil, expectedSubjectUserID: expectedSubjectUserID,
+                                                          callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
+                    XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
+                }
+                // Subject user ID does not match
+                do {
+                    let mismatchSubjectUserID = "\(expectedSubjectUserID)-2"
+                    let policy = DefaultCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: nil, expectedSubjectUserID: mismatchSubjectUserID,
+                                                          callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
+                    XCTAssertThrowsError(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) }) { error in
+                        guard CertificatePolicyError.subjectUserIDMismatch == error as? CertificatePolicyError else {
+                            return XCTFail("Expected CertificatePolicyError.subjectUserIDMismatch")
+                        }
+                    }
+                }
             }
             #endif
         }
@@ -359,16 +389,46 @@ class CertificatePolicyTests: XCTestCase {
 
             #if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
             // The Apple root certs come preinstalled on Apple platforms and they are automatically trusted
-            let policy = AppleDeveloperCertificatePolicy(trustedRootCertsDir: nil, additionalTrustedRootCerts: nil, expectedSubjectUserID: expectedSubjectUserID,
-                                                         callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
-            XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
+
+            // Subject user ID matches
+            do {
+                let policy = AppleDeveloperCertificatePolicy(trustedRootCertsDir: nil, additionalTrustedRootCerts: nil, expectedSubjectUserID: expectedSubjectUserID,
+                                                             callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
+                XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
+            }
+            // Subject user ID does not match
+            do {
+                let mismatchSubjectUserID = "\(expectedSubjectUserID)-2"
+                let policy = AppleDeveloperCertificatePolicy(trustedRootCertsDir: nil, additionalTrustedRootCerts: nil, expectedSubjectUserID: mismatchSubjectUserID,
+                                                             callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
+                XCTAssertThrowsError(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) }) { error in
+                    guard CertificatePolicyError.subjectUserIDMismatch == error as? CertificatePolicyError else {
+                        return XCTFail("Expected CertificatePolicyError.subjectUserIDMismatch")
+                    }
+                }
+            }
             #else
             // On other platforms we have to specify `trustedRootCertsDir` so the Apple root cert is trusted
             try withTemporaryDirectory { tmp in
                 try localFileSystem.copy(from: rootCAPath, to: tmp.appending(components: "AppleIncRoot.cer"))
-                let policy = AppleDeveloperCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: nil, expectedSubjectUserID: expectedSubjectUserID,
-                                                             callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
-                XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
+
+                // Subject user ID matches
+                do {
+                    let policy = AppleDeveloperCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: nil, expectedSubjectUserID: expectedSubjectUserID,
+                                                                 callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
+                    XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
+                }
+                // Subject user ID does not match
+                do {
+                    let mismatchSubjectUserID = "\(expectedSubjectUserID)-2"
+                    let policy = AppleDeveloperCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: nil, expectedSubjectUserID: mismatchSubjectUserID,
+                                                                 callbackQueue: DispatchQueue.global(), diagnosticsEngine: DiagnosticsEngine())
+                    XCTAssertThrowsError(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) }) { error in
+                        guard CertificatePolicyError.subjectUserIDMismatch == error as? CertificatePolicyError else {
+                            return XCTFail("Expected CertificatePolicyError.subjectUserIDMismatch")
+                        }
+                    }
+                }
             }
             #endif
         }
diff --git a/Tests/PackageCollectionsTests/JSONPackageCollectionProviderTests.swift b/Tests/PackageCollectionsTests/JSONPackageCollectionProviderTests.swift
diff --git a/Tests/PackageCollectionsTests/PackageCollectionSourceCertificatePolicyTests.swift b/Tests/PackageCollectionsTests/PackageCollectionSourceCertificatePolicyTests.swift

Original file line number	Diff line number	Diff line change
`@@ -22,6 +22,7 @@ private enum CollectionsError: Swift.Error {`
`22`	`22`	`case unsigned`
`23`	`23`	`case cannotVerifySignature`
`24`	`24`	`case invalidSignature`
	`25`	`+ case missingSignature`
`25`	`26`	`}`
`26`	`27`
`27`	`28`	`// FIXME: add links to docs in error messages`
`@@ -38,6 +39,8 @@ extension CollectionsError: CustomStringConvertible {`
`38`	`39`	`return "The collection's signature cannot be verified due to missing configuration. Please refer to documentations on how to set up trusted root certificates or rerun 'add' with '--skip-signature-check'."`
`39`	`40`	`case .invalidSignature:`
`40`	`41`	`return "The collection's signature is invalid. If you would still like to add it please rerun 'add' with '--skip-signature-check'."`
	`42`	`+ case .missingSignature:`
	`43`	`+ return "The collection is missing required signature, which means it might have been compromised. Please contact the collection's authors and alert them of the issue."`
`41`	`44`	`}`
`42`	`45`	`}`
`43`	`46`	`}`
`@@ -139,6 +142,8 @@ public struct SwiftPackageCollectionsTool: ParsableCommand {`
`139`	`142`	`throw CollectionsError.cannotVerifySignature`
`140`	`143`	`} catch PackageCollectionError.invalidSignature {`
`141`	`144`	`throw CollectionsError.invalidSignature`
	`145`	`+ } catch PackageCollectionError.missingSignature {`
	`146`	`+ throw CollectionsError.missingSignature`
`142`	`147`	`}`
`143`	`148`	`}`
`144`	`149`
Original file line number	Diff line number	Diff line change
`@@ -180,4 +180,6 @@ public enum PackageCollectionError: Equatable, Error {`
`180`	`180`	`case cannotVerifySignature`
`181`	`181`
`182`	`182`	`case invalidSignature`
	`183`	`+`
	`184`	`+ case missingSignature`
`183`	`185`	`}`
Original file line number	Diff line number	Diff line change
`@@ -34,10 +34,12 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {`
`34`	`34`	`private let decoder: JSONDecoder`
`35`	`35`	`private let validator: JSONModel.Validator`
`36`	`36`	`private let signatureValidator: PackageCollectionSignatureValidator`
	`37`	`+ private let sourceCertPolicy: PackageCollectionSourceCertificatePolicy`
`37`	`38`
`38`	`39`	`init(configuration: Configuration = .init(),`
`39`	`40`	`httpClient: HTTPClient? = nil,`
`40`	`41`	`signatureValidator: PackageCollectionSignatureValidator? = nil,`
	`42`	`+ sourceCertPolicy: PackageCollectionSourceCertificatePolicy = PackageCollectionSourceCertificatePolicy(),`
`41`	`43`	`fileSystem: FileSystem = localFileSystem,`
`42`	`44`	`diagnosticsEngine: DiagnosticsEngine) {`
`43`	`45`	`self.configuration = configuration`
`@@ -47,10 +49,11 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {`
`47`	`49`	`self.validator = JSONModel.Validator(configuration: configuration.validator)`
`48`	`50`	`self.signatureValidator = signatureValidator ?? PackageCollectionSigning(`
`49`	`51`	`trustedRootCertsDir: configuration.trustedRootCertsDir ?? fileSystem.dotSwiftPM.appending(components: "config", "trust-root-certs").asURL,`
`50`		`- additionalTrustedRootCerts: PackageCollectionSourceCertificatePolicy.allRootCerts,`
	`52`	`+ additionalTrustedRootCerts: sourceCertPolicy.allRootCerts,`
`51`	`53`	`callbackQueue: DispatchQueue.global(),`
`52`	`54`	`diagnosticsEngine: diagnosticsEngine`
`53`	`55`	`)`
	`56`	`+ self.sourceCertPolicy = sourceCertPolicy`
`54`	`57`	`}`
`55`	`58`
`56`	`59`	`func get(_ source: Model.CollectionSource, callback: @escaping (Result<Model.Collection, Error>) -> Void) {`
`@@ -109,7 +112,7 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {`
`109`	`112`	`return callback(.failure(Errors.invalidResponse("Body is empty")))`
`110`	`113`	`}`
`111`	`114`
`112`		`- let certPolicyKey = PackageCollectionSourceCertificatePolicy.certificatePolicyKey(for: source) ?? .default`
	`115`	`+ let certPolicyKey = self.sourceCertPolicy.certificatePolicyKey(for: source) ?? .default`
`113`	`116`	`self.decodeAndRunSignatureCheck(source: source, data: body, certPolicyKey: certPolicyKey, callback: callback)`
`114`	`117`	`}`
`115`	`118`	`}`
`@@ -149,7 +152,11 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {`
`149`	`152`	`}`
`150`	`153`	`}`
`151`	`154`	`} catch {`
`152`		`- // Collection is not signed`
	`155`	`+ // Bad: collection is supposed to be signed but it isn't`
	`156`	`+ guard !self.sourceCertPolicy.mustBeSigned(source: source) else {`
	`157`	`+ return callback(.failure(PackageCollectionError.missingSignature))`
	`158`	`+ }`
	`159`	`+ // Collection is unsigned`
`153`	`160`	`guard let collection = try? self.decoder.decode(JSONModel.Collection.self, from: data) else {`
`154`	`161`	`return callback(.failure(Errors.invalidJSON(error)))`
`155`	`162`	`}`