Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Add design doc for --initialize-secure #28482

Merged
merged 7 commits into from
Oct 20, 2021

Conversation

morgo
Copy link
Contributor

@morgo morgo commented Sep 29, 2021

What problem does this PR solve?

Issue Number: #28481

Problem Summary:

This adds a proposal for a secure bootstrap.

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

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Sep 29, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • bb7133
  • dveeden

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.

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

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 29, 2021
@morgo morgo requested review from bb7133 and dveeden September 29, 2021 16:29
@morgo morgo mentioned this pull request Sep 29, 2021
12 tasks
@morgo
Copy link
Contributor Author

morgo commented Sep 29, 2021

/run-check_dev_2

@morgo morgo mentioned this pull request Sep 29, 2021
3 tasks
@morgo morgo requested a review from ThomasYYYY September 30, 2021 03:43
Copy link
Contributor

@dveeden dveeden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to note that a socket connection is considered to be a "secure connection", just like TLS is. So if we use a random password instead we would also have to enable auto-tls and set REQUIRE SSL on the user to have the same security for the initial connection. If at any point we make caching_sha2_password the default instead of mysql_native_password this would require TLS as well. Using a UNIX socket allows us to get in to the server even if TLS isn't configured or if the TLS certificates are expired or are otherwise problematic.

docs/design/2021-09-29-secure-bootstrap.md Show resolved Hide resolved
docs/design/2021-09-29-secure-bootstrap.md Show resolved Hide resolved
2. Stale socket files are automatically cleaned up on server start [PR Merged](https://github.com/pingcap/tidb/pull/27886)
3. TiDB listens on both TCP **and** unix socket by default [PR in Review](https://github.com/pingcap/tidb/pull/28486)

## Test Design
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use ubuntu instead of root we should verify if there are other things that might break because of this.

Might be good to test br, dumpling, lightning, etc to see if they allow UNIX socket connections (should not be a blocker, and likely already the case for MySQL support)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Although in practice, this won't break if we modify tiup and Operator to use --initialize-insecure and generate a random password before we change the default bootstrap to secure (default change is not in scope yet for this proposal).


The design choice of `auth_socket` is based on our current requirement of only supporting unix-like operating systems, and not needing to support Windows. This is a reasonable choice, as we do not expect to support Windows in the future. There might be some minor added complexity if we currently do not handle cases well such as the socket file already existing.

Adding the option `--initialize-secure` itself does not add risk, but enabling it by default will add risk as it is a difference in behavior. This design tries to minimize the risk, but it is a large change. There is a risk if we don't update all the documentation and communicate the change effectively, it will become a support issue for new users.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my concern is for the deployment of a new version of TiDB by the old version of the deployment tool(TiUP/TiOperator), the users may be surprised by the --initialize-secure mode and very difficult to access by auth socket in a pod(TiOperator).

For now, I prefer to set the default option as --initialize-insecure and change the default option after all old versions of TiUP/TiOperator are deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, and this is is what is intended: The default is --initialize-insecure to give installers time to transition. Once transitioned, the default can change to secure.

By adding both options, it supports the transition correctly because initially --initialize-insecure will be a noop, but will later mean something as the default changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also the above comment: default change is not in scope yet for this proposal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thank you!

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 18, 2021
@morgo morgo requested a review from bb7133 October 20, 2021 02:45
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot 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 Oct 20, 2021
@bb7133
Copy link
Member

bb7133 commented Oct 20, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: dc3b0e6

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 20, 2021
@ti-chi-bot
Copy link
Member

@morgo: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot ti-chi-bot merged commit d0f8e02 into pingcap:master Oct 20, 2021

## Impacts & Risks

The design choice of `auth_socket` is based on our current requirement of only supporting unix-like operating systems, and not needing to support Windows. This is a reasonable choice, as we do not expect to support Windows in the future. There might be some minor added complexity if we currently do not handle cases well such as the socket file already existing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about if we consider the Windows OS?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants