CI: add pgp keys expiry check #10112
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @GustavoStingelin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've introduced a new CI job to proactively monitor the expiration dates of PGP keys. This job will ensure that any PGP keys (both primary and subkeys) located in the scripts/keys directory are checked, and the CI pipeline will fail if any key is set to expire within the next two weeks, helping to prevent unexpected service disruptions due to expired keys.
Highlights
- New CI Job for PGP Key Expiry: I've added a new job to the CI pipeline specifically designed to check the expiration status of PGP keys. This job will run automatically to ensure key validity.
- PGP Key Expiry Script: I've implemented a new shell script, scripts/check-pgp-expiry.sh, which is responsible for scanning PGP keys and identifying those that are nearing their expiration date.
- Two-Week Expiry Threshold: The script is configured to flag any PGP key that is set to expire within the next two weeks, providing an early warning system for key management.
- Error Handling and Reporting: The script includes robust error handling for missing key directories or invalid key files, and it will cause the CI job to fail if expiring keys are found or if it encounters issues parsing key information.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new shell script to check for PGP key expiration, which is a valuable addition to the CI pipeline. The script is well-structured and uses good practices like set -euo pipefail. My review includes a couple of suggestions to improve the script's robustness and code style, such as using a safer method for file globbing and optimizing the placement of a helper function. Overall, this is a great contribution.
GustavoStingelin
force-pushed
the
check-pgp-keys-expiry
branch
2 times, most recently
from
49e8fd9 to
5737c33
Compare
GustavoStingelin
force-pushed
the
check-pgp-keys-expiry
branch
from
5737c33 to
5422c7f
Compare
|
something is off with the comparison see the results of the key expiration test, why does it say expires soon if the keys are already expired ? Moreover I wonder if one subkey is expired that we should fail the verification if another subkey for encryption is still valid ? |
Yeah, wanted to say the same. I use sub keys so I can rotate them more easily. So only if all sub keys are expired should the script error out. |
It's just to simplify the logic, but I can implement a better xp |
Yeah, I was thinking about this while writing the script. I’ll implement that behavior since keeping old subkeys in the key file is more practical and avoids breaking the verification of past signatures. |
GustavoStingelin
force-pushed
the
check-pgp-keys-expiry
branch
from
5422c7f to
b4d0729
Compare
Script Improvements
|
|
@guggero I wonder how we should handle keys of contributors who already left ? Their signature and the keys are still verifiable and valid if the signature was made when the key was valid ? Should we keep them around forever ? Moreover people might already have an updated Pubkey on their local computer in if they sign it with it, it could be still verifiable with the pubkey which is in this repo but expired ? |
scripts/check-pgp-expiry.sh
Outdated
| fi | ||
|
|
||
| valid_sign_key_found=true | ||
| echo "INFO: $key_info is valid until $expiry_date" |
There was a problem hiding this comment.
why don't we print the fingerprint anymore ?
There was a problem hiding this comment.
To print the full fingerprint, we need to parse two lines per key, which adds complexity to the script. I chose to stick with a single-line parse and print only the key ID instead.
The trade-off here is simplicity versus completeness, but since the ID is part of the fingerprint, in practice, we’re showing a shortened version of it. IMO it's not worth the extra parsing complexity just to print the full fingerprint here.
Hmm, good question. Don't really have a good answer or strong opinion. |
Also, older releases will retain their expired keys since they were previously committed along with the tag. |
GustavoStingelin
force-pushed
the
check-pgp-keys-expiry
branch
from
b4d0729 to
48e2537
Compare
|
I think however we need to also fail if the primary key expired, because for every verification the primary key is used the make sure the subkey was issued by it. So we need to make sure the primary key is not expired. |
GustavoStingelin
force-pushed
the
check-pgp-keys-expiry
branch
from
48e2537 to
596696c
Compare
ok, I've added this check. |
There was a problem hiding this comment.
Looking good code wise, I would appreciate a more readable output of the script something like this (run it through my AI helper because currently the output is not super convenient to parse by a human:
--------------------------------------------------
Checking key file: ./scripts/keys/ViktorTigerstrom.asc
--------------------------------------------------
ERROR: ViktorTigerstrom.asc primary-key pub:B984570980684DCC (sc) has already expired (2025-06-05)
ERROR: ViktorTigerstrom.asc: primary key is invalid, skipping subkey check.
--------------------------------------------------
Checking key file: ./scripts/keys/bhandras.asc
--------------------------------------------------
INFO: bhandras.asc primary-key pub:80E5375C094198D8 (scESC) is valid until 2027-06-06
INFO: bhandras.asc has a valid primary signing key
--------------------------------------------------
Checking key file: ./scripts/keys/carlaKC.asc
--------------------------------------------------
INFO: carlaKC.asc primary-key pub:4CA7FE54A6213C91 (scESC) is valid until 2034-05-05
INFO: carlaKC.asc has a valid primary signing key
--------------------------------------------------
Checking key file: ./scripts/keys/ellemouton.asc
--------------------------------------------------
INFO: ellemouton.asc primary-key pub:D7D916376026F177 (scSC) is valid until 2026-06-21
INFO: ellemouton.asc has a valid primary signing key
--------------------------------------------------
Checking key file: ./scripts/keys/ffranr.asc
--------------------------------------------------
ERROR: ffranr.asc primary-key pub:B1F8848557AA29D2 (sc) has already expired (2024-10-05)
ERROR: ffranr.asc: primary key is invalid, skipping subkey check.
--------------------------------------------------
Checking key file: ./scripts/keys/guggero.asc
--------------------------------------------------
INFO: guggero.asc primary-key pub:8E4256593F177720 (scESC) is valid until 2034-04-28
INFO: guggero.asc has a valid primary signing key
--------------------------------------------------
Checking key file: ./scripts/keys/hieblmi.asc
--------------------------------------------------
ERROR: hieblmi.asc primary-key pub:F82D456EA023C9BF (sc) has already expired (2024-06-01)
ERROR: hieblmi.asc: primary key is invalid, skipping subkey check.
--------------------------------------------------
Checking key file: ./scripts/keys/positiveblue.asc
--------------------------------------------------
INFO: positiveblue.asc primary-key pub:E9FE7FE00AD163A4 (cC) does not expire
INFO: positiveblue.asc: primary key is not a signing key, checking subkeys
ERROR: positiveblue.asc sub:4FFF2510928804DC (s) has already expired (2022-09-23)
ERROR: positiveblue.asc does not have any valid signing key
--------------------------------------------------
Checking key file: ./scripts/keys/proofofkeags.asc
--------------------------------------------------
INFO: proofofkeags.asc primary-key pub:FA7E65C951F12439 (scESC) is valid until 2027-05-29
INFO: proofofkeags.asc has a valid primary signing key
--------------------------------------------------
Checking key file: ./scripts/keys/roasbeef.asc
--------------------------------------------------
INFO: roasbeef.asc primary-key pub:DC42612E89237182 (scESCA) does not expire
INFO: roasbeef.asc has a valid primary signing key
--------------------------------------------------
Checking key file: ./scripts/keys/sputn1ck.asc
--------------------------------------------------
ERROR: sputn1ck.asc primary-key pub:671103D881A5F0E4 (sc) has already expired (2024-01-05)
ERROR: sputn1ck.asc: primary key is invalid, skipping subkey check.
--------------------------------------------------
Checking key file: ./scripts/keys/suheb.asc
--------------------------------------------------
INFO: suheb.asc primary-key pub:00C9E2BC2E45666F (scESC) is valid until 2025-10-02
INFO: suheb.asc has a valid primary signing key
--------------------------------------------------
Checking key file: ./scripts/keys/yyforyongyu.asc
--------------------------------------------------
INFO: yyforyongyu.asc primary-key pub:9BCD95C4FF296868 (scESC) is valid until 2036-07-14
INFO: yyforyongyu.asc has a valid primary signing key
--------------------------------------------------
Checking key file: ./scripts/keys/ziggie1984.asc
--------------------------------------------------
INFO: ziggie1984.asc primary-key pub:1AFF9C4DCED6D666 (scESC) is valid until 2025-11-30
INFO: ziggie1984.asc has a valid primary signing key
--------------------------------------------------
ERROR: PGP key validation failed. Please check the logs.
GustavoStingelin
force-pushed
the
check-pgp-keys-expiry
branch
from
596696c to
78dce29
Compare
Ok, take a look at this version that I pushed |
|
@GustavoStingelin, remember to re-request review from reviewers when ready |
.github/workflows/main.yml
Outdated
| ######################## | ||
| # Check release signing keys | ||
| ######################## | ||
| pgp-key-expiration-check: |
There was a problem hiding this comment.
This feels more like a job that should be done in release.yaml than being checked for every PR?
There was a problem hiding this comment.
also it looks like the job failed, maybe missing a rebase?
There was a problem hiding this comment.
very good idea to only run it for releases !
There was a problem hiding this comment.
rebased here, but still have one invalid
Checking ViktorTigerstrom.asc...
WARN: pub:B984570980684DCC (sc) has already expired (2025-06-05)
ERROR: pub:B984570980684DCC (sc) primary key is invalid
ERROR: ViktorTigerstrom.asc does not have any valid sign key
There was a problem hiding this comment.
@yyforyongyu I moved this to the release workflow, please check if everything is correct. Using act -j pgp-key-expiration-check works on my side.
There was a problem hiding this comment.
yeah we need to fix that invalid key @ViktorTigerstrom
GustavoStingelin
force-pushed
the
check-pgp-keys-expiry
branch
from
78dce29 to
7ba28c4
Compare
|
I don't think it's a good idea to fail a release because of an expired key! At that point we've pushed a tag and fixing it would mean create a new PR to remove the key, then push another tag. So IMO we should keep the step in the release pipeline but make sure we continue with the release build even on failure. |
Yeah make sense - think we can either skip the error in the release build or move it to the daily job. |
Description
Closes #6281. This PR adds a new job to the CI pipeline that verifies the expiration of PGP keys. The job will fail if no valid signing-capable primary or subkey is found.
And, since we have some expired keys, the job is currently failing.
How to Test
Run the script manually:
Or simulate the CI job locally using:
To inspect the expiration dates for debugging, use:
gpg --with-colons --import-options show-only --import "$key_file"Output: