Skip to content

Commit 492c356

Browse files
yim-leeDougGregor
andauthored
[5.6] Cherry-pick TOFU changes from main branch (#3930)
* Disable fingerprint checking when storage is not available Motivation: Source compat test failure: https://ci.swift.org/job/swift-PR-source-compat-suite/5701/artifact/swift-source-compat-suite/ ``` error: Failed to get source control fingerprint for swift-log remoteSourceControl https://github.com/apple/swift-log.git version 1.4.2 from storage: Error Domain=NSCocoaErrorDomain Code=513 "You don't have permission to save the file "fingerprints" in the folder "security"." UserInfo={NSFilePath=/Users/buildnode/.swiftpm/security/fingerprints, NSUnderlyingError=0x7feaae439370 {Error Domain=NSPOSIXErrorDomain Code=1 "Operation not permitted"}} error: Error Domain=NSCocoaErrorDomain Code=513 "You don't have permission to save the file "fingerprints" in the folder "security"." UserInfo={NSFilePath=/Users/buildnode/.swiftpm/security/fingerprints, NSUnderlyingError=0x7feaae439370 {Error Domain=NSPOSIXErrorDomain Code=1 "Operation not permitted"}} ``` Modifications: - Make `PackageFingerprintStorage` optional in `RegistryClient` and `SourceControlPackageContainer`, which would turn off fingerprint read/write and essentially disable the TOFU feature. - `SwiftTool` will try to create the shared security directory (under which fingerprints are stored), and if it fails (e.g., permission errors) set `PackageFingerprintStorage` to none. - Don't perform integrity check on fingerprint write. The validation failure will happen on read. * Throw fingerprint write errors Per review feedback, reverting part of the changes introduced in #3928. * Create 'security' directory test should try writing files too (#3941) Motivation: Source compat test continues to fail even with #3928: ``` "You don’t have permission to save the file “fingerprints” in the folder “security” ``` https://ci.swift.org/job/swift-PR-source-compat-suite/5709/artifact/ The tests we did with #3938 shows that we can create directories but not write files. Modifications: Test creating `security` directory and writing file in it, and disable TOFU feature if the test fails. * Fix test that fails to compile (#3936) Co-authored-by: Doug Gregor <[email protected]>
1 parent a56c214 commit 492c356

File tree

8 files changed

+98
-62
lines changed

8 files changed

+98
-62
lines changed

Sources/Commands/SwiftTool.swift

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -640,6 +640,22 @@ public class SwiftTool {
640640
return .none
641641
}
642642
}
643+
644+
private func getSharedSecurityDirectory() throws -> AbsolutePath? {
645+
do {
646+
let fileSystem = localFileSystem
647+
let sharedSecurityDirectory = fileSystem.swiftPMSecurityDirectory
648+
if !fileSystem.exists(sharedSecurityDirectory) {
649+
try fileSystem.createDirectory(sharedSecurityDirectory, recursive: true)
650+
}
651+
// And make sure we can write files (locking the directory writes a lock file)
652+
try fileSystem.withLock(on: sharedSecurityDirectory, type: .exclusive) { }
653+
return sharedSecurityDirectory
654+
} catch {
655+
self.observabilityScope.emit(warning: "Failed creating shared security directory: \(error)")
656+
return .none
657+
}
658+
}
643659

644660
/// Returns the currently active workspace.
645661
func getActiveWorkspace() throws -> Workspace {
@@ -649,7 +665,8 @@ public class SwiftTool {
649665

650666
let delegate = ToolWorkspaceDelegate(self.outputStream, logLevel: self.logLevel, observabilityScope: self.observabilityScope)
651667
let provider = GitRepositoryProvider(processSet: processSet)
652-
let sharedCacheDirectory = try self.getSharedCacheDirectory()
668+
let sharedSecurityDirectory = try self.getSharedSecurityDirectory()
669+
let sharedCacheDirectory = try self.getSharedCacheDirectory()
653670
let sharedConfigurationDirectory = try self.getSharedConfigurationDirectory()
654671
let isXcodeBuildSystemEnabled = self.options.buildSystem == .xcode
655672
let workspace = try Workspace(
@@ -658,7 +675,7 @@ public class SwiftTool {
658675
workingDirectory: buildPath,
659676
editsDirectory: self.editsDirectory(),
660677
resolvedVersionsFile: self.resolvedVersionsFile(),
661-
sharedSecurityDirectory: localFileSystem.swiftPMSecurityDirectory,
678+
sharedSecurityDirectory: sharedSecurityDirectory,
662679
sharedCacheDirectory: sharedCacheDirectory,
663680
sharedConfigurationDirectory: sharedConfigurationDirectory
664681
),

Sources/PackageFingerprint/Model.swift

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,5 +65,4 @@ public typealias PackageFingerprints = [Version: [Fingerprint.Kind: Fingerprint]
6565
public enum FingerprintCheckingMode: String {
6666
case strict
6767
case warn
68-
case none
6968
}

Sources/PackageFingerprint/PackageFingerprintStorage.swift

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,16 @@ public extension PackageFingerprintStorage {
4646
}
4747
}
4848

49-
public enum PackageFingerprintStorageError: Error, Equatable {
49+
public enum PackageFingerprintStorageError: Error, Equatable, CustomStringConvertible {
5050
case conflict(given: Fingerprint, existing: Fingerprint)
5151
case notFound
52+
53+
public var description: String {
54+
switch self {
55+
case .conflict(let given, let existing):
56+
return "Fingerprint \(given) is different from previously recorded value \(existing)"
57+
case .notFound:
58+
return "Not found"
59+
}
60+
}
5261
}

Sources/PackageRegistry/RegistryClient.swift

Lines changed: 47 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -100,14 +100,14 @@ public final class RegistryClient {
100100
private let archiverProvider: (FileSystem) -> Archiver
101101
private let httpClient: HTTPClient
102102
private let authorizationProvider: HTTPClientAuthorizationProvider?
103-
private let fingerprintStorage: PackageFingerprintStorage
103+
private let fingerprintStorage: PackageFingerprintStorage?
104104
private let fingerprintCheckingMode: FingerprintCheckingMode
105105
private let jsonDecoder: JSONDecoder
106106

107107
public init(
108108
configuration: RegistryConfiguration,
109109
identityResolver: IdentityResolver,
110-
fingerprintStorage: PackageFingerprintStorage,
110+
fingerprintStorage: PackageFingerprintStorage? = .none,
111111
fingerprintCheckingMode: FingerprintCheckingMode,
112112
authorizationProvider: HTTPClientAuthorizationProvider? = .none,
113113
customHTTPClient: HTTPClient? = .none,
@@ -423,27 +423,29 @@ public final class RegistryClient {
423423
throw RegistryError.invalidSourceArchive
424424
}
425425

426-
self.fingerprintStorage.put(package: package,
427-
version: version,
428-
fingerprint: .init(origin: .registry(registry.url), value: checksum),
429-
observabilityScope: observabilityScope,
430-
callbackQueue: callbackQueue) { storageResult in
431-
switch storageResult {
432-
case .success:
433-
completion(.success(checksum))
434-
case .failure(PackageFingerprintStorageError.conflict(_, let existing)):
435-
switch self.fingerprintCheckingMode {
436-
case .strict:
437-
completion(.failure(RegistryError.checksumChanged(latest: checksum, previous: existing.value)))
438-
case .warn:
439-
observabilityScope.emit(warning: "The checksum \(checksum) from \(registry.url.absoluteString) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))")
440-
completion(.success(checksum))
441-
case .none:
426+
if let fingerprintStorage = self.fingerprintStorage {
427+
fingerprintStorage.put(package: package,
428+
version: version,
429+
fingerprint: .init(origin: .registry(registry.url), value: checksum),
430+
observabilityScope: observabilityScope,
431+
callbackQueue: callbackQueue) { storageResult in
432+
switch storageResult {
433+
case .success:
442434
completion(.success(checksum))
435+
case .failure(PackageFingerprintStorageError.conflict(_, let existing)):
436+
switch self.fingerprintCheckingMode {
437+
case .strict:
438+
completion(.failure(RegistryError.checksumChanged(latest: checksum, previous: existing.value)))
439+
case .warn:
440+
observabilityScope.emit(warning: "The checksum \(checksum) from \(registry.url.absoluteString) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))")
441+
completion(.success(checksum))
442+
}
443+
case .failure(let error):
444+
completion(.failure(error))
443445
}
444-
case .failure(let error):
445-
completion(.failure(error))
446446
}
447+
} else {
448+
completion(.success(checksum))
447449
}
448450
} catch {
449451
completion(.failure(RegistryError.failedRetrievingReleaseChecksum(error)))
@@ -535,8 +537,6 @@ public final class RegistryClient {
535537
return completion(.failure(RegistryError.invalidChecksum(expected: expectedChecksum, actual: actualChecksum)))
536538
case .warn:
537539
observabilityScope.emit(warning: "The checksum \(actualChecksum) does not match previously recorded value \(expectedChecksum)")
538-
case .none:
539-
break
540540
}
541541
}
542542

@@ -567,25 +567,33 @@ public final class RegistryClient {
567567

568568
// We either use a previously recorded checksum, or fetch it from the registry
569569
func withExpectedChecksum(body: @escaping (Result<String, Error>) -> Void) {
570-
self.fingerprintStorage.get(package: package,
571-
version: version,
572-
kind: .registry,
573-
observabilityScope: observabilityScope,
574-
callbackQueue: callbackQueue) { result in
575-
switch result {
576-
case .success(let fingerprint):
577-
body(.success(fingerprint.value))
578-
case .failure(let error):
579-
if error as? PackageFingerprintStorageError != .notFound {
580-
observabilityScope.emit(error: "Failed to get registry fingerprint for \(package) \(version) from storage: \(error)")
570+
if let fingerprintStorage = self.fingerprintStorage {
571+
fingerprintStorage.get(package: package,
572+
version: version,
573+
kind: .registry,
574+
observabilityScope: observabilityScope,
575+
callbackQueue: callbackQueue) { result in
576+
switch result {
577+
case .success(let fingerprint):
578+
body(.success(fingerprint.value))
579+
case .failure(let error):
580+
if error as? PackageFingerprintStorageError != .notFound {
581+
observabilityScope.emit(error: "Failed to get registry fingerprint for \(package) \(version) from storage: \(error)")
582+
}
583+
// Try fetching checksum from registry again no matter which kind of error it is
584+
self.fetchSourceArchiveChecksum(package: package,
585+
version: version,
586+
observabilityScope: observabilityScope,
587+
callbackQueue: callbackQueue,
588+
completion: body)
581589
}
582-
// Try fetching checksum from registry again no matter which kind of error it is
583-
self.fetchSourceArchiveChecksum(package: package,
584-
version: version,
585-
observabilityScope: observabilityScope,
586-
callbackQueue: callbackQueue,
587-
completion: body)
588590
}
591+
} else {
592+
self.fetchSourceArchiveChecksum(package: package,
593+
version: version,
594+
observabilityScope: observabilityScope,
595+
callbackQueue: callbackQueue,
596+
completion: body)
589597
}
590598
}
591599
}

Sources/Workspace/SourceControlPackageContainer.swift

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
5555
private let manifestLoader: ManifestLoaderProtocol
5656
private let toolsVersionLoader: ToolsVersionLoaderProtocol
5757
private let currentToolsVersion: ToolsVersion
58-
private let fingerprintStorage: PackageFingerprintStorage
58+
private let fingerprintStorage: PackageFingerprintStorage?
5959
private let fingerprintCheckingMode: FingerprintCheckingMode
6060
private let observabilityScope: ObservabilityScope
6161

@@ -79,7 +79,7 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
7979
manifestLoader: ManifestLoaderProtocol,
8080
toolsVersionLoader: ToolsVersionLoaderProtocol,
8181
currentToolsVersion: ToolsVersion,
82-
fingerprintStorage: PackageFingerprintStorage,
82+
fingerprintStorage: PackageFingerprintStorage?,
8383
fingerprintCheckingMode: FingerprintCheckingMode,
8484
observabilityScope: ObservabilityScope
8585
) throws {
@@ -150,6 +150,10 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
150150
}
151151

152152
func checkIntegrity(version: Version, revision: Revision) throws {
153+
guard let fingerprintStorage = self.fingerprintStorage else {
154+
return
155+
}
156+
153157
guard case .remoteSourceControl(let sourceControlURL) = self.package.kind else {
154158
return
155159
}
@@ -158,7 +162,7 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
158162
let fingerprint: Fingerprint
159163
do {
160164
fingerprint = try temp_await {
161-
self.fingerprintStorage.get(
165+
fingerprintStorage.get(
162166
package: packageIdentity,
163167
version: version,
164168
kind: .sourceControl,
@@ -172,7 +176,7 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
172176
// Write to storage if fingerprint not yet recorded
173177
do {
174178
try temp_await {
175-
self.fingerprintStorage.put(
179+
fingerprintStorage.put(
176180
package: packageIdentity,
177181
version: version,
178182
fingerprint: fingerprint,
@@ -182,14 +186,12 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
182186
)
183187
}
184188
} catch PackageFingerprintStorageError.conflict(_, let existing) {
185-
let message = "Revision \(revision.identifier) for \(self.package) version \(version) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))"
189+
let message = "Revision \(revision.identifier) for \(self.package) version \(version) does not match previously recorded value \(existing.value) from \(String(describing: existing.origin.url?.absoluteString))"
186190
switch self.fingerprintCheckingMode {
187191
case .strict:
188192
throw StringError(message)
189193
case .warn:
190194
observabilityScope.emit(warning: message)
191-
case .none:
192-
return
193195
}
194196
}
195197
} catch {
@@ -205,8 +207,6 @@ internal final class SourceControlPackageContainer: PackageContainer, CustomStri
205207
throw StringError(message)
206208
case .warn:
207209
observabilityScope.emit(warning: message)
208-
case .none:
209-
return
210210
}
211211
}
212212
}

Sources/Workspace/Workspace.swift

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ public class Workspace {
223223
fileprivate let checksumAlgorithm: HashAlgorithm
224224

225225
/// The package fingerprint storage
226-
fileprivate let fingerprintStorage: PackageFingerprintStorage
226+
fileprivate let fingerprintStorage: PackageFingerprintStorage?
227227

228228
/// Enable prefetching containers in resolver.
229229
fileprivate let resolverPrefetchingEnabled: Bool
@@ -265,6 +265,7 @@ public class Workspace {
265265
/// - customHTTPClient: A custom http client.
266266
/// - customArchiver: A custom archiver.
267267
/// - customChecksumAlgorithm: A custom checksum algorithm.
268+
/// - customFingerprintStorage: A custom fingerprint storage.
268269
/// - additionalFileRules: File rules to determine resource handling behavior.
269270
/// - resolverUpdateEnabled: Enables the dependencies resolver automatic version update check. Enabled by default. When disabled the resolver relies only on the resolved version file
270271
/// - resolverPrefetchingEnabled: Enables the dependencies resolver prefetching based on the resolved version file. Enabled by default.
@@ -314,10 +315,12 @@ public class Workspace {
314315
delegate: delegate.map(WorkspaceRepositoryManagerDelegate.init(workspaceDelegate:)),
315316
cachePath: sharedRepositoriesCacheEnabled ? location.sharedRepositoriesCacheDirectory : .none
316317
)
317-
let fingerprintStorage = customFingerprintStorage ?? FilePackageFingerprintStorage(
318-
fileSystem: fileSystem,
319-
directoryPath: location.sharedFingerprintsDirectory
320-
)
318+
let fingerprintStorage = customFingerprintStorage ?? location.sharedFingerprintsDirectory.map {
319+
FilePackageFingerprintStorage(
320+
fileSystem: fileSystem,
321+
directoryPath: $0
322+
)
323+
}
321324

322325
let registryClient = customRegistryClient ?? registries.map { configuration in
323326
RegistryClient(

Sources/Workspace/WorkspaceConfiguration.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ extension Workspace {
3232
public var resolvedVersionsFile: AbsolutePath
3333

3434
/// Path to the shared security directory
35-
public var sharedSecurityDirectory: AbsolutePath
35+
public var sharedSecurityDirectory: AbsolutePath?
3636

3737
/// Path to the shared cache directory
3838
public var sharedCacheDirectory: AbsolutePath?
@@ -61,8 +61,8 @@ extension Workspace {
6161
}
6262

6363
/// Path to the shared fingerprints directory.
64-
public var sharedFingerprintsDirectory: AbsolutePath {
65-
self.sharedSecurityDirectory.appending(component: "fingerprints")
64+
public var sharedFingerprintsDirectory: AbsolutePath? {
65+
self.sharedSecurityDirectory.map { $0.appending(component: "fingerprints") }
6666
}
6767

6868
/// Path to the shared repositories cache.
@@ -103,7 +103,7 @@ extension Workspace {
103103
workingDirectory: AbsolutePath,
104104
editsDirectory: AbsolutePath,
105105
resolvedVersionsFile: AbsolutePath,
106-
sharedSecurityDirectory: AbsolutePath,
106+
sharedSecurityDirectory: AbsolutePath?,
107107
sharedCacheDirectory: AbsolutePath?,
108108
sharedConfigurationDirectory: AbsolutePath?
109109
) {

Tests/WorkspaceTests/RegistryPackageContainerTests.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -418,7 +418,7 @@ class RegistryPackageContainerTests: XCTestCase {
418418
configuration: configuration!,
419419
identityResolver: identityResolver,
420420
fingerprintStorage: fingerprintStorage,
421-
fingerprintCheckingMode: .none,
421+
fingerprintCheckingMode: .strict,
422422
authorizationProvider: .none,
423423
customHTTPClient: HTTPClient(configuration: .init(), handler: { request, progress , completion in
424424
var pathComponents = request.url.pathComponents

0 commit comments

Comments
 (0)