hydra icon indicating copy to clipboard operation
hydra copied to clipboard

feat: backchannel logout request client tls configuration

Open aarmam opened this issue 3 years ago • 5 comments

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

  • [x] I have read the contributing guidelines.
  • [x] I have referenced an issue containing the design document if my change introduces a new feature.
  • [x] I am following the contributing code guidelines.
  • [x] I have read the security policy.
  • [x] 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.
  • [x] I have added tests that prove my fix is effective or that my feature works.
  • [x] I have added or changed the documentation.

aarmam avatar Nov 29 '21 07:11 aarmam

Codecov Report

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

:exclamation: 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%) :arrow_up:
driver/config/tls.go 91.80% <100.00%> (+6.08%) :arrow_up:

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

codecov[bot] avatar Nov 29 '21 07:11 codecov[bot]

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 avatar Dec 22 '21 15:12 aeneasr

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 avatar Jan 11 '22 13:01 aeneasr

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?

aarmam avatar Apr 19 '22 11:04 aarmam

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 avatar Dec 21 '22 16:12 aarmam