keyindex: op=index skips revoked UATs#4
Open
mneme-2026 wants to merge 3 commits into
Open
Conversation
In op=index (the compact public listing), revoked User Attribute Packets (photos whose owner has issued a 0x30 certification revocation on them) used to be shown like any other UAT — adding visual clutter without conveying current identity. They are now filtered out at display time. op=vindex is intentionally unchanged: its purpose is the full key state, history included, where a revoked UAT remains informative. Detection is the same heuristic the rest of keyindex.c already uses for sig revocations: signature version 4 or 5, sigtype 0x30. No attempt is made to authenticate which key issued the revocation; the listing is a display, not a trust decision.
The previous commit incremented imgindx only for UATs that were actually rendered. But op=photo's getphoto() iterates over every UAT in the key (revoked or not) and counts them with the same stride — so the HTML idx=N we emitted desynced from what getphoto() looked up. Concretely: when key has [revoked UAT, valid UAT] in that order, we hid the revoked one (correct), emitted idx=0 for the valid one (wrong: getphoto(0) returns the revoked blob the browser is then served). Fix: capture imgindx and increment it *before* the skip check, so the emitted idx always matches the position getphoto() will index into. Reported by foopgp's keyserver operator after deploying the previous commit and seeing the revoked photo show up in place of the valid one.
Owner
|
OOI, do you have an example key fingerprint this is seen with? I'm wondering if we should at least check the signature is from the key in question to avoid the trivial ability for anyone to hide the photo id line, though it's not exactly a big problem I guess. |
u1f35c
reviewed
May 27, 2026
| } | ||
| if ((sigs->packet->data[0] == 4 || | ||
| sigs->packet->data[0] == 5) && | ||
| sigs->packet->data[1] == 0x30) { |
Owner
There was a problem hiding this comment.
I think this should be OPENPGP_SIGTYPE_CERT_REV rather than the bare magic number.
u1f35c
reviewed
May 27, 2026
| * UATs out of the count would desync the HTML idx=N | ||
| * we emit from the index getphoto() looks up — making | ||
| * the browser fetch the wrong (typically: revoked) | ||
| * photo blob for what is shown as the valid UAT. |
Owner
There was a problem hiding this comment.
This isn't right, is it? The old code increments imgindx after we use it. I think you can just move the imgindx++ to the end of this block, and add an } else { to the skip bit rather than the continue?
Per u1f35c review on PR u1f35c#4: - Replace the bare 0x30 magic number in signedpacket_is_revoked() with OPENPGP_SIGTYPE_CERT_REV from openpgp.h (already transitively available, no new include needed). - Restructure the UAT loop body: drop the captured `this_idx` + `continue` shape in favour of `if (skip) {} else { print }` with a single `imgindx++` at the bottom of the UAT branch. Same behaviour (imgindx still increments for every UAT, keeping it aligned with getphoto()'s stride), but a single linear exit path per iteration is easier to read. Update the rationale comment to match.
Author
|
Both points addressed in c2c4ad9, thanks for the review !
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Small focused change. When a key has a User Attribute Packet (typically a JPEG photo) that the owner has revoked with a certification revocation (sigtype
0x30),op=indexkeeps showing it as a[photo id]line. In the compact public listing, this is visual noise — the photo is no longer part of what the owner currently asserts.The patch filters revoked UATs out of
op=indexonly.op=vindexis intentionally unchanged: its purpose is the full audit view, where the revoked UAT and itsrevsignatures remain informative.Detection uses the same heuristic the rest of
keyindex.calready applies for sig revocations (signature version 4 or 5, sigtype0x30). No attempt to authenticate which key issued the revocation — the listing is a display, not a trust decision.One static helper added (
signedpacket_is_revoked), 40 lines including comments. Compiles clean (CMake build with no new warnings).Submitted on behalf of foopgp (Friends of OpenPGP), which runs onak at https://keys.foopgp.org/. A short presentation email will follow.