You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/07/31 22:40:55 UTC

[GitHub] [trafficcontrol] hbeatty opened a new pull request #4933: Update TO for minimum TLS version

hbeatty opened a new pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933


   ## What does this PR (Pull Request) do?
   
   This update allows an administrator to set minimum TLS version for Traffic Ops.
   
   - [x] This is not related to any Issue <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
   
   ## Which Traffic Control components are affected by this PR?
   
   - Documentation
   - Traffic Ops
   
   ## What is the best way to verify this PR?
   
   - Install TO
   - Run `docker run -it drwetter/testssl.sh:latest -p <fqdn_of_traffic_ops>` and note the TLS versions supported
     ```
      Testing protocols via sockets except NPN+ALPN 
   
      SSLv2      not offered (OK)
      SSLv3      not offered (OK)
      TLS 1      offered (deprecated)
      TLS 1.1    offered (deprecated)
      TLS 1.2    offered (OK)
      TLS 1.3    not offered and downgraded to a weaker protocol
      NPN/SPDY   h2, http/1.1 (advertised)
      ALPN/HTTP2 h2, http/1.1 (offered)
     ```
   - Set `"min_tls_version": 770` in the `traffic_ops_golang` section of cdn.conf
   - restart TO
   - Run `docker run -it drwetter/testssl.sh:latest -p <fqdn_of_traffic_ops>` and note that TLS version 1 is now off
     ```
      Testing protocols via sockets except NPN+ALPN 
   
      SSLv2      not offered (OK)
      SSLv3      not offered (OK)
      TLS 1      not offered
      TLS 1.1    offered (deprecated)
      TLS 1.2    offered (OK)
      TLS 1.3    not offered and downgraded to a weaker protocol
      NPN/SPDY   h2, http/1.1 (advertised)
      ALPN/HTTP2 h2, http/1.1 (offered)
     ```
   ## The following criteria are ALL met by this PR
   <!-- Check the boxes to signify that the associated statement is true. To
   "check a box", replace the space inside of the square brackets with an 'x'.
   e.g.
   
   - [ x] <- Wrong
   - [x ] <- Wrong
   - [] <- Wrong
   - [*] <- Wrong
   - [x] <- Correct!
   
   -->
   
   - [x] This PR includes tests OR I have explained why tests are unnecessary
   - [x] This PR includes documentation OR I have explained why documentation is unnecessary
   - [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
   - [x] This PR includes any and all required license headers
   - [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.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.

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



[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r473258003



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -443,6 +443,20 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 		:disabled_routes: A list of API route IDs to disable. Requests matching these routes will receive a 503 response. To find the route ID for a given path you would like to disable, run ``./traffic_ops_golang`` using the :option:`--api-routes` option to view all the route information, including route IDs and paths.
 		:ignore_unknown_routes: If ``false`` (default) return an error and prevent startup if unknown route IDs are found. Otherwise, log a warning and continue startup.
 
+	:min_tls_version: An optional field to set the minimum TLS version. Integer value between 769 to 772.
+
+			+---------+------------------+
+			| Setting | Value            |
+			+=========+==================+
+			| 769     | TLS v1 (Default) |
+			+---------+------------------+
+			| 770     | TLS v1.1         |
+			+---------+------------------+
+			| 771     | TLS v1.2         |
+			+---------+------------------+
+			| 772     | TLS v1.3         |
+			+---------+------------------+

Review comment:
       pointing to the `net/tls.Config` GoDoc is actually a better solution; this comment is no longer relevant.




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r473347549



##########
File path: traffic_ops/traffic_ops_golang/config/config.go
##########
@@ -99,8 +100,9 @@ type ConfigTrafficOpsGolang struct {
 	RiakPort                 *uint                      `json:"riak_port"`
 	WhitelistedOAuthUrls     []string                   `json:"whitelisted_oauth_urls"`
 	OAuthClientSecret        string                     `json:"oauth_client_secret"`
-	RoutingBlacklist         `json:"routing_blacklist"`
-	SupportedDSMetrics       []string `json:"supported_ds_metrics"`
+	RoutingBlacklist                                    `json:"routing_blacklist"`
+	SupportedDSMetrics       []string                   `json:"supported_ds_metrics"`
+	TLSConfig                *tls.Config                `json:"tls_config"`

Review comment:
       > I don't know how to do that.
   
   To `go fmt` a file you just do `go fmt {{file}}`.
   
   > Is that going to keep this PR from being accepted?
   
   Well, yeah, it's listed in [the docs for Traffic Ops Go development requirements](https://traffic-control-cdn.readthedocs.io/en/latest/development/traffic_ops.html#go-implementation-requirements), but it's also an extremely simple fix.
   
   > ... I was following the format that was already there.
   
   Well, you lined up the struct tags. But you can see that [you actually changed the format](https://github.com/apache/trafficcontrol/pull/4933/files#diff-8f0e586303d36096ee3c6ae7a0a7bffdL102-R105):
   ```diff
   -  RoutingBlacklist         `json:"routing_blacklist"`
   +  RoutingBlacklist                                    `json:"routing_blacklist"`
   ```




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] hbeatty commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
hbeatty commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r473298804



##########
File path: traffic_ops/traffic_ops_golang/config/config.go
##########
@@ -99,8 +100,9 @@ type ConfigTrafficOpsGolang struct {
 	RiakPort                 *uint                      `json:"riak_port"`
 	WhitelistedOAuthUrls     []string                   `json:"whitelisted_oauth_urls"`
 	OAuthClientSecret        string                     `json:"oauth_client_secret"`
-	RoutingBlacklist         `json:"routing_blacklist"`
-	SupportedDSMetrics       []string `json:"supported_ds_metrics"`
+	RoutingBlacklist                                    `json:"routing_blacklist"`
+	SupportedDSMetrics       []string                   `json:"supported_ds_metrics"`
+	TLSConfig                *tls.Config                `json:"tls_config"`

Review comment:
       Uh, I don't know how to do that. Is that going to keep this PR from being accepted?




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r466509800



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -443,6 +443,20 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 		:disabled_routes: A list of API route IDs to disable. Requests matching these routes will receive a 503 response. To find the route ID for a given path you would like to disable, run ``./traffic_ops_golang`` using the :option:`--api-routes` option to view all the route information, including route IDs and paths.
 		:ignore_unknown_routes: If ``false`` (default) return an error and prevent startup if unknown route IDs are found. Otherwise, log a warning and continue startup.
 
+	:min_tls_version: An optional field to set the minimum TLS version. Integer value between 769 to 772.
+
+			+---------+------------------+
+			| Setting | Value            |
+			+=========+==================+
+			| 769     | TLS v1 (Default) |
+			+---------+------------------+
+			| 770     | TLS v1.1         |
+			+---------+------------------+
+			| 771     | TLS v1.2         |
+			+---------+------------------+
+			| 772     | TLS v1.3         |
+			+---------+------------------+

Review comment:
       @hbeatty ^^




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] hbeatty commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
hbeatty commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r473298804



##########
File path: traffic_ops/traffic_ops_golang/config/config.go
##########
@@ -99,8 +100,9 @@ type ConfigTrafficOpsGolang struct {
 	RiakPort                 *uint                      `json:"riak_port"`
 	WhitelistedOAuthUrls     []string                   `json:"whitelisted_oauth_urls"`
 	OAuthClientSecret        string                     `json:"oauth_client_secret"`
-	RoutingBlacklist         `json:"routing_blacklist"`
-	SupportedDSMetrics       []string `json:"supported_ds_metrics"`
+	RoutingBlacklist                                    `json:"routing_blacklist"`
+	SupportedDSMetrics       []string                   `json:"supported_ds_metrics"`
+	TLSConfig                *tls.Config                `json:"tls_config"`

Review comment:
       Uh, I don't know how to do that. Is that going to keep this PR from being accepted?
   
   And I was following the format that was already there. Is this an OSS project? I'm confused.




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r466521317



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -443,6 +443,20 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 		:disabled_routes: A list of API route IDs to disable. Requests matching these routes will receive a 503 response. To find the route ID for a given path you would like to disable, run ``./traffic_ops_golang`` using the :option:`--api-routes` option to view all the route information, including route IDs and paths.
 		:ignore_unknown_routes: If ``false`` (default) return an error and prevent startup if unknown route IDs are found. Otherwise, log a warning and continue startup.
 
+	:min_tls_version: An optional field to set the minimum TLS version. Integer value between 769 to 772.
+
+			+---------+------------------+
+			| Setting | Value            |
+			+=========+==================+
+			| 769     | TLS v1 (Default) |
+			+---------+------------------+
+			| 770     | TLS v1.1         |
+			+---------+------------------+
+			| 771     | TLS v1.2         |
+			+---------+------------------+
+			| 772     | TLS v1.3         |

Review comment:
       We should be accepting TLS version strings like `"1.3"`, `"1.2"`, etc. and translate that string to a valid `int` ourselves when reading the config. This is for `cdn.conf`, but for `riak.conf`, we already agreed to use version strings like that. See @rawlinp's [comment](https://github.com/apache/trafficcontrol/pull/4573#discussion_r400346222) in #4573:
   
   > 769, 770, etc are not very intuitive -- would it be better to use a string or float that we translate into `tls.VersionTLS11` from the `tls` package in the stdlib?




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] hbeatty commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
hbeatty commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r473298281



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -443,6 +447,8 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 		:disabled_routes: A list of API route IDs to disable. Requests matching these routes will receive a 503 response. To find the route ID for a given path you would like to disable, run ``./traffic_ops_golang`` using the :option:`--api-routes` option to view all the route information, including route IDs and paths.
 		:ignore_unknown_routes: If ``false`` (default) return an error and prevent startup if unknown route IDs are found. Otherwise, log a warning and continue startup.
 
+	:tls_config: An optional stanza for TLS configuration. The values of which can be found here: https://golang.org/pkg/crypto/tls/#Config

Review comment:
       :+1: 




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] hbeatty commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
hbeatty commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r473297208



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -399,6 +399,10 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 	:db_query_timeout_seconds: An optional field specifying a timeout on database *transactions* (not actually single queries in most cases) within API route handlers. Effectively this is a timeout on a single handler's ability to interact with the Traffic Ops Database. Default if not specified is the value of `DefaultDBQueryTimeoutSecs <https://godoc.org/github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/config#pkg-constants>`_.
 	:idle_timeout: An optional timeout in seconds for idle client connections to Traffic Ops. If set to zero, the value of ``read_timeout`` will be used instead. If both are zero, then the value of ``read_header_timeout`` will be used. If all three fields are zero, there is no timeout and connections will be kept alive indefinitely - **not** recommended. Default if not specified is zero.
 	:insecure: An optional boolean which, if set to ``true`` will cause Traffic Ops to skip verification of client certificates whenever necessary/possible. If set to ``false``, the normal verification behavior is exhibited. Default if not specified is ``false``.
+
+		.. deprecated:: 4.2

Review comment:
       :+1: 




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] rob05c merged pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
rob05c merged pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933


   


----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] hbeatty commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
hbeatty commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r468076898



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -443,6 +443,20 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 		:disabled_routes: A list of API route IDs to disable. Requests matching these routes will receive a 503 response. To find the route ID for a given path you would like to disable, run ``./traffic_ops_golang`` using the :option:`--api-routes` option to view all the route information, including route IDs and paths.
 		:ignore_unknown_routes: If ``false`` (default) return an error and prevent startup if unknown route IDs are found. Otherwise, log a warning and continue startup.
 
+	:min_tls_version: An optional field to set the minimum TLS version. Integer value between 769 to 772.
+
+			+---------+------------------+
+			| Setting | Value            |
+			+=========+==================+
+			| 769     | TLS v1 (Default) |
+			+---------+------------------+
+			| 770     | TLS v1.1         |
+			+---------+------------------+
+			| 771     | TLS v1.2         |
+			+---------+------------------+
+			| 772     | TLS v1.3         |
+			+---------+------------------+

Review comment:
       OK. I'll figure out how to do that.




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r473253507



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -399,6 +399,10 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 	:db_query_timeout_seconds: An optional field specifying a timeout on database *transactions* (not actually single queries in most cases) within API route handlers. Effectively this is a timeout on a single handler's ability to interact with the Traffic Ops Database. Default if not specified is the value of `DefaultDBQueryTimeoutSecs <https://godoc.org/github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/config#pkg-constants>`_.
 	:idle_timeout: An optional timeout in seconds for idle client connections to Traffic Ops. If set to zero, the value of ``read_timeout`` will be used instead. If both are zero, then the value of ``read_header_timeout`` will be used. If all three fields are zero, there is no timeout and connections will be kept alive indefinitely - **not** recommended. Default if not specified is zero.
 	:insecure: An optional boolean which, if set to ``true`` will cause Traffic Ops to skip verification of client certificates whenever necessary/possible. If set to ``false``, the normal verification behavior is exhibited. Default if not specified is ``false``.
+
+		.. deprecated:: 4.2

Review comment:
       This should be 5.0 - "4.2" was created as a part of our automatic version incrementing system, but it will never actually exist. The next planned release is 5.0.

##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -443,6 +447,8 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 		:disabled_routes: A list of API route IDs to disable. Requests matching these routes will receive a 503 response. To find the route ID for a given path you would like to disable, run ``./traffic_ops_golang`` using the :option:`--api-routes` option to view all the route information, including route IDs and paths.
 		:ignore_unknown_routes: If ``false`` (default) return an error and prevent startup if unknown route IDs are found. Otherwise, log a warning and continue startup.
 
+	:tls_config: An optional stanza for TLS configuration. The values of which can be found here: https://golang.org/pkg/crypto/tls/#Config

Review comment:
       To link to a GoDoc, use our custom `:godoc:` role. In this case, it'd be:
   ```rst
   The values of which conform to the :godoc:`crypto/tls.Config` structure.
   ```
   That creates an automatic link.

##########
File path: traffic_ops/traffic_ops_golang/config/config.go
##########
@@ -99,8 +100,9 @@ type ConfigTrafficOpsGolang struct {
 	RiakPort                 *uint                      `json:"riak_port"`
 	WhitelistedOAuthUrls     []string                   `json:"whitelisted_oauth_urls"`
 	OAuthClientSecret        string                     `json:"oauth_client_secret"`
-	RoutingBlacklist         `json:"routing_blacklist"`
-	SupportedDSMetrics       []string `json:"supported_ds_metrics"`
+	RoutingBlacklist                                    `json:"routing_blacklist"`
+	SupportedDSMetrics       []string                   `json:"supported_ds_metrics"`
+	TLSConfig                *tls.Config                `json:"tls_config"`

Review comment:
       This file needs to be `go fmt`-ed




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r464023059



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -443,6 +443,20 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 		:disabled_routes: A list of API route IDs to disable. Requests matching these routes will receive a 503 response. To find the route ID for a given path you would like to disable, run ``./traffic_ops_golang`` using the :option:`--api-routes` option to view all the route information, including route IDs and paths.
 		:ignore_unknown_routes: If ``false`` (default) return an error and prevent startup if unknown route IDs are found. Otherwise, log a warning and continue startup.
 
+	:min_tls_version: An optional field to set the minimum TLS version. Integer value between 769 to 772.
+
+			+---------+------------------+
+			| Setting | Value            |
+			+=========+==================+
+			| 769     | TLS v1 (Default) |
+			+---------+------------------+
+			| 770     | TLS v1.1         |
+			+---------+------------------+
+			| 771     | TLS v1.2         |
+			+---------+------------------+
+			| 772     | TLS v1.3         |
+			+---------+------------------+

Review comment:
       Tables should be wrapped in a `.. table` directive
   




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r466532935



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -443,6 +443,20 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 		:disabled_routes: A list of API route IDs to disable. Requests matching these routes will receive a 503 response. To find the route ID for a given path you would like to disable, run ``./traffic_ops_golang`` using the :option:`--api-routes` option to view all the route information, including route IDs and paths.
 		:ignore_unknown_routes: If ``false`` (default) return an error and prevent startup if unknown route IDs are found. Otherwise, log a warning and continue startup.
 
+	:min_tls_version: An optional field to set the minimum TLS version. Integer value between 769 to 772.
+
+			+---------+------------------+
+			| Setting | Value            |
+			+=========+==================+
+			| 769     | TLS v1 (Default) |
+			+---------+------------------+
+			| 770     | TLS v1.1         |
+			+---------+------------------+
+			| 771     | TLS v1.2         |
+			+---------+------------------+
+			| 772     | TLS v1.3         |

Review comment:
       Rather than adding `MinTLSVersion`, I think we should put the entire `tls.Config` in the Config. We should do this for all Go apps. There are many config settings in `tls.Config`, such as disabling H2, allows Ciphers, and more. 
   If we keep adding individual options, every time we need something else, it's a code change and a new version.
   
   Whereas if we put `tls.Config` in the app config, it's just a config change for users to add whatever they need with respect to TLS.




----------------------------------------------------------------
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.

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



[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4933: Update TO for minimum TLS version

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4933:
URL: https://github.com/apache/trafficcontrol/pull/4933#discussion_r468121523



##########
File path: docs/source/admin/traffic_ops.rst
##########
@@ -443,6 +443,20 @@ This file deals with the configuration parameters of running Traffic Ops itself.
 		:disabled_routes: A list of API route IDs to disable. Requests matching these routes will receive a 503 response. To find the route ID for a given path you would like to disable, run ``./traffic_ops_golang`` using the :option:`--api-routes` option to view all the route information, including route IDs and paths.
 		:ignore_unknown_routes: If ``false`` (default) return an error and prevent startup if unknown route IDs are found. Otherwise, log a warning and continue startup.
 
+	:min_tls_version: An optional field to set the minimum TLS version. Integer value between 769 to 772.
+
+			+---------+------------------+
+			| Setting | Value            |
+			+=========+==================+
+			| 769     | TLS v1 (Default) |
+			+---------+------------------+
+			| 770     | TLS v1.1         |
+			+---------+------------------+
+			| 771     | TLS v1.2         |
+			+---------+------------------+
+			| 772     | TLS v1.3         |

Review comment:
       `tls.Config` is well-documented in [the tls GoDoc](https://godoc.org/crypto/tls#Config), so I'm fine with sticking to that for now with the caveat that I wouldn't oppose adding `MinTLSVersion` in a future PR.




----------------------------------------------------------------
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.

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