feat: Split TLS config into admin and public interfaces #2476
Conversation
|
Wow thank you! I will have to give this a good look which I will be able to do this week :) |
|
Sorry for taking some time on this - it would be great if you could reduce the PR to changes which affect HTTPS only as it will make it easier for me to understand what you did :) |
|
Sure, no probs. |
svrakitin
force-pushed
the
split-public-admin-tls-config
branch
from
d0dbc5a
to
e47c479
Compare
|
Updated PR to include only changes related to split. |
There was a problem hiding this comment.
Thank you, this looks really good - nice job! Just one question, I don't fully understand what tls.strict is for? Is it because you wanted to add the ability in this PR to use TLS for public but not for admin, and needed a place to define this? If so, maybe we need to make --dangerous-force-http only error if there is no TLS set for public, but not so for admin?
|
Yeah, this is just to be able to turn off strict TLS for admin and force TLS only for public. This flag makes the setup more obvious and straightforward for an operator than termination allow-list. --dangerous-force-http now makes TLS non-strict for both public and admin to keep backward compatible behavior. Effectively this PR deprecates this flag in favor of "*tls.strict" parameters, which can be sourced from the config or env vars. I updated defaults in config spec to have strict TLS only for public. Let me know if admin should force TLS by default as well. |
|
Note to self: check all the quickstarts and articles to see if this works with this change |
…ic-admin-tls-config
…ic-admin-tls-config
|
@aeneasr Done, kept |
|
@svrakitin completely forgot that we should add some docs 🤦♂️ Could you maybe add a bit of guidance on how to use your new features here: https://www.ory.sh/hydra/docs/production Would highly appreciate it! :) |
|
@aeneasr Sure, I will dedicate some time tomorrow for this. |
|
Thank you!!! |
Adds the possibility to specify TLS certificates for admin and public endpoints individually. Also improves compatibility for internal networks (e.g. Kubernetes) by removing the need for having TLS termination on admin endpoints. This can be enabled by setting `serve.admin.tls.enabled` to false. Closes ory#1231 Closes ory#1962
Related issue
Closes #1962
Proposed changes
ServeInterfaceinterface param. This is some kind of visitor pattern.New config keys:
serve.public.tls.allow_termination_fromserve.public.tls.cert.*serve.admin.tls.enabledserve.admin.tls.allow_termination_fromserve.admin.tls.cert.*Flag
--dangerous-force-httpdisables TLS for both interfaces to keep compatibility.Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Comments
Probably it is possible to remove duplication even further, but it would increase the size of this PR and it is already rather huge. It should be easier to refactor further by removing dependency on specific interface (admin or public) down the stack.