registry/handles/app: always append default urls regexps #2035

runcom · 2016-11-02T14:20:14Z

Even when validation is off, we don't want to allow manifests with foreign layers urls.

Signed-off-by: Antonio Murdaca [email protected]

codecov-io · 2016-11-02T14:36:54Z

Current coverage is 51.10% (diff: 100%)

Merging #2035 into master will decrease coverage by 8.72%

@@             master      #2035   diff @@
==========================================
  Files           128        128           
  Lines         11402      11404      +2   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
- Hits           6822       5828    -994   
- Misses         3714       4825   +1111   
+ Partials        866        751    -115

Powered by Codecov. Last update 314144a...0fb25dd

aaronlehmann · 2016-11-03T17:45:06Z

Hi @runcom - I feel like I'm missing some context here. Why should foreign layer URLs be prohibited when validation is disabled? It seems a bit counter-intuitive.

runcom · 2016-11-03T17:46:59Z

Hi @runcom - I feel like I'm missing some context here. Why should foreign layer URLs be prohibited when validation is disabled? It seems a bit counter-intuitive.

I know, but we didn't want default installation to allow layers urls, did we?

aaronlehmann · 2016-11-03T17:48:58Z

I wonder why validation is not enabled by default.

runcom · 2016-11-03T17:51:55Z

I wonder why validation is not enabled by default.

I don't have any clue, great question.

runcom · 2016-11-30T21:17:06Z

I wonder why validation is not enabled by default.

does anyone have any idea on @aaronlehmann thought?

dmcgowan · 2016-11-30T21:43:51Z

So this change is effectively changing the default behavior from support ".*" in the URLs to support "$^", not sure if anyone would be effected by this. This would also mean if you want ".*" for some reason, you should have to configure it using validation.

I know, but we didn't want default installation to allow layers urls, did we?

I also don't know if this was an intentional decision, can ping @stevvooe as well. Either way let's make that decision now. I think I agree that it is better to be more restrictive by default with foreign URLs since it has the potential for malicious use.

stevvooe · 2016-11-30T21:46:56Z

registry/handlers/app.go

@@ -244,6 +244,8 @@ func NewApp(ctx context.Context, config *configuration.Configuration) *App {
 				options = append(options, storage.ManifestURLsDenyRegexp(re))
 			}
 		}
+	} else {
+		options = append(options, storage.ManifestURLsAllowRegexp(regexp.MustCompile("^$")))


Didn't realize we just plopped all this in the middle of a function. Sigh.

Can we refactor this later but at least agree on whether we should make this change or not....

@runcom I'm just lamenting. Let's not make big changes in this PR.

stevvooe · 2016-11-30T22:01:20Z

@dmcgowan @runcom The backwards compatibility policy would dictate that we continue to be relaxed about extra fields.

Let's introduce a Validation.Disabled field, which should be favored over Validation.Enabled and transition this to the default:

if !Validation.Enabled {
  Validation.Enabled = !Validation.Disabled
}

if Validation.Enabled {
 // default validation, with or without entries
} else {
  // no validation
}

There may be a way to simplify that...

runcom · 2016-12-01T08:31:33Z

@dmcgowan @aaronlehmann @stevvooe PTAL

stevvooe · 2016-12-02T03:00:48Z

configuration/configuration.go

@@ -190,6 +190,8 @@ type Configuration struct {
 	Validation struct {
 		// Enabled enables the other options in this section.


Add a deprecated note here.

stevvooe · 2016-12-02T03:01:26Z

LGTM

May need more to let people know about the deprecation.

dmcgowan · 2016-12-05T18:17:20Z

LGTM

Could we add note in the ### Disabled doc section about it deprecating enabled?

runcom · 2016-12-05T18:50:13Z

@dmcgowan updated

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2016-12-06T14:04:21Z

is this ready to go?

dmcgowan · 2017-04-27T18:00:29Z

Note for 2.7 change list,
Existing configurations configuration which had set Enabled: false will need to be updated to do Disabled: true to keep the validation disabled. This needs to be put in the upgrade notes

stanhu · 2019-01-03T00:33:08Z

I just saw this issue after creating #2795.
This was confusing because our configuration had omitted enabled from the config, which I had erroneously assumed was on by default. We added disabled, and all was well again.

GordonTheTurtle added the status/0-triage label Nov 2, 2016

runcom mentioned this pull request Nov 2, 2016

distribution, layer: extend external layer urls to other os moby/moby#27961

Closed

dmcgowan added status/1-design-review and removed status/0-triage labels Nov 23, 2016

stevvooe reviewed Nov 30, 2016

View reviewed changes

runcom force-pushed the fix-foreign-urls-check branch from 9ec2a07 to d67e749 Compare December 1, 2016 08:31

stevvooe reviewed Dec 2, 2016

View reviewed changes

runcom force-pushed the fix-foreign-urls-check branch from d67e749 to fde1d11 Compare December 3, 2016 08:27

dmcgowan added status/2-code-review and removed status/1-design-review labels Dec 5, 2016

runcom force-pushed the fix-foreign-urls-check branch from fde1d11 to b81eb3a Compare December 5, 2016 18:45

dmcgowan added status/4-merge and removed status/2-code-review labels Dec 5, 2016

registry/handles/app: always append default urls regexps

0fb25dd

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the fix-foreign-urls-check branch from b81eb3a to 0fb25dd Compare December 5, 2016 19:28

dmcgowan merged commit 67095fb into distribution:master Dec 6, 2016

runcom deleted the fix-foreign-urls-check branch December 6, 2016 18:22

dmcgowan added the impact/changelog label Apr 27, 2017

dmcgowan added the documentation label Apr 27, 2017

stanhu mentioned this pull request Jan 3, 2019

docker/distribution 2.7.0 does not accept foreign layers by default #2795

Closed

mattmoor mentioned this pull request Dec 9, 2020

Fix multi-arch ko build in CI mattmoor/mink#342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

registry/handles/app: always append default urls regexps #2035

registry/handles/app: always append default urls regexps #2035

runcom commented Nov 2, 2016

codecov-io commented Nov 2, 2016 •

edited

Loading

aaronlehmann commented Nov 3, 2016

runcom commented Nov 3, 2016

aaronlehmann commented Nov 3, 2016

runcom commented Nov 3, 2016

runcom commented Nov 30, 2016

dmcgowan commented Nov 30, 2016 •

edited

Loading

stevvooe Nov 30, 2016

runcom Nov 30, 2016 •

edited

Loading

stevvooe Nov 30, 2016

stevvooe commented Nov 30, 2016

runcom commented Dec 1, 2016

stevvooe Dec 2, 2016

runcom Dec 3, 2016

stevvooe commented Dec 2, 2016

dmcgowan commented Dec 5, 2016

runcom commented Dec 5, 2016

runcom commented Dec 6, 2016

dmcgowan commented Apr 27, 2017

stanhu commented Jan 3, 2019

		@@ -190,6 +190,8 @@ type Configuration struct {
		Validation struct {
		// Enabled enables the other options in this section.

registry/handles/app: always append default urls regexps #2035

registry/handles/app: always append default urls regexps #2035

Conversation

runcom commented Nov 2, 2016

codecov-io commented Nov 2, 2016 • edited Loading

Current coverage is 51.10% (diff: 100%)

aaronlehmann commented Nov 3, 2016

runcom commented Nov 3, 2016

aaronlehmann commented Nov 3, 2016

runcom commented Nov 3, 2016

runcom commented Nov 30, 2016

dmcgowan commented Nov 30, 2016 • edited Loading

stevvooe Nov 30, 2016

Choose a reason for hiding this comment

runcom Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

stevvooe Nov 30, 2016

Choose a reason for hiding this comment

stevvooe commented Nov 30, 2016

runcom commented Dec 1, 2016

stevvooe Dec 2, 2016

Choose a reason for hiding this comment

runcom Dec 3, 2016

Choose a reason for hiding this comment

stevvooe commented Dec 2, 2016

dmcgowan commented Dec 5, 2016

runcom commented Dec 5, 2016

runcom commented Dec 6, 2016

dmcgowan commented Apr 27, 2017

stanhu commented Jan 3, 2019

codecov-io commented Nov 2, 2016 •

edited

Loading

dmcgowan commented Nov 30, 2016 •

edited

Loading

runcom Nov 30, 2016 •

edited

Loading