*: move config file option require_secure_transport to sysvar by Alkaagr81 · Pull Request #34261 · pingcap/tidb

Alkaagr81 · 2022-04-26T18:00:03Z

What problem does this PR solve?

Issue Number: ref #33769

Problem Summary:

The option require-secure-transport has historically been a config option. But based on requirements from cloud & PM it should instead be a sysvar.

What is changed and how it works?

Remove them from the config list and add them to global sysvars.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility
Previously, if TLS certificates were provided by in configuration they were not validated unless require-secure-transport was set. They are now always validated, and the tidb-server will refuse to start if they are invalid. This helps prevent a misconfiguration where TLS downgrades could occur, and duplicates Fail server startup when opening TLS context fails #26065 (it does not affect users who specify no TLS certs).

This might be a bit confusing during upgrade for users who have previously set require-secure-transport (most users haven't). It needs to be documented in the release notes that it has been replaced with the system variable require_secure_transport.

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

The TiDB configuration file option `require-secure-transport` and command line flag have been replaced by the system variable `require_secure_transport`. This change makes it easier to modify this setting on a cluster wide basis.

ti-chi-bot · 2022-04-26T18:00:06Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

dveeden
morgo

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Alkaagr81 · 2022-04-26T18:01:17Z

Please review @morgo

Alkaagr81 · 2022-04-26T18:06:24Z

server/tidb_serial_test.go

 	ts.runTestLoadDataForListColumnPartition2(t)
 }

+func TestInvalidTLS(t *testing.T) {


to check for the invalid server credential

sre-bot · 2022-04-26T18:10:30Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/a3a9ae711009a69c4fc553fcbddfc7faa912f29d

Alkaagr81 · 2022-04-26T18:12:14Z

util/tls/tls.go

+)
+
+// RequireSecureTransport Process global variables
+var RequireSecureTransport = atomic.NewBool(false)


added util/tls package to remove the import dependency to add the atomic value.

dveeden

So the -require-secure-transport flag gets removed as well? I think that's good. However this doesn't seem to be noted in the release notes line of the description of this PR.

Looks like we need to be more careful when switching this on on a running server as it seems to allow one to set global require_secure_transport=on when TLS is not enabled. This leaves UNIX sockets as the remaining option. But we probably don't check if we actually listen on a UNIX socket. This would lock us out of the server completely.

Also this seems not to work as intended with UNIX sockets:

[dvaneeden@dve-carbon ~]$ mysql -u root -S /tmp/tidb-4000.sock 
ERROR 3159 (HY000): Connections using insecure transport are prohibited while --require_secure_transport=ON.

A possible fix would be to only allow one to set this variable from a secure (TLS or socket) connection.

morgo · 2022-04-28T13:54:14Z

So the -require-secure-transport flag gets removed as well? I think that's good. However this doesn't seem to be noted in the release notes line of the description of this PR.

I'm not sure I understand this point. The release note is:

The TiDB configuration file option `require-secure-transport` has been replaced by the system variable `require_secure_transport`. This change makes it easier to modify this setting on a cluster wide basis.

(Replaced does imply removal).

[dvaneeden@dve-carbon ~]$ mysql -u root -S /tmp/tidb-4000.sock 
ERROR 3159 (HY000): Connections using insecure transport are prohibited while --require_secure_transport=ON.

OK that's a bug we'll need to fix. I think we can do it in this PR.

A possible fix would be to only allow one to set this variable from a secure (TLS or socket) connection.

Agree

dveeden · 2022-05-02T09:10:51Z

So the -require-secure-transport flag gets removed as well? I think that's good. However this doesn't seem to be noted in the release notes line of the description of this PR.

I'm not sure I understand this point. The release note is:
The TiDB configuration file option `require-secure-transport` has been replaced by the system variable `require_secure_transport`. This change makes it easier to modify this setting on a cluster wide basis.
(Replaced does imply removal).

Without this PR the require secure transport feature can be enabled by:

Adding require-secure-transport in the security section of tidb.toml as described on https://docs.pingcap.com/tidb/stable/tidb-configuration-file#require-secure-transport
Starting TiDB Server like ./bin/tidb-server -require-secure-transport

With this PR the only option is:

Setting the require_secure_transport global variable to ON

I think the changelog entry correctly states that the configuration file option gets replaced by the global variable. However it doesn't say anything about the replacement/removal of the flag for the tidb-server binary.

morgo · 2022-05-02T13:58:53Z

I think the changelog entry correctly states that the configuration file option gets replaced by the global variable. However it doesn't say anything about the replacement/removal of the flag for the tidb-server binary.

OK! I understand now. I've changed the release note message to:

The TiDB configuration file option require-secure-transport and command line flag have been replaced by the system variable require_secure_transport.

…ansport

morgo · 2022-05-03T19:00:30Z

@Alkaagr81 please fix conflicts. Thx!

dveeden

Somehow it looks like it is not considering a socket connection as secure when setting the variable.

mysql> \s
--------------
mysql  Ver 8.0.29 for Linux on x86_64 (MySQL Community Server - GPL)

Connection id:		407
Current database:	
Current user:		root@localhost
SSL:			Not in use
Current pager:		stdout
Using outfile:		''
Using delimiter:	;
Server version:		5.7.25-TiDB-v6.1.0-alpha-439-g0db763f3a TiDB Server (Apache License 2.0) Community Edition, MySQL 5.7 compatible
Protocol version:	10
Connection:		Localhost via UNIX socket
Server characterset:	utf8mb4
Db     characterset:	utf8mb4
Client characterset:	utf8mb4
Conn.  characterset:	utf8mb4
UNIX socket:		/tmp/tidb.sock
Binary data as:		Hexadecimal
Uptime:			44 sec

Threads: 0  Questions: 0  Slow queries: 0  Opens: 0  Flush tables: 0  Open tables: 0  Queries per second avg: 0.000
--------------

mysql> set global require_secure_transport=1;
ERROR 1105 (HY000): require_secure_transport can only be set to ON if the connection issuing the change is secure

morgo · 2022-05-17T13:46:37Z

Somehow it looks like it is not considering a socket connection as secure when setting the variable.

I actually thought that was a good behavior, since it requires you to prove connection security is configured correctly. This helps reduce misconfiguration/lockouts. We can revisit in future if required.

morgo · 2022-05-17T13:47:44Z

/merge

ti-chi-bot · 2022-05-17T13:47:47Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 0db763f

dveeden · 2022-05-18T07:24:06Z

Somehow it looks like it is not considering a socket connection as secure when setting the variable.

I actually thought that was a good behavior, since it requires you to prove connection security is configured correctly. This helps reduce misconfiguration/lockouts. We can revisit in future if required.

I see your point, and this would indeed ensure TLS is working before we enable require_secure_transport, which is good. However this means it is a bit inconsistent: sockets are are considered secure when the setting is enabled, but not when setting it. Maybe we should update the message to say that setting the variable requires TLS instead of saying that it requires a secure connection.

Happy to see this merged, this is very useful.

bb7133 · 2022-05-18T11:14:33Z

config/config.go

 	"alter-primary-key":                  {}, // use NONCLUSTERED keyword instead
 	"enable-streaming":                   {},
 	"performance.mem-profile-interval":   {},
+	"require-secure-transport":           {},


It should be 'security.require-secure-transport', see #34771

Alkaagr81 added 2 commits April 26, 2022 09:32

move config file option require_secure_transport to sysvar

a834fb3

Merge

dbd255c

Alkaagr81 requested a review from a team as a code owner April 26, 2022 18:00

ti-chi-bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 26, 2022

Alkaagr81 commented Apr 26, 2022

View reviewed changes

ti-chi-bot removed the do-not-merge/needs-linked-issue label Apr 26, 2022

morgo requested review from dveeden and morgo April 26, 2022 19:26

morgo added the compatibility-breaker Violation of forwards/backwards compatibility in a design-time piece. label Apr 27, 2022

dveeden reviewed Apr 28, 2022

View reviewed changes

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 28, 2022

Changes done to fix the bug for socket connection and merge confilict

0f41996

morgo mentioned this pull request May 2, 2022

Convert several config options to Global sysvars #33769

Open

Alkaagr81 added 2 commits May 2, 2022 09:55

Merge remote-tracking branch 'upstream/master' into require_secure_tr…

dc7bf7b

…ansport

Changes done to add new test case

b527dc7

morgo approved these changes May 2, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 2, 2022

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 3, 2022

Conflict resolved

2ffae73

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 10, 2022

Alkaagr81 requested review from dveeden and morgo May 10, 2022 22:39

Conflict Solved

b7b558f

morgo approved these changes May 11, 2022

View reviewed changes

Alkaagr81 added 3 commits May 13, 2022 14:04

Conflict resolved

45378cc

Merge done

0f8827f

Merge

0db763f

dveeden approved these changes May 17, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 17, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 17, 2022

Merge branch 'master' into require_secure_transport

a3a9ae7

ti-chi-bot merged commit d2ada35 into pingcap:master May 17, 2022

bb7133 reviewed May 18, 2022

View reviewed changes

bb7133 mentioned this pull request May 18, 2022

when tidb has some deprecated configurations , upgrade from 6.0.0 to nightly fail #34771

Closed

dveeden mentioned this pull request Jan 24, 2024

Various TLS related updates pingcap/docs#16267

Merged

17 tasks

This was referenced Feb 27, 2024

Various TLS related updates (#16267) pingcap/docs#16621

Merged

Various TLS related updates (#16267) pingcap/docs#16622

Merged

Oreoxmt mentioned this pull request Apr 2, 2024

Various TLS related updates pingcap/docs-cn#16982

Merged

17 tasks

Conversation

Alkaagr81 commented Apr 26, 2022 • edited by morgo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alkaagr81 commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alkaagr81 Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

sre-bot commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alkaagr81 Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

dveeden left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morgo commented Apr 28, 2022

Uh oh!

dveeden commented May 2, 2022

Uh oh!

morgo commented May 2, 2022

Uh oh!

morgo commented May 3, 2022

Uh oh!

dveeden left a comment

Choose a reason for hiding this comment

Uh oh!

morgo commented May 17, 2022

Uh oh!

morgo commented May 17, 2022

Uh oh!

ti-chi-bot commented May 17, 2022

Uh oh!

dveeden commented May 18, 2022

Uh oh!

bb7133 May 18, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Alkaagr81 commented Apr 26, 2022 •

edited by morgo

Loading

ti-chi-bot commented Apr 26, 2022 •

edited

Loading

Alkaagr81 commented Apr 26, 2022 •

edited

Loading

sre-bot commented Apr 26, 2022 •

edited

Loading

dveeden left a comment •

edited

Loading