diff --git a/internal/tuf/tuf.go b/internal/tuf/tuf.go index 249646ac..5fe9ffcd 100644 --- a/internal/tuf/tuf.go +++ b/internal/tuf/tuf.go @@ -55,6 +55,7 @@ var ( ErrDuplicateNetworkRepository = errors.New("network repository already exists") ErrInvalidPrincipalID = errors.New("principal ID is invalid") ErrInvalidPrincipalType = errors.New("invalid principal type (do you have the right gittuf version?)") + ErrInvalidCustomMetadataKey = errors.New("custom metadata key contains invalid characters") ErrPrincipalNotFound = errors.New("principal not found") ErrPrincipalStillInUse = errors.New("principal is still in use") ErrRuleNotFound = errors.New("cannot find rule entry") diff --git a/internal/tuf/v02/targets.go b/internal/tuf/v02/targets.go index 0b44dbc8..f1d86940 100644 --- a/internal/tuf/v02/targets.go +++ b/internal/tuf/v02/targets.go @@ -331,7 +331,12 @@ func (d *Delegations) addPrincipal(principal tuf.Principal) error { } switch principal := principal.(type) { - case *Key, *Person: + case *Key: + d.Principals[principal.ID()] = principal + case *Person: + if err := principal.Validate(); err != nil { + return err + } d.Principals[principal.ID()] = principal default: return tuf.ErrInvalidPrincipalType @@ -353,7 +358,12 @@ func (d *Delegations) updatePrincipal(principal tuf.Principal) error { } switch principal := principal.(type) { - case *Key, *Person: + case *Key: + d.Principals[principalID] = principal + case *Person: + if err := principal.Validate(); err != nil { + return err + } d.Principals[principalID] = principal default: return tuf.ErrInvalidPrincipalType diff --git a/internal/tuf/v02/targets_test.go b/internal/tuf/v02/targets_test.go index 1f47ead9..9b607ec3 100644 --- a/internal/tuf/v02/targets_test.go +++ b/internal/tuf/v02/targets_test.go @@ -650,3 +650,30 @@ func TestAllowRule(t *testing.T) { assert.Empty(t, allowRule.PrincipalIDs) assert.Equal(t, 1, allowRule.Threshold) } + +func TestAddAndUpdatePrincipalValidatesPerson(t *testing.T) { + targetsMetadata := initialTestTargetsMetadata(t) + key := NewKeyFromSSLibKey(ssh.NewKeyFromBytes(t, targets1PubKeyBytes)) + + invalidPerson := &Person{ + PersonID: "jane.doe", + PublicKeys: map[string]*Key{key.KeyID: key}, + Custom: map[string]string{"invalid key": "value"}, + } + + err := targetsMetadata.AddPrincipal(invalidPerson) + assert.ErrorIs(t, err, tuf.ErrInvalidCustomMetadataKey) + assert.NotContains(t, targetsMetadata.Delegations.Principals, invalidPerson.PersonID) + + validPerson := &Person{ + PersonID: "jane.doe", + PublicKeys: map[string]*Key{key.KeyID: key}, + Custom: map[string]string{"department": "engineering"}, + } + if err := targetsMetadata.AddPrincipal(validPerson); err != nil { + t.Fatal(err) + } + + err = targetsMetadata.UpdatePrincipal(invalidPerson) + assert.ErrorIs(t, err, tuf.ErrInvalidCustomMetadataKey) +} diff --git a/internal/tuf/v02/tuf.go b/internal/tuf/v02/tuf.go index 2d94f166..24c30635 100644 --- a/internal/tuf/v02/tuf.go +++ b/internal/tuf/v02/tuf.go @@ -9,12 +9,18 @@ package v02 import ( "fmt" + "regexp" "github.com/gittuf/gittuf/internal/common/set" + "github.com/gittuf/gittuf/internal/tuf" v01 "github.com/gittuf/gittuf/internal/tuf/v01" "github.com/secure-systems-lab/go-securesystemslib/signerverifier" ) +// customMetadataKeyRegexp restricts custom metadata keys to a conservative +// character set. It excludes spaces and parentheses. +var customMetadataKeyRegexp = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._/-]*$`) + const ( associatedIdentityKey = "(associated identity)" ) @@ -71,6 +77,17 @@ func (p *Person) CustomMetadata() map[string]string { return metadata } +// Validate checks that the person is well-formed. +func (p *Person) Validate() error { + for key := range p.Custom { + if !customMetadataKeyRegexp.MatchString(key) { + return fmt.Errorf("%w: %q", tuf.ErrInvalidCustomMetadataKey, key) + } + } + + return nil +} + // Role records common characteristics recorded in a role entry in Root metadata // and in a delegation entry. type Role struct { diff --git a/internal/tuf/v02/tuf_test.go b/internal/tuf/v02/tuf_test.go index e56514b9..bb85b526 100644 --- a/internal/tuf/v02/tuf_test.go +++ b/internal/tuf/v02/tuf_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/gittuf/gittuf/internal/signerverifier/ssh" + "github.com/gittuf/gittuf/internal/tuf" "github.com/secure-systems-lab/go-securesystemslib/signerverifier" "github.com/stretchr/testify/assert" ) @@ -102,3 +103,55 @@ func TestPerson(t *testing.T) { assert.Equal(t, test.expectedCustomMetadata, customMetadata, fmt.Sprintf("unexpected custom metadata in test '%s'", name)) } } + +func TestPersonValidate(t *testing.T) { + keyR := ssh.NewKeyFromBytes(t, rootPubKeyBytes) + key := NewKeyFromSSLibKey(keyR) + + newPerson := func(custom map[string]string) *Person { + return &Person{ + PersonID: "jane.doe", + PublicKeys: map[string]*Key{ + key.KeyID: key, + }, + AssociatedIdentities: map[string]string{ + "https://github.com": "jane.doe", + }, + Custom: custom, + } + } + + rejected := map[string]string{ + "colliding associated identity key": fmt.Sprintf("%s https://github.com", associatedIdentityKey), + "reserved prefix alone": associatedIdentityKey, + "contains a space": "some key", + "contains an opening parenthesis": "key(1)", + "contains a newline": "key\nreserved value", + "trailing newline": "key\n", + "empty key": "", + } + + for name, badKey := range rejected { + t.Run("rejected: "+name, func(t *testing.T) { + person := newPerson(map[string]string{badKey: "value"}) + + err := person.Validate() + assert.ErrorIs(t, err, tuf.ErrInvalidCustomMetadataKey) + }) + } + + accepted := map[string]string{ + "simple word": "department", + "dotted namespace": "example.com/team", + "digits and dashes": "cost-center-42", + "underscores": "internal_id", + } + + for name, goodKey := range accepted { + t.Run("accepted: "+name, func(t *testing.T) { + person := newPerson(map[string]string{goodKey: "value"}) + + assert.NoError(t, person.Validate()) + }) + } +} diff --git a/pkg/rsl/repository_test.go b/pkg/rsl/repository_test.go index 393f1885..eaa8e97f 100644 --- a/pkg/rsl/repository_test.go +++ b/pkg/rsl/repository_test.go @@ -2069,3 +2069,55 @@ func TestAnnotationEntryCommitStorerErrors(t *testing.T) { assert.ErrorIs(t, err, injected) }) } + +func TestCommitWithoutNumberIgnoresNumber(t *testing.T) { + t.Run("reference entry", func(t *testing.T) { + repo := gitinterface.CreateTestGitRepository(t, t.TempDir(), false) + + entry := &ReferenceEntry{RefName: "refs/heads/main", TargetID: gitinterface.ZeroHash, Number: 42} + require.Nil(t, entry.CommitWithoutNumber(repo)) + + got, err := GetLatestEntry(repo) + require.Nil(t, err) + assert.Equal(t, uint64(0), got.GetNumber()) + }) + + t.Run("annotation entry", func(t *testing.T) { + repo := gitinterface.CreateTestGitRepository(t, t.TempDir(), false) + require.Nil(t, NewReferenceEntry("refs/heads/main", gitinterface.ZeroHash).CommitWithoutNumber(repo)) + + refEntry, err := GetLatestEntry(repo) + require.Nil(t, err) + + annotation := &AnnotationEntry{RSLEntryIDs: []githash.Hash{refEntry.GetID()}, Skip: false, Message: annotationMessage, Number: 42} + require.Nil(t, annotation.CommitWithoutNumber(repo)) + + got, err := GetLatestEntry(repo) + require.Nil(t, err) + assert.Equal(t, uint64(0), got.GetNumber()) + }) +} + +func TestCommitRejectsLineBreakInField(t *testing.T) { + repo := gitinterface.CreateTestGitRepository(t, t.TempDir(), false) + + t.Run("reference entry Commit", func(t *testing.T) { + err := NewReferenceEntry("refs/heads/main\ntargetID: injected", gitinterface.ZeroHash).Commit(repo, false) + assert.ErrorIs(t, err, ErrInvalidRSLEntry) + }) + + t.Run("reference entry CommitWithoutNumber", func(t *testing.T) { + err := NewReferenceEntry("refs/heads/main\ntargetID: injected", gitinterface.ZeroHash).CommitWithoutNumber(repo) + assert.ErrorIs(t, err, ErrInvalidRSLEntry) + }) + + t.Run("reference entry CommitUsingSpecificKey", func(t *testing.T) { + err := NewReferenceEntry("refs/heads/main\ntargetID: injected", gitinterface.ZeroHash).CommitUsingSpecificKey(repo, artifacts.SSHED25519Private) + assert.ErrorIs(t, err, ErrInvalidRSLEntry) + }) + + t.Run("propagation entry Commit", func(t *testing.T) { + err := NewPropagationEntry("refs/heads/main", gitinterface.ZeroHash, "https://example.com/repo\ninjected", gitinterface.ZeroHash).Commit(repo, false) + assert.ErrorIs(t, err, ErrInvalidRSLEntry) + }) +} diff --git a/pkg/rsl/rsl.go b/pkg/rsl/rsl.go index b8024aec..59c3852f 100644 --- a/pkg/rsl/rsl.go +++ b/pkg/rsl/rsl.go @@ -147,10 +147,12 @@ func (e *ReferenceEntry) Commit(storer gitstore.Storer, sign bool) error { return err } - message, _ := e.createCommitMessage(true) // we have an error return for annotations, always nil here + message, err := e.createCommitMessage(true) + if err != nil { + return err + } - err := commitEntry(storer, message, sign) - return err + return commitEntry(storer, message, sign) } // CommitUsingSpecificKey creates a commit object in the RSL for the @@ -165,10 +167,12 @@ func (e *ReferenceEntry) CommitUsingSpecificKey(storer gitstore.Storer, signingK return err } - message, _ := e.createCommitMessage(true) // we have an error return for annotations, always nil here + message, err := e.createCommitMessage(true) + if err != nil { + return err + } - err := commitEntryUsingSpecificKey(storer, message, signingKeyBytes) - return err + return commitEntryUsingSpecificKey(storer, message, signingKeyBytes) } func (e *ReferenceEntry) GetNumber() uint64 { @@ -203,7 +207,17 @@ func (e *ReferenceEntry) setEntryNumber(storer gitstore.Storer) error { return nil } +func checkSingleLineField(value string) error { + if strings.ContainsAny(value, "\n\r") { + return fmt.Errorf("%w: field value contains a line break", ErrInvalidRSLEntry) + } + return nil +} + func (e *ReferenceEntry) createCommitMessage(includeNumber bool) (string, error) { + if err := checkSingleLineField(e.RefName); err != nil { + return "", err + } lines := []string{ ReferenceEntryHeader, "", @@ -220,10 +234,12 @@ func (e *ReferenceEntry) createCommitMessage(includeNumber bool) (string, error) // producing a legacy unnumbered entry. It exists to exercise the RSL's support // for repositories that transition from unnumbered to numbered entries. func (e *ReferenceEntry) CommitWithoutNumber(storer gitstore.Storer) error { - message, _ := e.createCommitMessage(true) // we have an error return for annotations, always nil here + message, err := e.createCommitMessage(false) + if err != nil { + return err + } - err := commitEntry(storer, message, false) - return err + return commitEntry(storer, message, false) } // AnnotationEntry is a type of RSL record that references prior items in the @@ -389,7 +405,7 @@ func (a *AnnotationEntry) CommitWithoutNumber(storer gitstore.Storer) error { } } - message, err := a.createCommitMessage(true) + message, err := a.createCommitMessage(false) if err != nil { return err } @@ -455,10 +471,12 @@ func (e *PropagationEntry) Commit(storer gitstore.Storer, sign bool) error { return err } - message, _ := e.createCommitMessage(true) // we have an error return for annotations, always nil here + message, err := e.createCommitMessage(true) + if err != nil { + return err + } - err := commitEntry(storer, message, sign) - return err + return commitEntry(storer, message, sign) } // CommitUsingSpecificKey creates a commit object in the RSL for the @@ -473,10 +491,12 @@ func (e *PropagationEntry) CommitUsingSpecificKey(storer gitstore.Storer, signin return err } - message, _ := e.createCommitMessage(true) // we have an error return for annotations, always nil here + message, err := e.createCommitMessage(true) + if err != nil { + return err + } - err := commitEntryUsingSpecificKey(storer, message, signingKeyBytes) - return err + return commitEntryUsingSpecificKey(storer, message, signingKeyBytes) } func (e PropagationEntry) GetNumber() uint64 { @@ -500,6 +520,12 @@ func (e *PropagationEntry) setEntryNumber(storer gitstore.Storer) error { } func (e *PropagationEntry) createCommitMessage(includeNumber bool) (string, error) { + if err := checkSingleLineField(e.RefName); err != nil { + return "", err + } + if err := checkSingleLineField(e.UpstreamRepository); err != nil { + return "", err + } lines := []string{ PropagationEntryHeader, "", diff --git a/pkg/rsl/rsl_test.go b/pkg/rsl/rsl_test.go index 0c3f5fdc..aa33486d 100644 --- a/pkg/rsl/rsl_test.go +++ b/pkg/rsl/rsl_test.go @@ -451,6 +451,66 @@ func TestParseRSLEntryTextRejectsMalformed(t *testing.T) { } } +func TestReferenceEntryRefNameWithNewline(t *testing.T) { + t.Parallel() + + targetID, err := NewHash("abcdef12345678900987654321fedcbaabcdef12") + if err != nil { + t.Fatal(err) + } + + refNames := map[string]string{ + "forged targetID": fmt.Sprintf("refs/heads/main\n%s: %s", TargetIDKey, githash.ZeroHash.String()), + "forged number": fmt.Sprintf("refs/heads/main\n%s: 999", NumberKey), + "bare trailing newline": "refs/heads/main\n", + "carriage return": "refs/heads/main\rrefs/heads/other", + } + + for name, refName := range refNames { + t.Run(name, func(t *testing.T) { + t.Parallel() + + entry := &ReferenceEntry{RefName: refName, TargetID: targetID} + _, err := entry.createCommitMessage(true) + assert.ErrorIs(t, err, ErrInvalidRSLEntry) + }) + } +} + +func TestPropagationEntryFieldWithNewline(t *testing.T) { + t.Parallel() + + targetID, err := NewHash("abcdef12345678900987654321fedcbaabcdef12") + if err != nil { + t.Fatal(err) + } + + base := PropagationEntry{ + RefName: "refs/heads/main", + TargetID: targetID, + UpstreamRepository: "https://git.example.com/example/repository", + UpstreamEntryID: githash.ZeroHash, + } + + t.Run("ref name with newline", func(t *testing.T) { + t.Parallel() + + entry := base + entry.RefName = fmt.Sprintf("refs/heads/main\n%s: 999", NumberKey) + _, err := entry.createCommitMessage(true) + assert.ErrorIs(t, err, ErrInvalidRSLEntry) + }) + + t.Run("upstream repository with newline", func(t *testing.T) { + t.Parallel() + + entry := base + entry.UpstreamRepository = fmt.Sprintf("https://git.example.com/repo\n%s: 999", NumberKey) + _, err := entry.createCommitMessage(true) + assert.ErrorIs(t, err, ErrInvalidRSLEntry) + }) +} + func TestParseRSLEntryTextForwardCompatibility(t *testing.T) { t.Parallel()