bindings: fix handling of env secrets in remote builds #27602
Conversation
Previously, using --secret=id=foo,env=BAR in remote mode would fail because the client sent the env var name to the server, which tried to resolve it locally. This patch modifies the client to resolve the environment variable locally, write it to a temp file, and send it as a file-based secret. Fixes containers#27494 Signed-off-by: ZuhairM7 <ZuhairM7>
openshift-ci
bot
added
the
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ZuhairM7 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
lsm5
left a comment
lsm5
left a comment
There was a problem hiding this comment.
@ZuhairM7 thanks for the PR.
For fixing the DCO job, you'll need to signoff on your commit.
Please also check out the Validate source code changes job failure. That needs to get fixed to get the other jobs moving.
Can you also add a test?
| // read specified env into a tmp file | ||
| // move tmp file to tar and change secret source to relative tmp file | ||
| secretVal := os.Getenv(val) | ||
| tmpSecretFilePath, err := tempManager.CreateTempFileFromReader(contextDir, "podman-build-secret-*", strings.NewReader(secretVal)) |
There was a problem hiding this comment.
Looks like this could leak - a defer os.Remove(...) after the error check would fix that
There was a problem hiding this comment.
Cleanup is handled by TempFileManager.
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
Thanks, it looks good, except for the thing the others mentioned. I fixed the release notes in the PR description.
| // read specified env into a tmp file | ||
| // move tmp file to tar and change secret source to relative tmp file | ||
| secretVal := os.Getenv(val) | ||
| tmpSecretFilePath, err := tempManager.CreateTempFileFromReader(contextDir, "podman-build-secret-*", strings.NewReader(secretVal)) |
There was a problem hiding this comment.
Cleanup is handled by TempFileManager.
openshift-ci
bot
added
release-note
and removed
do-not-merge/release-note-label-needed
4 participants
Previously, using --secret=id=foo,env=BAR in remote mode would fail because the client sent the env var name to the server, which tried to resolve it locally. This patch modifies the client to resolve the environment variable locally, write it to a temp file, and send it as a file-based secret.
This aligns Podman's remote behavior with Docker's behavior and ensures secrets are correctly passed to the build container.
Test to verify fix:
export MYSECRET='qwerty' podman build --secret=id=mysecret,env=MYSECRET -f - . <<EOF FROM alpine RUN --mount=type=secret,id=mysecret cat /run/secrets/mysecret EOFBefore (Remote Client): The secret is empty because the server cannot find MYSECRET in its environment.
STEP 1/2: FROM alpine STEP 2/2: RUN --mount=type=secret,id=mysecret cat /run/secrets/mysecret COMMIT --> 256d6061a45dAfter (Remote Client): The client resolves MYSECRET to qwerty and sends it to the server.
STEP 1/2: FROM alpine STEP 2/2: RUN --mount=type=secret,id=mysecret cat /run/secrets/mysecret qwerty COMMIT --> d5c40b8863ceFixes #27494
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?