You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by "dsouza93 (via GitHub)" <gi...@apache.org> on 2023/09/06 17:05:17 UTC

[GitHub] [trafficcontrol] dsouza93 opened a new pull request, #7781: Adding Client Auth Configs to Infra Ansible

dsouza93 opened a new pull request, #7781:
URL: https://github.com/apache/trafficcontrol/pull/7781

   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?
   <!-- Please delete all components from this list that are NOT affected by this PR.
   Feel free to add the name of a tool or script that is affected but not on the list.
   -->
   - Automation - Lab Ansible Infrastructure
   
   ## What is the best way to verify this PR?
   Build an environment setting the following two variables
   - to_client_cert_root_directory = path to root cert file
   - to_go_client_auth_enabled = 1
   
   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
   - [ ] This PR has tests - This PR is an improvement to automated infra that is involved in testing.
   - [] This PR has documentation - I'm enabling already implemented and documented features.
   - [] This PR has a CHANGELOG.md entry 
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://apache.org/security) for details)
   
   <!--
   Licensed to the Apache Software Foundation (ASF) under one
   or more contributor license agreements.  See the NOTICE file
   distributed with this work for additional information
   regarding copyright ownership.  The ASF licenses this file
   to you under the Apache License, Version 2.0 (the
   "License"); you may not use this file except in compliance
   with the License.  You may obtain a copy of the License at
   
       http://www.apache.org/licenses/LICENSE-2.0
   
   Unless required by applicable law or agreed to in writing,
   software distributed under the License is distributed on an
   "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
   KIND, either express or implied.  See the License for the
   specific language governing permissions and limitations
   under the License.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] dsouza93 commented on a diff in pull request #7781: Adding Client Auth Configs to Infra Ansible

Posted by "dsouza93 (via GitHub)" <gi...@apache.org>.
dsouza93 commented on code in PR #7781:
URL: https://github.com/apache/trafficcontrol/pull/7781#discussion_r1317607639


##########
infrastructure/ansible/roles/traffic_ops/templates/cdn.conf.j2:
##########
@@ -68,6 +68,9 @@
       "plugins": {{ to_plugin_config.keys() | list | to_json }},
       "plugin_config" : {{ to_plugin_config | to_nice_json(indent=2) }},
       "traffic_vault_backend": "{{ to_traffic_vault_backend }}",
+      "tls_config": {
+         "ClientAuth": "{{ to_go_client_auth_enabled }}"
+      },

Review Comment:
   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?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] zrhoffman merged pull request #7781: Adding Client Auth Configs to Infra Ansible

Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman merged PR #7781:
URL: https://github.com/apache/trafficcontrol/pull/7781


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7781: Adding Client Auth Configs to Infra Ansible

Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7781:
URL: https://github.com/apache/trafficcontrol/pull/7781#discussion_r1317597246


##########
infrastructure/ansible/roles/traffic_ops/templates/cdn.conf.j2:
##########
@@ -92,5 +95,8 @@
       "convert_self_signed": {{ to_le_convert_self_signed | bool | lower }},
       "renew_days_before_expiration": {{ to_le_renew_days_before_expiration | int }},
       "environment": "{{ to_le_environment }}"
+   },
+   "client_certificate_authentication" : {
+      "root_certificates_directory" : "{{ to_client_cert_root_directory }}"

Review Comment:
   What creates `to_client_cert_root_directory`? How do we know it exists?



##########
infrastructure/ansible/roles/traffic_ops/defaults/main.yml:
##########
@@ -98,10 +98,13 @@ to_go_supported_ds_metrics:
   - tps_4xx
   - tps_5xx
 
+to_go_client_auth_enabled: 0
 to_go_whitelisted_oauth_urls: ""
 to_go_oauth_user_attribute: ""
 to_go_oauth_client_secret: ""
 
+to_client_cert_root_directory: ""

Review Comment:
   How about a more sane default for `to_client_cert_root_directory` like `"/etc/pki/tls/traffic_ops/"`?



##########
infrastructure/ansible/roles/traffic_ops/templates/cdn.conf.j2:
##########
@@ -68,6 +68,9 @@
       "plugins": {{ to_plugin_config.keys() | list | to_json }},
       "plugin_config" : {{ to_plugin_config | to_nice_json(indent=2) }},
       "traffic_vault_backend": "{{ to_traffic_vault_backend }}",
+      "tls_config": {
+         "ClientAuth": "{{ to_go_client_auth_enabled }}"
+      },

Review Comment:
   Also needs
   
   ```json
           "MinVersion": 769,
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] dsouza93 commented on a diff in pull request #7781: Adding Client Auth Configs to Infra Ansible

Posted by "dsouza93 (via GitHub)" <gi...@apache.org>.
dsouza93 commented on code in PR #7781:
URL: https://github.com/apache/trafficcontrol/pull/7781#discussion_r1317604470


##########
infrastructure/ansible/roles/traffic_ops/templates/cdn.conf.j2:
##########
@@ -92,5 +95,8 @@
       "convert_self_signed": {{ to_le_convert_self_signed | bool | lower }},
       "renew_days_before_expiration": {{ to_le_renew_days_before_expiration | int }},
       "environment": "{{ to_le_environment }}"
+   },
+   "client_certificate_authentication" : {
+      "root_certificates_directory" : "{{ to_client_cert_root_directory }}"

Review Comment:
   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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] ragen14 commented on pull request #7781: Adding Client Auth Configs to Infra Ansible

Posted by "ragen14 (via GitHub)" <gi...@apache.org>.
ragen14 commented on PR #7781:
URL: https://github.com/apache/trafficcontrol/pull/7781#issuecomment-1708784786

   LGTM


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] dsouza93 commented on a diff in pull request #7781: Adding Client Auth Configs to Infra Ansible

Posted by "dsouza93 (via GitHub)" <gi...@apache.org>.
dsouza93 commented on code in PR #7781:
URL: https://github.com/apache/trafficcontrol/pull/7781#discussion_r1317608370


##########
infrastructure/ansible/roles/traffic_ops/templates/cdn.conf.j2:
##########
@@ -92,5 +95,8 @@
       "convert_self_signed": {{ to_le_convert_self_signed | bool | lower }},
       "renew_days_before_expiration": {{ to_le_renew_days_before_expiration | int }},
       "environment": "{{ to_le_environment }}"
+   },
+   "client_certificate_authentication" : {
+      "root_certificates_directory" : "{{ to_client_cert_root_directory }}"

Review Comment:
   I added a conditional block to leave this out if the client-cert authentication is disabled.



##########
infrastructure/ansible/roles/traffic_ops/defaults/main.yml:
##########
@@ -98,10 +98,13 @@ to_go_supported_ds_metrics:
   - tps_4xx
   - tps_5xx
 
+to_go_client_auth_enabled: 0
 to_go_whitelisted_oauth_urls: ""
 to_go_oauth_user_attribute: ""
 to_go_oauth_client_secret: ""
 
+to_client_cert_root_directory: ""

Review Comment:
   done



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [trafficcontrol] zrhoffman commented on a diff in pull request #7781: Adding Client Auth Configs to Infra Ansible

Posted by "zrhoffman (via GitHub)" <gi...@apache.org>.
zrhoffman commented on code in PR #7781:
URL: https://github.com/apache/trafficcontrol/pull/7781#discussion_r1317615793


##########
infrastructure/ansible/roles/traffic_ops/templates/cdn.conf.j2:
##########
@@ -68,6 +68,9 @@
       "plugins": {{ to_plugin_config.keys() | list | to_json }},
       "plugin_config" : {{ to_plugin_config | to_nice_json(indent=2) }},
       "traffic_vault_backend": "{{ to_traffic_vault_backend }}",
+      "tls_config": {
+         "ClientAuth": "{{ to_go_client_auth_enabled }}"
+      },

Review Comment:
   > 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
   
   https://github.com/apache/trafficcontrol/blob/708a95cd87334ca6da6dc75c3849e00b995be81a/traffic_ops/traffic_ops_golang/traffic_ops_golang.go#L191-L193



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org