-
Notifications
You must be signed in to change notification settings - Fork 353
Adding Client Auth Configs to Infra Ansible #7781
Conversation
|
LGTM |
| "tls_config": { | ||
| "ClientAuth": "{{ to_go_client_auth_enabled }}" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs
"MinVersion": 769,There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no default in TO that I am leaving this up to?
What does the tls_config get set to without this block as it was before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no default in TO that I am leaving this up to?
Not currently, no
What does the tls_config get set to without this block as it was before?
Whatever the TLS config is, empty or no, is used as the server's TLS config. The defaults are internal Go HTTP server logic
trafficcontrol/traffic_ops/traffic_ops_golang/traffic_ops_golang.go
Lines 191 to 193 in 708a95c
| httpServer := &http.Server{ | |
| Addr: ":" + cfg.Port, | |
| TLSConfig: cfg.TLSConfig, |
| "environment": "{{ to_le_environment }}" | ||
| }, | ||
| "client_certificate_authentication" : { | ||
| "root_certificates_directory" : "{{ to_client_cert_root_directory }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What creates to_client_cert_root_directory? How do we know it exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not exist - for our purposes I handled it in ansible-pull. It already exists on all of our nightly boxes. I will handle this the same way when going to production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a conditional block to leave this out if the client-cert authentication is disabled.
Adding variables to configure client certificate authentication in the ansible infrastructure's cdn.conf file. Defaulted the values to disable client certificate authentication.
d3d010b to
708a95c
Compare
zrhoffman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine. It's untested, but it's also a relatively small change. I trust that you'll open a follow-up PR if this breaks anything.
| "tls_config": { | ||
| "ClientAuth": "{{ to_go_client_auth_enabled }}" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no default in TO that I am leaving this up to?
Not currently, no
What does the tls_config get set to without this block as it was before?
Whatever the TLS config is, empty or no, is used as the server's TLS config. The defaults are internal Go HTTP server logic
trafficcontrol/traffic_ops/traffic_ops_golang/traffic_ops_golang.go
Lines 191 to 193 in 708a95c
| httpServer := &http.Server{ | |
| Addr: ":" + cfg.Port, | |
| TLSConfig: cfg.TLSConfig, |
Adding variables to configure client certificate authentication in the ansible infrastructure's cdn.conf file.
Defaulted the values to disable client certificate authentication.
Which Traffic Control components are affected by this PR?
What is the best way to verify this PR?
Build an environment setting the following two variables
You should see the config values set in cdn.conf.
If this is a bugfix, which Traffic Control versions contained the bug?
Not a bug fix
PR submission checklist