maj fix certificates - #1157
Open
jma wants to merge 2 commits into
Open
Conversation
jma
commented
Aug 4, 2026
jma
commented
Contributor
- fix(shibboleth): update SWITCH edu-ID identity provider certificates
- fix(certificates): generate development certificates instead of versioning them
The signing certificates of both edu-ID identity providers had expired, in January 2021 for production and October 2020 for the test environment. Replace them with the current ones, valid until 2033, taken from the SWITCH federation metadata and checked against their SHA-256 fingerprints. The test certificate was also truncated by one line, which made it impossible to parse. Co-Authored-By: Johnny Mariéthoz <johnny.mariethoz@rero.ch>
WalkthroughThe project now generates development HTTPS certificates with Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/ui/test_utils.py (1)
73-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the one-sided missing-key cases.
The guard must reject a configuration when either key is absent. This test removes both keys at once, so it does not detect an
andregression or a configuration with only one key. (raw.githubusercontent.com)Add separate assertions for only
x509certand onlyprivate_key.Suggested coverage
- app.config["SHIBBOLETH_SERVICE_PROVIDER"] = {"entity_id": "entity_id"} - assert get_switch_aai_providers() == [] + for service_provider in ( + {"entity_id": "entity_id", "x509cert": "cert"}, + {"entity_id": "entity_id", "private_key": "key"}, + {"entity_id": "entity_id"}, + ): + app.config["SHIBBOLETH_SERVICE_PROVIDER"] = service_provider + assert get_switch_aai_providers() == []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui/test_utils.py` around lines 73 - 76, Extend the get_switch_aai_providers() tests to cover each one-sided configuration separately: assert an empty provider list when only entity_id and x509cert are present, and again when only entity_id and private_key are present. Keep the existing both-keys-missing assertion and ensure each case replaces the service-provider configuration independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Line 269: Add scripts/gen-certs to MANIFEST.in alongside the other
individually listed scripts so source distributions include the executable
required by the certs task.
In `@scripts/server`:
- Around line 62-65: Move the certificate path assignments and the preflight
check using cert and key before the Celery worker startup block in
scripts/server. Ensure missing certificates trigger the existing error message
and exit before Celery is launched, while preserving the existing cleanup and
startup behavior for valid certificates.
---
Nitpick comments:
In `@tests/ui/test_utils.py`:
- Around line 73-76: Extend the get_switch_aai_providers() tests to cover each
one-sided configuration separately: assert an empty provider list when only
entity_id and x509cert are present, and again when only entity_id and
private_key are present. Keep the existing both-keys-missing assertion and
ensure each case replaces the service-provider configuration independently.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1232d8b3-85a4-48f0-9b09-04c0d6a9e741
⛔ Files ignored due to path filters (2)
docker/haproxy/haproxy_cert.pemis excluded by!**/*.pemdocker/nginx/sp.pemis excluded by!**/*.pem
📒 Files selected for processing (28)
.dockerignore.gitignoreINSTALL.mdMANIFEST.indata/idp_certificates/eduid.crtdata/idp_certificates/eduidtest.crtdata/idp_certificates/idp.crtdocker/haproxy/Dockerfiledocker/haproxy/haproxy.cfgdocker/nginx/Dockerfiledocker/nginx/sp.keydocker/nginx/test.crtdocker/nginx/test.keypyproject.tomlscripts/bootstrapscripts/gen-certsscripts/serversonar/modules/shibboleth_authenticator/auth.pysonar/modules/shibboleth_authenticator/config.pysonar/modules/shibboleth_authenticator/ext.pysonar/modules/utils.pysonar/theme/templates/sonar/accounts/login.htmlsonar/theme/templates/sonar/accounts/signup.htmltests/conftest.pytests/ui/shibboleth_authenticator/conftest.pytests/ui/shibboleth_authenticator/test_shibboleth_auth.pytests/ui/test_utils.pytests/ui/test_views.py
💤 Files with no reviewable changes (4)
- docker/nginx/test.crt
- docker/nginx/test.key
- docker/nginx/sp.key
- data/idp_certificates/idp.crt
…oning them Three private keys were versioned in this public repository, exposed since 2018 and 2019: the TLS key of the development server, its duplicate in the HAProxy bundle, and the SAML service provider key. All the versioned certificates had expired as well, which left the HTTPS development server unusable by any modern browser. Certificates are now generated where they are needed: - `scripts/gen-certs` produces the development server pair in `.certs/`, using mkcert when available so that browsers accept it without warning, and falling back to openssl elsewhere. It is called by the bootstrap, which never regenerates an existing pair unless `--force-certs` is given, and skips the step on deploy. - The nginx and HAProxy images generate their own certificate at build time. HAProxy needs a separate stage, its base image ships no openssl command. - The tests build an ephemeral key pair in a temporary directory, so the suite no longer depends on any certificate file. The service provider key pair has no default any more. It identifies the instance to the identity providers, so it has to be registered in the SWITCH edu-ID resource registry and cannot be generated locally: each environment provides its own and references it in `invenio.cfg`. Until then SAML stays disabled and the SWITCHaai button is hidden rather than leading to an error. The identity providers certificates directory becomes configurable, which also removes the constraint of running the application from the repository root. `scripts/server` now verifies the certificates before starting anything. The check used to run after the celery worker had been spawned, and the cleanup trap was registered later still, so a missing pair left an orphaned worker behind. The worker PID was also captured in a differently spelled variable than the one the trap and the final wait read; spelling it consistently makes both cover the worker on a normal exit too. Co-Authored-By: Johnny Mariéthoz <johnny.mariethoz@rero.ch>
jma
force-pushed
the
maj-fix-certificates
branch
from
August 11, 2026 13:27
901cfcb to
498f656
Compare
Garfield-fr
approved these changes
Aug 12, 2026
anouillz
approved these changes
Aug 12, 2026
PascalRepond
approved these changes
Aug 13, 2026
rerowep
approved these changes
Aug 24, 2026
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
5 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.