Using node pull credentials during builds, image stream imports and pull-through #136
Conversation
openshift-ci-robot
added
the
size/L
|
/assign @adambkaplan @bparees @dmage @gabemontero |
| build and import image streams from this registry. | ||
| - Deploy an image registry using self-signed certificates and provide its pull | ||
| secrets and certificates after the install. Attempt to build and import image | ||
| streams from this registry. |
There was a problem hiding this comment.
missing test coverage for "run builds using images that can only be pulled using node credentials".
Needs to cover:
- pulling the base image
- pulling images being used for content extraction/input
- pushing images (This one may need more discussion... i'm not sure if we want to allow regular users to leverage the node credentials for pushing images to registries.. that feels like a bit of an escalation from what users can currently accomplish by way of the node credentials today)
There was a problem hiding this comment.
pushing images (This one may need more discussion... i'm not sure if we want to allow regular users to leverage the node credentials for pushing images to registries.. that feels like a bit of an escalation from what users can currently accomplish by way of the node credentials today)
I agree pushing with the node credential presents a problem. I can foresee admins installing OpenShift with a pull secret that allows image push and not being aware that this pull secret could be used for push by anyone via builds.
There was a problem hiding this comment.
I have been looking at the builder source code and we could use node credentials only during pull, never for push. If you are OK I would add another item under section "Risks and Mitigations" and a new test case for this scenario.
There was a problem hiding this comment.
i think we've got consensus on that.
There was a problem hiding this comment.
+1 to only using node creds for pull if pull secrets are otherwise not provided for the build.
There was a problem hiding this comment.
I have added a new item under Risks and Mitigations to cover these aspects we have discussed on this thread. PTAL.
There was a problem hiding this comment.
/cc @mfojtik @sttts @soltysh
for impacts to openshift-apiserver
/cc @umohnani8 @mrunalp
for node interactions
| build and import image streams from this registry. | ||
| - Deploy an image registry using self-signed certificates and provide its pull | ||
| secrets and certificates after the install. Attempt to build and import image | ||
| streams from this registry. |
There was a problem hiding this comment.
pushing images (This one may need more discussion... i'm not sure if we want to allow regular users to leverage the node credentials for pushing images to registries.. that feels like a bit of an escalation from what users can currently accomplish by way of the node credentials today)
I agree pushing with the node credential presents a problem. I can foresee admins installing OpenShift with a pull secret that allows image push and not being aware that this pull secret could be used for push by anyone via builds.
openshift-ci-robot
requested review from
mfojtik,
mrunalp,
soltysh,
sttts and
umohnani8
|
@ricardomaraschini note that the node pull secret would be in addition to the pull secret for the internal registry, which is specific to each service account in a given namespace. Builds at minimum would need to search both pull secret files to find which credential to use. |
|
@nalind ptal. |
ricardomaraschini
changed the title
There was a problem hiding this comment.
@ricardomaraschini mostly nits, plus my note clarifying the behavior of pull secrets with builds.
Otherwise looks good!
openshift-ci-robot
added
needs-rebase
ricardomaraschini
changed the title
|
@dmage @adambkaplan I have submitted a new commit adding a comment on how we should treat if the namespace contains credentials that are also present on the node. PTAL. |
| #### Image Stream Import | ||
|
|
||
| - Mount the pull secret as `readOnly`, `hostPathreadOnly` in | ||
| `openshift-apiserver` deployment under `/node/var/lib/kubelet/config.json`. |
There was a problem hiding this comment.
@dmage we need to come up with an agreement when it comes to the place where we are going to mount this. What is your idea?
There was a problem hiding this comment.
my idea is to use the same path inside the pod as on the node, but I'd like to hear ideas from someone who can approve PRs for openshift-apiserver.
There was a problem hiding this comment.
and I'd say it should be configurable, i.e. the operator should mount /var/lib/kubelet/config.json and pass this name into the configuration file.
There was a problem hiding this comment.
@csrwng we would also need to track mounting this into the hypershift-toolkit.
There was a problem hiding this comment.
Following @dmage suggestion I have updated the proposal, now we mount pull secrets on /var/lib/kubelet/config.json
| Mitigations: | ||
|
|
||
| As far as I verified(and this needs to be once more tested) it is impossible | ||
| to spawn a shell inside the builder pods. I also tried to copy the credentials |
There was a problem hiding this comment.
Ephemeral pods will allow sharing process namespace and thus provide you with access to it. That's a major problem.
There was a problem hiding this comment.
There was a problem hiding this comment.
This issue also applies to "openshift-apiserver" during image stream import or is something only for builds?
There was a problem hiding this comment.
@ricardomaraschini what @soltysh is saying is that even though we block oc rsh into a build-pod, someone (may) be able to create an ephemeral container in the build-pod's process namespace and use that mechanism to get into it instead.
@deads2k are there security mechanisms that would prevent someone from abusing this? much like we don't allow you to exec/rsh into a pod you can't "create" yourself (so normal users can't exec/rsh into a privileged container, as i understand it), does the ephemeral container api have similar restrictions?
There was a problem hiding this comment.
@bparees there is no limitation at this time to my knowledge. its not clear to me that we could use rbac appropriately in a namespace to block the subresource to create the ephemeral container as build pods would be in the same namespace as other pods we would want to allow ephemeral access.
There was a problem hiding this comment.
that subresource effectively updates the pod resource. If i can't create privileged pods, i would not expect to be able to edit prvileged pods and i'd expect that restriction to extend to subresources that modify the pod. No?
There was a problem hiding this comment.
spoke with @derekwaynecarr offline. SCC will need to handle this (ensure that a user who can't create a particular pod also cannot invoke this subresource which edits the pod).
between @openshift/sig-developer-experience and @openshift/sig-master we need to ensure this is covered by the time the ephemeral container api goes beta (at which point it will be on by default).
There was a problem hiding this comment.
I have enabled EphemeralContainers on kube-apiserver and simulated the feature, as kubeadmin I have been able to patch the pod normally as expected but as a regular user I received the following message back:
$ oc replace --raw /api/v1/namespaces/build/pods/img-1-build/ephemeralcontainers -f ./ec.json
Error from server (Forbidden): pods "img-1-build" is forbidden: User "user" cannot update resource "pods/ephemeralcontainers" in API group "" in the namespace "build"
$
ec.json:
$ cat ec.json
{
"apiVersion": "v1",
"kind": "EphemeralContainers",
"metadata": {
"name": "img-1-build"
},
"ephemeralContainers": [{
"command": [
"sh"
],
"image": "busybox",
"imagePullPolicy": "IfNotPresent",
"name": "debugger",
"stdin": true,
"tty": true,
"terminationMessagePolicy": "File"
}]
}
It might be that this is already supported by the current codebase.
| - Allow users to run `builds` based on images hosted in any registry provided | ||
| during or after cluster installation without providing any extra credentials. | ||
| - Allow `image-registry` to execute pull-through using the node's pull | ||
| credentials. |
There was a problem hiding this comment.
What I'm missing here is an explicit opt-in for that functionality, not every cluster-wide secret can and should be exposed. I see the requirement, but the risks are much higher, so this should not be a default, rather an opt-in.
There was a problem hiding this comment.
@soltysh this enhancement is narrowly focused on the pull-secret added to the openshift-config namespace at install time. This pull secret is implicitly shared cluster-wide on all nodes - I am not aware of any opt-out other than deleting this secret.
I agree that a general "share any secret across a cluster" feature should absolutely be opt-in. DevEx has discussed this a bit as a future enhancement proposal [1]
|
|
||
| This is the node's filesystem path we are taking into account: | ||
|
|
||
| - `/var/lib/kubelet/config.json` contains the node's pull secret. |
There was a problem hiding this comment.
can this file change? E.g. a user can ssh into the machine and append secrets there.
There was a problem hiding this comment.
This has influence on openshift/cluster-openshift-apiserver-operator#284, i.e. we need to reread that file. Is that the case?
There was a problem hiding this comment.
it can be changed via the MCO which causes node reboots so rereading is moot.
if the MCO is ever updated to allow node filesystem changes w/o rebooting the nodes, we'd have to setup a watch on the mounted file in our pod.
I believe these steps are what lead to the /var/lib/kubelet/config.json being updated:
https://docs.openshift.com/container-platform/4.3/openshift_images/managing-images/using-image-pull-secrets.html#images-update-global-pull-secret_using-image-pull-secrets
There was a problem hiding this comment.
Pull secrets are read in a "per image stream import request" basis, we don't keep it in memory.
This commit added a proposal of using node's pull secrets and during image stream imports, image-registry pull-through operations and builds.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, ricardomaraschini 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 |
openshift-ci-robot
added
the
approved
|
|
||
| Mitigations | ||
|
|
||
| Mounted path always exist on worker nodes. |
There was a problem hiding this comment.
Interesting. The confirmation we have is that this path does not exist only on bootstrap nodes. @adambkaplan could we tag someone from node's team to get a final answer on this?
There was a problem hiding this comment.
@cgwalters can you please confirm that the cluster pull secret always exists at /var/lib/kubelet/config.json
There was a problem hiding this comment.
I don't want to make that permanent "ABI" though; see also openshift/machine-config-operator#1190 which proposes ensuring all MCO configuration (including pull secret) lives in /etc so it can be made transactional - (but we'd probably have a backcompat symlink for a while at least).
I think probably the best bet is for whatever wants it to parse the kubelet config or so perhaps?
Or, I guess we could make /etc/kubernetes/pull-secret.json -> /var/lib/kubelet/config.json today and people could start using the new name. (Or probably better introduce the that new /etc name upstream)
There was a problem hiding this comment.
I don't want to make that permanent "ABI" though; see also openshift/machine-config-operator#1190 which proposes ensuring all MCO configuration (including pull secret) lives in
/etcso it can be made transactional - (but we'd probably have a backcompat symlink for a while at least).
It is clear that we must make the file path easily configurable.
I think probably the best bet is for whatever wants it to parse the kubelet config or so perhaps?
@cgwalters Do you mean to parse /etc/kubernetes/kubelet.conf to know from where to read pull secrets? I could not find any directive on it pointing to /var/lib/kubelet/config.json, pardon if I misunderstood you, could you clarify?
Or, I guess we could make
/etc/kubernetes/pull-secret.json -> /var/lib/kubelet/config.jsontoday and people could start using the new name. (Or probably better introduce the that new/etcname upstream)
I guess this would take some time to get done and we already have some PRs in place to implement this enhancement. I am inclined to stick with /var/lib/kubelet/config.json, making it easily configurable so we can easily migrate in the future.
There was a problem hiding this comment.
@cgwalters are you going to change root-dir for kubelet?
This commit added a proposal of using node's artifacts(pull secrets and
certificates) during image stream imports and builds.