Root rotation (previously root cert rotation) #648
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cyli
force-pushed
the
cert-rotation
branch
5 times, most recently
from
d0a5b2c to
54468a5
Compare
|
|
||
| // RotateRootCert generates a new certificate to replace oldCert and adds | ||
| // a changelist entry to update the root role with this certificate replacement | ||
| // shen r.Publish() is called. |
cyli
force-pushed
the
cert-rotation
branch
4 times, most recently
from
b464289 to
833f3d3
Compare
| for _, cert := range certs { | ||
| id, err := trustmanager.FingerprintCert(cert) | ||
| if err != nil { | ||
| id = fmt.Sprintf("[Error %s]", err) |
There was a problem hiding this comment.
nit: since we're using this function for debug strings, can we maybe include Debug or a similar verb to the function name so it isn't accidentally called for other purposes?
There was a problem hiding this comment.
Renamed it prettyFormatCertIDs
|
(sorry, tried closing and reopening to try to kick the docker/dco-signed job) |
This is a refactoring with no behavior change, but it will allow adding more parameters to the function in the future. Signed-off-by: Miloslav Trmač <[email protected]>
Tell signed.Sign how many signatures are necessary to sign a role, and have it fail if it cannot create that many. For most uses this does not make much of a difference because the threshold tends to be 1 and signed.Sign was already failing if no key could be found or if no signature could be created; only >1-threshold roles now (correctly) fail in additional situations. But the knowledge of a role’s threshold will be useful in a future commit. Always use ErrInsufficientSignatures for this failure, whether this is when loading the keys or actually using them (also fixing ErrInsufficentSignature documentation to refer to signing and not verification). ErrNoKeys is no longer returned by signed.Sign. So, adjust the “snapshot key is not available” logic in NotaryRepository.Publish accordingly, which also makes it more precise (actually triggering only when no snapshot key is available). Now that role's threshold is enforced when signing, update TestValidateRootInvalidTimestampThreshold to create the second key necessary to correctly sign the timestamp role. Signed-off-by: Miloslav Trmač <[email protected]>
Ordinarily we don't want to continue operating on signed data if the role's threshold of signatures cannot be me and the signature is unsuable. OTOH we want to keep signing root.json with all older keys if they are available (to allow migration), but in that case a missing key is not a fatal error. So, split the keys passed to signed.Sign into primary and optional, treating all current uses as primary and enforcing the role's threshold only on primary keys. Also update the single existing test which uses a missing/unusable key to use the optionalKeys parameter. Note that only the _presence_ of optionalKeys is optional; if an optional key exists but signing using it fails, the function will fail. This temporarily breaks the second ErrInsufficientSignatures check (optional keys count against the role threshold), but that will be fixed soon. Signed-off-by: Miloslav Trmač <[email protected]>
Instead of silently ignoring any signing errors, and failing with a generic ErrInsufficientSignatures, just pass through any signing errors. As long as the role's threshold is met, some primary keys may be missing. But if any key does exist (whether primary or optional), failures using that key are fatal. (This is why signed.Sign needs to know the role's threshold; making any missing key fatal would break signatures of roles with 2 valid keys and threshold of 1.) As a side effect, this improves the “snapshot key is not available” detection in NotaryRepository.Publish, to trigger only when a key is not available, not also when it should be available but using it is failing. Similarly, the ErrInsufficientSignatures error now mentions only the missing keys, not the successfully used keys now that present but failing keys are handled by using other error types. (This removes the second ErrInsufficientSignatures check because it is no longer necessary, which also fixes the fact that it was broken by introducing optional keys. The first, now only, ErrInsufficientSignatures check does correctly distinguish between primary and optional keys.) Signed-off-by: Miloslav Trmač <[email protected]>
The only thing depending on signed.Sign keeping old signatures was two tests; all real users were modifying the signed data without clearing the old signatures, and therefore implicitly relying on signing with the keys which were used for the old signatures as well. This broke signing an updated root with a new certificate when the old certificate was no longer available. It could have been fixed by keeping signed.Sign as is and adding the clearing to all users, but noting actually needs the appending semantics, the appending semantics is surprising, and switching to replacing signatures is less code. Signed-off-by: Miloslav Trmač <[email protected]>
Repo.RotateBaseKeys() can now be used to replace the root keypair (i.e. primarily the expiring certificate), and Repo.SignRoot will make signatures using the old keypairs if they are available (but not fail if they are not; only the keypairs listed in the role as trusted are mandatory). To do this, we need to keep even the old, possibly currently untrusted, keypairs around (to allow rollover from clients which trust these old keys): The private keys are simply not deleted from the repo's CryptoService; this means no change for the current setup with a long-term private key and periodically expiring certificates, but a true rotation of the private key will eventually require explicit management of the preserved long-term private keys (if we are keeping several of them around, but most are either obsolete and non-preferred or possibly even known to be compromised, we will want to make sure that we always use the new/preferred private key for new certificate generation). The public keys are tricker: 1) We need to keep a list of them; the private keys can be looked up by their IDs, and that allows extracting the public part as well, but we need a list of the key IDs. We can't just keep the key IDs included in the role's list of authorized keys, that would make it impossible to rotate away from a suspect or known compromsied key. 2) With the X.509 certificate “public keys”, key ID is not actually sufficient to retrieve the full public key even if we have access to the private key; we actually need to store the full public key == certificate somewhere. And preferably without having tuf.Repo depend on a certs.Manager, designed to deal with concepts of trust at a higher level than TUF cares about. Actually, to the extent certs.Manager's purpose is to manage and verify trust, storing old, possibly suspect or known compromised certificates would be explicitly contrary to its mission. So, this patch keeps around full copies of the certificates in the root.json “keys” map (not the “roles” map of trusted keys). It means sending to clients a little data which they don't need but it is otherwise harmless; and keeping the certificates within the structured and managed tuf.data.Root format could allow us to build nice UI (e.g. show me all certificates we still carry and keep signing with, let me drop two of them now that our company has changed a name and does not want to advertise the history) if we ever needed to something like this Signed-off-by: Ying Li <[email protected]>
NotaryRepository can now list root certificates, and generate new versions (as changelists to be applied on Publish). This is a pretty mechanical encapsulation of the root certificate rotation support in Repo.AddBaseKeys and Repo.RemoveBaseKeys. The only slightly interesting part is ListRootCert, which requires on-line access to ensure fresh data, and depends on CertStore doing some verification for us. Signed-off-by: Miloslav Trmač <[email protected]>
This is a trivial wrapper around the NotaryRepository functionality. The UI is simplest possible, a single (notary cert rotate) rotates all certificates. This handles the common case (only a single certificate) perfectly If there were multiple certificates, rotating all of them regardless of age does not really hurt; we can easily extend this to give the user more control (having the user specify a cert ID, for example) later if necessary. Signed-off-by: Miloslav Trmač <[email protected]>
This reverts commit aca1cf6, and modifies a test to expect 2 signatures. Signed-off-by: Ying Li <[email protected]>
Signed-off-by: Ying Li <[email protected]>
…ed as a versioned root role in the root.json. That way we can figure out which keys were previously root keys. Update tuf.Repo.sign to take a list of required roles (at most two, for root rotations, because only the immediate root after a rotation absolutely needs to correctly validate against the previous root role and the new root role) instead of just a single role. tuf.Repo.sign now ensures that the number of signatures on the metadata satisfy role requirements for every required role. Then it tries to sign with whatever optional keys it can, ignoring errors and not requiring that any particular number of signatures were produced with the optional keys. Signed-off-by: Ying Li <[email protected]>
Signed-off-by: Ying Li <[email protected]>
Signed-off-by: Ying Li <[email protected]>
This reverts commit 684c17867740e77460f2940d3d76023f7a9647ed, and extra cert rotate test changes Signed-off-by: Ying Li <[email protected]>
| err = userRepo.Update(false) | ||
| require.NoError(t, err) | ||
| logRepoTrustRoot(t, "user refresh 1", userRepo) | ||
| require.Equal(t, newRootCertID, rootRoleCertID(t, userRepo)) |
There was a problem hiding this comment.
We should test that making some other modification in the root.json (like rotating the targets key), causing it to be signed again still signs with both the old and new keys.
…ely previous role Signed-off-by: Ying Li <[email protected]>
Signed-off-by: Ying Li <[email protected]>
…igned back into the repo if signing was successful. This way, we don't mutate the existing root in a failed attempt to sign it. Signed-off-by: Ying Li <[email protected]>
|
LGTM!!! In the immortal words of Queen: "It finally happened" |
|
LGTM! |
Closed
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the rebased version + some changes from #267.
To summarize: that PR added library support and CLI support for rotating the public x509 root public key in a root.json, in case it expires. This does not rotate the root private key.
There are a couple additional commits that address some comments from the previous PR:
tuf.Repo'sSignRoot, which right now requires that the threshold signatures be met for every required role.tuf.Repo.Root.Signed.Keysobject, but there is otherwise no way in spec to keep track of which of those keys are actually old keys, vs cruft. This PR adds versioned root roles every time root keys are changed to the list of roles intuf.Repo.Root.Signed.Roles- this technically does not violate the spec, because it doesn't change existing roles, and is not used for validation, only hints w.r.t. which keys to use for signing. It may also be helpful in maintaining a history of root key changes).This is probably ready for first look. @mtrmac?