Fix subdomain matching FIXMEs in commit 373440662 #1366

lsm5 · 2021-08-30T13:51:44Z

FIXMEs addressed:

Signed-off-by: Lokesh Mandvekar [email protected]

lsm5 · 2021-08-30T14:43:07Z

ready for review (afaict)

FIXMEs addressed: 1. containers#1191 (comment) 2. containers#1191 (comment) 3. containers#1191 (comment) 4. containers#1191 (comment) 5. containers#1191 (comment) Signed-off-by: Lokesh Mandvekar <[email protected]>

vrothberg · 2021-08-31T07:42:59Z

Before I start looking at the changes: the commit message isn't really helpful since it merely points to 5 comments "somewhere". The reader has to put the puzzle pieces together of what the changes are intended to do but I really want the author to explain the changes in detail. Since there are 5 different comments, it may be worth splitting them into 5 different commits.

vrothberg · 2021-08-31T07:44:56Z

pkg/sysregistriesv2/system_registries_v2.go

@@ -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 {


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

vrothberg · 2021-08-31T07:45:33Z

pkg/sysregistriesv2/system_registries_v2.go

+	return prefix[:2] == "*."
+}
+
+func EmptyLocationWildcardedPrefixCheck(location string, prefix string) int {


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

vrothberg · 2021-08-31T07:59:11Z

pkg/sysregistriesv2/system_registries_v2.go

-			// 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 != "" {


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,

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.

lsm5 · 2021-08-31T12:45:51Z

Before I start looking at the changes: the commit message isn't really helpful since it merely points to 5 comments "somewhere". The reader has to put the puzzle pieces together of what the changes are intended to do but I really want the author to explain the changes in detail. Since there are 5 different comments, it may be worth splitting them into 5 different commits.

sgtm, will do.

mtrmac · 2021-08-31T14:48:10Z

pkg/sysregistriesv2/system_registries_v2.go

@@ -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 {


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

mtrmac · 2021-08-31T14:49:12Z

pkg/sysregistriesv2/system_registries_v2.go

+	return prefix[:2] == "*."
+}
+
+func EmptyLocationWildcardedPrefixCheck(location string, prefix string) int {


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.

mtrmac · 2021-08-31T14:51:31Z

pkg/sysregistriesv2/system_registries_v2.go

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


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.

mtrmac · 2021-08-31T14:56:27Z

pkg/sysregistriesv2/system_registries_v2.go

-	// and does not prevent correct operation.
-	// https://github.com/containers/image/pull/1191#discussion_r610122617
-	//
-	//	if trimmed == "" {


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

mtrmac · 2021-08-31T14:59:56Z

pkg/sysregistriesv2/system_registries_v2.go

+// 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) {


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

mtrmac · 2021-08-31T15:02:21Z

pkg/sysregistriesv2/system_registries_v2.go

 		}

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


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.

mtrmac · 2021-08-31T15:02:39Z

pkg/sysregistriesv2/system_registries_v2.go

 			mir.Location, err = parseLocation(mir.Location)
 			if err != nil {
 				return err
 			}
-
-			//FIXME: unqualifiedSearchRegistries now also accepts empty values


Has anything been done to fix that?

mtrmac · 2021-08-31T15:02:54Z

pkg/sysregistriesv2/system_registries_v2.go

-	// 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, "/@:") {


What replaces the validation?

lsm5 · 2021-09-01T02:12:33Z

Thanks for all the comments @mtrmac @vrothberg . Closing this. I'll open a separate PR for each FIXME. Starting with: #1368

lsm5 force-pushed the wildcard-cert-fixes branch 5 times, most recently from d66525d to b5b7ba0 Compare August 30, 2021 14:42

lsm5 changed the title ~~[DO NOT MERGE] Fix subdomain matching FIXMEs in commit 373440662~~ Fix subdomain matching FIXMEs in commit 373440662 Aug 30, 2021

Fix subdomain matching FIXMEs in commit 3734406

207f85d

FIXMEs addressed: 1. containers#1191 (comment) 2. containers#1191 (comment) 3. containers#1191 (comment) 4. containers#1191 (comment) 5. containers#1191 (comment) Signed-off-by: Lokesh Mandvekar <[email protected]>

lsm5 force-pushed the wildcard-cert-fixes branch from b5b7ba0 to 207f85d Compare August 30, 2021 14:51

vrothberg reviewed Aug 31, 2021

View reviewed changes

mtrmac reviewed Aug 31, 2021

View reviewed changes

lsm5 closed this Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix subdomain matching FIXMEs in commit 373440662 #1366

Fix subdomain matching FIXMEs in commit 373440662 #1366

lsm5 commented Aug 30, 2021 •

edited

Loading

lsm5 commented Aug 30, 2021

vrothberg commented Aug 31, 2021

vrothberg Aug 31, 2021

vrothberg Aug 31, 2021

vrothberg Aug 31, 2021

mtrmac Aug 31, 2021

lsm5 commented Aug 31, 2021

mtrmac Aug 31, 2021

mtrmac Aug 31, 2021

mtrmac Aug 31, 2021

mtrmac Aug 31, 2021

mtrmac Aug 31, 2021

mtrmac Aug 31, 2021

mtrmac Aug 31, 2021

mtrmac Aug 31, 2021

lsm5 commented Sep 1, 2021

Fix subdomain matching FIXMEs in commit 373440662 #1366

Fix subdomain matching FIXMEs in commit 373440662 #1366

Conversation

lsm5 commented Aug 30, 2021 • edited Loading

lsm5 commented Aug 30, 2021

vrothberg commented Aug 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsm5 commented Aug 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsm5 commented Sep 1, 2021

lsm5 commented Aug 30, 2021 •

edited

Loading