Skip to content

Encrypt GitHub access tokens #11585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Jul 16, 2025

We currently save the GitHub OAuth access tokens in plaintext in our database, which isn't a huge issue as long as nobody unauthorized gains access to our database. But from a security standpoint it's better to encrypt them so that an attacker would need both access to the database and to the encryption key.

This PR implements step 1 of the migration towards using encrypted access tokens. It adds a new column to the database, changes the code to start saving encrypted tokens to that column and adds an admin tool to backfill the column based on the current plaintext tokens.

This also adds a new required GITHUB_TOKEN_ENCRYPTION_KEY environment variable without which the server and background worker won't start. The env var expects a 64 character hex string, which is then decoded to a 32 byte AES key. I've already generated corresponding values for our staging and production environments via openssl rand -hex 32.

The encryption uses "AES-256-GCM" which from my understanding should be a reasonable algorithm for this situation and requires no extra dependencies since it was already used transitively.

For completeness: Phase 2 will turn the column non-nullable and will start using the encrypted tokens instead of the plaintext tokens. Phase 3 will then remove the plaintext token column. These phases should be deployed separately for obvious reasons 😅

@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Jul 16, 2025
@Turbo87 Turbo87 requested a review from a team July 16, 2025 17:58
@mdtro
Copy link
Contributor

mdtro commented Jul 16, 2025

Not something to block this PR, but we should think about what would be needed to support key rotation here in the future.

Using a KMS would make this easier, but if we want to keep this all localized there are some options.

  1. Adjust the environment variable to support a list/dictionary.
  2. Assign a random key ID (kid) to each key.
  3. Embed or save the kid in a separate column used to encrypt the token in the database. It could be embedded into the "associated data" of the AEAD Payload (https://docs.rs/aead/0.5.2/aead/struct.Payload.html).
  4. Annotate which key is the "active" one for new encryption operations.

Decryption operations would perform the lookup via the kid.
Encryption operations would always use the key that's marked as "active".

@LawnGnome
Copy link
Contributor

Without doing a full review, my gut reaction is the same as @mdtro's — I'd prefer to use a KMS for rotation and logging purposes. Not going to call it a blocker just yet, but I do want think about this some more.

@walterhpearce, you've been thinking about this kind of problem recently in the context of signing things. Any thoughts?

@Turbo87
Copy link
Member Author

Turbo87 commented Jul 17, 2025

I'm not opposed to using a KMS, but I have zero experience with it. You are both welcome to work on it and coordinate the deployment with the infra team though 😅

I'm a bit worried that we're letting "perfect" be the enemy of "good" here (or whatever the saying is). I would assume that we could still retrofit a KMS later even with the current design. We also have a few other keys that should maybe be moved into a KMS, so this seems like a bigger task that shouldn't necessarily prevent this quick win proposed here.

Aside from that, the use of per-user GitHub access tokens might potentially turn out to not be viable for us anymore in the future. Once we support other identity providers then not every user might have a corresponding GitHub OAuth token anymore, in which case we might not even need to save them since we will need to use some other method anyway. In other words, I currently don't expect us to keep these around for the next ten years, so key rotation might not be quite as critical?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants