Fix: validate pacman signing key before allowing update#9706
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @Naveen-Boddepalli on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @warpdotdev/oss-maintainers. Comment Powered by Oz |
|
Hi! This PR adds validation for pacman signing key validity to prevent update failures due to invalid keys. Happy to make any changes if needed. |
There was a problem hiding this comment.
Overview
This PR updates the pacman signing-key detection path to inspect GPG's colon-formatted pub validity field before treating the key as configured, while preserving the existing expiration checks.
Concerns
- No blocking correctness or security concerns found in the changed hunk.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
recheck |
|
/oz-review |
2 similar comments
|
/oz-review |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @warpdotdev/oss-maintainers. Comment Powered by Oz |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @warpdotdev/oss-maintainers. Comment Powered by Oz |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: @warpdotdev/oss-maintainers. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR adds a pub: validity check to the pacman signing-key detection path before preserving the existing expiry validation. The change causes revoked, expired, unknown, or otherwise insufficiently trusted keys to be treated as needing reconfiguration before update.
Concerns
- No blocking correctness or security concerns found in the changed lines.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
Overview
This PR adds a GPG colon-format validity check before the existing pacman signing-key expiry validation, so invalid, revoked, expired, or untrusted keys trigger key reconfiguration before update.
Concerns
- GnuPG documents the validity field as a leading letter that may include additional appended data in future versions; exact string matching can create false negatives. The inline suggestion keeps the same policy while parsing the documented leading letter.
Verdict
Found: 0 critical, 0 important, 1 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Co-authored-by: oz-for-oss[bot] <277970191+oz-for-oss[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Overview
This PR adds a GPG colon-format validity check before the existing pacman signing-key expiry validation, so invalid, revoked, expired, or untrusted keys trigger key reconfiguration before update.
Concerns
- GnuPG documents the validity field as a leading letter that may include additional appended data in future versions; exact string matching can create false negatives. The inline suggestion keeps the same policy while parsing the documented leading letter.
Verdict
Found: 0 critical, 0 important, 1 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Co-authored-by: oz-for-oss[bot] <277970191+oz-for-oss[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Overview
This PR adds validation of the GPG pub: validity field for Warp's pacman signing key before preserving the existing expiry check. Invalid or unusable trust states now cause the update flow to reconfigure the key before running pacman.
Concerns
- No blocking correctness or security concerns found in the changed lines.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
hey @Naveen-Boddepalli, was there a particular issue you or another user ran into that this change is fixing? |
|
Hi! I didn’t encounter a specific user-facing failure personally, but while looking at the pacman key validation logic I noticed that we only check for key presence and expiration, and not the GPG validity state. In cases where a key is revoked, expired, or not trusted, GPG marks it as invalid (e.g., e, r, q, etc.), but the current logic could still treat it as usable if the expiration check passes. This could allow pacman to proceed with a key that isn’t actually valid, potentially causing failures during package verification. This change ensures we catch those cases earlier and trigger key reconfiguration before attempting the update, aligning better with GPG’s trust model. Happy to adjust the approach if you think this should be handled differently. |
vorporeal
left a comment
There was a problem hiding this comment.
that makes sense! just wanted to understand the source of the PR. thanks for the contribution!
| if !matches!(validity, 'f' | 'u') { | ||
| return false; // Force key reconfiguration | ||
| } | ||
| if !matches!(validity, 'f' | 'u') { | ||
| return false; // Force key reconfiguration | ||
| } |
There was a problem hiding this comment.
can you remove the duplicated check? (the suggested diffs sometimes don't target the correct range, leading to issues like this)
There was a problem hiding this comment.
Removed the duplicated validity check — thanks for catching that.
Removed redundant validity check for key reconfiguration.
) ## Description ## Summary This PR improves the pacman update flow by validating the signing key before proceeding with the update. Currently, Warp checks for the presence and expiration of the pacman signing key, but does not verify whether the key is valid at the trust level (e.g., revoked, expired, or otherwise unusable). This can lead to update failures during package installation due to signature verification errors. ## Changes - Added parsing of the `pub:` line from GPG output to extract the validity field - Ensured only valid keys (`f` = full, `u` = ultimate) are accepted - Return false for invalid states (`e`, `r`, `-`, `q`) to trigger key reconfiguration - Preserved existing expiry checks and overall logic flow ## Impact Prevents update attempts with invalid or misconfigured pacman signing keys, improving reliability and user experience on pacman-based systems. Happy to make further changes if needed. ## Linked Issue N/A - [ ] The linked issue is labeled `ready-to-spec` or `ready-to-implement`. - [ ] Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes). ## Screenshots / Videos N/A ## Testing - Verified correctness of GPG output parsing logic - Ensured compatibility with existing expiry validation - Confirmed no changes to external interfaces or update flow beyond validation No new tests were added as this is a small internal validation improvement. ## Agent Mode - [ ] Warp Agent Mode - This PR was created via Warp's AI Agent Mode --------- Co-authored-by: oz-for-oss[bot] <277970191+oz-for-oss[bot]@users.noreply.github.com>
) ## Description ## Summary This PR improves the pacman update flow by validating the signing key before proceeding with the update. Currently, Warp checks for the presence and expiration of the pacman signing key, but does not verify whether the key is valid at the trust level (e.g., revoked, expired, or otherwise unusable). This can lead to update failures during package installation due to signature verification errors. ## Changes - Added parsing of the `pub:` line from GPG output to extract the validity field - Ensured only valid keys (`f` = full, `u` = ultimate) are accepted - Return false for invalid states (`e`, `r`, `-`, `q`) to trigger key reconfiguration - Preserved existing expiry checks and overall logic flow ## Impact Prevents update attempts with invalid or misconfigured pacman signing keys, improving reliability and user experience on pacman-based systems. Happy to make further changes if needed. ## Linked Issue N/A - [ ] The linked issue is labeled `ready-to-spec` or `ready-to-implement`. - [ ] Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes). ## Screenshots / Videos N/A ## Testing - Verified correctness of GPG output parsing logic - Ensured compatibility with existing expiry validation - Confirmed no changes to external interfaces or update flow beyond validation No new tests were added as this is a small internal validation improvement. ## Agent Mode - [ ] Warp Agent Mode - This PR was created via Warp's AI Agent Mode --------- Co-authored-by: oz-for-oss[bot] <277970191+oz-for-oss[bot]@users.noreply.github.com>
Description
Summary
This PR improves the pacman update flow by validating the signing key before proceeding with the update.
Currently, Warp checks for the presence and expiration of the pacman signing key, but does not verify whether the key is valid at the trust level (e.g., revoked, expired, or otherwise unusable). This can lead to update failures during package installation due to signature verification errors.
Changes
pub:line from GPG output to extract the validity fieldf= full,u= ultimate) are acceptede,r,-,q) to trigger key reconfigurationImpact
Prevents update attempts with invalid or misconfigured pacman signing keys, improving reliability and user experience on pacman-based systems.
Happy to make further changes if needed.
Linked Issue
N/A
ready-to-specorready-to-implement.Screenshots / Videos
N/A
Testing
No new tests were added as this is a small internal validation improvement.
Agent Mode