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

Support rotate root manifests #967

Merged
merged 11 commits into from
Dec 9, 2020
Merged

Support rotate root manifests #967

merged 11 commits into from
Dec 9, 2020

Conversation

lucklove
Copy link
Member

@lucklove lucklove commented Dec 3, 2020

What problem does this PR solve?

In TiUP's repository, there is a root.json(the root manifest) file, which records the keys we used for the whole repository. It's the foundation of the trust chain. Related document of the manifest mechanism: https://github.com/pingcap/tiup/blob/master/doc/design/manifest.md

The root.json is signed by 5 different people (with 5 different keys) and it has an expires field, so the 5 people must re-sign the root.json before it really expires. We should do the follow things:

  • Update the expires and version field in root.json
  • The 5 people who has root key signs the modified root.json
  • Update snapshot.json to record the new version of root.json
  • Re-sign snapshot.json because it's content has been changed
  • Update timestamp.json since the snapshot.json has been changed
  • Re-sign timestamp.json since it's content has been changed

Do these things manually will be boring and it's easy to make mistake (what if some one is publishing component to the repository while you modify timestamp.json and snapshot.json manually?). So we'd better make it automated, at least semi-automated.

What is changed and how it works?

  • Enhance the tiup mirror sign command, make it possible to sign the manifest via network by tiup mirror sign http:/xxx/yyy.json, in a similar way as signing a manifest file in local filesystem.
  • Implement a temp server, it will open an editor to let you edit root.json and start to serve the edited root.json via the internet
  • Implement a Rotate method on Mirror interface, which will update snapshot.json and timestamp.json automatically according to the updated root.json

By this way, we can update the root.json with these steps:

  • The one who response to update root.json execute the command tiup mirror rotate and edit the root.json (update expires field .etc.), after saving the file, a server will be started, and the address to sign the manifest will be printed on the console.
  • The 5 people who have the key use the command tiup mirror sign http://rotate-address to sign the modified root.json
  • After all people signed, the temp-server will close automatically and send the signed root.json to the real tiup-server (if you use remote mirror) or just push it to local mirror (if you use offline mirror)
  • The mirror will check the permission and update snapshot and timestamp

By this way, we can do things easier and avoid transfering private key via the internet (we should never do this)

Check List

Tests

  • Manual test (add detailed scripts or steps below)

The manual test steps:

In terminal 1:

  • tiup mirror init /tmp/test-mirror
  • tiup mirror set /tmp/test-mirror
  • tiup mirror rotate: in the eidtor, increase the manifest version by one and save, this should start a server

In terminal 2:

  • tiup mirror sign http://<my-ip>:8080/rotate/root.json --key /tmp/test-mirror/keys/<hash1>-root.json
  • tiup mirror sign http://<my-ip>:8080/rotate/root.json --key /tmp/test-mirror/keys/<hash2>-root.json
  • tiup mirror sign http://<my-ip>:8080/rotate/root.json --key /tmp/test-mirror/keys/<hash3>-root.json

After these steps, the server of terminal 1 will closed automatically and the /tmp/test-mirror/2.root.json should be produced.

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release notes:

Support rotating the root manifest

@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2020
@ti-chi-bot ti-chi-bot requested a review from lonng December 3, 2020 13:35
@ti-chi-bot ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 3, 2020
@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #967 (851b376) into master (15151f1) will decrease coverage by 4.25%.
The diff coverage is 13.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #967      +/-   ##
==========================================
- Coverage   55.95%   51.69%   -4.26%     
==========================================
  Files         263      264       +1     
  Lines       19645    19856     +211     
==========================================
- Hits        10992    10265     -727     
- Misses       6925     7956    +1031     
+ Partials     1728     1635      -93     
Flag Coverage Δ
cluster 38.14% <0.93%> (-5.41%) ⬇️
dm 24.03% <0.93%> (-0.19%) ⬇️
integrate 45.99% <11.79%> (-4.19%) ⬇️
playground 20.29% <0.93%> (-0.28%) ⬇️
tiup 16.45% <11.79%> (-0.34%) ⬇️
unittest 22.87% <17.85%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/repository/mirror.go 39.35% <0.00%> (-5.19%) ⬇️
server/rotate/rotate_server.go 0.00% <0.00%> (ø)
pkg/repository/v1_repository.go 63.48% <9.09%> (-1.60%) ⬇️
pkg/repository/model/model.go 46.46% <14.75%> (-10.88%) ⬇️
cmd/mirror.go 48.96% <21.21%> (-4.91%) ⬇️
pkg/repository/store/txn.go 59.68% <100.00%> (+0.14%) ⬆️
components/cluster/command/check.go 6.27% <0.00%> (-73.27%) ⬇️
pkg/cluster/task/limits.go 0.00% <0.00%> (-68.75%) ⬇️
pkg/cluster/task/sysctl.go 0.00% <0.00%> (-66.67%) ⬇️
components/cluster/command/audit.go 27.27% <0.00%> (-54.55%) ⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15151f1...851b376. Read the comment docs.

@lucklove lucklove marked this pull request as ready for review December 8, 2020 13:03
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2020
@AstroProfundis AstroProfundis added category/security Categorizes issue or PR as a security enhancement. type/new-feature Categorizes pr as related to a new feature. labels Dec 9, 2020
cmd/mirror.go Outdated Show resolved Hide resolved
cmd/mirror.go Outdated Show resolved Hide resolved
pkg/repository/mirror.go Outdated Show resolved Hide resolved
pkg/repository/model/model.go Outdated Show resolved Hide resolved
pkg/repository/model/model.go Outdated Show resolved Hide resolved
server/rotate/rotate_server.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 9, 2020
@lucklove lucklove merged commit 6bc6f3f into pingcap:master Dec 9, 2020
@lucklove lucklove deleted the rotate branch December 9, 2020 11:26
lucklove added a commit to lucklove/tiup that referenced this pull request Dec 16, 2020
Introduced by pingcap#967

The tiup mirror sign doesn't handle --key flag correctly for local manifest file
@lucklove lucklove mentioned this pull request Dec 16, 2020
ti-chi-bot added a commit that referenced this pull request Dec 17, 2020
Introduced by #967

The tiup mirror sign doesn't handle --key flag correctly for local manifest file

Co-authored-by: Ti Prow Robot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/security Categorizes issue or PR as a security enhancement. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/LGT1 Indicates that a PR has LGTM 1. type/new-feature Categorizes pr as related to a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants