rsl, tuf: stricter validation for entry and metadata fields

Reject line breaks in RSL entry ref names and upstream repository fields
on write. Restrict Person custom metadata keys to a conservative
character set.

Assisted-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Paulo Gomes <paulo@entire.io>

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")

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

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)
+}

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 {

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())
+		})
+	}
+}

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)
+	})
+}

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,
 		"",

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()