Skip to content

Add support for two OpenID Connect key-pairs#11626

Merged
mitchellhenke merged 11 commits intomainfrom
mitchellhenke/openid-connect-cert-rotation
Dec 13, 2024
Merged

Add support for two OpenID Connect key-pairs#11626
mitchellhenke merged 11 commits intomainfrom
mitchellhenke/openid-connect-cert-rotation

Conversation

@mitchellhenke
Copy link
Copy Markdown
Contributor

🛠 Summary of changes

Following up on #11612, this adds a secondary OpenID Connect key-pair that gives us the ability to do a zero downtime rotation. The two externally facing elements that require changes are:

  • Our /api/openid_connect/certs route which relying parties use to verify our signed payloads (token, security events)
  • Our verification of the deprecated id_token_hint parameter in OIDC Logout

Previously, we did not allow "missing" app artifacts, but we may not always have a secondary key-pair, so the class was modified to allow it (though it still defaults to not allowing it).

@mitchellhenke mitchellhenke requested review from a team and Sgtpluck December 11, 2024 22:33
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/openid-connect-cert-rotation branch from 8fa74a0 to a0ed94d Compare December 12, 2024 14:14
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM.

One last thought is that maybe we could redo the naming of the primary/secondary? The existing pattern we have for PII fingerprinting, attribute encrypting, and other rotatable secrets is like is hmac_fingerprinter_key for the current one and hmac_fingerprinter_key_queue for older ones.

So my proposal here would be something like oidc_private_key and oidc_private_key_queue, then oidc_public_key and oidc_public_key_queue?

Mitchell Henke and others added 10 commits December 12, 2024 12:26
changelog: Internal, OpenID Connect, Support two OIDC key-pairs
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/openid-connect-cert-rotation branch from 848d9cb to 935a489 Compare December 12, 2024 18:26
@mitchellhenke
Copy link
Copy Markdown
Contributor Author

LGTM.

One last thought is that maybe we could redo the naming of the primary/secondary? The existing pattern we have for PII fingerprinting, attribute encrypting, and other rotatable secrets is like is hmac_fingerprinter_key for the current one and hmac_fingerprinter_key_queue for older ones.

So my proposal here would be something like oidc_private_key and oidc_private_key_queue, then oidc_public_key and oidc_public_key_queue?

Yeah, I had been hoping to find a way to do that, but I'm not sure on the past way to handle it. I added a commit to put it in the Rails application config, which feels alright? Open to suggestions though.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/openid-connect-cert-rotation branch from 935a489 to bac71fe Compare December 12, 2024 18:33
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/openid-connect-cert-rotation branch from bac71fe to 33b57db Compare December 12, 2024 18:41
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.

2 participants