container driver: copy ca and user tls registries certs #787

crazy-max · 2021-09-29T15:43:24Z

Fixes docker/build-push-action#436
Fixes moby/buildkit#1055
Fixes #567
Fixes #80
Fixes #306
Fixes #455

Follow-up moby/buildkit#2361 and moby/buildkit#2377

Since moby/buildkit#1410 we can provide rootCA and keyPairs for registries but we have to copy those certs in the container after the builder is created.

This PR allows to copy certificates to the container when a docker-container builder is created by reading the buildkitd toml configuration. Wonder if this should be opt-in or not for security purpose.

Same logic might be used upstream for buildctl moby/buildkit#1050.

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

driver/docker-container/driver.go

tonistiigi · 2021-10-19T07:07:04Z

driver/docker-container/driver.go

@@ -205,6 +213,211 @@ func (d *Driver) copyLogs(ctx context.Context, l progress.SubLogger) error {
 	return rc.Close()
 }

+// copyToContainer is based on the implementation from docker/cli
+// https://github.com/docker/cli/blob/master/cli/command/container/cp.go
+func (d *Driver) copyToContainer(ctx context.Context, srcPath string, dstPath string) error {


Could we reuse some of this code in k8s driver? cc @morlay

copyBuildKitConfToContainer could.

In k8s, we have to read file data and store it in ConfigMap or Secrets, and mount it into running containter.

I prefix add an interface for Driver to add files, like

inteface Driver { AddFile(mountPath string, data []byte, secure bool) }

Then diffrent drivers do each mounting.

And copyBuildKitConfToContainer(...) could be copyBuildKitConfForDriver(d Dirver, ....) to reuse

Actually, this part might be fine to be driver-specific but the part that modifies the toml file and copies certs under .docker should not be specific to the driver but shared.

So let driver have ability to add file will be useful.

if the driver not need those files, it could skip AddFile

But driver like docker-container and kubernetes need, it could be easy to AddFile with different ways.

At least, copyBuildKitConfToContainer is needed to kubernetes driver too.

But consider to read file data to memory and write it the temp path in copyToContainer (may rename as AddFile).

cc @crazy-max

@tonistiigi

I don't understand why you have two volumes in your example when they both modify /etc/buildkit

it is not modify /etc/buildkit, it is modify files and sub dirs of /etc/buildkit.

if only mount single /etc/buildkit, we have to store toml and files in ConfigMap like

data: buildkit.toml: xxx key.pem: xxx certs.pem: xxx

The mounted /etc/buildkit will be

/etc/buildkit/ buildkit.toml key.pem certs.pem

With my example, it could mount any file in any location.

even put all files under /etc/buildkit, it could be same as container-driver did

/etc/buildkit/ certs/myregistry.io/ key.pem certs.pem buildkit.toml

we can't copy a file in a folder that does not exist in the container with the CopyToContainer Docker API.

Yes, but we could write to temp files before CopyToContainer in AddFile

If worry about copy cost. we could design the API like https://docs.min.io/docs/golang-client-api-reference.html#PutObject

type PutFileOptions struct { Secure bool } type Driver interface { PutFile(ctx context.Context, containerAbsPath string, r io.Reader, putOpts PutFileOptions) error }

So configmap can not contain subdirectories?

@tonistiigi Yes. / is not allowed in ConfigMap data key, it could be a flatten KV.

For subdirectories, we have to rename the absolute path, and resolve it back with volumes and volumeMounts like my example did.

This why I prefer have a PutFile() interface to make driver could do mounting files,
Then we could just convert and mount files for diffrent drivers in PutFile().

Yes we could use an interface but like @tonistiigi said we can't use the same paths defined on the host for ca and user tls registries certs inside the container, that's why the buildkitd toml needs to be modified anyway.

Maybe we could impl a buildkitd.override.toml on BuildKit.

util/bkutil/config.go

util/bkutil/container.go

driver/docker-container/driver.go

tonistiigi · 2021-10-26T03:54:57Z

util/bkutil/container.go

+	}
+
+	// Temp dir that will be copied to the container
+	tmpDir, err := os.MkdirTemp("", "buildkitd-config")


why temp directory? just save it under .docker/buildx or do you want to do this separately?

yes will do that in a follow-up to keep the current state

util/bkutil/container.go

driver/docker-container/driver.go

tonistiigi · 2021-10-26T04:11:50Z

driver/docker-container/driver.go

+	if err != nil {
+		return err
+	}
+	defer preparedArchive.Close()


I don't think we need any of this between 218-278. Just calling archive.Tar to get ReadCloser should be enough for this case.

I think you can remove more. As I said, I think only a single archive.Tar call is needed here.

docs/reference/buildx_create.md

tonistiigi · 2021-10-28T17:23:58Z

driver/docker-container/driver.go

@@ -205,6 +202,28 @@ func (d *Driver) copyLogs(ctx context.Context, l progress.SubLogger) error {
 	return rc.Close()
 }

+func (d *Driver) copyToContainer(ctx context.Context, srcPath string, dstDir string) error {
+	srcPath, err := resolveLocalPath(srcPath)


I don't think this is needed either. Nor the /. prefix.

tonistiigi · 2021-10-28T17:25:02Z

driver/docker-container/driver.go

+	if err != nil {
+		return err
+	}
+	srcArchive, err := dockerarchive.TarWithOptions(srcPath, &dockerarchive.TarOptions{})


nit: dockerarchive.Tar simpler for this case

dockerarchive.Tar requires a Compression arg, that's why I use dockerarchive.TarWithOptions instead. That API feels odd and should be changed I think.

dockerarchive.Uncompressed

tonistiigi · 2021-10-28T17:25:31Z

driver/docker-container/driver.go

+	}
+	defer srcArchive.Close()
+	return d.DockerAPI.CopyToContainer(ctx, d.Name, dstDir, srcArchive, dockertypes.CopyToContainerOptions{
+		AllowOverwriteDirWithFile: false,


If it is default anyway then don't set it.

crazy-max · 2021-10-28T17:57:13Z

@tonistiigi I will add tests in a follow-up.

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

crazy-max commented Sep 29, 2021

View reviewed changes

driver/docker-container/driver.go Outdated Show resolved Hide resolved

crazy-max force-pushed the inject-certs branch from e05cbee to 5a42259 Compare September 29, 2021 15:48

crazy-max mentioned this pull request Sep 29, 2021

x509: certificate signed by unknown authority push to local registry docker/build-push-action#436

Closed

tonistiigi added this to the v0.7.0 milestone Oct 18, 2021

crazy-max force-pushed the inject-certs branch from 5a42259 to 3e23b67 Compare October 18, 2021 19:48

crazy-max changed the title ~~Copy CA and user TLS registries certs to the container~~ container driver: copy ca and user tls registries certs Oct 18, 2021

crazy-max force-pushed the inject-certs branch 2 times, most recently from 6f611e6 to 8c8ac92 Compare October 18, 2021 21:00

tonistiigi reviewed Oct 19, 2021

View reviewed changes

crazy-max force-pushed the inject-certs branch 3 times, most recently from c1bc817 to 3965144 Compare October 21, 2021 09:28

crazy-max marked this pull request as ready for review October 21, 2021 16:52

crazy-max force-pushed the inject-certs branch from 3965144 to ddd5135 Compare October 21, 2021 17:04

tonistiigi reviewed Oct 26, 2021

View reviewed changes

docs/reference/buildx_create.md Outdated Show resolved Hide resolved

crazy-max force-pushed the inject-certs branch 3 times, most recently from ce4ae00 to 806ae3d Compare October 27, 2021 10:41

crazy-max requested a review from tonistiigi October 27, 2021 10:42

crazy-max force-pushed the inject-certs branch 2 times, most recently from f8d0a5f to 3abe6f8 Compare October 28, 2021 09:15

tonistiigi reviewed Oct 28, 2021

View reviewed changes

crazy-max force-pushed the inject-certs branch 2 times, most recently from 64b8ffe to 6626f1c Compare October 28, 2021 17:55

container driver: copy ca and user tls registries certs

3f716f0

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>

crazy-max force-pushed the inject-certs branch from 6626f1c to 3f716f0 Compare October 28, 2021 20:32

tonistiigi approved these changes Oct 28, 2021

View reviewed changes

tonistiigi merged commit 49342dd into docker:master Oct 28, 2021

crazy-max deleted the inject-certs branch October 28, 2021 23:07

erichorwath mentioned this pull request Oct 30, 2021

self signed certificates not working - "x509: certificate signed by unknown authority" error with private Docker repository docker/setup-buildx-action#112

Closed

crazy-max mentioned this pull request May 14, 2024

Fix k8s driver with certs cannot boot #2454

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

container driver: copy ca and user tls registries certs #787

container driver: copy ca and user tls registries certs #787

crazy-max commented Sep 29, 2021 •

edited

tonistiigi Oct 19, 2021

morlay Oct 20, 2021 •

edited

tonistiigi Oct 20, 2021

morlay Oct 20, 2021 •

edited

morlay Oct 20, 2021 •

edited

morlay Oct 22, 2021 •

edited

morlay Oct 22, 2021

tonistiigi Oct 22, 2021

morlay Oct 22, 2021 •

edited

crazy-max Oct 22, 2021

tonistiigi Oct 26, 2021

crazy-max Oct 26, 2021

tonistiigi Oct 26, 2021

tonistiigi Oct 26, 2021

crazy-max Oct 27, 2021

tonistiigi Oct 28, 2021

tonistiigi Oct 28, 2021

tonistiigi Oct 28, 2021

crazy-max Oct 28, 2021 •

edited

tonistiigi Oct 28, 2021

tonistiigi Oct 28, 2021

crazy-max commented Oct 28, 2021

container driver: copy ca and user tls registries certs #787

container driver: copy ca and user tls registries certs #787

Conversation

crazy-max commented Sep 29, 2021 • edited

Choose a reason for hiding this comment

morlay Oct 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay Oct 20, 2021 • edited

Choose a reason for hiding this comment

morlay Oct 20, 2021 • edited

Choose a reason for hiding this comment

morlay Oct 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morlay Oct 22, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazy-max Oct 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazy-max commented Oct 28, 2021

crazy-max commented Sep 29, 2021 •

edited

morlay Oct 20, 2021 •

edited

morlay Oct 20, 2021 •

edited

morlay Oct 20, 2021 •

edited

morlay Oct 22, 2021 •

edited

morlay Oct 22, 2021 •

edited

crazy-max Oct 28, 2021 •

edited