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.

Sign up for GitHub

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

Jump to bottom

security: Encrypt region boundary keys, Part 2 - server changes #2976

Merged
merged 30 commits into from
Oct 11, 2020
Merged

security: Encrypt region boundary keys, Part 2 - server changes #2976

merged 30 commits into from
Oct 11, 2020

Conversation

yiwu-arbug
Copy link

@yiwu-arbug yiwu-arbug commented Sep 16, 2020 •
edited
Loading

Signed-off-by: Yi Wu [email protected]

What problem does this PR solve?

This is part 2 for adding TDE support to PD. pingcap/tidb#18262 This PR adds changes on the server layer.

What is changed and how it works?

This PR adds the following:

  • Define config struct for encryption (see config.toml changes)
  • Update storage.go and region_storage.go to encrypt region boundary keys
  • And empty KeyManager implementation. Its implementation will be done in the next PR. With the empty implementation, the encryption is an no-op.
  • server.go changes to create KeyManager and notify the KeyManager when the current node become PD leader. The KeyManager is supposed to update encryption metadata (e.g. key rotation, handle dynamic config change) only when the node is the PD leader.

Check List

Tests

  • CI.
  • Some manual end-to-end test with a WIP KeyManager implementation.

Side effects

  • Shouldn't have any side effects, as the encrypt is an no-op before KeyManager is implemented.

Related changes

Release note

  • No release note

Yi Wu added 6 commits September 17, 2020 08:04
update errno and config template
b6f7ba0 
Signed-off-by: Yi Wu <[email protected]>
fix typo
0bf0304 
Signed-off-by: Yi Wu <[email protected]>
fix tests
23954db 
Signed-off-by: Yi Wu <[email protected]>
fix tests
2bad0b9 
Signed-off-by: Yi Wu <[email protected]>
fix tests
f88d26a 
Signed-off-by: Yi Wu <[email protected]>
fix tests
6f6d93a 
Signed-off-by: Yi Wu <[email protected]>
@yiwu-arbug
Copy link
Author

looks like tests caught some issue in the change. still fixing.

address comment in #2931
928de26 
Signed-off-by: Yi Wu <[email protected]>
@yiwu-arbug yiwu-arbug mentioned this pull request Sep 17, 2020
Merged
@yiwu-arbug
Copy link
Author

/run-all-tests

fix lint
c16ecd8 
Signed-off-by: Yi Wu <[email protected]>
@yiwu-arbug yiwu-arbug added the component/security Security logic. label Sep 18, 2020
Yi Wu added 2 commits September 18, 2020 08:27
fix comments
2428248 
Signed-off-by: Yi Wu <[email protected]>
fix comment
ce83a42 
Signed-off-by: Yi Wu <[email protected]>
@yiwu-arbug
Copy link
Author

@HunDunDM @Yisaer ready for review. thanks.

@yiwu-arbug
Copy link
Author

@HunDunDM @Yisaer kindly ping

HunDunDM
HunDunDM reviewed Sep 23, 2020
View reviewed changes
server/core/region_storage.go Outdated Show resolved Hide resolved
server/core/region_storage.go Outdated Show resolved Hide resolved
server/core/region_storage.go Show resolved Hide resolved
server/core/storage.go Outdated Show resolved Hide resolved
@yiwu-arbug
Copy link
Author

yiwu-arbug commented Sep 25, 2020 •
edited
Loading

sorry for delay. updated.

HunDunDM
HunDunDM reviewed Sep 27, 2020
View reviewed changes
Copy link
Member

@HunDunDM HunDunDM left a comment

Choose a reason for hiding this comment

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

rest LGTM

server/core/region_storage.go Outdated Show resolved Hide resolved
server/core/storage.go Outdated Show resolved Hide resolved
server/core/storage.go Outdated Show resolved Hide resolved
@yiwu-arbug
Copy link
Author

Updated

HunDunDM
HunDunDM approved these changes Sep 30, 2020
View reviewed changes
Copy link
Member

@HunDunDM HunDunDM 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-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 30, 2020
@yiwu-arbug
Copy link
Author

@Yisaer ping

Yisaer
Yisaer reviewed Oct 9, 2020
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

server/core/region_storage.go Outdated Show resolved Hide resolved
pkg/encryption/config.go Show resolved Hide resolved
@ti-srebot
Copy link
Contributor

@yiwu-arbug,Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments.See the corresponding SIG page for more information. Related SIG: scheduling(slack).

Yisaer
Yisaer approved these changes Oct 11, 2020
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer 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-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Oct 11, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Oct 11, 2020
@Yisaer
Copy link
Contributor

Yisaer commented Oct 11, 2020

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 11, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 6a418ce into tikv:master Oct 11, 2020
@yiwu-arbug yiwu-arbug deleted the enc_server branch October 11, 2020 20:34
@yiwu-arbug yiwu-arbug mentioned this pull request Oct 12, 2020
Merged
@yiwu-arbug yiwu-arbug mentioned this pull request Nov 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yisaer Yisaer Yisaer approved these changes

@HunDunDM HunDunDM HunDunDM approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

@nolouch nolouch Awaiting requested review from nolouch

Assignees
No one assigned
Labels
component/security Security logic. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yiwu-arbug @ti-srebot @Yisaer @HunDunDM