letsencrypt / letsencrypt/boulder

bad-key-revoker: mitigate potential race condition

Open
#5,686 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
5.8k
Forks
649
Avg merge
3d 23h
Merged PRs (30d)
24

Description

I believe there's a race condition involving compromised keys. Consider the following scenario:

  • A certificate issuance begins, and passes its final goodkey/sa.KeyBlocked check before signing.
  • A compromised key is inserted into blockedKeys.
  • bad-key-revoker begins a run, and executes its SELECT on keyHashToSerial.
  • sa.AddKeyHash is only now called for the certificate issuance.

bad-key-revoker would miss this certificate.

This scenario is very unlikely if all or most of the queries run against the primary database node, but we are about to direct SELECTs to replicas much more frequently. Replication lag on replicas could make this scenario more likely.

This could be avoided by wrapping parts of the issuance pipeline and bad-key-revoker in MySQL transactions, such that the transaction would fail and roll back if the data underlying the transaction's SELECTs (e.g. on blockedKeys or keyHashToSerial) had changed before commitment. However, this would require deep refactoring.

I think an easier approach would be to modify bad-key-revoker to run multiple cycles on each row, with a configurable delay between cycles. Operators would ideally set that delay to the same amount of time for which their database layer is set to guarantee consistency.

This would not require a schema change if extantCertificatesChecked were treated as an integer representing the number of cycles that have been run. It's already a tinyint(1), not a boolean.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with cmd/bad-key-revoker/main.go and the linked SELECT on keyHashToSerial. Read how extantCertificatesChecked is stored and used, then trace the issuance and revocation data flow to assess repeated cycles and a configurable delay. Done means the race scenario is mitigated without a schema change and is covered by relevant tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, mysql
Domain
backend, databases, security
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.