Reduce errors from manifest signature validation #6325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please smoke test |
yim-lee
commented
Mar 24, 2023
| @@ -201,6 +204,88 @@ struct SignatureValidation { | |||
| } | |||
| } | |||
|
|
|||
| private func validateSourceArchiveSignature( | |||
There was a problem hiding this comment.
This is the same as the original validateSignature.
There was a problem hiding this comment.
What's the reason for this function itself not being async? Only trying to understand whether we're trying to avoid async functions completely for now or is there some boundary along within which we should keep functions callback-based?
There was a problem hiding this comment.
RegistryClient (the caller of this) uses callback so async has to stop somewhere (for now at least).
|
In case of an error during manifest signature validation, I would expect archive validation to not even happen because we wouldn't know which archive to even download. Am I missing something? |
yim-lee
force-pushed
the
reduce-manifests-errors
branch
from
9169ece to
c7975a0
Compare
|
@swift-ci please smoke test |
|
@swift-ci please test Windows platform |
yim-lee
commented
Mar 24, 2023
| @@ -2259,14 +2259,29 @@ extension RegistryReleaseMetadata { | |||
| private struct RegistryClientSignatureValidationDelegate: SignatureValidation.Delegate { | |||
| let underlying: RegistryClient.Delegate? | |||
|
|
|||
| private let onUnsignedResponseCache = ThreadSafeKeyValueStore<ResponseCacheKey, Bool>() | |||
| private let onUntrustedResponseCache = ThreadSafeKeyValueStore<ResponseCacheKey, Bool>() | |||
There was a problem hiding this comment.
Added caching to avoid prompting repeatedly
| case .success(let signingEntity): | ||
| // Always do signing entity TOFU check at the end, | ||
| // whether the manifest is signed or not. | ||
| self.signingEntityTOFU.validate( |
| @@ -304,48 +412,38 @@ struct SignatureValidation { | |||
| version: version | |||
| ) | |||
|
|
|||
| // Prompt if configured, otherwise just continue (this differs | |||
| // from source archive to minimize duplicate loggings). | |||
tomerd
approved these changes
Mar 27, 2023
Motivation: Manifest signature validation works similarly as source archive signature validation, meaning user could see duplicate errors (e.g., source archive not signed). Modifications: - Change most error throwing to logging diagnostics for manifest signature validation - Continue to prompt if that's what user has configured for unsigned packages or untrusted signers; cache responses in memory to prevent repeatedly prompting for manifest and source archive downloads for the same package version. - Wire up publisher TOFU for manifest signing - Adjust tests
yim-lee
force-pushed
the
reduce-manifests-errors
branch
from
c7975a0 to
d0accdc
Compare
|
@swift-ci please smoke test |
|
@swift-ci please test Windows platform |
neonichu
approved these changes
Mar 27, 2023
|
windows CI has been unstable all day. okay to merge without. |
|
@swift-ci please test Windows platform |
yim-lee
added a commit
to yim-lee/swift-package-manager
that referenced
this pull request
Mar 28, 2023
Motivation: Manifest signature validation works similarly as source archive signature validation, meaning user could see duplicate errors (e.g., source archive not signed). Modifications: - Change most error throwing to logging diagnostics for manifest signature validation - Continue to prompt if that's what user has configured for unsigned packages or untrusted signers; cache responses in memory to prevent repeatedly prompting for manifest and source archive downloads for the same package version. - Wire up publisher TOFU for manifest signing - Adjust tests
yim-lee
added a commit
that referenced
this pull request
Mar 28, 2023
Motivation: Manifest signature validation works similarly as source archive signature validation, meaning user could see duplicate errors (e.g., source archive not signed). Modifications: - Change most error throwing to logging diagnostics for manifest signature validation - Continue to prompt if that's what user has configured for unsigned packages or untrusted signers; cache responses in memory to prevent repeatedly prompting for manifest and source archive downloads for the same package version. - Wire up publisher TOFU for manifest signing - Adjust tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Manifest signature validation works similarly as source archive signature validation, meaning user could see duplicate errors (e.g., source archive not signed).
Modifications: