Implement TOFU for package downloads #3890
Conversation
yim-lee
requested review from
abertelrud,
elsh,
neonichu and
tomerd
as code owners
|
@swift-ci please smoke test |
yim-lee
force-pushed
the
tofu
branch
2 times, most recently
from
9f14b98
to
16dd8d8
Compare
yim-lee
changed the title
|
@swift-ci please smoke test |
| private let jsonDecoder: JSONDecoder | ||
|
|
||
| public init(configuration: RegistryConfiguration, | ||
| identityResolver: IdentityResolver, | ||
| customArchiverProvider: ((FileSystem) -> Archiver)? = nil, | ||
| customHTTPClient: HTTPClient? = nil, | ||
| authorizationProvider: HTTPClientAuthorizationProvider? = nil) | ||
| authorizationProvider: HTTPClientAuthorizationProvider? = nil, | ||
| fingerprintStorage: PackageFingerprintStorage? = nil) |
There was a problem hiding this comment.
should authorizationProvider and fingerprintStorage be non-optional? they feel like they should be required (ie they are not customization like customHTTPClient which is optional). If this was made optional for unit tests, we can create extensions in the test module to make it easier to construct while keeping the main constructor "correct"
There was a problem hiding this comment.
The only reason fingerprintStorage is optional is because sharedCacheDirectory is optional. What should we default storage path to in that case?
There was a problem hiding this comment.
fingerprintStorage is non-optional 036385b
|
cc @andyp-apple and @robertlacroix |
|
very nice 👍 |
| guard revision.identifier == fingerprint.value else { | ||
| throw StringError("Source control fingerprint \(revision.identifier) for \(self.package) version=\(version) does not match previously recorded value \(fingerprint.value)") | ||
| } | ||
| } | ||
|
|
||
| /// Returns revision for the given tag. |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
| } | ||
|
|
||
| // The revision (i.e., hash) must match that in fingerprint storage otherwise the integrity check fails | ||
| if revision.identifier != fingerprint.value { |
There was a problem hiding this comment.
should this check move to L169 (inside the first case basically)?
There was a problem hiding this comment.
It could be, but then any error thrown would get caught in the catch block (if you recall the code was checking for StringError specifically) and the code wouldn't be as clean.
Wire up fingerprint storage such that it is used for integrity checks of package downloads. Fingerprint must match previously recorded value (if any) or else it would result in an error.
|
@swift-ci please smoke test |
This is a continuation of #3879.
Wire up fingerprint storage such that it is used for integrity checks of package downloads. Fingerprint must match previously recorded value (if any) or else it would result in an error.
strictFingerprintChecking) to ignore TOFU failures (i.e., error -> warning).swiftpm/security/fingerprints/