Skip to content

Use RSA keys for openid token signing#2777

Merged
j-chmielewski merged 22 commits into
release/2.0from
rsa-openid
Apr 22, 2026
Merged

Use RSA keys for openid token signing#2777
j-chmielewski merged 22 commits into
release/2.0from
rsa-openid

Conversation

@j-chmielewski
Copy link
Copy Markdown
Contributor

  • Adds openid key settings column
  • Copies the key from config to db if present during migration, generates new one otherwise
  • Introduces "hmac" flag for backwards compatibility

@j-chmielewski j-chmielewski marked this pull request as ready for review April 22, 2026 11:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates OpenID token signing to use an RSA private key stored in the settings table (DER-encoded), with an opt-in deprecated hmac flag to force legacy HS256 behavior.

Changes:

  • Add a new settings.openid_signing_key_der column (BYTEA) and remove the previous openid_signing_key column.
  • Generate/validate/persist the RSA signing key via Settings::initialize_runtime_defaults() and expose runtime retrieval via Settings::openid_key_required().
  • Update OpenID handlers and integration tests to use the DB-backed key, plus add a test for the “missing RSA key” failure path.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
migrations/20260422113000_[2.0.0]_openid_signing_key_der.up.sql Drops legacy OpenID signing key column and adds DER-backed key column.
migrations/20260422113000_[2.0.0]_openid_signing_key_der.down.sql Reverts schema back to the legacy text signing key column.
crates/defguard_common/src/db/models/settings.rs Adds openid_signing_key_der storage, validation, RSA key generation, and required-key accessor.
crates/defguard_core/src/handlers/openid_flow.rs Switches JWKS discovery and token signing to use the runtime settings-backed RSA key (or HS256 under hmac).
crates/defguard_core/tests/integration/api/openid.rs Adds helpers/coverage for RSA key seeding and missing-key behavior in OpenID integration tests.
crates/defguard_common/src/config.rs Deprecates config-provided OpenID RSA key and adds deprecated hmac compatibility flag.
crates/defguard/src/main.rs Updates startup logging to reflect RSA-by-default and deprecated HMAC forcing.
.sqlx/query-*.json Updates SQLx offline metadata for settings SELECT/UPDATE to include openid_signing_key_der.
Comments suppressed due to low confidence (1)

crates/defguard_core/tests/integration/api/openid.rs:497

  • Like test_openid_flow, this test performs /oauth discovery + token exchange but doesn’t seed settings.openid_signing_key_der (and the shared test harness doesn’t initialize runtime defaults). With RSA now required by default, this will fail unless the signing key is generated/seeded before discovery/token requests.
    let (client, _) = make_test_client(pool.clone()).await;

    let issuer_url = IssuerUrl::from_url(Settings::url().unwrap().clone());

    // discover OpenID service
    let provider_metadata =
        CoreProviderMetadata::discover_async(issuer_url, &|r| http_client(r, &client))
            .await

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/defguard_common/src/db/models/settings.rs Outdated
Comment thread crates/defguard_core/src/handlers/openid_flow.rs Outdated
Comment thread crates/defguard_core/tests/integration/api/openid.rs
Comment thread crates/defguard_common/src/db/models/settings.rs
Comment thread crates/defguard_core/tests/integration/api/openid.rs Outdated
Comment thread crates/defguard_common/src/db/models/settings.rs Outdated
Comment thread crates/defguard_common/src/db/models/settings.rs Outdated
@j-chmielewski j-chmielewski merged commit 0c46327 into release/2.0 Apr 22, 2026
13 checks passed
@j-chmielewski j-chmielewski deleted the rsa-openid branch April 22, 2026 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants