From bd6b6a28d4fb19c3e1cb526c72af3490b8dceb76 Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Sat, 16 Dec 2023 13:29:20 +0530 Subject: [PATCH 01/12] policy: Verify threshold in delegation graph walk Signed-off-by: Aditya Sirish --- internal/policy/policy.go | 5 +++++ internal/policy/verify.go | 6 ++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/policy/policy.go b/internal/policy/policy.go index f55f9a033..519c25859 100644 --- a/internal/policy/policy.go +++ b/internal/policy/policy.go @@ -413,6 +413,11 @@ func (s *State) FindVerifiersForPath(ctx context.Context, path string) ([]*Verif verifiers = append(verifiers, verifier) if s.HasTargetsRole(delegation.Name) { + env := s.DelegationEnvelopes[delegation.Name] + if err := verifier.Verify(ctx, nil, env); err != nil { + return nil, err + } + delegatedMetadata, err := s.GetTargetsMetadata(delegation.Name) if err != nil { return nil, err diff --git a/internal/policy/verify.go b/internal/policy/verify.go index f9a06618f..5c8ca1c36 100644 --- a/internal/policy/verify.go +++ b/internal/policy/verify.go @@ -770,9 +770,9 @@ func (v *Verifier) Verify(ctx context.Context, gitObject object.Object, attestat } if attestation != nil { - // combining the attestation and the git object we still do not have - // sufficient signatures if (1 + len(attestation.Signatures)) < v.threshold { + // Combining the attestation and the git object we still do not have + // sufficient signatures return ErrVerifierConditionsUnmet } } @@ -832,6 +832,8 @@ func (v *Verifier) Verify(ctx context.Context, gitObject object.Object, attestat verifiers := make([]sslibdsse.Verifier, 0, len(v.keys)-1) for _, key := range v.keys { if key.KeyID == keyIDUsed { + // Do not create a DSSE verifier for the key used to verify the Git + // signature continue } From 3b5134727f62c6b75463d2fc4a2fbb83116ca396 Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Sat, 16 Dec 2023 03:14:17 -0500 Subject: [PATCH 02/12] policy: Verify root + targets during graph walk Signed-off-by: Aditya Sirish --- internal/policy/policy.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/internal/policy/policy.go b/internal/policy/policy.go index 519c25859..2764f1bea 100644 --- a/internal/policy/policy.go +++ b/internal/policy/policy.go @@ -370,6 +370,20 @@ func (s *State) FindVerifiersForPath(ctx context.Context, path string) ([]*Verif return nil, err } + rootVerifier := s.getRootVerifier() + if err := rootVerifier.Verify(ctx, nil, s.RootEnvelope); err != nil { + return nil, err + } + + targetsVerifier, err := s.getTargetsVerifier() + if err != nil { + return nil, err + } + + if err := targetsVerifier.Verify(ctx, nil, s.TargetsEnvelope); err != nil { + return nil, err + } + targetsMetadata, err := s.GetTargetsMetadata(TargetsRoleName) if err != nil { return nil, err @@ -729,6 +743,28 @@ func (s *State) HasTargetsRole(roleName string) bool { return ok } +func (s *State) getRootVerifier() *Verifier { + return &Verifier{ + keys: s.RootPublicKeys, + threshold: len(s.RootPublicKeys), + } +} + +func (s *State) getTargetsVerifier() (*Verifier, error) { + rootMetadata, err := s.GetRootMetadata() + if err != nil { + return nil, err + } + + verifier := &Verifier{keys: make([]*tuf.Key, 0, len(rootMetadata.Roles[TargetsRoleName].KeyIDs))} + for _, keyID := range rootMetadata.Roles[TargetsRoleName].KeyIDs { + verifier.keys = append(verifier.keys, rootMetadata.Keys[keyID]) + } + verifier.threshold = rootMetadata.Roles[TargetsRoleName].Threshold + + return verifier, nil +} + func (s *State) findDelegationEntry(roleName string) (*tuf.Delegation, error) { topLevelTargetsMetadata, err := s.GetTargetsMetadata(TargetsRoleName) if err != nil { From 05c5df2657832bd069440612d9a110c86564b324 Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Mon, 18 Dec 2023 15:49:59 -0500 Subject: [PATCH 03/12] policy: Fix docs Signed-off-by: Aditya Sirish --- internal/policy/policy.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/policy/policy.go b/internal/policy/policy.go index 2764f1bea..ebe2d30d9 100644 --- a/internal/policy/policy.go +++ b/internal/policy/policy.go @@ -314,6 +314,8 @@ func (s *State) FindAuthorizedSigningKeyIDs(ctx context.Context, roleName string // FindPublicKeysForPath identifies the trusted keys for the path. If the path // protected in gittuf policy, the trusted keys are returned. +// +// Deprecated: use FindVerifiersForPath. func (s *State) FindPublicKeysForPath(ctx context.Context, path string) ([]*tuf.Key, error) { if err := s.Verify(ctx); err != nil { return nil, err @@ -365,11 +367,15 @@ func (s *State) FindPublicKeysForPath(ctx context.Context, path string) ([]*tuf. } } +// FindVerifiersForPath identifies the trusted set of verifiers for the +// specified path. While walking the delegation graph for the path, signatures +// for delegated metadata files are verified using the verifier context. func (s *State) FindVerifiersForPath(ctx context.Context, path string) ([]*Verifier, error) { if err := s.Verify(ctx); err != nil { return nil, err } + // TODO: verify root from original state rootVerifier := s.getRootVerifier() if err := rootVerifier.Verify(ctx, nil, s.RootEnvelope); err != nil { return nil, err From cc6879b3228a3fe32f4567878ae82e0ff65817c6 Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Mon, 18 Dec 2023 15:54:52 -0500 Subject: [PATCH 04/12] policy: Deprecate state.Verify() uses Signed-off-by: Aditya Sirish --- internal/policy/policy.go | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/internal/policy/policy.go b/internal/policy/policy.go index ebe2d30d9..fa436b164 100644 --- a/internal/policy/policy.go +++ b/internal/policy/policy.go @@ -201,10 +201,6 @@ func LoadStateForEntry(ctx context.Context, repo *git.Repository, entry *rsl.Ref state.RootPublicKeys = append(state.RootPublicKeys, key) } - if err := state.Verify(ctx); err != nil { - return nil, err - } - return state, nil } @@ -282,6 +278,14 @@ func (s *State) PublicKeys() (map[string]*tuf.Key, error) { // FindAuthorizedSigningKeyIDs traverses the policy metadata to identify the // keys trusted to sign for the specified role. +// +// Deprecated: we want to avoid promiscuous delegations where multiple roles may +// delegate to the same role and we can't clarify up front which role's trusted +// keys we must use. We only know if a delegated role is trusted when we're +// actively walking the graph for a specific path. See: +// https://github.com/theupdateframework/specification/issues/19, +// https://github.com/theupdateframework/specification/issues/214, and +// https://github.com/theupdateframework/python-tuf/issues/660. func (s *State) FindAuthorizedSigningKeyIDs(ctx context.Context, roleName string) ([]string, error) { if err := s.Verify(ctx); err != nil { return nil, err @@ -371,10 +375,6 @@ func (s *State) FindPublicKeysForPath(ctx context.Context, path string) ([]*tuf. // specified path. While walking the delegation graph for the path, signatures // for delegated metadata files are verified using the verifier context. func (s *State) FindVerifiersForPath(ctx context.Context, path string) ([]*Verifier, error) { - if err := s.Verify(ctx); err != nil { - return nil, err - } - // TODO: verify root from original state rootVerifier := s.getRootVerifier() if err := rootVerifier.Verify(ctx, nil, s.RootEnvelope); err != nil { @@ -464,6 +464,14 @@ func (s *State) FindVerifiersForPath(ctx context.Context, path string) ([]*Verif // Verify performs a self-contained verification of all the metadata in the // State starting from the Root. Any metadata that is unreachable in the // delegations graph returns an error. +// +// Deprecated: we want to avoid promiscuous delegations where multiple roles may +// delegate to the same role and we can't clarify up front which role's trusted +// keys we must use. We only know if a delegated role is trusted when we're +// actively walking the graph for a specific path. See: +// https://github.com/theupdateframework/specification/issues/19, +// https://github.com/theupdateframework/specification/issues/214, and +// https://github.com/theupdateframework/python-tuf/issues/660. func (s *State) Verify(ctx context.Context) error { rootVerifiers := []sslibdsse.Verifier{} for _, k := range s.RootPublicKeys { @@ -601,10 +609,6 @@ func (s *State) Verify(ctx context.Context) error { // Commit verifies and writes the State to the policy namespace. It also creates // an RSL entry recording the new tip of the policy namespace. func (s *State) Commit(ctx context.Context, repo *git.Repository, commitMessage string, signCommit bool) error { - if err := s.Verify(ctx); err != nil { - return err - } - if len(commitMessage) == 0 { commitMessage = DefaultCommitMessage } @@ -771,6 +775,16 @@ func (s *State) getTargetsVerifier() (*Verifier, error) { return verifier, nil } +// findDelegationEntry finds the delegation entry for some role in the parent +// role. +// +// Deprecated: we want to avoid promiscuous delegations where multiple roles may +// delegate to the same role and we can't clarify up front which role's trusted +// keys we must use. We only know if a delegated role is trusted when we're +// actively walking the graph for a specific path. See: +// https://github.com/theupdateframework/specification/issues/19, +// https://github.com/theupdateframework/specification/issues/214, and +// https://github.com/theupdateframework/python-tuf/issues/660. func (s *State) findDelegationEntry(roleName string) (*tuf.Delegation, error) { topLevelTargetsMetadata, err := s.GetTargetsMetadata(TargetsRoleName) if err != nil { @@ -807,7 +821,6 @@ func (s *State) findDelegationEntry(roleName string) (*tuf.Delegation, error) { } if s.HasTargetsRole(delegation.Name) { - // TODO: clarifying terminating / reachability delegationsQueue = append(delegationsQueue, delegationTargetsMetadata[delegation.Name].Delegations.Roles...) } } From 27df6b477f07032659dd74868fefea20bf640336 Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Mon, 18 Dec 2023 16:00:39 -0500 Subject: [PATCH 05/12] policy: Support verifying only DSSE envelope Signed-off-by: Aditya Sirish --- internal/policy/verify.go | 73 ++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/internal/policy/verify.go b/internal/policy/verify.go index 5c8ca1c36..34e24e4a0 100644 --- a/internal/policy/verify.go +++ b/internal/policy/verify.go @@ -780,50 +780,53 @@ func (v *Verifier) Verify(ctx context.Context, gitObject object.Object, attestat var keyIDUsed string gitObjectVerified := false - // First, verify the gitObject's signature - switch o := gitObject.(type) { - case *object.Commit: - for _, key := range v.keys { - err := gitinterface.VerifyCommitSignature(ctx, o, key) - if err == nil { - // Signature verification succeeded - keyIDUsed = key.KeyID - gitObjectVerified = true - break - } - if errors.Is(err, gitinterface.ErrUnknownSigningMethod) { - continue - } - if !errors.Is(err, gitinterface.ErrIncorrectVerificationKey) { - return err - } - } - case *object.Tag: - for _, key := range v.keys { - err := gitinterface.VerifyTagSignature(ctx, o, key) - if err == nil { - // Signature verification succeeded - keyIDUsed = key.KeyID - gitObjectVerified = true - break - } - if errors.Is(err, gitinterface.ErrUnknownSigningMethod) { - continue + // First, verify the gitObject's signature if one is presented + if gitObject != nil { + switch o := gitObject.(type) { + case *object.Commit: + for _, key := range v.keys { + err := gitinterface.VerifyCommitSignature(ctx, o, key) + if err == nil { + // Signature verification succeeded + keyIDUsed = key.KeyID + gitObjectVerified = true + break + } + if errors.Is(err, gitinterface.ErrUnknownSigningMethod) { + continue + } + if !errors.Is(err, gitinterface.ErrIncorrectVerificationKey) { + return err + } } - if !errors.Is(err, gitinterface.ErrIncorrectVerificationKey) { - return err + case *object.Tag: + for _, key := range v.keys { + err := gitinterface.VerifyTagSignature(ctx, o, key) + if err == nil { + // Signature verification succeeded + keyIDUsed = key.KeyID + gitObjectVerified = true + break + } + if errors.Is(err, gitinterface.ErrUnknownSigningMethod) { + continue + } + if !errors.Is(err, gitinterface.ErrIncorrectVerificationKey) { + return err + } } + default: + return ErrUnknownObjectType } - default: - return ErrUnknownObjectType } - // If threshold is 1, we can return + // If threshold is 1 and the Git signature is verified, we can return if v.threshold == 1 && gitObjectVerified { return nil } - // Second, verify signatures on the attestation + // Second, verify signatures on the attestation, subtracting the threshold + // by 1 to account for a verified git signature envelopeThreshold := v.threshold if gitObjectVerified { envelopeThreshold-- From f5a8ee17f6a5124728a3135410b7f6f8c6c54fdf Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Mon, 18 Dec 2023 16:03:30 -0500 Subject: [PATCH 06/12] policy: Generalize DSSE support in verifier Signed-off-by: Aditya Sirish --- internal/policy/verify.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/policy/verify.go b/internal/policy/verify.go index 34e24e4a0..5c39c08ec 100644 --- a/internal/policy/verify.go +++ b/internal/policy/verify.go @@ -759,18 +759,20 @@ func (v *Verifier) Threshold() int { // Verify is used to check for a threshold of signatures using the verifier. The // threshold of signatures may be met using a combination of at most one Git -// signature and in-toto attestation signatures. -func (v *Verifier) Verify(ctx context.Context, gitObject object.Object, attestation *sslibdsse.Envelope) error { +// signature and signatures embedded in a DSSE envelope. Verify does not inspect +// the envelope's payload, but instead only verifies the signatures. The caller +// must ensure the validity of the envelope's contents. +func (v *Verifier) Verify(ctx context.Context, gitObject object.Object, env *sslibdsse.Envelope) error { if v.threshold < 1 { return ErrInvalidVerifier } - if gitObject == nil && attestation == nil { + if gitObject == nil && env == nil { return ErrVerifierConditionsUnmet } - if attestation != nil { - if (1 + len(attestation.Signatures)) < v.threshold { + if env != nil { + if (1 + len(env.Signatures)) < v.threshold { // Combining the attestation and the git object we still do not have // sufficient signatures return ErrVerifierConditionsUnmet @@ -847,7 +849,7 @@ func (v *Verifier) Verify(ctx context.Context, gitObject object.Object, attestat verifiers = append(verifiers, verifier) } - if err := dsse.VerifyEnvelope(ctx, attestation, verifiers, envelopeThreshold); err != nil { + if err := dsse.VerifyEnvelope(ctx, env, verifiers, envelopeThreshold); err != nil { return ErrVerifierConditionsUnmet } From 2bf6ed0ec3fe4db09a086c2c0c38b193cbcd3ced Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Mon, 18 Dec 2023 16:08:51 -0500 Subject: [PATCH 07/12] repository: Remove check for valid targets signer Signed-off-by: Aditya Sirish --- internal/repository/targets.go | 70 ++++++++++++---------------------- 1 file changed, 24 insertions(+), 46 deletions(-) diff --git a/internal/repository/targets.go b/internal/repository/targets.go index 87196586f..3e4caa1bc 100644 --- a/internal/repository/targets.go +++ b/internal/repository/targets.go @@ -20,10 +20,6 @@ func (r *Repository) InitializeTargets(ctx context.Context, targetsKeyBytes []by if err != nil { return err } - keyID, err := sv.KeyID() - if err != nil { - return err - } state, err := policy.LoadCurrentState(ctx, r.r) if err != nil { @@ -33,15 +29,12 @@ func (r *Repository) InitializeTargets(ctx context.Context, targetsKeyBytes []by return ErrCannotReinitialize } - // Verify if targetsKeyBytes is authorized to sign for the role - authorizedKeyIDs, err := state.FindAuthorizedSigningKeyIDs(ctx, targetsRoleName) - if err != nil { - return err - } - - if !isKeyAuthorized(authorizedKeyIDs, keyID) { - return ErrUnauthorizedKey - } + // TODO: verify is role can be signed using the presented key. This requires + // the user to pass in the delegating role as well as we do not want to + // assume which role is the delegating role (diamond delegations are legal). + // See: https://github.com/theupdateframework/specification/issues/19, + // https://github.com/theupdateframework/specification/issues/214, and + // https://github.com/theupdateframework/python-tuf/issues/660. targetsMetadata := policy.InitializeTargetsMetadata() @@ -76,10 +69,6 @@ func (r *Repository) AddDelegation(ctx context.Context, signingKeyBytes []byte, if err != nil { return err } - keyID, err := sv.KeyID() - if err != nil { - return err - } state, err := policy.LoadCurrentState(ctx, r.r) if err != nil { @@ -89,13 +78,12 @@ func (r *Repository) AddDelegation(ctx context.Context, signingKeyBytes []byte, return policy.ErrMetadataNotFound } - authorizedKeyIDsForRole, err := state.FindAuthorizedSigningKeyIDs(ctx, targetsRoleName) - if err != nil { - return err - } - if !isKeyAuthorized(authorizedKeyIDsForRole, keyID) { - return ErrUnauthorizedKey - } + // TODO: verify is role can be signed using the presented key. This requires + // the user to pass in the delegating role as well as we do not want to + // assume which role is the delegating role (diamond delegations are legal). + // See: https://github.com/theupdateframework/specification/issues/19, + // https://github.com/theupdateframework/specification/issues/214, and + // https://github.com/theupdateframework/python-tuf/issues/660. authorizedKeys := []*tuf.Key{} for _, kb := range authorizedKeysBytes { @@ -147,10 +135,6 @@ func (r *Repository) RemoveDelegation(ctx context.Context, signingKeyBytes []byt if err != nil { return err } - keyID, err := sv.KeyID() - if err != nil { - return err - } state, err := policy.LoadCurrentState(ctx, r.r) if err != nil { @@ -160,13 +144,12 @@ func (r *Repository) RemoveDelegation(ctx context.Context, signingKeyBytes []byt return policy.ErrMetadataNotFound } - authorizedKeyIDsForRole, err := state.FindAuthorizedSigningKeyIDs(ctx, targetsRoleName) - if err != nil { - return err - } - if !isKeyAuthorized(authorizedKeyIDsForRole, keyID) { - return ErrUnauthorizedKey - } + // TODO: verify is role can be signed using the presented key. This requires + // the user to pass in the delegating role as well as we do not want to + // assume which role is the delegating role (diamond delegations are legal). + // See: https://github.com/theupdateframework/specification/issues/19, + // https://github.com/theupdateframework/specification/issues/214, and + // https://github.com/theupdateframework/python-tuf/issues/660. targetsMetadata, err := state.GetTargetsMetadata(targetsRoleName) if err != nil { @@ -208,10 +191,6 @@ func (r *Repository) AddKeyToTargets(ctx context.Context, signingKeyBytes []byte if err != nil { return err } - keyID, err := sv.KeyID() - if err != nil { - return err - } state, err := policy.LoadCurrentState(ctx, r.r) if err != nil { @@ -221,13 +200,12 @@ func (r *Repository) AddKeyToTargets(ctx context.Context, signingKeyBytes []byte return policy.ErrMetadataNotFound } - authorizedKeyIDsForRole, err := state.FindAuthorizedSigningKeyIDs(ctx, targetsRoleName) - if err != nil { - return err - } - if !isKeyAuthorized(authorizedKeyIDsForRole, keyID) { - return ErrUnauthorizedKey - } + // TODO: verify is role can be signed using the presented key. This requires + // the user to pass in the delegating role as well as we do not want to + // assume which role is the delegating role (diamond delegations are legal). + // See: https://github.com/theupdateframework/specification/issues/19, + // https://github.com/theupdateframework/specification/issues/214, and + // https://github.com/theupdateframework/python-tuf/issues/660. authorizedKeys := []*tuf.Key{} keyIDs := "" From a1a6065c8caca7319fe47f6be790a4091d6a5175 Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Mon, 18 Dec 2023 16:19:48 -0500 Subject: [PATCH 08/12] policy: Verify root & top level targets only once Signed-off-by: Aditya Sirish --- internal/policy/policy.go | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/internal/policy/policy.go b/internal/policy/policy.go index fa436b164..17da78379 100644 --- a/internal/policy/policy.go +++ b/internal/policy/policy.go @@ -201,6 +201,24 @@ func LoadStateForEntry(ctx context.Context, repo *git.Repository, entry *rsl.Ref state.RootPublicKeys = append(state.RootPublicKeys, key) } + // TODO: verify root from original state? We have consecutive verification + // in place elsewhere. + rootVerifier := state.getRootVerifier() + if err := rootVerifier.Verify(ctx, nil, state.RootEnvelope); err != nil { + return nil, err + } + + if state.TargetsEnvelope != nil { + targetsVerifier, err := state.getTargetsVerifier() + if err != nil { + return nil, err + } + + if err := targetsVerifier.Verify(ctx, nil, state.TargetsEnvelope); err != nil { + return nil, err + } + } + return state, nil } @@ -375,21 +393,13 @@ func (s *State) FindPublicKeysForPath(ctx context.Context, path string) ([]*tuf. // specified path. While walking the delegation graph for the path, signatures // for delegated metadata files are verified using the verifier context. func (s *State) FindVerifiersForPath(ctx context.Context, path string) ([]*Verifier, error) { - // TODO: verify root from original state - rootVerifier := s.getRootVerifier() - if err := rootVerifier.Verify(ctx, nil, s.RootEnvelope); err != nil { - return nil, err - } - - targetsVerifier, err := s.getTargetsVerifier() - if err != nil { - return nil, err - } - - if err := targetsVerifier.Verify(ctx, nil, s.TargetsEnvelope); err != nil { - return nil, err + if !s.HasTargetsRole(TargetsRoleName) { + // No policies exist + return nil, ErrMetadataNotFound } + // This envelope is verified when state is loaded, as this is + // the start for all delegation graph searches targetsMetadata, err := s.GetTargetsMetadata(TargetsRoleName) if err != nil { return nil, err From 6a362ef8c5379986afee9737fe74bde35ee5988b Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Mon, 18 Dec 2023 16:22:35 -0500 Subject: [PATCH 09/12] policy: Test FindVerifiers for no policies Signed-off-by: Aditya Sirish --- internal/policy/policy_test.go | 88 +++++++++++++++++++--------------- 1 file changed, 49 insertions(+), 39 deletions(-) diff --git a/internal/policy/policy_test.go b/internal/policy/policy_test.go index 5159c4f1b..34d8afaac 100644 --- a/internal/policy/policy_test.go +++ b/internal/policy/policy_test.go @@ -197,48 +197,58 @@ func TestStateGetRootMetadata(t *testing.T) { } func TestStateFindVerifiersForPath(t *testing.T) { - state := createTestStateWithPolicy(t) + t.Run("with policy", func(t *testing.T) { + state := createTestStateWithPolicy(t) - gpgKey, err := gpg.LoadGPGKeyFromBytes(gpgPubKeyBytes) - if err != nil { - t.Fatal(err) - } + gpgKey, err := gpg.LoadGPGKeyFromBytes(gpgPubKeyBytes) + if err != nil { + t.Fatal(err) + } - tests := map[string]struct { - path string - verifiers []*Verifier - }{ - "verifiers for refs/heads/main": { - path: "git:refs/heads/main", - verifiers: []*Verifier{{ - name: "protect-main", - keys: []*tuf.Key{gpgKey}, - threshold: 1, - }}, - }, - "verifiers for files": { - path: "file:1", - verifiers: []*Verifier{{ - name: "protect-files-1-and-2", - keys: []*tuf.Key{gpgKey}, - threshold: 1, - }}, - }, - "verifiers for unprotected branch": { - path: "git:refs/heads/unprotected", - verifiers: []*Verifier{}, - }, - "verifiers for unprotected files": { - path: "file:unprotected", - verifiers: []*Verifier{}, - }, - } + tests := map[string]struct { + path string + verifiers []*Verifier + }{ + "verifiers for refs/heads/main": { + path: "git:refs/heads/main", + verifiers: []*Verifier{{ + name: "protect-main", + keys: []*tuf.Key{gpgKey}, + threshold: 1, + }}, + }, + "verifiers for files": { + path: "file:1", + verifiers: []*Verifier{{ + name: "protect-files-1-and-2", + keys: []*tuf.Key{gpgKey}, + threshold: 1, + }}, + }, + "verifiers for unprotected branch": { + path: "git:refs/heads/unprotected", + verifiers: []*Verifier{}, + }, + "verifiers for unprotected files": { + path: "file:unprotected", + verifiers: []*Verifier{}, + }, + } - for name, test := range tests { - verifiers, err := state.FindVerifiersForPath(context.Background(), test.path) - assert.Nil(t, err, fmt.Sprintf("unexpected error in test '%s'", name)) - assert.Equal(t, test.verifiers, verifiers, fmt.Sprintf("policy verifiers for path '%s' don't match expected verifiers in test '%s'", test.path, name)) - } + for name, test := range tests { + verifiers, err := state.FindVerifiersForPath(context.Background(), test.path) + assert.Nil(t, err, fmt.Sprintf("unexpected error in test '%s'", name)) + assert.Equal(t, test.verifiers, verifiers, fmt.Sprintf("policy verifiers for path '%s' don't match expected verifiers in test '%s'", test.path, name)) + } + }) + + t.Run("without policy", func(t *testing.T) { + state := createTestStateWithOnlyRoot(t) + + verifiers, err := state.FindVerifiersForPath(context.Background(), "test-path") + assert.Nil(t, verifiers) + assert.ErrorIs(t, err, ErrMetadataNotFound) + }) } func TestStateFindPublicKeysForPath(t *testing.T) { From 326d9bb5d328afa4bdac94ad20637e44254da061 Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Mon, 18 Dec 2023 16:40:38 -0500 Subject: [PATCH 10/12] policy: Repurpose Verify() Signed-off-by: Aditya Sirish --- internal/policy/policy.go | 160 ++++---------------------------------- 1 file changed, 14 insertions(+), 146 deletions(-) diff --git a/internal/policy/policy.go b/internal/policy/policy.go index 17da78379..d2472c5f7 100644 --- a/internal/policy/policy.go +++ b/internal/policy/policy.go @@ -11,8 +11,6 @@ import ( "github.com/gittuf/gittuf/internal/gitinterface" "github.com/gittuf/gittuf/internal/rsl" - "github.com/gittuf/gittuf/internal/signerverifier" - "github.com/gittuf/gittuf/internal/signerverifier/dsse" "github.com/gittuf/gittuf/internal/tuf" "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" @@ -203,22 +201,10 @@ func LoadStateForEntry(ctx context.Context, repo *git.Repository, entry *rsl.Ref // TODO: verify root from original state? We have consecutive verification // in place elsewhere. - rootVerifier := state.getRootVerifier() - if err := rootVerifier.Verify(ctx, nil, state.RootEnvelope); err != nil { + if err := state.Verify(ctx); err != nil { return nil, err } - if state.TargetsEnvelope != nil { - targetsVerifier, err := state.getTargetsVerifier() - if err != nil { - return nil, err - } - - if err := targetsVerifier.Verify(ctx, nil, state.TargetsEnvelope); err != nil { - return nil, err - } - } - return state, nil } @@ -471,28 +457,11 @@ func (s *State) FindVerifiersForPath(ctx context.Context, path string) ([]*Verif } } -// Verify performs a self-contained verification of all the metadata in the -// State starting from the Root. Any metadata that is unreachable in the -// delegations graph returns an error. -// -// Deprecated: we want to avoid promiscuous delegations where multiple roles may -// delegate to the same role and we can't clarify up front which role's trusted -// keys we must use. We only know if a delegated role is trusted when we're -// actively walking the graph for a specific path. See: -// https://github.com/theupdateframework/specification/issues/19, -// https://github.com/theupdateframework/specification/issues/214, and -// https://github.com/theupdateframework/python-tuf/issues/660. +// Verify verifies the signatures of the Root role and the top level Targets +// role if it exists. func (s *State) Verify(ctx context.Context) error { - rootVerifiers := []sslibdsse.Verifier{} - for _, k := range s.RootPublicKeys { - sv, err := signerverifier.NewSignerVerifierFromTUFKey(k) - if err != nil { - return err - } - - rootVerifiers = append(rootVerifiers, sv) - } - if err := dsse.VerifyEnvelope(ctx, s.RootEnvelope, rootVerifiers, len(rootVerifiers)); err != nil { + rootVerifier := s.getRootVerifier() + if err := rootVerifier.Verify(ctx, nil, s.RootEnvelope); err != nil { return err } @@ -500,125 +469,21 @@ func (s *State) Verify(ctx context.Context) error { return nil } - rootMetadata := &tuf.RootMetadata{} - rootContents, err := s.RootEnvelope.DecodeB64Payload() - if err != nil { - return err - } - if err := json.Unmarshal(rootContents, rootMetadata); err != nil { - return err - } - - targetsVerifiers := []sslibdsse.Verifier{} - for _, keyID := range rootMetadata.Roles[TargetsRoleName].KeyIDs { - key := rootMetadata.Keys[keyID] - sv, err := signerverifier.NewSignerVerifierFromTUFKey(key) - if err != nil { - return err - } - - targetsVerifiers = append(targetsVerifiers, sv) - } - if err := dsse.VerifyEnvelope(ctx, s.TargetsEnvelope, targetsVerifiers, rootMetadata.Roles[TargetsRoleName].Threshold); err != nil { - return err - } - - if len(s.DelegationEnvelopes) == 0 { - return nil - } - - delegationEnvelopes := map[string]*sslibdsse.Envelope{} - for k, v := range s.DelegationEnvelopes { - delegationEnvelopes[k] = v - } - - targetsMetadata := &tuf.TargetsMetadata{} - targetsContents, err := s.TargetsEnvelope.DecodeB64Payload() + targetsVerifier, err := s.getTargetsVerifier() if err != nil { return err } - if err := json.Unmarshal(targetsContents, targetsMetadata); err != nil { - return err - } - if err := targetsMetadata.Validate(); err != nil { - return err - } - - // Note: If targetsMetadata.Delegations == nil while delegationEnvelopes is - // not empty, we probably want to error out. This should panic. - delegationKeys := targetsMetadata.Delegations.Keys - delegationsQueue := targetsMetadata.Delegations.Roles - - // We can likely process top level targets and all delegated envelopes in - // the loop below by combining the two but this separated model seems easier - // to reason about. Else, we define a custom starting delegation from root - // to targets in the queue and start this loop from there. - - for { - if len(delegationsQueue) == 0 { - break - } - - delegation := delegationsQueue[0] - delegationsQueue = delegationsQueue[1:] - - delegationEnvelope, ok := delegationEnvelopes[delegation.Name] - if !ok { - // Delegation does not have an envelope to verify - continue - } - delete(delegationEnvelopes, delegation.Name) - - delegationVerifiers := make([]sslibdsse.Verifier, 0, len(delegation.KeyIDs)) - for _, keyID := range delegation.KeyIDs { - key := delegationKeys[keyID] - sv, err := signerverifier.NewSignerVerifierFromTUFKey(key) - if err != nil { - return err - } - - delegationVerifiers = append(delegationVerifiers, sv) - } - - if err := dsse.VerifyEnvelope(ctx, delegationEnvelope, delegationVerifiers, delegation.Threshold); err != nil { - return err - } - - delegationMetadata := &tuf.TargetsMetadata{} - delegationContents, err := delegationEnvelope.DecodeB64Payload() - if err != nil { - return err - } - if err := json.Unmarshal(delegationContents, delegationMetadata); err != nil { - return err - } - - if err := delegationMetadata.Validate(); err != nil { - return err - } - - if delegationMetadata.Delegations == nil { - continue - } - - for keyID, key := range delegationMetadata.Delegations.Keys { - delegationKeys[keyID] = key - } - - delegationsQueue = append(delegationsQueue, delegationMetadata.Delegations.Roles...) - } - - if len(delegationEnvelopes) != 0 { - return ErrDanglingDelegationMetadata - } - - return nil + return targetsVerifier.Verify(ctx, nil, s.TargetsEnvelope) } // Commit verifies and writes the State to the policy namespace. It also creates // an RSL entry recording the new tip of the policy namespace. func (s *State) Commit(ctx context.Context, repo *git.Repository, commitMessage string, signCommit bool) error { + if err := s.Verify(ctx); err != nil { + return err + } + if len(commitMessage) == 0 { commitMessage = DefaultCommitMessage } @@ -764,6 +629,9 @@ func (s *State) HasTargetsRole(roleName string) bool { } func (s *State) getRootVerifier() *Verifier { + // TODO: validate against the root metadata itself? + // This eventually goes back to how the very first root is bootstrapped + // See: https://github.com/gittuf/gittuf/issues/117 return &Verifier{ keys: s.RootPublicKeys, threshold: len(s.RootPublicKeys), From 41f854d01fc3e39da4736556fe1814c0a712799a Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Mon, 18 Dec 2023 16:42:35 -0500 Subject: [PATCH 11/12] policy: Update Verify test to include policies Signed-off-by: Aditya Sirish --- internal/policy/policy_test.go | 36 ++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/internal/policy/policy_test.go b/internal/policy/policy_test.go index 34d8afaac..999fd5ad5 100644 --- a/internal/policy/policy_test.go +++ b/internal/policy/policy_test.go @@ -145,23 +145,33 @@ func TestStateKeys(t *testing.T) { } func TestStateVerify(t *testing.T) { - state := createTestStateWithOnlyRoot(t) + t.Run("only root", func(t *testing.T) { + state := createTestStateWithOnlyRoot(t) - if err := state.Verify(context.Background()); err != nil { - t.Error(err) - } + if err := state.Verify(testCtx); err != nil { + t.Error(err) + } + + rootKeys := []*tuf.Key{} + copy(rootKeys, state.RootPublicKeys) + state.RootPublicKeys = []*tuf.Key{} - rootKeys := []*tuf.Key{} - copy(rootKeys, state.RootPublicKeys) - state.RootPublicKeys = []*tuf.Key{} + err := state.Verify(context.Background()) + assert.NotNil(t, err) - err := state.Verify(context.Background()) - assert.NotNil(t, err) + state.RootPublicKeys = rootKeys + state.RootEnvelope.Signatures = []sslibdsse.Signature{} + err = state.Verify(context.Background()) + assert.NotNil(t, err) + }) + + t.Run("with policy", func(t *testing.T) { + state := createTestStateWithPolicy(t) - state.RootPublicKeys = rootKeys - state.RootEnvelope.Signatures = []sslibdsse.Signature{} - err = state.Verify(context.Background()) - assert.NotNil(t, err) + if err := state.Verify(testCtx); err != nil { + t.Error(err) + } + }) } func TestStateCommit(t *testing.T) { From f2251131a2c66760aad10314c1b1330a7f9ca060 Mon Sep 17 00:00:00 2001 From: Aditya Sirish Date: Fri, 22 Dec 2023 12:34:47 -0500 Subject: [PATCH 12/12] *: Update docs for promiscuous delegations Based on pull request feedback from @wlynch, this commit updates some of the inline discussion / TODO items surrounding promiscuous delegations in TUF. Signed-off-by: Aditya Sirish --- internal/policy/policy.go | 24 ++++++++++++++---------- internal/repository/targets.go | 16 ++++------------ 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/internal/policy/policy.go b/internal/policy/policy.go index d2472c5f7..69eb65da2 100644 --- a/internal/policy/policy.go +++ b/internal/policy/policy.go @@ -199,8 +199,6 @@ func LoadStateForEntry(ctx context.Context, repo *git.Repository, entry *rsl.Ref state.RootPublicKeys = append(state.RootPublicKeys, key) } - // TODO: verify root from original state? We have consecutive verification - // in place elsewhere. if err := state.Verify(ctx); err != nil { return nil, err } @@ -283,10 +281,13 @@ func (s *State) PublicKeys() (map[string]*tuf.Key, error) { // FindAuthorizedSigningKeyIDs traverses the policy metadata to identify the // keys trusted to sign for the specified role. // -// Deprecated: we want to avoid promiscuous delegations where multiple roles may -// delegate to the same role and we can't clarify up front which role's trusted -// keys we must use. We only know if a delegated role is trusted when we're -// actively walking the graph for a specific path. See: +// Deprecated: diamond delegations are legal in policy. So, role A and role B +// can both independently delegate to role C, and they *don't* need to specify +// the same set of keys / threshold. So, when signing role C, we actually can't +// determine if the keys being used to sign it are valid. It depends strictly on +// how role C is reached, whether via role A or role B. In turn, that depends on +// the exact namespace being verified. In TUF, this issue is known as +// "promiscuous delegations". See: // https://github.com/theupdateframework/specification/issues/19, // https://github.com/theupdateframework/specification/issues/214, and // https://github.com/theupdateframework/python-tuf/issues/660. @@ -656,10 +657,13 @@ func (s *State) getTargetsVerifier() (*Verifier, error) { // findDelegationEntry finds the delegation entry for some role in the parent // role. // -// Deprecated: we want to avoid promiscuous delegations where multiple roles may -// delegate to the same role and we can't clarify up front which role's trusted -// keys we must use. We only know if a delegated role is trusted when we're -// actively walking the graph for a specific path. See: +// Deprecated: diamond delegations are legal in policy. So, role A and role B +// can both independently delegate to role C, and they *don't* need to specify +// the same set of keys / threshold. So, when signing role C, we actually can't +// determine if the keys being used to sign it are valid. It depends strictly on +// how role C is reached, whether via role A or role B. In turn, that depends on +// the exact namespace being verified. In TUF, this issue is known as +// "promiscuous delegations". See: // https://github.com/theupdateframework/specification/issues/19, // https://github.com/theupdateframework/specification/issues/214, and // https://github.com/theupdateframework/python-tuf/issues/660. diff --git a/internal/repository/targets.go b/internal/repository/targets.go index 3e4caa1bc..a99a918b3 100644 --- a/internal/repository/targets.go +++ b/internal/repository/targets.go @@ -32,9 +32,7 @@ func (r *Repository) InitializeTargets(ctx context.Context, targetsKeyBytes []by // TODO: verify is role can be signed using the presented key. This requires // the user to pass in the delegating role as well as we do not want to // assume which role is the delegating role (diamond delegations are legal). - // See: https://github.com/theupdateframework/specification/issues/19, - // https://github.com/theupdateframework/specification/issues/214, and - // https://github.com/theupdateframework/python-tuf/issues/660. + // See: https://github.com/gittuf/gittuf/issues/246. targetsMetadata := policy.InitializeTargetsMetadata() @@ -81,9 +79,7 @@ func (r *Repository) AddDelegation(ctx context.Context, signingKeyBytes []byte, // TODO: verify is role can be signed using the presented key. This requires // the user to pass in the delegating role as well as we do not want to // assume which role is the delegating role (diamond delegations are legal). - // See: https://github.com/theupdateframework/specification/issues/19, - // https://github.com/theupdateframework/specification/issues/214, and - // https://github.com/theupdateframework/python-tuf/issues/660. + // See: https://github.com/gittuf/gittuf/issues/246. authorizedKeys := []*tuf.Key{} for _, kb := range authorizedKeysBytes { @@ -147,9 +143,7 @@ func (r *Repository) RemoveDelegation(ctx context.Context, signingKeyBytes []byt // TODO: verify is role can be signed using the presented key. This requires // the user to pass in the delegating role as well as we do not want to // assume which role is the delegating role (diamond delegations are legal). - // See: https://github.com/theupdateframework/specification/issues/19, - // https://github.com/theupdateframework/specification/issues/214, and - // https://github.com/theupdateframework/python-tuf/issues/660. + // See: https://github.com/gittuf/gittuf/issues/246. targetsMetadata, err := state.GetTargetsMetadata(targetsRoleName) if err != nil { @@ -203,9 +197,7 @@ func (r *Repository) AddKeyToTargets(ctx context.Context, signingKeyBytes []byte // TODO: verify is role can be signed using the presented key. This requires // the user to pass in the delegating role as well as we do not want to // assume which role is the delegating role (diamond delegations are legal). - // See: https://github.com/theupdateframework/specification/issues/19, - // https://github.com/theupdateframework/specification/issues/214, and - // https://github.com/theupdateframework/python-tuf/issues/660. + // See: https://github.com/gittuf/gittuf/issues/246. authorizedKeys := []*tuf.Key{} keyIDs := ""