hydra
                                
                                
                                
                                    hydra copied to clipboard
                            
                            
                            
                        feat: backchannel logout request client tls configuration
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.
 
Codecov Report
Merging #2875 (0c8b78a) into master (1b1899e) will increase coverage by
0.07%. The diff coverage is95.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.
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!
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!
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: :)
- If 
client.default.tls/client.back_channel_logout.tlsare not set, then golang defaults are used like previously (?) - If 
client.default.tlsis set, then the settings are merged with golang default settings and would be used for all Hydra clients, that decide to use this functionality. - If only 
client.back_channel_logout.tlsis specified, then only backchannel client will have custom tls configuration merged with golang default settings. - If both 
client.default.tls/client.back_channel_logout.tlsare set, then backchannel client will have default and fallback settings merged with golang default settings. 
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?
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.