Signing a bundle now imply resolving image references into immutable references #428

simonferquel · 2018-11-15T17:47:22Z

This aims at having signed bundles truly immutable. That means that all referenced images (invocationImages or images) without a digest are modified into digested references:

if an image is available in the docker engine, and it has already a RepoDigest, we use it
if an image is not available in the docker engine, we pull it and get its RepoDigest
if an image is available in the docker engine, but has no RepoDigest, we consider that it has been built locally and has not yet been pushed. In that case we fail, and tell the user to push it.

Signed-off-by: Simon Ferquel simon.ferquel@docker.com

technosophos · 2018-11-15T20:22:01Z

I am very much in favor of this.

technosophos · 2018-11-15T20:23:02Z

make lint failed with a style error. I didn't look too carefully at it.

simonferquel · 2018-11-16T08:30:24Z

Fixed linting issues.
A follow-up PR should be written to make "build" output an unsigned bundle (as build output references freshly built images, not yet in the registry).

Before merging this, I would also like to add a "--push-local-images" flag to the sign subcommand, leading to a "--push-images-and-sign" kind of flag for the build command.

technosophos · 2018-11-16T15:26:56Z

/cc @radu-matei

technosophos · 2018-11-16T15:30:17Z

cmd/duffle/default_user.go

@@ -23,6 +24,12 @@ func defaultUserID() signature.UserID {
 		name = account.Name
 		username = account.Username
 	}
+	// on Windows, account name are prefixed with '<machinename>\' which makes the generated email invalid
+	// and makes the key user identity parser fail
+	if ix := strings.Index(username, "\\"); ix != -1 {


Ah! Glad you caught that. Thanks

pkg/bundle/bundle.go

simonferquel · 2018-11-16T16:35:24Z

PTAL @technosophos

simonferquel · 2018-11-22T17:16:59Z

Rebased and splitted the change to fix user-identity resolution on Windows

Also add an option to the sign command, to push local images to a registry Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

…-registry

jeremyrickard · 2018-11-28T23:44:22Z

@simonferquel I went ahead and updated this to fix the conflicts and resolve the repo move.

I also updated this todo the following:

Updated duffle build so that by default it pushes the images (the build process image resolver defaults to true ) since we desire to have signing happen by default

In summary:

$ duffle bundle sign <...> will fail if an image isn't pushed, but you can tell it to
$ duffle build will default push images to the registry to get the digest and update the bundle before the signing happens

Repeating: When you do a duffle build it will by default push images to your registry since we need that to get the digests

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

bacongobbler · 2018-12-03T16:36:51Z

cmd/duffle/build.go

@@ -134,6 +135,19 @@ func (b *buildCmd) run() (err error) {
 	if err := bldr.Build(ctx, app); err != nil {
 		return err
 	}
+	cli := command.NewDockerCli(os.Stdin, os.Stdout, os.Stderr, false)
+	if err := cli.Initialize(dockerflags.NewClientOptions()); err != nil {


is there a reason we aren't using b.dockerClientOptions here?

bacongobbler · 2018-12-03T16:37:39Z

cmd/duffle/bundle_sign.go

@@ -48,13 +52,20 @@ func newBundleSignCmd(w io.Writer) *cobra.Command {
 			if err != nil {
 				return err
 			}
-			return sign.signBundle(bundle, secring)
+			cli := command.NewDockerCli(os.Stdin, os.Stdout, os.Stderr, false)
+			if err := cli.Initialize(flags.NewClientOptions()); err != nil {


we should probably add all the docker CLI flags similar to duffle build if we're pushing images with duffle bundle sign.

bacongobbler · 2018-12-03T16:59:53Z

I'm not a huge fan of the experience of having to push when building or signing bundles. It breaks the most basic first-time user story, which is pasted all across the documentation:

$ duffle create helloworld
Creating helloworld
$ duffle build .\helloworld\
Step 1/5 : FROM alpine:latest
 ---> 196d12cf6ab1
Step 2/5 : RUN apk add -u bash
 ---> Using cache
 ---> 54b3a85c5c2e
Step 3/5 : COPY Dockerfile /cnab/Dockerfile
 ---> Using cache
 ---> cd6f4ff8d83d
Step 4/5 : COPY app /cnab/app
 ---> Using cache
 ---> 17c792d82fd8
Step 5/5 : CMD ["/cnab/app/run"]
 ---> Using cache
 ---> 918e05577bd7
Successfully built 918e05577bd7
Successfully tagged microsoft/helloworld-cnab:b96538fe33ad4eb65c70e254af58b0f7187ee408
The push refers to repository [docker.io/microsoft/helloworld-cnab]

?[1Bdcd4ba9d: Preparing
?[1B22981d33: Preparing
?[1Bb351a0cb: Preparing
?[1BError: denied: requested access to the resource is denied

In my opinion this is an incredibly poor user experience and we're going to have a very hard time explaining this to first-time users. My view is that given we know there's a technical limitation with how docker's CLI requires pushing to a registry to compute a digest, we should not merge this and instead focus on replacing the docker driver with buildkit as explained in #563. I feel that users being on-boarded to the project would be more understanding about us holding out for a better solution than shoehorning something in last minute that significantly affects the first-time user experience.

squillace · 2018-12-03T18:13:03Z

I've thought a lot about this, because from about almost every technical and usability angle it's a poor story with little defense except for "stuff is this way at the moment". I'm going to suggest that this be closed in favor of something like #563, however tempting it is to move things along. It is just not the right way to do this.

I'll leave the PR open for comments for today, but absent a better argument, I'll close it tomorrow morning. (I hate closing PRs on well-submitted code. booo.)

bacongobbler · 2018-12-06T19:15:27Z

closing in favour of #563.

technosophos reviewed Nov 16, 2018

View reviewed changes

pkg/bundle/bundle.go Show resolved Hide resolved

simonferquel mentioned this pull request Nov 16, 2018

WIP: Push pull to image registry #442

Open

This was referenced Nov 21, 2018

WIP: Adding the rewrite command #263

Closed

WIP Bundle Rewrite #489

Closed

simonferquel and others added 7 commits November 23, 2018 10:01

Sign now verifies that images are digested

cc9fac0

Also add an option to the sign command, to push local images to a registry Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Merge branch 'master' into sign-push-pull-to-registry

c7626dd

Update imports to reflect deislabs move

94d7f16

Merge remote-tracking branch 'upstream/master' into sign-push-pull-to…

ac1ee91

…-registry

Make build fixup images, swap push-local-images to default to true

ee49605

Merge remote-tracking branch 'upstream/master' into sign-push-pull-to…

9ad9144

…-registry

Merge remote-tracking branch 'upstream/master' into sign-push-pull-to…

c9c704e

…-registry

jeremyrickard and others added 5 commits November 28, 2018 16:52

sign will not push by default, build will

887828f

Fix images pushed on other registry

d215698

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Docker Driver can accept an already initialized dockerCli

8616362

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

NewResolver uses an existing CLI

74ab1bd

Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

Fixing Lint

9564289

jeremyrickard added this to In progress in MVP via automation Dec 3, 2018

jeremyrickard moved this from In progress to Needs review in MVP Dec 3, 2018

bacongobbler reviewed Dec 3, 2018

View reviewed changes

jeremyrickard mentioned this pull request Dec 3, 2018

Update duffle documentation to highlight Docker image digests issues #565

Closed

bacongobbler closed this Dec 6, 2018

MVP automation moved this from Needs review to Done Dec 6, 2018

jeremyrickard mentioned this pull request Aug 12, 2019

Investigate using buildkit and containerd for digesting cnabio/cnab-go#102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signing a bundle now imply resolving image references into immutable references #428

Signing a bundle now imply resolving image references into immutable references #428

simonferquel commented Nov 15, 2018

technosophos commented Nov 15, 2018

technosophos commented Nov 15, 2018

simonferquel commented Nov 16, 2018

technosophos commented Nov 16, 2018

technosophos Nov 16, 2018

simonferquel commented Nov 16, 2018

simonferquel commented Nov 22, 2018

jeremyrickard commented Nov 28, 2018 •

edited

Loading

bacongobbler Dec 3, 2018

bacongobbler Dec 3, 2018

bacongobbler commented Dec 3, 2018

squillace commented Dec 3, 2018

bacongobbler commented Dec 6, 2018

Signing a bundle now imply resolving image references into immutable references #428

Signing a bundle now imply resolving image references into immutable references #428

Conversation

simonferquel commented Nov 15, 2018

technosophos commented Nov 15, 2018

technosophos commented Nov 15, 2018

simonferquel commented Nov 16, 2018

technosophos commented Nov 16, 2018

technosophos Nov 16, 2018

Choose a reason for hiding this comment

simonferquel commented Nov 16, 2018

simonferquel commented Nov 22, 2018

jeremyrickard commented Nov 28, 2018 • edited Loading

bacongobbler Dec 3, 2018

Choose a reason for hiding this comment

bacongobbler Dec 3, 2018

Choose a reason for hiding this comment

bacongobbler commented Dec 3, 2018

squillace commented Dec 3, 2018

bacongobbler commented Dec 6, 2018

jeremyrickard commented Nov 28, 2018 •

edited

Loading