tunnel: allow `remote` and `API` to accept `--secrets` #12414

flouthoc · 2021-11-25T11:45:47Z

Following commit makes sure that build api can accept external
secret and allows currently NOOP podman-remote build -t tag --secret id=mysecret,src=/path/on/host to become functional.

TLDR:

Adds support for
podman-remote build -t tag --secret id=mysecret,src=/path/on/host .

Just like docker following api is a hidden field and only exposed to
podman-remote but could document it if it needs exposed on swagger.

Closes: #12415

flouthoc · 2021-11-25T12:11:26Z

Solves: #12415 (comment)

vrothberg

[NO NEW TESTS NEEDED]

Please add tests. Skipping adding tests is something we must discourage, and I've reminded on multiple occasions that we must explain why if there is really no way to test a change.

Changes LGTM

flouthoc · 2021-11-25T12:19:36Z

@vrothberg Sure I am just waiting for discussion to close on something will add tests as soon as this is ready to be merged. Marking this as draft for now. Since this still does not solves the actual problem.

flouthoc · 2021-11-25T12:22:42Z

@vrothberg We could copy secrets over to remote VM but not sure if its the right thing to do. I think we should do a bind mount hence this would still not solve cases like podman-remote build -t tag --secret id=mysecret,src=/path/on/host .

rhatdan · 2021-11-26T09:53:54Z

It is the right thing to do, copy the secret to the remote machine, but destroy it when the container is removed. Secrets are secret to the container, IE they do not get committed to the image. They are not secrets between the client and server. You might want to clarify this in the man pages, but they need to be copied. Mounting will not work if you are actually using a remote system as opposed to a VM.

flouthoc · 2021-11-26T10:05:25Z

@rhatdan Sure makes sense. I was also worried about this difference between mac and podman-remote server. Copy should work for both of these cases. I'll amend my PR.

test/e2e/build_test.go

pkg/bindings/images/build.go

test/e2e/build_test.go

vrothberg · 2021-11-29T09:06:45Z

@ashley-cui PTAL

flouthoc · 2021-11-29T10:38:29Z

@vrothberg thanks resolved in latest commit.

ashley-cui

LGTM, thanks!

mheon · 2021-11-29T14:36:00Z

Should we consider any additional security here, given this is secret data? I'm a little concerned that they might be included in built images unintentionally if an image does something like COPY *

rhatdan · 2021-11-29T14:45:19Z

/approve

openshift-ci · 2021-11-29T14:45:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2021-11-29T14:48:04Z

@flouthoc please test out @mheon possiblity. Basically try a Containerfile with

from fedor
copy * /test/

And then run a test like

podman --remote -f /LOCATIONOF/Containerfile --secret ... /NOTLOCATIONOFContainerfile

Then make sure the secret and Containerfile do not end up i the image.

flouthoc · 2021-11-29T15:25:03Z

@mheon @rhatdan I tested this with docker and buildkit : COPY * / is a user driven command even docker copies everything to working directory from current context. If build secret file is in current context and user wants to run COPY * / all files will be copied over since that is what user wants.

I guess we could workaround that by adding extra .dockerignore if we want but that would a different behavior though.

PS: Docker copies everything including Dockerfile and so do we.

mheon · 2021-11-29T15:26:39Z

But does Buildkit move all secrets into the build context, if they weren't already? Because it seems like that's what this code is doing.

flouthoc · 2021-11-29T15:35:06Z

@mheon I haven't tried it on mac but i think it should not add secrets to context if they not part of it by default cause they must be binding files instead of copying it to context.

I guess we could have a similar thing by checking if secret was not part of current context then add it to .dockerignore that should be able to achieve us a similar functionality.

Following commit makes sure that `build` api can accept external secret and allows currently `NOOP` `podman-remote build -t tag --secret id=mysecret,src=/path/on/remote` to become functional. Just like `docker` following api is a hidden field and only exposed to `podman-remote` but could document it if it needs exposed on `swagger`. Signed-off-by: Aditya Rajan <[email protected]>

Podman remote must treat build secrets as part of context directory. If secret path is absolute path on host copy it to tar file and pass it to remote server. Signed-off-by: Aditya Rajan <[email protected]>

flouthoc · 2021-11-30T08:51:10Z

@flouthoc please test out @mheon possiblity. Basically try a Containerfile with

from fedor copy * /test/

And then run a test like

podman --remote -f /LOCATIONOF/Containerfile --secret ... /NOTLOCATIONOFContainerfile

Then make sure the secret and Containerfile do not end up i the image.

@mheon @rhatdan Following case of leaking secrets into image is handled in a new commit also added a new test to verify the same.

Prevents temp secrets leaking into image by moving it away from context directory to parent builder directory. Builder directory automatically gets cleaned up when we are done with the build. Signed-off-by: Aditya Rajan <[email protected]>

flouthoc · 2021-11-30T12:06:12Z

@rhatdan @mheon PTAL. New test verifies the concern.

mheon · 2021-11-30T15:02:08Z

pkg/api/handlers/compat/images_build.go

+							builderDirectory, _ := filepath.Split(contextDirectory)
+							// following path is outside build context
+							newSecretPath := filepath.Join(builderDirectory, arr[1])
+							oldSecretPath := filepath.Join(contextDirectory, arr[1])


Do we clean this directory out anywhere, remove the files inside it? Is it shared between multiple builds, potentially?

@mheon Yes build directory is automatically cleaned up and no its a new temp directory for each build.

mheon · 2021-11-30T15:34:43Z

LGTM

flouthoc · 2021-11-30T16:05:44Z

@rhatdan PTAL

rhatdan · 2021-11-30T18:27:23Z

/lgtm

flouthoc mentioned this pull request Nov 25, 2021

secret is not mounted during build (macos) #12415

Closed

vrothberg reviewed Nov 25, 2021

View reviewed changes

flouthoc marked this pull request as draft November 25, 2021 12:19

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2021

flouthoc force-pushed the api-allow-secrets branch 6 times, most recently from e2b9d35 to 4a8f997 Compare November 26, 2021 17:47

flouthoc marked this pull request as ready for review November 26, 2021 20:01

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 26, 2021

vrothberg reviewed Nov 29, 2021

View reviewed changes

test/e2e/build_test.go Outdated Show resolved Hide resolved

pkg/bindings/images/build.go Outdated Show resolved Hide resolved

test/e2e/build_test.go Show resolved Hide resolved

flouthoc force-pushed the api-allow-secrets branch from 4a8f997 to dbb7729 Compare November 29, 2021 09:26

flouthoc requested a review from vrothberg November 29, 2021 10:38

ashley-cui reviewed Nov 29, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2021

flouthoc force-pushed the api-allow-secrets branch 2 times, most recently from 8a22f22 to 316b5fe Compare November 30, 2021 08:48

flouthoc added 2 commits November 30, 2021 14:19

podman-remote: copy secret to contextdir is absolute path on host

e720417

Podman remote must treat build secrets as part of context directory. If secret path is absolute path on host copy it to tar file and pass it to remote server. Signed-off-by: Aditya Rajan <[email protected]>

flouthoc force-pushed the api-allow-secrets branch from 316b5fe to a4609d7 Compare November 30, 2021 08:50

flouthoc force-pushed the api-allow-secrets branch from a4609d7 to d5267ad Compare November 30, 2021 09:14

flouthoc force-pushed the api-allow-secrets branch from d5267ad to c80a2e4 Compare November 30, 2021 10:14

mheon reviewed Nov 30, 2021

View reviewed changes

flouthoc requested a review from mheon November 30, 2021 15:21

openshift-ci bot assigned rhatdan Nov 30, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2021

openshift-merge-robot merged commit 85101f6 into containers:main Nov 30, 2021

edsantiago mentioned this pull request Nov 30, 2021

pprof test is flaking again #12167

Closed

flouthoc mentioned this pull request Jan 24, 2022

podman build --secret does not behave the same as docker #12985

Closed

flouthoc mentioned this pull request Jun 16, 2022

Using docker build with secrets backed by a Podman controlled docker.sock fails on macOS #14615

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tunnel: allow `remote` and `API` to accept `--secrets` #12414

tunnel: allow `remote` and `API` to accept `--secrets` #12414

flouthoc commented Nov 25, 2021 •

edited

Loading

flouthoc commented Nov 25, 2021

vrothberg left a comment

flouthoc commented Nov 25, 2021 •

edited

Loading

flouthoc commented Nov 25, 2021

rhatdan commented Nov 26, 2021

flouthoc commented Nov 26, 2021

vrothberg commented Nov 29, 2021

flouthoc commented Nov 29, 2021

ashley-cui left a comment

mheon commented Nov 29, 2021

rhatdan commented Nov 29, 2021

openshift-ci bot commented Nov 29, 2021

rhatdan commented Nov 29, 2021

flouthoc commented Nov 29, 2021 •

edited

Loading

mheon commented Nov 29, 2021

flouthoc commented Nov 29, 2021 •

edited

Loading

flouthoc commented Nov 30, 2021 •

edited

Loading

flouthoc commented Nov 30, 2021

mheon Nov 30, 2021

flouthoc Nov 30, 2021

mheon commented Nov 30, 2021

flouthoc commented Nov 30, 2021

rhatdan commented Nov 30, 2021

tunnel: allow remote and API to accept --secrets #12414

tunnel: allow remote and API to accept --secrets #12414

Conversation

flouthoc commented Nov 25, 2021 • edited Loading

TLDR:

flouthoc commented Nov 25, 2021

vrothberg left a comment

Choose a reason for hiding this comment

flouthoc commented Nov 25, 2021 • edited Loading

flouthoc commented Nov 25, 2021

rhatdan commented Nov 26, 2021

flouthoc commented Nov 26, 2021

vrothberg commented Nov 29, 2021

flouthoc commented Nov 29, 2021

ashley-cui left a comment

Choose a reason for hiding this comment

mheon commented Nov 29, 2021

rhatdan commented Nov 29, 2021

openshift-ci bot commented Nov 29, 2021

rhatdan commented Nov 29, 2021

flouthoc commented Nov 29, 2021 • edited Loading

mheon commented Nov 29, 2021

flouthoc commented Nov 29, 2021 • edited Loading

flouthoc commented Nov 30, 2021 • edited Loading

flouthoc commented Nov 30, 2021

mheon Nov 30, 2021

Choose a reason for hiding this comment

flouthoc Nov 30, 2021

Choose a reason for hiding this comment

mheon commented Nov 30, 2021

flouthoc commented Nov 30, 2021

rhatdan commented Nov 30, 2021

tunnel: allow `remote` and `API` to accept `--secrets` #12414

tunnel: allow `remote` and `API` to accept `--secrets` #12414

flouthoc commented Nov 25, 2021 •

edited

Loading

flouthoc commented Nov 25, 2021 •

edited

Loading

flouthoc commented Nov 29, 2021 •

edited

Loading

flouthoc commented Nov 29, 2021 •

edited

Loading

flouthoc commented Nov 30, 2021 •

edited

Loading