Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix subdomain matching FIXMEs in commit 373440662 #1366

Closed
wants to merge 1 commit into from
Closed

Fix subdomain matching FIXMEs in commit 373440662 #1366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 52 additions & 48 deletions pkg/sysregistriesv2/system_registries_v2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,10 @@ func (e *Endpoint) rewriteReference(ref reference.Named, prefix string) (referen
}
// In the case of an empty `location` field, simply return the original
// input ref as-is.
//
// FIXME: already validated in postProcessRegistries, so check can probably
// be dropped.
// https://github.com/containers/image/pull/1191#discussion_r610621608
if e.Location == "" {
if prefix[:2] != "*." {
return nil, fmt.Errorf("invalid prefix '%v' for empty location, should be in the format: *.example.com", prefix)
}
locationPrefixCheck := EmptyLocationWildcardedPrefixCheck(e.Location, prefix)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FIXME was to drop the check for prefix starting with "*." in this location, (after ensuring postProcessRegistries enforces that constraint), not to keep exactly the same check and move it into a single-use function. That move, along with the undocumented 0/1/-1 values, just makes this harder to read with AFAICS no change to behavior.

if locationPrefixCheck == -1 {
return nil, fmt.Errorf("invalid prefix '%v' for empty location, should be in the format: *.example.com", prefix)
} else if locationPrefixCheck == 1 {
return ref, nil
}
newNamedRef = e.Location + refString[prefixLen:]
Expand Down Expand Up @@ -271,16 +267,6 @@ func (e *InvalidRegistries) Error() string {
func parseLocation(input string) (string, error) {
trimmed := strings.TrimRight(input, "/")

// FIXME: This check needs to exist but fails for empty Location field with
// wildcarded prefix. Removal of this check "only" allows invalid input in,
// and does not prevent correct operation.
// https://github.com/containers/image/pull/1191#discussion_r610122617
//
// if trimmed == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request around #1191 (comment) was to reintroduce this check, and handle the cases that do allow "" into the callers . See more in there.

// return "", &InvalidRegistries{s: "invalid location: cannot be empty"}
// }
//

if strings.HasPrefix(trimmed, "http://") || strings.HasPrefix(trimmed, "https://") {
msg := fmt.Sprintf("invalid location '%s': URI schemes are not supported", input)
return "", &InvalidRegistries{s: msg}
Expand All @@ -289,6 +275,20 @@ func parseLocation(input string) (string, error) {
return trimmed, nil
}

// parsePrefix parses the input string, performs some sanity checks and returns
// the sanitized input string. An error is returned if the input string is
// empty or if contains an "http{s,}://" prefix.
func parsePrefix(input string) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this differ from parseLocation now? Only in the error message? IIRC location was supposed to reject wildcards, and the like…

trimmed := strings.TrimRight(input, "/")

if strings.HasPrefix(trimmed, "http://") || strings.HasPrefix(trimmed, "https://") {
msg := fmt.Sprintf("invalid prefix '%s': URI schemes are not supported", input)
return "", &InvalidRegistries{s: msg}
}

return trimmed, nil
}

// ConvertToV2 returns a v2 config corresponding to a v1 one.
func (config *V1RegistriesConf) ConvertToV2() (*V2RegistriesConf, error) {
regMap := make(map[string]*Registry)
Expand Down Expand Up @@ -343,6 +343,21 @@ func (config *V1RegistriesConf) ConvertToV2() (*V2RegistriesConf, error) {
// anchoredDomainRegexp is an internal implementation detail of postProcess, defining the valid values of elements of UnqualifiedSearchRegistries.
var anchoredDomainRegexp = regexp.MustCompile("^" + reference.DomainRegexp.String() + "$")

// WildcardedPrefixCheck is the only place to check if a prefix contains "*."
func WildcardedPrefixCheck(prefix string) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this private. We try to keep new public APIs at the bare minimum to prevent bumping the major version.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically (not quite always), it’s more readable if function names are either commands (saveFile) or predicates (isPrime).

return prefix[:2] == "*."
}

func EmptyLocationWildcardedPrefixCheck(location string, prefix string) int {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Please add a comment on the return value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think a comment is enough — it should either be structured in some completely different way, or use an enum, not hard-coded 0/1/-1.

if location == "" {
if !(WildcardedPrefixCheck(prefix)) {
return -1
}
return 1
}
return 0
}

// postProcess checks the consistency of all the configuration, looks for conflicts,
// and normalizes the configuration (e.g., sets the Prefix to Location if not set).
func (config *V2RegistriesConf) postProcessRegistries() error {
Expand All @@ -357,36 +372,36 @@ func (config *V2RegistriesConf) postProcessRegistries() error {
return err
}

if reg.Prefix == "" {
if reg.Location == "" {
return &InvalidRegistries{s: "invalid condition: both location and prefix are unset"}
}
reg.Prefix = reg.Location
} else {
reg.Prefix, err = parseLocation(reg.Prefix)
if reg.Prefix != "" {
reg.Prefix, err = parsePrefix(reg.Prefix)
if err != nil {
return err
}
// FIXME: allow config authors to always use Prefix.
// https://github.com/containers/image/pull/1191#discussion_r610622495
if reg.Prefix[:2] != "*." && reg.Location == "" {
return &InvalidRegistries{s: "invalid condition: location is unset and prefix is not in the format: *.example.com"}
if reg.Location != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks suspicious. Why are we allowing a config with an empty location?

If that's desired, we need tests for that case and probably some sanity checks and some massaging of the docs. For instance, unless it's blocked, it would require at least one mirror to make sense which in turn would bring up the question: why lift the requirement to specify a location,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See all the “prefix is a wildcard” and “prefix is empty” conditions in https://github.com/openshift/runtime-utils/pull/11/files EditRegistriesConfig.

Right now, if the user wants to set properties (insecure/blocked/mirrors) on a registry without redirecting it, there are two different syntaxes: (prefix=*.domain, location="") or (prefix="", location="non-wildcard.domain/…"), with little reason to for the two to differ. It would conceptually be much simpler if we always required prefix to exist as a lookup key, and made location entirely optional for redirection only; we can’t do that any more due to compatibility, but at least allowing software that writes entries from scratch to always use prefix could simplify it.

reg.Location, err = parseLocation(reg.Location)
if err != nil {
return err
}
}
} else if reg.Location != "" {
reg.Prefix, err = parseLocation(reg.Location)
if err != nil {
return err
}
reg.Location = reg.Prefix
} else {
return &InvalidRegistries{s: "invalid condition: both location and prefix are unset"}
}

// make sure mirrors are valid
for _, mir := range reg.Mirrors {
if mir.Location == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I don’t see that the caller of parseLocation should need to specifically reject ""
  • Moving the check for "" before parseLocation changes semantics; "/" was previously rejected, now it is allowed.

return &InvalidRegistries{s: "invalid condition: mirror location is unset"}
}
mir.Location, err = parseLocation(mir.Location)
if err != nil {
return err
}

//FIXME: unqualifiedSearchRegistries now also accepts empty values
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has anything been done to fix that?

//and shouldn't
// https://github.com/containers/image/pull/1191#discussion_r610623216
if mir.Location == "" {
return &InvalidRegistries{s: "invalid condition: mirror location is unset"}
}
}
if reg.Location == "" {
regMap[reg.Prefix] = append(regMap[reg.Prefix], reg)
Expand Down Expand Up @@ -804,7 +819,7 @@ func refMatchingSubdomainPrefix(ref, prefix string) int {
// (This is split from the caller primarily to make testing easier.)
func refMatchingPrefix(ref, prefix string) int {
switch {
case prefix[0:2] == "*.":
case WildcardedPrefixCheck(prefix):
return refMatchingSubdomainPrefix(ref, prefix)
case len(ref) < len(prefix):
return -1
Expand Down Expand Up @@ -919,17 +934,6 @@ func loadConfigFile(path string, forceV2 bool) (*parsedConfig, error) {
res.shortNameMode = types.ShortNameModeInvalid
}

// Valid wildcarded prefixes must be in the format: *.example.com
// FIXME: Move to postProcessRegistries
// https://github.com/containers/image/pull/1191#discussion_r610623829
for i := range res.partialV2.Registries {
prefix := res.partialV2.Registries[i].Prefix
if prefix[:2] == "*." && strings.ContainsAny(prefix, "/@:") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What replaces the validation?

msg := fmt.Sprintf("Wildcarded prefix should be in the format: *.example.com. Current prefix %q is incorrectly formatted", prefix)
return nil, &InvalidRegistries{s: msg}
}
}

// Parse and validate short-name aliases.
cache, err := newShortNameAliasCache(path, &res.partialV2.shortNameAliasConf)
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion pkg/sysregistriesv2/system_registries_v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,8 @@ func TestInvalidV2Configs(t *testing.T) {
{"testdata/insecure-conflicts.conf", "registry 'registry.com' is defined multiple times with conflicting 'insecure' setting"},
{"testdata/blocked-conflicts.conf", "registry 'registry.com' is defined multiple times with conflicting 'blocked' setting"},
{"testdata/missing-mirror-location.conf", "invalid condition: mirror location is unset"},
{"testdata/invalid-prefix.conf", "invalid location"},
{"testdata/invalid-location.conf", "invalid location"},
{"testdata/invalid-prefix.conf", "invalid prefix"},
{"testdata/this-does-not-exist.conf", "no such file or directory"},
} {
_, err := GetRegistries(&types.SystemContext{SystemRegistriesConfPath: c.path})
Expand Down
13 changes: 13 additions & 0 deletions pkg/sysregistriesv2/testdata/invalid-location.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
[[registry]]
location = "http://registry.com:5000"
prefix = "http://schema-is-invalid.com"

[[registry]]
location = "https://registry.com:5000"
prefix = "http://schema-is-invalid.com"

[[registry]]
location = "http://registry.com:5000"

[[registry]]
location = "https://registry.com:5000"