Skip to content

Commit 0b10964

Browse files
rdimitrovclaude
rdimitrov
and
claude
authored
Validate the multi-repo map file before initializing any client (#760)
* Validate the multi-repo map file before initializing any client Follow-up to #750. That PR fixed the out-of-bounds access on a repository with no URL, but left the check inside initTUFClients, where it runs interleaved with client initialization. Move all map file validation into a single validateRepoMap helper that runs before initTUFClients, and extend it: - Repository names and URLs are now validated in one place, so New either returns a fully initialized client or fails without having created cache directories for an arbitrary subset of the repositories. - Repository names are checked in sorted order, so a map file with more than one defect reports the same error on every run instead of depending on map iteration order. - A mapping that references a repository absent from the top-level repositories object is now rejected. Previously it left no TUF client for that name and GetTargetInfo dereferenced a nil *updater.Updater, panicking once a target path matched the mapping. - A null mapping entry and a config with no repository map are rejected for the same reason, rather than panicking. Empty-URL and empty-slice rejection now wraps the exported ErrMissingRepoURL sentinel, matching the existing ErrInvalidRepoName convention so callers can use errors.Is. Adds regression tests for each case; #750 shipped without any. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com> * Reject a nil config in New instead of panicking New dereferenced config.RepoMap without checking the config pointer, so New(nil) panicked. NewConfig returns a nil config alongside its error, so a caller that ignores the error reaches this path. Also move client construction after validation, so the invalid-input paths no longer allocate a client they immediately discard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com> --------- Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f74a11d commit 0b10964

2 files changed

Lines changed: 181 additions & 14 deletions

File tree

‎metadata/multirepo/multirepo.go‎

Lines changed: 66 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"encoding/json"
2222
"errors"
2323
"fmt"
24+
"maps"
2425
"os"
2526
"path/filepath"
2627
"regexp"
@@ -35,6 +36,14 @@ import (
3536
// components or is otherwise invalid for use as a directory name.
3637
var ErrInvalidRepoName = errors.New("invalid repository name")
3738

39+
// ErrMissingRepoURL is returned when a repository listed in the map file has no
40+
// usable URL, i.e. an empty URL list or an empty first URL.
41+
var ErrMissingRepoURL = errors.New("repository has no URL configured")
42+
43+
// ErrUnknownMappingRepo is returned when a mapping in the map file references a
44+
// repository that is not declared in the top-level repositories object.
45+
var ErrUnknownMappingRepo = errors.New("mapping references an unknown repository")
46+
3847
// validRepoNamePattern defines the allowed characters for repository names.
3948
// Names must start with an alphanumeric character and may contain alphanumeric
4049
// characters, dots, hyphens, and underscores. This prevents path traversal
@@ -113,19 +122,23 @@ func NewConfig(repoMap []byte, roots map[string][]byte) (*MultiRepoConfig, error
113122

114123
// New returns a multi-repository TUF client. All repositories described in the provided map file are initialized too
115124
func New(config *MultiRepoConfig) (*MultiRepoClient, error) {
125+
if config == nil {
126+
return nil, fmt.Errorf("no multi-repository config provided")
127+
}
128+
129+
// validate the map file before initializing anything, so that a malformed map
130+
// file is reported as such instead of surfacing later as an obscure
131+
// initialization failure or a panic during target lookup
132+
if err := validateRepoMap(config.RepoMap); err != nil {
133+
return nil, err
134+
}
135+
116136
// create a multi repo client instance
117137
client := &MultiRepoClient{
118138
Config: config,
119139
TUFClients: map[string]*updater.Updater{},
120140
}
121141

122-
// validate repository names before using them as filesystem paths
123-
for repoName := range config.RepoMap.Repositories {
124-
if err := validateRepoName(repoName); err != nil {
125-
return nil, fmt.Errorf("repository %q: %w", repoName, err)
126-
}
127-
}
128-
129142
// create TUF clients for each repository listed in the map file
130143
if err := client.initTUFClients(); err != nil {
131144
return nil, err
@@ -138,13 +151,9 @@ func (client *MultiRepoClient) initTUFClients() error {
138151
log := metadata.GetLogger()
139152

140153
// loop through each repository listed in the map file and initialize it
154+
// note: the map file has already been validated by validateRepoMap, so each
155+
// repository is guaranteed to have a usable URL at index 0
141156
for repoName, repoURL := range client.Config.RepoMap.Repositories {
142-
143-
// Make sure we have at least one repo URL
144-
if len(repoURL) == 0 || repoURL[0] == "" {
145-
return fmt.Errorf("repository %q has no URL configured", repoName)
146-
}
147-
148157
log.Info("Initializing", "name", repoName, "url", repoURL[0])
149158

150159
// get the trusted root file from the location specified in the map file relevant to its path
@@ -393,6 +402,50 @@ func (cfg *MultiRepoConfig) EnsurePathsExist() error {
393402
return nil
394403
}
395404

405+
// validateRepoMap checks that a map file is internally consistent before any
406+
// repository is initialized. It enforces that every repository name is safe to
407+
// use as a filesystem path, that every repository has a usable URL, and that
408+
// every repository referenced by a mapping is actually declared.
409+
//
410+
// Validating up front keeps New atomic: it either returns a fully initialized
411+
// client or fails without having created cache directories for a subset of the
412+
// repositories. It also makes the reported error deterministic, which map
413+
// iteration order alone would not guarantee.
414+
func validateRepoMap(repoMap *MultiRepoMapType) error {
415+
if repoMap == nil {
416+
return fmt.Errorf("no repository map provided")
417+
}
418+
419+
// sort the repository names so that a map file with more than one problem
420+
// always reports the same error rather than a random one
421+
for _, repoName := range slices.Sorted(maps.Keys(repoMap.Repositories)) {
422+
if err := validateRepoName(repoName); err != nil {
423+
return fmt.Errorf("repository %q: %w", repoName, err)
424+
}
425+
426+
// only the first URL is used, as the client supports a single mirror per
427+
// repository for the time being
428+
if repoURL := repoMap.Repositories[repoName]; len(repoURL) == 0 || repoURL[0] == "" {
429+
return fmt.Errorf("repository %q: %w", repoName, ErrMissingRepoURL)
430+
}
431+
}
432+
433+
// every repository named in a mapping must have a corresponding TUF client,
434+
// otherwise GetTargetInfo would dereference a nil client during target lookup
435+
for i, eachMap := range repoMap.Mapping {
436+
if eachMap == nil {
437+
return fmt.Errorf("mapping at index %d is null", i)
438+
}
439+
for _, repoName := range eachMap.Repositories {
440+
if _, ok := repoMap.Repositories[repoName]; !ok {
441+
return fmt.Errorf("mapping at index %d: %w - %s", i, ErrUnknownMappingRepo, repoName)
442+
}
443+
}
444+
}
445+
446+
return nil
447+
}
448+
396449
// validateRepoName checks that a repository name is safe to use as a directory
397450
// component. Repository names must start with an alphanumeric character and
398451
// contain only alphanumeric characters, dots, hyphens, and underscores.

‎metadata/multirepo/multirepo_test.go‎

Lines changed: 115 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,4 +122,118 @@ func TestNewRejectsInvalidRepoNames(t *testing.T) {
122122
}
123123
})
124124
}
125-
}
125+
}
126+
127+
func TestNewRejectsRepositoriesWithoutURL(t *testing.T) {
128+
tests := []struct {
129+
name string
130+
repoURLs string
131+
}{
132+
{"empty URL list", `[]`},
133+
{"null URL list", `null`},
134+
{"empty URL string", `[""]`},
135+
}
136+
137+
for _, tt := range tests {
138+
t.Run(tt.name, func(t *testing.T) {
139+
mapJSON := []byte(`{
140+
"repositories": {
141+
"my-repo": ` + tt.repoURLs + `
142+
},
143+
"mapping": []
144+
}`)
145+
146+
rootBytes := []byte(`{"signatures":[],"signed":{}}`)
147+
148+
cfg, err := NewConfig(mapJSON, map[string][]byte{"my-repo": rootBytes})
149+
if err != nil {
150+
t.Fatalf("NewConfig() unexpected error: %v", err)
151+
}
152+
153+
_, err = New(cfg)
154+
if err == nil {
155+
t.Fatalf("New() should reject repository with URLs %s", tt.repoURLs)
156+
}
157+
158+
if !errors.Is(err, ErrMissingRepoURL) {
159+
t.Errorf("New() error should wrap ErrMissingRepoURL, got: %v", err)
160+
}
161+
})
162+
}
163+
}
164+
165+
func TestNewRejectsMappingWithUnknownRepository(t *testing.T) {
166+
// A mapping that references a repository absent from the top-level
167+
// "repositories" object leaves no TUF client for that name, which makes
168+
// GetTargetInfo dereference a nil *updater.Updater.
169+
mapJSON := []byte(`{
170+
"repositories": {
171+
"real-repo": ["https://example.com/repo"]
172+
},
173+
"mapping": [
174+
{
175+
"paths": ["*"],
176+
"repositories": ["typo-repo"],
177+
"threshold": 1,
178+
"terminating": true
179+
}
180+
]
181+
}`)
182+
183+
rootBytes := []byte(`{"signatures":[],"signed":{}}`)
184+
185+
cfg, err := NewConfig(mapJSON, map[string][]byte{"real-repo": rootBytes})
186+
if err != nil {
187+
t.Fatalf("NewConfig() unexpected error: %v", err)
188+
}
189+
190+
_, err = New(cfg)
191+
if err == nil {
192+
t.Fatal("New() should reject a mapping referencing an unknown repository")
193+
}
194+
195+
if !errors.Is(err, ErrUnknownMappingRepo) {
196+
t.Errorf("New() error should wrap ErrUnknownMappingRepo, got: %v", err)
197+
}
198+
}
199+
200+
func TestNewRejectsNullMapping(t *testing.T) {
201+
// "mapping": [null] unmarshals into a []*Mapping holding a nil element,
202+
// which GetTargetInfo would dereference while walking the mappings.
203+
mapJSON := []byte(`{
204+
"repositories": {
205+
"real-repo": ["https://example.com/repo"]
206+
},
207+
"mapping": [null]
208+
}`)
209+
210+
rootBytes := []byte(`{"signatures":[],"signed":{}}`)
211+
212+
cfg, err := NewConfig(mapJSON, map[string][]byte{"real-repo": rootBytes})
213+
if err != nil {
214+
t.Fatalf("NewConfig() unexpected error: %v", err)
215+
}
216+
217+
_, err = New(cfg)
218+
if err == nil {
219+
t.Fatal("New() should reject a null mapping entry")
220+
}
221+
}
222+
223+
func TestNewRejectsConfigWithoutRepoMap(t *testing.T) {
224+
// MultiRepoConfig has exported fields, so callers can build one directly
225+
// without going through NewConfig and leave RepoMap unset.
226+
_, err := New(&MultiRepoConfig{})
227+
if err == nil {
228+
t.Fatal("New() should reject a config with no repository map")
229+
}
230+
}
231+
232+
func TestNewRejectsNilConfig(t *testing.T) {
233+
// NewConfig returns a nil config alongside its error, so a caller that
234+
// ignores the error passes nil straight into New.
235+
_, err := New(nil)
236+
if err == nil {
237+
t.Fatal("New() should reject a nil config")
238+
}
239+
}

0 commit comments

Comments
 (0)