Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alternative to support multiple x509 Certificates via procs #211

Merged
merged 7 commits into from
Nov 1, 2024

Conversation

pelted
Copy link
Contributor

@pelted pelted commented Jul 31, 2024

What this PR does

  • allows for looking up x509 certificates with a proc in the SamlIdp config
  • calls the proc or falls back to the original behavior where appropriate
  • adds short example in the REDME documentation

Why

This is an alternative approach to supporting multiple x509 certificates, secret keys, and password. This uses procs in the SamlIdp configuration as an alternative to #186 and #209.

I have been using this approach for some time now using a service object as a finder for the appropriate cert and secret key.

Some examples for looking these up:

config.x509_certificate = -> { File.read("cert.pem") }
config.secret_key = -> { SecretKeyFinder.key_for(id: 1) }
config.password = -> { Rails.application.credentials.dig(:saml_idp, :password ) }

@pelted
Copy link
Contributor Author

pelted commented Aug 6, 2024

Not sure if that's a flaky test in CI, still trying to reproduce the issue locally to resolve.

@Zogoo
Copy link
Collaborator

Zogoo commented Aug 13, 2024

@pelted in here also, I think you have valid points, let us properly look at it and merge it after you fix the tests.

@Zogoo
Copy link
Collaborator

Zogoo commented Sep 5, 2024

Not sure if that's a flaky test in CI, still trying to reproduce the issue locally to resolve.

You are right it seems like failing everywhere, let me look at it.

@atishedcast
Copy link

any progress on this?
I am looking for a similar solution.

@pelted
Copy link
Contributor Author

pelted commented Oct 17, 2024

@Zogoo I believe my latest commit should resolve the failing test if we could kick off the CI workflow and verify.

@Zogoo Zogoo self-assigned this Nov 1, 2024
Copy link
Collaborator

@Zogoo Zogoo left a comment

Choose a reason for hiding this comment

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

This is a valuable change in that the application can set the certificates on the fly.

@Zogoo
Copy link
Collaborator

Zogoo commented Nov 1, 2024

@jphenow I think this is an alternative way to support configuring SamlIdP on the fly.

@Zogoo
Copy link
Collaborator

Zogoo commented Nov 1, 2024

@pelted can you update again with the latest master branch?

@Zogoo Zogoo merged commit 6f832af into saml-idp:master Nov 1, 2024
22 of 23 checks passed
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.

4 participants