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

feat: backchannel logout request client tls configuration #2875

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aarmam
Copy link
Contributor

@aarmam aarmam commented Nov 29, 2021

This pull request introduces feature to configure backchannel logout request client TLS min/max versions and supported cipher suites.

Feature update:

  • Added insecure_skip_verify configuration option.
  • Added reading proxy configuration from environment variables HTTP_PROXY and HTTPS_PROXY

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

@aarmam aarmam requested a review from aeneasr as a code owner November 29, 2021 07:27
@codecov
Copy link

codecov bot commented Nov 29, 2021

Codecov Report

Merging #2875 (0c8b78a) into master (1b1899e) will increase coverage by 0.07%.
The diff coverage is 95.00%.

❗ Current head 0c8b78a differs from pull request most recent head 87e14c7. Consider uploading reports for the commit 87e14c7 to get more accurate results

@@            Coverage Diff             @@
##           master    #2875      +/-   ##
==========================================
+ Coverage   76.85%   76.92%   +0.07%     
==========================================
  Files         124      124              
  Lines        9175     9212      +37     
==========================================
+ Hits         7051     7086      +35     
- Misses       1674     1675       +1     
- Partials      450      451       +1     
Impacted Files Coverage Δ
consent/strategy_default.go 69.72% <85.71%> (+0.21%) ⬆️
driver/config/tls.go 91.80% <100.00%> (+6.08%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch 2 times, most recently from dbd2976 to 30e5db8 Compare November 29, 2021 09:00
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch from 30e5db8 to b80598c Compare December 8, 2021 09:44
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Looking great! :) Could you please add a guide that explains how to use this feature in this doc: https://github.com/ory/hydra/blob/master/docs/docs/guides/logout.mdx ?

Thank you!

spec/config.json Outdated Show resolved Hide resolved
consent/strategy_default.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Dec 22, 2021

Thank you, this looks great! The CI is failing because some files are formatted incorrectly. To format them, run:

$ make format
$ git commit -a -m "styles: format code"
$ git push

Thank you!

@aeneasr
Copy link
Member

aeneasr commented Jan 11, 2022

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

@aeneasr aeneasr marked this pull request as draft January 11, 2022 13:22
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch 2 times, most recently from 99c099f to 8d3e42f Compare March 21, 2022 14:53
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch from 8d3e42f to 29612a5 Compare March 23, 2022 09:29
@aarmam aarmam marked this pull request as ready for review March 23, 2022 14:46
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch from 7a1e8ac to 34039db Compare March 25, 2022 12:01
@aarmam aarmam marked this pull request as draft April 12, 2022 18:15
@aarmam aarmam marked this pull request as ready for review April 14, 2022 08:18
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you! I've got one question. This PR is currently targeting exactly one backchannel logout URL. However, most environments will have 1..n backchannel logout URLs, depending on the clients that define those URLs. To me it appears that setting this TLS configuration at a global level will be problematic if you have two or more OAuth2 clients with separate TLS configs (e.g. one special TLS config and one regular HTTPs with a public certificate).

What's your take on this?

spec/config.json Outdated Show resolved Hide resolved
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch from 34039db to f3182f8 Compare April 19, 2022 07:57
@aarmam
Copy link
Contributor Author

aarmam commented Apr 19, 2022

This PR is currently targeting exactly one backchannel logout URL. However, most environments will have 1..n backchannel logout URLs, depending on the clients that define those URLs.

This is how its implemented before this PR also - global golang default for all backchannel requests?

To me it appears that setting this TLS configuration at a global level will be problematic if you have two or more OAuth2 clients with separate TLS configs (e.g. one special TLS config and one regular HTTPs with a public certificate).

And I did not add this problem with current implementation?

Just reiterating my thoughts so I'm not missing anything: :)

This is functionality is MVP for us, but it could be further extended to check client specific settings when iterating over clients in executeBackChannelLogout() and it would not go in conflict with current implementation - in contrary would give possibility to further override these global settings when needed?

@aeneasr aeneasr requested review from aeneasr April 28, 2022 21:52
@aarmam aarmam marked this pull request as draft May 9, 2022 12:31
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch from 249d2c0 to bd96bd4 Compare May 10, 2022 08:23
@aarmam aarmam marked this pull request as ready for review May 10, 2022 09:39
@aarmam aarmam marked this pull request as draft September 6, 2022 10:47
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch 4 times, most recently from 7c266d7 to 2b96742 Compare September 7, 2022 08:02
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch 3 times, most recently from f4bb8e8 to 4914aa4 Compare September 14, 2022 07:55
@aarmam aarmam marked this pull request as ready for review September 14, 2022 13:52
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch 4 times, most recently from f60fcad to 4f7cf12 Compare November 4, 2022 11:02
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you @aarmam as always for your great contributions! After thinking about this for quite some time, I don't think that we should merge this feature upstream. The reason is the following:

The backchannel logout URL is something that is configured on a per-client basis. Adding a config option which allows us only to configure a global TLS certificate does not really match the underlying principle that an authorization server can have almost unlimited backchannel logout targets.

If we were to introduce this, we would need to keep for backwards compatibility and also add to Ory Network. Therefore, we can't merge the feature as is.

One option is to add this feature to the client configuration. However, I think that the backchannel logout URL should use TLS with a trusted certificate, and not trust untrusted certificates. To work around this on local environments, one could use Caddy or something similar to add a trusted CA to the OS certificate store, or set up LE in staging environments.

So generally speaking, I don't think we should add TLS exceptions here. It will only open questions like: Why does JWKs fetching not have a custom TLS certificate? Or webhooks, or feature X, Y, Z

@aarmam
Copy link
Contributor Author

aarmam commented Dec 21, 2022

Thank you @aarmam as always for your great contributions!

Thanks! :)

Adding a config option which allows us only to configure a global TLS certificate does not really match the underlying principle that an authorization server can have almost unlimited backchannel logout targets.

This is actually not what this pr introduces. I'll give you an example:

client.default.tls:
  min_version: tls12
  max_version: tls13
  cipher_suites:
    - TLS_AES_128_GCM_SHA256
client.back_channel_logout.tls:
  cipher_suites:
    - TLS_AES_128_GCM_SHA256
    - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
    - TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256

This configuration limits all outgoing TLS connections to use min/max tls versions and specified cipher suites - thats all this pr introduces. It does not configure global TLS certificate nor trusted certificates - just global outgoing connection settings for min/max tls version and cypher suites.
In this example client.back_channel_logout.tls.cipher_suites overrides client.default.tls.cipher_suites.

@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch 2 times, most recently from 14acfc3 to 8443a87 Compare December 21, 2022 16:39
@aarmam aarmam marked this pull request as draft December 21, 2022 16:58
@aarmam aarmam marked this pull request as ready for review December 21, 2022 16:58
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch from 8443a87 to 9b42cc6 Compare April 10, 2023 11:05
@aarmam aarmam force-pushed the feature/backchannel-tls-clientconf branch from 9b42cc6 to 87e14c7 Compare April 17, 2023 13:36
alarkvell pushed a commit to e-gov/GovSSO-Session that referenced this pull request Nov 28, 2023
alarkvell pushed a commit to e-gov/GovSSO-Session that referenced this pull request Nov 29, 2023
alarkvell pushed a commit to e-gov/GovSSO-Session that referenced this pull request Nov 29, 2023
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