Transport-abstracted signature policy #33

mtrmac · 2016-07-01T03:35:19Z

@runcom PTAL . This is by no means finished, but shows the direction I’m planning to use, early feedback on the API would help me avoid a dead end. Start with types.go I think, I will comment more on the code.

mtrmac · 2016-07-01T03:36:29Z

See also containers/skopeo#138 for the consumer.

mtrmac · 2016-07-01T03:39:54Z

transports.go

+		docker.Transport,
+		oci.Transport,
+		openshift.Transport,
+	} {


This would nicely fit into image, but it needs to be a separate package because these transport-dependent packages depend on image, which would cause a circular dependency.

Adding a containers/image/transports for this small file feels a bit silly, and having the top-level containers/image package point at the contained transports seems to make sense — OTOH using both containers/image and containers/image/image in a single source file then ends up pretty very ugly.

runcom · 2016-07-01T06:47:46Z

Generally, based on comments in the code also, seems like a very well welcome improvement and it makes up space for cleaning other interface parts (like IntentedDockerReference etc etc)

So I'm +1 here

mtrmac · 2016-07-16T03:31:25Z

Updated:

Merges/includes Reference abstraction #39, so that it can be reviewed in pieces. I will rebase as necessary.
Changed naming, now the methods are explicitly PolicyConfigurationIdentity/PolicyConfigurationNamespaces, naming things for what the intent and promised properties are, instead of vague/too general FullyExplicitString.
Now includes the necessary policy configuration updates.
''Note'': oci: now prohibits : characters anywhere in the path, otherwise policy scopes like /some/path:with become ambiguous: is this an image at /some/path with tag :with, or a namespace for images in subdirectories of /some/path:with, using any tag? . I am not sure that this restriction reasonable, we could instead change the policy configuration naming for oci, e.g. to be /full/path/:tag, requiring a /: to separate path and tag.

WIP because I need to re-integrate into mtrmac/image:integrate-all-the-things before being 100% certain, a corresponding skopeo branch is missing, and it seems highly desirable to validate policy scope names when parsing the policy; but it is very unlikely that anything in this PR will change.

mtrmac · 2016-07-18T14:32:17Z

Rebased and ready for individual review; I will add scope name verification when parsing the policy.

mtrmac · 2016-07-18T17:26:27Z

Scope name validation added; works with

make test-skopeo SKOPEO_REPO=mtrmac/skopeo SKOPEO_BRANCH=location-namespaced-signatures

@runcom PTAL

runcom · 2016-07-18T17:39:28Z

oci/oci_transport.go

+	// This is necessary to prevent directory paths returned by PolicyConfigurationNamespaces
+	// from being ambiguous with values of PolicyConfigurationIdentity.
+	if strings.Contains(resolved, ":") {
+		return nil, fmt.Errorf("Invalid OCI reference %s:%s: path %s contains a colon", dir, tag, resolved)


I think this ok now. Eventually this OCI isn't mean for directory like output like dir - that means oci_dest.go will work exactly as docker_img_dest.go works (remote services). I have a TODO to create a new OCITar destination probably which will adhere to this and not allow colon in path.

Does “This is OK now" mean “this naming restriction is acceptable” or “we need to allow such paths”?

acceptable, I don't see why not. OCI_dest right now is kind of the same as dir - OCI image transportation and distribution isn't even yet defined so I treat oci: as dir: and this lgtm

runcom · 2016-07-18T17:50:21Z

The rest LGTM - if I understand correctly the last 2 commits leave some todos around also

runcom · 2016-07-18T17:50:30Z

lgtm

mtrmac · 2016-07-18T17:57:42Z

The rest LGTM - if I understand correctly the last 2 commits leave some todos around also

If you mean cd5e886 , that "docker" lookup is replaced by 77f24aa .

0ac31b2 is left as a todo (or rather, FIXME?), yes.

This makes the core policy lookup logic Docker-independent, with the underlying Docker-specific implementation moved to docker/policyconfiguration/naming.go. Does not change behavior, only docker: and atomic: transports currently implement the policy configuration naming, in a compatible way, and the policy configuration is still not transport-aware. That happens next. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Instead of specific/$dockerreference, use transports/$transport/$policyconfigurationscope This will allow transport-specific policies in the future. This changes the data structures and parsing code, but mostly not yet the functionality; everything is looked up through transports/"docker"/... , and Docker-reference-less transports are not yet supported. The only new functionality is that there is a transport-level default scope (transports/docker/""); at the moment it is redundant with the global default, but soon it will not be so. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Users will be added imminently; this is used to convert an arbitrary path for dir: and oci: image names into a path suitable for policy lookup (absolute, with no symlinks or special components). Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Implement PolicyConfigurationIdentity and PolicyConfigurationNamespaces for all transports, which makes them all possible to use in policy configurations. Then use Transport().Name instead of hard-coded "docker" in policy transport lookup. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

This e.g. rejects relative paths for directories. Notably docker: transport is the only one which does not do any real validation; we could copy&paste the character ranges and lengths directly from docker/distribution/reference, which feels fairly ugly. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac · 2016-07-18T19:12:56Z

Updated:

private.ResolvePathToFullyExplicit →explicitfilepath.ResolvePathToFullyExplicit (explicitfilepath.Resolve seemed too unclear to me)
private.DockerPolicyConfiguration{Identity,Namespaces} → policyconfiguration.DockerReferenceIdentity,Namespaces}
A few trivial error message, comment and commit message improvements.

OK to commit?

runcom · 2016-07-18T19:16:03Z

ACK

mtrmac · 2016-07-18T19:16:58Z

👍

mtrmac mentioned this pull request Jul 1, 2016

Use transport abstraction and transport-abstracted references containers/skopeo#138

Merged

mtrmac reviewed Jul 1, 2016
View reviewed changes

mtrmac force-pushed the location-namespaced-signatures branch 12 times, most recently from 367fd7f to 85a62e2 Compare July 8, 2016 22:32

mtrmac force-pushed the location-namespaced-signatures branch from 85a62e2 to 522f718 Compare July 8, 2016 23:48

mtrmac force-pushed the location-namespaced-signatures branch from 27a97d2 to 8e535e1 Compare July 16, 2016 03:18

mtrmac mentioned this pull request Jul 16, 2016

Reference abstraction #39

Merged

mtrmac changed the title ~~RFC WIP: Transport abstraction and transport-abstracted references, to be transport-abstracted signatures~~ WIP: Transport abstraction and transport-abstracted references, to be transport-abstracted signatures Jul 16, 2016

mtrmac mentioned this pull request Jul 16, 2016

Cleanups #40

Merged

mtrmac changed the title ~~WIP: Transport abstraction and transport-abstracted references, to be transport-abstracted signatures~~ WIP: Transport-abstracted signature policy Jul 16, 2016

mtrmac mentioned this pull request Jul 18, 2016

WIP: Use the "default" policy section for unidentified images #15

Closed

mtrmac force-pushed the location-namespaced-signatures branch from 8e535e1 to 2903ae4 Compare July 18, 2016 14:30

mtrmac changed the title ~~WIP: Transport-abstracted signature policy~~ Transport-abstracted signature policy Jul 18, 2016

mtrmac force-pushed the location-namespaced-signatures branch from 2903ae4 to 0ac31b2 Compare July 18, 2016 17:20

runcom reviewed Jul 18, 2016
View reviewed changes

mtrmac added 2 commits July 18, 2016 20:50

mtrmac force-pushed the location-namespaced-signatures branch from 0ac31b2 to ff31c8d Compare July 18, 2016 19:05

mtrmac added 3 commits July 18, 2016 21:07

mtrmac force-pushed the location-namespaced-signatures branch from ff31c8d to 55bcc37 Compare July 18, 2016 19:08

mtrmac merged commit f9af9e6 into containers:master Jul 18, 2016

mtrmac deleted the location-namespaced-signatures branch July 18, 2016 19:17

Transport-abstracted signature policy #33

Transport-abstracted signature policy #33

Uh oh!

Conversation

mtrmac commented Jul 1, 2016

Uh oh!

mtrmac commented Jul 1, 2016

Uh oh!

mtrmac Jul 1, 2016

Choose a reason for hiding this comment

Uh oh!

runcom commented Jul 1, 2016

Uh oh!

mtrmac commented Jul 16, 2016

Uh oh!

mtrmac commented Jul 18, 2016

Uh oh!

mtrmac commented Jul 18, 2016

Uh oh!

runcom Jul 18, 2016

Choose a reason for hiding this comment

Uh oh!

mtrmac Jul 18, 2016

Choose a reason for hiding this comment

Uh oh!

runcom Jul 18, 2016

Choose a reason for hiding this comment

Uh oh!

runcom commented Jul 18, 2016

Uh oh!

runcom commented Jul 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Jul 18, 2016

Uh oh!

mtrmac commented Jul 18, 2016

Uh oh!

runcom commented Jul 18, 2016

Uh oh!

mtrmac commented Jul 18, 2016 • edited by runcom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

runcom commented Jul 18, 2016 •

edited

Loading

mtrmac commented Jul 18, 2016 •

edited by runcom

Loading