chore: remove dead CRL-to-nginx sync (nginx never loads the file)#54
Open
omarsy wants to merge 1 commit into
Open
chore: remove dead CRL-to-nginx sync (nginx never loads the file)#54omarsy wants to merge 1 commit into
omarsy wants to merge 1 commit into
Conversation
The register worker and the crl_sync_worker fetched the manager's CRL and copied it into the nginx container as /root/revoked.crl. No nginx config ever referenced that file: ssl_crl only takes effect together with ssl_verify_client, and this nginx has never done client-cert verification (no ssl_verify_client anywhere in repo history). The copy has been a no-op since the first commit, surviving the Django-to-FastAPI port as preserved legacy behavior. Dead security machinery is worse than none: it reads as if revocation is enforced on the greffer when it is not. Remove the worker, the register-step fetch, the crl_sync_interval setting, and their tests. The manager keeps issuing CRLs and serving /api/greffer/ca/crl/ (certs embed it as their CRL distribution point); enforcement is tracked in the platform security backlog and lands with the planned step-ca migration, which replaces the built-in CA. No behavior change for any traffic path. The greffer now runs two background workers (register / monitor) instead of three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Omar Sy <syomar.pro@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
What
Removes the greffer-side CRL distribution machinery:
app/workers/crl.py(thecrl_sync_workerforever-loop) and its tests_fetch_and_store_crlstep at the end ofregister_workercrl_sync_intervalsettingThe greffer now runs two background workers (register / monitor) instead of three.
Why
The sync fetched
GET /api/greffer/ca/crl/from the manager and copied it into the nginx container as/root/revoked.crl. Nothing ever read that file:ssl_crl, which only takes effect withssl_verify_client(mTLS). Neither directive has ever existed in this repo (verified across full git history; the CRL copy dates to the first commit).X-GREFFON-TOKENbearer token, so enablingssl_verify_clienttoday would break the control plane, not harden it.So the copy has been a silent no-op for the life of the project, surviving the Django to FastAPI port as preserved legacy behavior. Dead security machinery is worse than none: it reads as if revocation is enforced on the greffer when it is not.
What this does NOT change
/api/greffer/ca/crl/. Issued certs embed that URL as their CRL distribution point. Nothing manager-side is touched.Context
Revocation enforcement (manager-side, where greffer server certs are actually validated) is documented as an explicitly accepted gap in the root repo security backlog, deferred to the M2 step-ca + Let's Encrypt migration which replaces the built-in CA. Companion root-repo docs PR follows.
Testing
🤖 Generated with Claude Code