Better OCSP error handling and limit request rate in tests

yim-lee · yim-lee · commit 1f3f797dfef1 · 2021-02-25T19:09:20.000-08:00
diff --git a/Sources/PackageCollectionsSigning/Certificate/CertificatePolicy.swift b/Sources/PackageCollectionsSigning/Certificate/CertificatePolicy.swift
@@ -179,7 +179,8 @@ extension CertificatePolicy {
 
         if certChain.count >= 1, let httpClient = httpClient {
             // Whether cert chain can be trusted depends on OCSP result
-            self.BoringSSL_OCSP_isGood(certificate: certChain[0], issuer: certChain[1], httpClient: httpClient, callbackQueue: callbackQueue, callback: callback)
+            self.BoringSSL_OCSP_isGood(certificate: certChain[0], issuer: certChain[1], httpClient: httpClient,
+                                       diagnosticsEngine: diagnosticsEngine, callbackQueue: callbackQueue, callback: callback)
         } else {
             wrappedCallback(.success(()))
         }
@@ -188,6 +189,7 @@ extension CertificatePolicy {
     private func BoringSSL_OCSP_isGood(certificate: Certificate,
                                        issuer: Certificate,
                                        httpClient: HTTPClient,
+                                       diagnosticsEngine: DiagnosticsEngine,
                                        callbackQueue: DispatchQueue,
                                        callback: @escaping (Result<Void, Error>) -> Void) {
         let wrappedCallback: (Result<Void, Error>) -> Void = { result in callbackQueue.async { callback(result) } }
@@ -224,13 +226,14 @@ extension CertificatePolicy {
 
         let requestData = Data(UnsafeBufferPointer(start: out.pointee, count: count))
 
-        let results = ThreadSafeArrayStore<Bool>()
+        let results = ThreadSafeArrayStore<Result<Bool, Error>>()
         let group = DispatchGroup()
 
         // Query each OCSP responder and record result
         for index in 0 ..< ocspURLCount {
             guard let urlStr = CCryptoBoringSSL_sk_OPENSSL_STRING_value(ocspURLs, numericCast(index)),
                 let url = String(validatingUTF8: urlStr).flatMap({ URL(string: $0) }) else {
+                results.append(.failure(OCSPError.badURL))
                 continue
             }
 
@@ -246,16 +249,20 @@ extension CertificatePolicy {
                 defer { group.leave() }
 
                 switch result {
-                case .failure:
-                    return
+                case .failure(let error):
+                    results.append(.failure(error))
                 case .success(let response):
-                    guard let responseData = response.body else { return }
+                    guard let responseData = response.body else {
+                        results.append(.failure(OCSPError.emptyResponseBody))
+                        return
+                    }
 
                     let bytes = responseData.copyBytes()
                     // Convert response to bio then OCSP response
                     guard let bio = CCryptoBoringSSL_BIO_new(CCryptoBoringSSL_BIO_s_mem()),
                         CCryptoBoringSSL_BIO_write(bio, bytes, numericCast(bytes.count)) > 0,
                         let response = d2i_OCSP_RESPONSE_bio(bio, nil) else {
+                        results.append(.failure(OCSPError.responseConversionFailure))
                         return
                     }
                     defer { CCryptoBoringSSL_BIO_free(bio) }
@@ -266,6 +273,7 @@ extension CertificatePolicy {
                         CCryptoBoringSSL_OBJ_obj2nid(response.pointee.responseBytes.pointee.responseType) == NID_id_pkix_OCSP_basic,
                         let basicResp = OCSP_response_get1_basic(response),
                         let basicRespData = basicResp.pointee.tbsResponseData?.pointee else {
+                        results.append(.failure(OCSPError.badResponse))
                         return
                     }
                     defer { OCSP_BASICRESP_free(basicResp) }
@@ -274,11 +282,12 @@ extension CertificatePolicy {
                     for i in 0 ..< sk_OCSP_SINGLERESP_num(basicRespData.responses) {
                         guard let singleResp = sk_OCSP_SINGLERESP_value(basicRespData.responses, numericCast(i)),
                             let certStatus = singleResp.pointee.certStatus else {
-                            continue
+                            results.append(.failure(OCSPError.badResponse))
+                            return
                         }
 
                         // Is the certificate in good status?
-                        results.append(certStatus.pointee.type == V_OCSP_CERTSTATUS_GOOD)
+                        results.append(.success(certStatus.pointee.type == V_OCSP_CERTSTATUS_GOOD))
                         break
                     }
                 }
@@ -287,11 +296,12 @@ extension CertificatePolicy {
 
         group.notify(queue: callbackQueue) {
             // If there's no result then something must have gone wrong
-            guard !results.isEmpty else {
+            guard !results.isEmpty, results.compactMap({ $0.failure }).isEmpty else {
+                diagnosticsEngine.emit(error: "OCSP failed. All results: \(results)")
                 return wrappedCallback(.failure(CertificatePolicyError.ocspFailure))
             }
             // Is there response "bad status" response?
-            guard results.get().first(where: { !$0 }) == nil else {
+            guard results.compactMap({ $0.success }).first(where: { !$0 }) == nil else {
                 return wrappedCallback(.failure(CertificatePolicyError.invalidCertChain))
             }
             wrappedCallback(.success(()))
@@ -411,6 +421,13 @@ enum CertificatePolicyError: Error, Equatable {
     case ocspFailure
 }
 
+private enum OCSPError: Error {
+    case badURL
+    case emptyResponseBody
+    case responseConversionFailure
+    case badResponse
+}
+
 // MARK: - Certificate policies
 
 /// Default policy for validating certificates used to sign package collections.
@@ -459,9 +476,7 @@ struct DefaultCertificatePolicy: CertificatePolicy {
         self.diagnosticsEngine = diagnosticsEngine
 
         #if !os(macOS)
-        var httpClientConfig = HTTPClientConfiguration()
-        httpClientConfig.callbackQueue = callbackQueue
-        self.httpClient = HTTPClient(configuration: httpClientConfig)
+        self.httpClient = HTTPClient.makeDefault(callbackQueue: callbackQueue)
         #endif
     }
 
@@ -547,9 +562,7 @@ struct AppleDeveloperCertificatePolicy: CertificatePolicy {
         self.diagnosticsEngine = diagnosticsEngine
 
         #if !os(macOS)
-        var httpClientConfig = HTTPClientConfiguration()
-        httpClientConfig.callbackQueue = callbackQueue
-        self.httpClient = HTTPClient(configuration: httpClientConfig)
+        self.httpClient = HTTPClient.makeDefault(callbackQueue: callbackQueue)
         #endif
     }
 
@@ -611,3 +624,34 @@ public enum CertificatePolicyKey: Equatable, Hashable {
     public static let `default` = CertificatePolicyKey.default(subjectUserID: nil)
     public static let appleDistribution = CertificatePolicyKey.appleDistribution(subjectUserID: nil)
 }
+
+// MARK: - Utilities
+
+private extension Result {
+    var failure: Failure? {
+        switch self {
+        case .failure(let failure):
+            return failure
+        case .success:
+            return nil
+        }
+    }
+
+    var success: Success? {
+        switch self {
+        case .failure:
+            return nil
+        case .success(let value):
+            return value
+        }
+    }
+}
+
+private extension HTTPClient {
+    static func makeDefault(callbackQueue: DispatchQueue) -> HTTPClient {
+        var httpClientConfig = HTTPClientConfiguration()
+        httpClientConfig.callbackQueue = callbackQueue
+        httpClientConfig.requestTimeout = .seconds(1)
+        return HTTPClient(configuration: httpClientConfig)
+    }
+}
diff --git a/Tests/PackageCollectionsSigningTests/CertificatePolicyTests.swift b/Tests/PackageCollectionsSigningTests/CertificatePolicyTests.swift
@@ -197,20 +197,26 @@ class CertificatePolicyTests: XCTestCase {
             try withTemporaryDirectory { tmp in
                 try localFileSystem.copy(from: rootCAPath, to: tmp.appending(components: "AppleIncRoot.cer"))
 
+                waitBetweenOCSPRequests()
+
                 // Specify `trustedRootCertsDir`
                 do {
                     let policy = DefaultCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: nil,
                                                           callbackQueue: callbackQueue, diagnosticsEngine: diagnosticsEngine)
                     XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
                 }
 
+                waitBetweenOCSPRequests()
+
                 // Another way is to pass in `additionalTrustedRootCerts`
                 do {
                     let policy = DefaultCertificatePolicy(trustedRootCertsDir: nil, additionalTrustedRootCerts: [rootCA],
                                                           callbackQueue: callbackQueue, diagnosticsEngine: diagnosticsEngine)
                     XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
                 }
 
+                waitBetweenOCSPRequests()
+
                 // What if the same cert is in both `trustedRootCertsDir` and `additionalTrustedRootCerts`?
                 do {
                     let policy = DefaultCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: [rootCA],
@@ -262,20 +268,26 @@ class CertificatePolicyTests: XCTestCase {
             try withTemporaryDirectory { tmp in
                 try localFileSystem.copy(from: rootCAPath, to: tmp.appending(components: "AppleIncRoot.cer"))
 
+                waitBetweenOCSPRequests()
+
                 // Specify `trustedRootCertsDir`
                 do {
                     let policy = AppleDeveloperCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: nil,
                                                                  callbackQueue: callbackQueue, diagnosticsEngine: diagnosticsEngine)
                     XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
                 }
 
+                waitBetweenOCSPRequests()
+
                 // Another way is to pass in `additionalTrustedRootCerts`
                 do {
                     let policy = AppleDeveloperCertificatePolicy(trustedRootCertsDir: nil, additionalTrustedRootCerts: [rootCA],
                                                                  callbackQueue: callbackQueue, diagnosticsEngine: diagnosticsEngine)
                     XCTAssertNoThrow(try tsc_await { callback in policy.validate(certChain: certChain, callback: callback) })
                 }
 
+                waitBetweenOCSPRequests()
+
                 // What if the same cert is in both `trustedRootCertsDir` and `additionalTrustedRootCerts`?
                 do {
                     let policy = AppleDeveloperCertificatePolicy(trustedRootCertsDir: tmp.asURL, additionalTrustedRootCerts: [rootCA],
diff --git a/Tests/PackageCollectionsSigningTests/PackageCollectionSigningTest.swift b/Tests/PackageCollectionsSigningTests/PackageCollectionSigningTest.swift
@@ -252,6 +252,8 @@ class PackageCollectionSigningTests: XCTestCase {
             try withTemporaryDirectory { tmp in
                 try localFileSystem.copy(from: rootCAPath, to: tmp.appending(components: "AppleIncRoot.cer"))
 
+                waitBetweenOCSPRequests()
+
                 // Specify `trustedRootCertsDir`
                 do {
                     let signing = PackageCollectionSigning(trustedRootCertsDir: tmp.asURL, callbackQueue: callbackQueue, diagnosticsEngine: diagnosticsEngine)
@@ -266,6 +268,8 @@ class PackageCollectionSigningTests: XCTestCase {
                     })
                 }
 
+                waitBetweenOCSPRequests()
+
                 // Another way is to pass in `additionalTrustedRootCerts`
                 do {
                     let signing = PackageCollectionSigning(additionalTrustedRootCerts: [rootCAData.base64EncodedString()],
@@ -344,6 +348,8 @@ class PackageCollectionSigningTests: XCTestCase {
             try withTemporaryDirectory { tmp in
                 try localFileSystem.copy(from: rootCAPath, to: tmp.appending(components: "AppleIncRoot.cer"))
 
+                waitBetweenOCSPRequests()
+
                 // Specify `trustedRootCertsDir`
                 do {
                     let signing = PackageCollectionSigning(trustedRootCertsDir: tmp.asURL, callbackQueue: callbackQueue, diagnosticsEngine: diagnosticsEngine)
@@ -358,6 +364,8 @@ class PackageCollectionSigningTests: XCTestCase {
                     })
                 }
 
+                waitBetweenOCSPRequests()
+
                 // Another way is to pass in `additionalTrustedRootCerts`
                 do {
                     let signing = PackageCollectionSigning(additionalTrustedRootCerts: [rootCAData.base64EncodedString()],
diff --git a/Tests/PackageCollectionsSigningTests/Utilities.swift b/Tests/PackageCollectionsSigningTests/Utilities.swift
@@ -174,4 +174,16 @@ extension String {
 
 extension XCTestCase {
     func skipIfUnsupportedPlatform() throws {}
+
+    func waitBetweenOCSPRequests() {
+        // Use this for "real" tests only (i.e., gated by ENABLE_REAL_CERT_TEST)!
+        //
+        // The OCSP implementation using PackageCollectionsSigningLibc does not cache
+        // OCSP responses so we'd hit the responder on every query. It seems like a
+        // responder may decide not to respond at all if we make too many requests too fast,
+        // causing tests to fail due to HTTP timed out error.
+        //
+        // The Security framework does caching already so we shouldn't sleep at all.
+        Thread.sleep(forTimeInterval: 2)
+    }
 }
