webhook: dedicated AEAD key (separate from auth.totp_key_b64)#203
Merged
Conversation
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
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.
Summary
shithub's webhook secrets table stores secrets encrypted at rest
via AEAD, but the AEAD key was the same box used for TOTP
secrets (
auth.totp_key_b64). That's an internal hygiene issuenot a feature gap — GitHub-parity is "secrets are encrypted at
rest," which we already did. But:
docs/internal/runbooks/rotate-secrets.mdWebhook AEADsection described a
webhook.aead_keyrotation procedurereferencing config keys that didn't exist in code.
auth.totp_key_b64also exposed every webhooksecret in the DB.
webhook row — coordination friction we didn't need.
This PR adds a dedicated
webhook.aead_key(envSHITHUB_WEBHOOK__AEAD_KEY) with a clean transition path:OpenSecrettakes (primary, legacy) and tries primary first,falls back to legacy. The deliverer wires
primary=webhook.aead_key,legacy=auth.totp_key_b64duringthe transition window.
shithubd admin re-encrypt-webhookswalks the table andre-encrypts each row from legacy to primary. Idempotent +
resumable + safe to interrupt.
SHITHUB_WEBHOOK__AEAD_KEYwhen theinventory var is set (else fall back to current behavior —
zero change for un-upgraded operators).
The deploy path is safe to take in either order (set the
key first, then re-encrypt — or re-encrypt-as-a-no-op first,
then set the key). The fallback keeps rows decryptable through
the transition. Operators who never run
re-encrypt-webhooksalso keep working — they just don't get the separation benefit.
Per-commit reading order
687effaconfig: addwebhook.aead_keyfieldad01cd8OpenSecret(primary, legacy, ...)signature change80d5191tests for the fallback chainbf95125wire primary+legacy through worker → deliverer243bca6web.env.j2renders new env var conditionally8a860ffworker.env.j2samebaa53cfproduction.exampledocuments inventory vardbf13c5newListWebhookSecretsForReencryptsqlc query902f629admin command9e6a3barunbook rewritefeb6338CHANGELOG entryTest plan
go build ./...cleango vet ./...cleangofmt -lclean (touched files)TestOpenSecret_*cases pass:and verify at the receiver
shithubd admin re-encrypt-webhooks --dry-runagainstprod reports a non-zero row count (proves the migration
walker sees existing rows)
Deploy plan
(No new migrations in this PR — pure code change.)
openssl rand -base64 32shithub_webhook_aead_key_b64=…to your localinventory/production. This unlocks the separation butdoesn't activate it yet — Ansible needs to run to render
the new env var into
/etc/shithub/{web,worker}.env.make deploy ANSIBLE_INVENTORY=production(renders new env,restarts services). At this point: new webhook writes go
under the dedicated key; existing rows still decrypt via the
legacy (TOTP) fallback.
ssh root@shithub.sh "sudo -u shithub /usr/local/bin/shithubd admin re-encrypt-webhooks --dry-run"to count migrate-able rows.ssh root@shithub.sh "sudo -u shithub /usr/local/bin/shithubd admin re-encrypt-webhooks"that has a webhook configured) and confirming the receiver
sees a valid HMAC signature.
After step 6, every row is on the dedicated key. The legacy
fallback is dead code in practice — operators can leave it
configured for safety, or remove
auth.totp_key_b64from thedeliverer's reach in a future PR if they want clean separation.