Fixes #14153 - Allow configuration of SSL_CLIENT_CERT header name #392
Conversation
|
There were the following issues with the commit message:
If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project. More guidelines are available on the Foreman wiki. This message was auto-generated by Foreman's prprocessor |
mateusz-gozdek-sociomantic
force-pushed
the
develop
branch
from
89a0102
to
2887d94
Compare
mateusz-gozdek-sociomantic
changed the title
| @@ -25,7 +25,8 @@ def log_halt code=nil, exception=nil | |||
|
|
|||
| # read the HTTPS client certificate from the environment and extract its CN | |||
| def https_cert_cn | |||
| certificate_raw = request.env['SSL_CLIENT_CERT'].to_s | |||
| ssl_client_cert_header = Proxy::SETTINGS.ssl_client_cert_header || 'SSL_CLIENT_CERT' | |||
There was a problem hiding this comment.
You can avoid nil checks by defining a default value for the setting, which can be done here: https://github.com/theforeman/smart-proxy/blob/develop/lib/proxy/settings/global.rb#L3.
There was a problem hiding this comment.
Updated as you suggested, thank you!
|
[test] |
mateusz-gozdek-sociomantic
force-pushed
the
develop
branch
from
2887d94
to
2cd3642
Compare
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
mateusz-gozdek-sociomantic
force-pushed
the
develop
branch
from
2cd3642
to
4adf75b
Compare
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
|
mateusz-gozdek-sociomantic
force-pushed
the
develop
branch
from
4adf75b
to
5c619de
Compare
In passenger 5, passenger_set_header nginx option is adding HTTP_ prefix to each header, so we need to be able to set proper header name. gsub(/[\t]+/, "\n") is required, because nginx is adding \t after each line of $ssl_client_cert, which makes certificate invalid with PEM format.
mateusz-gozdek-sociomantic
force-pushed
the
develop
branch
from
5c619de
to
ee81bd3
Compare
|
Updated as we discussed in comments. But I don't really know how to write tests for this. |
|
BTW, this raw certificate fix + tests should be applied to foreman too. |
|
[test] |
|
It may be hard for me to provide unit tests for that one, so if someone could add it, it would be really nice, as they will be required for Foreman as well. |
There was a problem hiding this comment.
I can't speak for @witlessbird but I'd start with having a look at test/sinatra/trusted_hosts_test.rb. Personally I'd try to add the expectation that it would be called with the correct parameters. I generally also like integration tests where I just pass in a certificate and verify it returns the correct result, but not sure where those should be placed.
| @@ -25,11 +25,15 @@ def log_halt code=nil, exception=nil | |||
|
|
|||
| # read the HTTPS client certificate from the environment and extract its CN | |||
| def https_cert_cn | |||
| certificate_raw = request.env['SSL_CLIENT_CERT'].to_s | |||
| log_halt 403, 'could not read client cert from environment' if certificate_raw.empty? | |||
| certificate_raw = request.env[Proxy::SETTINGS.ssl_client_cert_header] | |||
|
Hello, it's been a while. This is a critical portion of our code (every request executes this bit) and also security-related. Good test coverage is a must. I am closing for now, please come back later when you figure it out. Thanks for the contribution so far! |
In passenger 5,
passenger_set_headernginx option is adding HTTP_ prefix to each header, so we need to be able to set proper header name in order to validate client certificate.