Skip to content

Commit 5d0e191

Browse files
authored
[Collections] Add skip signature check option to 'add' (#3291)
Motivation: On non-Apple platforms since there are no trusted root certificates by default, signature checks will most certainly fail. To help people get started more quickly, and to offer people a way to opt-out, we will add a `--skip-signature-check` flag to the `add` collection command. User's selection is persisted to collection sources config such that `refresh` would honor it as well. Modifications: - Add `skipSignatureCheck` to `CollectionSource` - Add `--skip-signature-check` to CLI - Add `isVerified` to `Collection.SignatureData`. It is `false` when `skipSignatureCheck == true`. - Update logic in `JSONPackageCollectionProvider` to honor `skipSignatureCheck` - Update CLI to handle `cannotVerifySignature` error
1 parent d612436 commit 5d0e191

10 files changed

+144
-39
lines changed

Sources/Commands/SwiftPackageCollectionsTool.swift

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,10 @@ private enum CollectionsError: Swift.Error {
2020
case invalidArgument(String)
2121
case invalidVersionString(String)
2222
case unsigned
23+
case cannotVerifySignature
2324
}
2425

26+
// FIXME: add links to docs in error messages
2527
extension CollectionsError: CustomStringConvertible {
2628
var description: String {
2729
switch self {
@@ -31,6 +33,8 @@ extension CollectionsError: CustomStringConvertible {
3133
return "Invalid version string '\(versionString)'"
3234
case .unsigned:
3335
return "The collection is not signed. If you would still like to add it please rerun 'add' with '--trust-unsigned'."
36+
case .cannotVerifySignature:
37+
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."
3438
}
3539
}
3640
}
@@ -106,12 +110,15 @@ public struct SwiftPackageCollectionsTool: ParsableCommand {
106110
@Flag(name: .long, help: "Trust the collection even if it is unsigned")
107111
var trustUnsigned: Bool = false
108112

113+
@Flag(name: .long, help: "Skip signature check if the collection is signed")
114+
var skipSignatureCheck: Bool = false
115+
109116
mutating func run() throws {
110117
guard let collectionUrl = URL(string: collectionUrl) else {
111118
throw CollectionsError.invalidArgument("collectionUrl")
112119
}
113120

114-
let source = PackageCollectionsModel.CollectionSource(type: .json, url: collectionUrl)
121+
let source = PackageCollectionsModel.CollectionSource(type: .json, url: collectionUrl, skipSignatureCheck: self.skipSignatureCheck)
115122
let collection: PackageCollectionsModel.Collection = try with { collections in
116123
do {
117124
let userTrusted = self.trustUnsigned
@@ -125,6 +132,8 @@ public struct SwiftPackageCollectionsTool: ParsableCommand {
125132
}
126133
} catch PackageCollectionError.trustConfirmationRequired, PackageCollectionError.untrusted {
127134
throw CollectionsError.unsigned
135+
} catch PackageCollectionError.cannotVerifySignature {
136+
throw CollectionsError.cannotVerifySignature
128137
}
129138
}
130139

Sources/PackageCollections/API.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,4 +174,8 @@ public enum PackageCollectionError: Equatable, Error {
174174

175175
/// Package collection is not signed and user explicitly marks it untrusted
176176
case untrusted
177+
178+
/// There are no trusted root certificates. Signature check cannot be done in this case since it involves validating
179+
/// the certificate chain that is used for signing and one requirement is that the root certificate must be trusted.
180+
case cannotVerifySignature
177181
}

Sources/PackageCollections/Model/Collection.swift

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,13 +100,17 @@ extension PackageCollectionsModel {
100100
/// Indicates if the source is explicitly trusted or untrusted by the user
101101
public var isTrusted: Bool?
102102

103+
/// Indicates if the source can skip signature validation
104+
public var skipSignatureCheck: Bool
105+
103106
/// The source's absolute file system path, if its URL is of 'file' scheme.
104107
let absolutePath: AbsolutePath?
105108

106-
public init(type: CollectionSourceType, url: URL, isTrusted: Bool? = nil) {
109+
public init(type: CollectionSourceType, url: URL, isTrusted: Bool? = nil, skipSignatureCheck: Bool = false) {
107110
self.type = type
108111
self.url = url
109112
self.isTrusted = isTrusted
113+
self.skipSignatureCheck = skipSignatureCheck
110114

111115
if url.scheme?.lowercased() == "file", let absolutePath = try? AbsolutePath(validating: url.path) {
112116
self.absolutePath = absolutePath
@@ -192,8 +196,12 @@ extension PackageCollectionsModel {
192196
/// Details about the certificate used to generate the signature
193197
public let certificate: Certificate
194198

195-
public init(certificate: Certificate) {
199+
/// Indicates if the signature has been validated. This is set to false if signature check didn't take place.
200+
public let isVerified: Bool
201+
202+
public init(certificate: Certificate, isVerified: Bool) {
196203
self.certificate = certificate
204+
self.isVerified = isVerified
197205
}
198206

199207
public struct Certificate: Equatable, Codable {

Sources/PackageCollections/PackageCollections.swift

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,9 @@ public struct PackageCollections: PackageCollectionsProtocol {
140140
self.refreshCollectionFromSource(source: source, trustConfirmationProvider: trustConfirmationProvider) { collectionResult in
141141
switch collectionResult {
142142
case .failure(let error):
143-
// Don't delete the source if we are either pending user confirmation or have recorded user's preference
144-
if let error = error as? PackageCollectionError, error == .trustConfirmationRequired || error == .untrusted {
143+
// Don't delete the source if we are either pending user confirmation or have recorded user's preference.
144+
// It is also possible that we can't verify signature (yet) due to config issue, which user can fix and we retry later.
145+
if let error = error as? PackageCollectionError, error == .trustConfirmationRequired || error == .untrusted || error == .cannotVerifySignature {
145146
return callback(.failure(error))
146147
}
147148
// Otherwise remove source since it fails to be fetched

Sources/PackageCollections/Providers/JSONPackageCollectionProvider.swift

Lines changed: 36 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
import Basics
1212
import Dispatch
13+
import struct Foundation.Data
1314
import struct Foundation.Date
1415
import class Foundation.JSONDecoder
1516
import struct Foundation.URL
@@ -49,14 +50,9 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
4950
if let absolutePath = source.absolutePath {
5051
do {
5152
let fileContents = try localFileSystem.readFileContents(absolutePath)
52-
let collection: JSONModel.Collection = try fileContents.withData { data in
53-
do {
54-
return try self.decoder.decode(JSONModel.Collection.self, from: data)
55-
} catch {
56-
throw Errors.invalidJSON(error)
57-
}
53+
return fileContents.withData { data in
54+
self.decodeAndRunSignatureCheck(source: source, data: data, callback: callback)
5855
}
59-
return callback(self.makeCollection(from: collection, source: source, signature: nil))
6056
} catch {
6157
return callback(.failure(error))
6258
}
@@ -97,33 +93,43 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
9793
return callback(.failure(Errors.invalidResponse("Body is empty")))
9894
}
9995

100-
do {
101-
// parse json and construct result
102-
do {
103-
// This fails if "signature" is missing
104-
let signature = try JSONModel.SignedCollection.signature(from: body, using: self.decoder)
105-
// TODO: Check collection's signature
106-
// If signature is
107-
// a. valid: process the collection; set isSigned=true
108-
// b. invalid: includes expired cert, untrusted cert, signature-payload mismatch => return error
109-
let collection = try JSONModel.SignedCollection.collection(from: body, using: self.decoder)
110-
callback(self.makeCollection(from: collection, source: source, signature: Model.SignatureData(from: signature)))
111-
} catch {
112-
// Collection is not signed
113-
guard let collection = try response.decodeBody(JSONModel.Collection.self, using: self.decoder) else {
114-
return callback(.failure(Errors.invalidResponse("Invalid body")))
115-
}
116-
callback(self.makeCollection(from: collection, source: source, signature: nil))
117-
}
118-
} catch {
119-
callback(.failure(Errors.invalidJSON(error)))
120-
}
96+
self.decodeAndRunSignatureCheck(source: source, data: body, callback: callback)
12197
}
12298
}
12399
}
124100
}
125101
}
126102

103+
private func decodeAndRunSignatureCheck(source: Model.CollectionSource,
104+
data: Data,
105+
callback: @escaping (Result<Model.Collection, Error>) -> Void) {
106+
do {
107+
// This fails if "signature" is missing
108+
let signature = try JSONModel.SignedCollection.signature(from: data, using: self.decoder)
109+
if source.skipSignatureCheck {
110+
// Don't validate signature but set isVerified=false
111+
let collection = try JSONModel.SignedCollection.collection(from: data, using: self.decoder)
112+
callback(self.makeCollection(from: collection, source: source, signature: Model.SignatureData(from: signature, isVerified: false)))
113+
} else {
114+
// TODO: Signature validator should throw "cannot verify" error on non-Apple platforms
115+
// if there are no trusted root certs set up, in which case we should throw PackageCollectionError.cannotVerifySignature
116+
117+
// TODO: Check collection's signature
118+
// If signature is
119+
// a. valid: process the collection; set isSigned=true
120+
// b. invalid: includes expired cert, untrusted cert, signature-payload mismatch => return error
121+
let collection = try JSONModel.SignedCollection.collection(from: data, using: self.decoder)
122+
callback(self.makeCollection(from: collection, source: source, signature: Model.SignatureData(from: signature, isVerified: true)))
123+
}
124+
} catch {
125+
// Collection is not signed
126+
guard let collection = try? self.decoder.decode(JSONModel.Collection.self, from: data) else {
127+
return callback(.failure(Errors.invalidJSON(error)))
128+
}
129+
callback(self.makeCollection(from: collection, source: source, signature: nil))
130+
}
131+
}
132+
127133
private func makeCollection(from collection: JSONModel.Collection, source: Model.CollectionSource, signature: Model.SignatureData?) -> Result<Model.Collection, Error> {
128134
if let errors = self.validator.validate(collection: collection)?.errors() {
129135
return .failure(MultipleErrors(errors))
@@ -380,8 +386,9 @@ extension Model.License {
380386
}
381387

382388
extension Model.SignatureData {
383-
fileprivate init(from: JSONModel.Signature) {
389+
fileprivate init(from: JSONModel.Signature, isVerified: Bool) {
384390
self.certificate = .init(from: from.certificate)
391+
self.isVerified = isVerified
385392
}
386393
}
387394

Sources/PackageCollections/Storage/FilePackageCollectionsSourcesStorage.swift

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ private enum StorageModel {
187187
let type: String
188188
let value: String
189189
let isTrusted: Bool?
190+
let skipSignatureCheck: Bool?
190191
}
191192

192193
enum SourceType: String {
@@ -204,7 +205,7 @@ private extension Model.CollectionSource {
204205

205206
switch from.type {
206207
case StorageModel.SourceType.json.rawValue:
207-
self.init(type: .json, url: url, isTrusted: from.isTrusted)
208+
self.init(type: .json, url: url, isTrusted: from.isTrusted, skipSignatureCheck: from.skipSignatureCheck ?? false)
208209
default:
209210
throw SerializationError.unknownType(from.type)
210211
}
@@ -213,7 +214,8 @@ private extension Model.CollectionSource {
213214
func source() -> StorageModel.Source {
214215
switch self.type {
215216
case .json:
216-
return .init(type: StorageModel.SourceType.json.rawValue, value: self.url.absoluteString, isTrusted: self.isTrusted)
217+
return .init(type: StorageModel.SourceType.json.rawValue, value: self.url.absoluteString,
218+
isTrusted: self.isTrusted, skipSignatureCheck: self.skipSignatureCheck)
217219
}
218220
}
219221
}

Tests/PackageCollectionsTests/JSONPackageCollectionProviderTests.swift

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,69 @@ class JSONPackageCollectionProviderTests: XCTestCase {
132132
XCTAssertNotNil(version.createdAt)
133133
XCTAssertTrue(collection.isSigned)
134134
let signature = collection.signature!
135+
XCTAssertTrue(signature.isVerified)
136+
XCTAssertEqual("Sample Subject", signature.certificate.subject.commonName)
137+
XCTAssertEqual("Sample Issuer", signature.certificate.issuer.commonName)
138+
}
139+
}
140+
141+
func testGoodSigned_skipSignatureCheck() throws {
142+
fixture(name: "Collections") { directoryPath in
143+
let path = directoryPath.appending(components: "JSON", "good_signed.json")
144+
let url = URL(string: "https://www.test.com/collection.json")!
145+
let data = Data(try localFileSystem.readFileContents(path).contents)
146+
147+
let handler: HTTPClient.Handler = { request, _, completion in
148+
XCTAssertEqual(request.url, url, "url should match")
149+
switch request.method {
150+
case .head:
151+
completion(.success(.init(statusCode: 200,
152+
headers: .init([.init(name: "Content-Length", value: "\(data.count)")]))))
153+
case .get:
154+
completion(.success(.init(statusCode: 200,
155+
headers: .init([.init(name: "Content-Length", value: "\(data.count)")]),
156+
body: data)))
157+
default:
158+
XCTFail("method should match")
159+
}
160+
}
161+
162+
var httpClient = HTTPClient(handler: handler)
163+
httpClient.configuration.circuitBreakerStrategy = .none
164+
httpClient.configuration.retryStrategy = .none
165+
let provider = JSONPackageCollectionProvider(httpClient: httpClient)
166+
// Skip signature check
167+
let source = PackageCollectionsModel.CollectionSource(type: .json, url: url, skipSignatureCheck: true)
168+
let collection = try tsc_await { callback in provider.get(source, callback: callback) }
169+
170+
XCTAssertEqual(collection.name, "Sample Package Collection")
171+
XCTAssertEqual(collection.overview, "This is a sample package collection listing made-up packages.")
172+
XCTAssertEqual(collection.keywords, ["sample package collection"])
173+
XCTAssertEqual(collection.createdBy?.name, "Jane Doe")
174+
XCTAssertEqual(collection.packages.count, 2)
175+
let package = collection.packages.first!
176+
XCTAssertEqual(package.repository, .init(url: "https://www.example.com/repos/RepoOne.git"))
177+
XCTAssertEqual(package.summary, "Package One")
178+
XCTAssertEqual(package.keywords, ["sample package"])
179+
XCTAssertEqual(package.readmeURL, URL(string: "https://www.example.com/repos/RepoOne/README")!)
180+
XCTAssertEqual(package.license, .init(type: .Apache2_0, url: URL(string: "https://www.example.com/repos/RepoOne/LICENSE")!))
181+
XCTAssertEqual(package.versions.count, 1)
182+
let version = package.versions.first!
183+
XCTAssertEqual(version.summary, "Fixed a few bugs")
184+
let manifest = version.manifests.values.first!
185+
XCTAssertEqual(manifest.packageName, "PackageOne")
186+
XCTAssertEqual(manifest.targets, [.init(name: "Foo", moduleName: "Foo")])
187+
XCTAssertEqual(manifest.products, [.init(name: "Foo", type: .library(.automatic), targets: [.init(name: "Foo", moduleName: "Foo")])])
188+
XCTAssertEqual(manifest.toolsVersion, ToolsVersion(string: "5.1")!)
189+
XCTAssertEqual(manifest.minimumPlatformVersions, [SupportedPlatform(platform: .macOS, version: .init("10.15"))])
190+
XCTAssertEqual(version.verifiedCompatibility?.count, 3)
191+
XCTAssertEqual(version.verifiedCompatibility!.first!.platform, .macOS)
192+
XCTAssertEqual(version.verifiedCompatibility!.first!.swiftVersion, SwiftLanguageVersion(string: "5.1")!)
193+
XCTAssertEqual(version.license, .init(type: .Apache2_0, url: URL(string: "https://www.example.com/repos/RepoOne/LICENSE")!))
194+
XCTAssertNotNil(version.createdAt)
195+
XCTAssertTrue(collection.isSigned)
196+
let signature = collection.signature!
197+
XCTAssertFalse(signature.isVerified)
135198
XCTAssertEqual("Sample Subject", signature.certificate.subject.commonName)
136199
XCTAssertEqual("Sample Issuer", signature.certificate.issuer.commonName)
137200
}

Tests/PackageCollectionsTests/PackageCollectionsSourcesStorageTest.swift

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,16 @@ final class PackageCollectionsSourcesStorageTest: XCTestCase {
8686
source.isTrusted = !(source.isTrusted ?? false)
8787
_ = try tsc_await { callback in storage.update(source: source, callback: callback) }
8888
let listAfter = try tsc_await { callback in storage.list(callback: callback) }
89-
XCTAssertEqual(source.isTrusted, listAfter.first!.isTrusted, "item should match")
89+
XCTAssertEqual(source.isTrusted, listAfter.first!.isTrusted, "isTrusted should match")
90+
}
91+
92+
do {
93+
let list = try tsc_await { callback in storage.list(callback: callback) }
94+
var source = list.first!
95+
source.skipSignatureCheck = !source.skipSignatureCheck
96+
_ = try tsc_await { callback in storage.update(source: source, callback: callback) }
97+
let listAfter = try tsc_await { callback in storage.list(callback: callback) }
98+
XCTAssertEqual(source.skipSignatureCheck, listAfter.first!.skipSignatureCheck, "skipSignatureCheck should match")
9099
}
91100
}
92101

Tests/PackageCollectionsTests/PackageCollectionsTests.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -831,7 +831,8 @@ final class PackageCollectionsTests: XCTestCase {
831831
certificate: PackageCollectionsModel.SignatureData.Certificate(
832832
subject: .init(userID: nil, commonName: nil, organizationalUnit: nil, organization: nil),
833833
issuer: .init(userID: nil, commonName: nil, organizationalUnit: nil, organization: nil)
834-
)
834+
),
835+
isVerified: true
835836
)
836837
callback(.success(PackageCollectionsModel.Collection(source: source, name: "", overview: nil, keywords: nil, packages: [], createdAt: Date(), createdBy: nil, signature: signature)))
837838
}

Tests/PackageCollectionsTests/Utility.swift

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,8 @@ func makeMockCollections(count: Int = Int.random(in: 50 ... 100), maxPackages: I
8888
certificate: PackageCollectionsModel.SignatureData.Certificate(
8989
subject: .init(userID: nil, commonName: "subject-\(collectionIndex)", organizationalUnit: nil, organization: nil),
9090
issuer: .init(userID: nil, commonName: "issuer-\(collectionIndex)", organizationalUnit: nil, organization: nil)
91-
)
91+
),
92+
isVerified: true
9293
)
9394
}
9495

0 commit comments

Comments
 (0)