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

[RFE] allow bring your own certs for the oidc-disicovery-provider components #3825

Closed
raffaelespazzoli opened this issue Feb 3, 2023 · 7 comments · Fixed by #4190
Closed
Assignees
Labels
help wanted Issues with this label are ready to start work but are in need of someone to do it priority/backlog Issue is approved and in the backlog

Comments

@raffaelespazzoli
Copy link

currently the oidc discovery provider component assumes it can use let's encrypt to create it;s own certs. In general this is a strong assumption that will impede adoption as not all scenarios allow for that. This RFE is about enbaling the use case of bring your own pre-existing certs.

@amartinezfayo amartinezfayo added the triage/in-progress Issue triage is in progress label Feb 7, 2023
@evan2645
Copy link
Member

Hello @raffaelespazzoli, thanks for opening this and sorry for the delay. The project has previously avoided allowing these kinds of configurations because it felt antithetical to the goals of SPIFFE/SPIRE, which includes treating rotation as a first class citizen for all keys in the system. With this in mind, an expiring cert/key in a SPIRE deployment is likely to catch folks by surprise, and is not the experience that we want our users to have.

That said, we also recognize the pain that this stance has caused. We've revisited the topic in light of this issue, and came to the conclusion that this should be supported, however there should be adequate warnings in place. I had a look at the current configurables, and am proposing the following:

  • New configuration section static_serving_cert
    • To be added as a one-of option alongside acme and listen_socket_path
  • cert_file_path and key_file_path configurables inside static_serving_cert section, both mandatory
  • When static_serving_cert is set, a warning is emitted informing the user that SPIRE is unable to rotate the configured static key, and the day/time that the certificate will expire

The configuration shape of this component has grown organically over the years and is not very ergonomic at all. Unfortunately, my proposed changes above only serve to make it worse. I can't think of any way to improve it that doesn't involve configurable deprecation, which I feel is out-of-scope for this issue.

@evan2645 evan2645 added help wanted Issues with this label are ready to start work but are in need of someone to do it priority/backlog Issue is approved and in the backlog and removed triage/in-progress Issue triage is in progress labels Feb 16, 2023
@evan2645 evan2645 removed their assignment Feb 16, 2023
@raffaelespazzoli
Copy link
Author

I don't like the name of the parameter: static_serving_cert , it assumes that external systems cannot rotate certs.
Instead the code should be monitoring those paths for possible changes.
It's ironic, in my setup I ended up using let's encrypt, but I need to be in control of the process. So if the oidc-discovery-provider was able to read externally supplied certs, it would see those certs being rotated.

@evan2645
Copy link
Member

I see - if we want to add logic to detect updates to these files and apply them to the TLS stack without restarting, that sounds like a good idea and in that case the static prefix makes less sense. How about serving_cert_file as the section name? I think we still need to log a warning though .. you may know that there exists rotation mechanisms for this file, but SPIRE doesn't and certainly not all users will do that

@raffaelespazzoli
Copy link
Author

I like it!

@evan2645
Copy link
Member

👍 Ok great .. I labeled this issue as help wanted, in case you or someone else in the community is in a good position to make the contribution 😊

@guilhermocc
Copy link
Contributor

Hi @evan2645, I'm willing to contribute to this issue by opening a PR with the changes. Could you assign it to me, please?

@amartinezfayo
Copy link
Member

Just assigned it to you @guilhermocc. Thank you for taking it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues with this label are ready to start work but are in need of someone to do it priority/backlog Issue is approved and in the backlog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants