Add SPKI certificate pinning support#910
Conversation
… get handler logic
…lementation # Conflicts: # Package@swift-6.0.swift
|
@czechboy0 @FranzBusch can you review this? |
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Thanks! This mostly looks good but I left some feedback inline which needs addressing.
| // | ||
| // This source file is part of the AsyncHTTPClient open source project | ||
| // | ||
| // Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors |
There was a problem hiding this comment.
It's 2026
| // Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors | |
| // Copyright (c) 2026 Apple Inc. and the AsyncHTTPClient project authors |
| public var tlsConfiguration: TLSConfiguration? | ||
|
|
||
| /// Optional SPKI pinning configuration for TLS certificate validation. | ||
| public var tlsPinning: SPKIPinningConfiguration? |
There was a problem hiding this comment.
I noted in the description that the Network.framework backend ignores this. I think it'd be better to validate the configuration and throw an error if pinning is enabled and Network.framework is being used, otherwise a user may not realize that they aren't actually using pinning. This behavior should also be documented. Same goes for using NIOSSL with unsupported platforms (i.e. macOS < 10.15, however unlikely that is)
| /// are rejected early using public knowledge (algorithm-determined digest size), | ||
| /// which cannot leak secret information. | ||
| internal func constantTimeAnyMatch(_ target: Data, _ candidates: [SPKIHash]) -> Bool { | ||
| guard !candidates.isEmpty else { return false } |
There was a problem hiding this comment.
nit: if X { ... } is typically more readable than guard !X else { ... }
| guard !candidates.isEmpty else { return false } | |
| if candidates.isEmpty { return false } |
| public static func == (lhs: Self, rhs: Self) -> Bool { | ||
| lhs.rawValue == rhs.rawValue | ||
| } | ||
|
|
||
| public func hash(into hasher: inout Hasher) { | ||
| hasher.combine(rawValue) | ||
| } |
There was a problem hiding this comment.
Are these necessary? The compiler should be able to synthesize them.
| /// Use in production environments where security is paramount. | ||
| public static let strict = SPKIPinningPolicy(rawValue: .strict) | ||
|
|
||
| public var description: String { |
There was a problem hiding this comment.
Did you want this type to conform to CustomStringConvertible?
| } | ||
|
|
||
| func testSPKIPinning_ValidPin_AllowsConnection() { | ||
| XCTAsyncTest { |
There was a problem hiding this comment.
I don't think XCTAsyncTest is required anymore, XCTest has had solid async support for long enough now.
| /// - Throws: `HTTPClientError.invalidDigestLength` if length doesn't match algorithm. | ||
| @available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *) | ||
| public init<Algorithm: HashFunction>(algorithm: Algorithm.Type, base64: String) throws { | ||
| guard let data = Data(base64Encoded: base64) else { | ||
| throw HTTPClientError.invalidDigestLength |
There was a problem hiding this comment.
Should this be a separate error case?
| // | ||
| // This source file is part of the AsyncHTTPClient open source project | ||
| // | ||
| // Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors |
| } | ||
| } | ||
|
|
||
| func testSPKIPinning_ValidPin_AllowsConnection() { |
There was a problem hiding this comment.
Can you add tests for http/1.1 as well?
There was a problem hiding this comment.
There's also a lot of duplicated code across these four tests, can you refactor to have a test function which varies the pin kind (valid/invalid) and policy?
| // | ||
| // This source file is part of the AsyncHTTPClient open source project | ||
| // | ||
| // Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors |
2 participants
This PR introduces SPKI-based certificate pinning to
AsyncHTTPClientto mitigate MITM attacks and compromised CAs.Highlights:
.strictand.auditmodes with multi-algorithm hashing (SHA-256/384/512) and constant-time comparison.