Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI:DOCS] Pass secrets from the host down to internal podman containers #20659

Merged
merged 1 commit into from Nov 17, 2023

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Nov 11, 2023

This change will allow RHEL subscriptions from the host to flow to internal containers.

Fixes: containers/common#1735

Does this PR introduce a user-facing change?

RHEL Subscriptions from the host now flow through to quay.io/podman/* images

Copy link
Contributor

openshift-ci bot commented Nov 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 11, 2023
@rhatdan
Copy link
Member Author

rhatdan commented Nov 11, 2023

@gardar could you make sure this does what you want?

@gardar
Copy link
Contributor

gardar commented Nov 11, 2023

@rhatdan sure, thanks! I'll do it once I get back to the office after the weekend.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tentative LGTM.

Would be great to have this tested in CI.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand what you are trying to do it seems that /run/secrets is overloaded. If you create the outer container with a secret it will be written to /run/secrets. Thus this patch will leak all secrets to the inner container which seems undesirable if not outright dangerous as I cannot imagine anyone expecting this.
Is there a way to only limit this to the RHEL subscription?

@gardar
Copy link
Contributor

gardar commented Nov 13, 2023

Is there a way to only limit this to the RHEL subscription?

Yes, with /run/secrets/etc-pki-entitlement and /run/secrets/rhsm as I suggested here: containers/common#1735 (comment)

@rhatdan
Copy link
Member Author

rhatdan commented Nov 13, 2023

I agree that their could be a leak, but doing /run/secrets/etc-pki-entitlement:/run/secrets/etc-pki-entitlement will fail if /run/secrets/etc-pki-entitlement does not exists within the first container.

Since this is not a general purpose container, I think it is safe to do /run/secrets:/run/secrets

@rhatdan
Copy link
Member Author

rhatdan commented Nov 13, 2023

We could add an optional volume construct,
-/run/secrets/etc-pki-entitlement:/run/secrets/etc-pki-entitlement which would only volume mount if the source existed, but I am not sure if that is worth it.

Could go all the way to

+/run/secrets/etc-pki-entitlement:/run/secrets/etc-pki-entitlement

Which would create the source volume if it did not exists (Matching Docker behavior).

@Luap99
Copy link
Member

Luap99 commented Nov 13, 2023

Well I think secrets contain generally sensitive data, thus leaking it without any user content to the inner container may be very bad.
Also it seems we already special hande the case were a directory does not exists and only print a warning: https://github.com/containers/common/blob/7aab1cc4bf70e6da448c0dbe86f0fec8d8cfcd6a/pkg/subscriptions/subscriptions.go#L230-L236
We could easily drop this down to info level.

This change will allow RHEL subscriptions from the host to flow
to internal containers.

Fixes: containers/common#1735

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@gardar
Copy link
Contributor

gardar commented Nov 13, 2023

Am I understanding it correctly that this proposed change only fixes the issue when using the podmanimage container as the "parent" and using other containers would still cause issues? Or is podmanimage some specialized container image that podman runs internally when running Podman in Podman?

@rhatdan
Copy link
Member Author

rhatdan commented Nov 13, 2023

This will only fix this for podmanimage(s), but gives a guide to others on how to handle it.

@gardar
Copy link
Contributor

gardar commented Nov 14, 2023

This will only fix this for podmanimage(s), but gives a guide to others on how to handle it.

Ok that what I thought.
Would it not be more feasible to fix this at a level where it would work for any container? By extending the code in common/pkg/subscriptions

@rhatdan
Copy link
Member Author

rhatdan commented Nov 17, 2023

You could potentially do something their but it would be an abuse and hard coding RHEL semantics into the tool. Currently the tool is generalized to look for a file and then copies the contents of that file into the container. I would not want to hard code this path into the podman/buildah.

@rhatdan rhatdan added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 857d610 into containers:main Nov 17, 2023
42 checks passed
@Luap99
Copy link
Member

Luap99 commented Nov 17, 2023

@rhatdan it seems like you didn't fix #20659 (comment)

This means podman will print a bunch of warnings if the container is not run on non RHEL/fedora based host.

@github-actions 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 Feb 16, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested redhat podman containers don't get subscription from host (common/pkg/subscriptions)
4 participants