kubelet: ensure secret pulled image #114847

pacoxu · 2023-01-05T10:11:06Z

/kind feature
/cc @mikebrow

Implement support in kubelet to ensure images pulled with pod imagePullSecrets are authenticated
by other pods that do not have the same credentials.

Design Details: track a hash map for the credentials in kubelet

kubernetes/enhancements#3532

Kubelet will track, in memory, a hash map for the credentials that were successfully used to pull an image. It has been decided that the hash map will be persisted to disk, in alpha.

What to store

image: string
auth hash (keys): string
ensuredBySecret: bool

Where to store the data

store in node status (need to be confirmed with sig-node): it needs API-change to add a list field like ensuredPulledImage in node status spec.
store in a local file like cpu policy state (should it be secured by an encryption scheme). [No API change, but a new file on every node.]
store in a secure volume? too complex?

Should the stored data be outdated after several hours for security reasons?
maybe 1, 6 hours, or 12 hours.

**Scenarios that should be taken care of **

kubelet restart (that is why we need to store the data)
pinned image: any special logic for a pinned image?

TODO List

ensure secret pulled images enhancements#1608
code complete
[x] feature gate added for issue comment
usage (not needed for alpha)
docs (not needed for alpha)
test bucket(s)
unit buckets complete.. for FG on and off

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/2535-ensure-secret-pulled-images/README.md

Test demo:

[root@daocloud ~]# bash -c "cat /var/lib/kubelet/image_manager_state | jq ."
{
  "images": {
    "sha256:eb6cbbefef909d52f4b2b29f8972bbb6d86fc9dba6528e65aad4f119ce469f7a": {
      "authHash": {
        "115b8808c3e7f073": {
          "ensured": true,
          "dueDate": "2023-05-26T18:00:09.802153792+08:00"
        }
      },
      "name": "daocloud.io/daocloud/dce-registry-tool:3.0.8"
    }
  }
}

[root@daocloud ~]# cat ensure.yaml
apiVersion: v1
kind: Pod
metadata:
  name: need-secret-but-has-no
spec:
  containers:
    - name: test-container
      image: daocloud.io/daocloud/dce-registry-tool:3.0.8

      command: [ "sleep", "100000" ]

---

apiVersion: v1
kind: Pod
metadata:
  name: need-secret-and-has-secret
spec:
  containers:
    - name: test-container
      image: daocloud.io/daocloud/dce-registry-tool:3.0.8
      command: [ "sleep", "100000" ]
  imagePullSecrets:
  - name: regcred

---

apiVersion: v1
kind: Pod
metadata:
  name: need-secret-and-has-wrong-secret
spec:
  containers:
    - name: test-container
      image: daocloud.io/daocloud/dce-registry-tool:3.0.8
      command: [ "sleep", "100000" ]
  imagePullSecrets:
  - name: regcred.1

[root@daocloud ~]#  kubectl get pod -o wide
NAME                               READY   STATUS             RESTARTS   AGE     IP               NODE       NOMINATED NODE   READINESS GATES
need-secret-and-has-secret         1/1     Running            0          2m5s    172.32.230.235   daocloud   <none>           <none>
need-secret-and-has-secret-2       1/1     Running            0          2m1s    172.32.230.234   daocloud   <none>           <none>
need-secret-and-has-wrong-secret   0/1     ImagePullBackOff   0          105s    172.32.230.236   daocloud   <none>           <none>
need-secret-but-has-no             0/1     ErrImagePull       0          70s     172.32.230.237   daocloud   <none>           <none>

pacoxu · 2023-01-05T10:22:26Z

/priority important-soon
/triage accepted

pkg/kubelet/images/image_manager.go

liggitt · 2024-03-07T15:50:06Z

pkg/kubelet/images/image_manager.go

+	}
+
+	// if the image is in the ensured secret pulled image list, it is pulled by secret
+	pulledBySecret := m.ensureSecretPulledImages[imageRef] != nil


how is this in-memory map accurate if the kubelet restarts?

checkpointing was out then in .. now out again for the alpha.. TODO: restore from checkpoint..

checkpointing was out then in .. now out again for the alpha.. TODO: restore from checkpoint..

Ok, sorry for asking questions which have already been answered :-)

Seems like checkpointing is the hard part of the problem. I'll defer to node approvers on that decision, but I'm a little surprised we're deferring tackling it.

I was the one that proposed we drop the checkpointing. The motivation was the recheck period was proposed to be 24hr, and I think the kubelet restarting is an infrequent operation. Redundantly checking the creds is part of the feature's intention, so the admin is opting into checking more than necessary and that's the point. Not having checkpointing reduces the code complexity while not materially changing the behavior IMO

If I'm reading correctly, the current implementation means images previously pulled to the node using pull secrets are treated as non-secret-pulled images after kubelet restart and would never be required to revalidate their pull credentials. Am I reading that correctly?

https://github.com/kubernetes/kubernetes/pull/94899/files#r691688013 prior discussion on this context

the point of this KEP is to add unnecessary rechecks

original point was to provide a path to get off pull always

The desire to do intermittent rechecks (e.g. after kubelet restart or after duration) for better security than if not present and better performance over pull always is at conflict with the desire to not have any rechecks because a recheck might fail in cases where a user does not use pull always or pull never and is expecting pull if not present to behave like pull never.

With rechecking enabled for duration.. in the scope of the if not present policy the checkpointing has minimal value.

With rechecking disabled for duration but enabled for multi-pod permission... in the scope of the if not present policy.. the checkpointing is needed before we go beta otherwise "how do we know which pre-existing images are intentionally preloaded and which are previously-pulled-by-secret." We would need to take the new presumption that preloaded was just for layers and we should recheck manifest permissions after restart.

So it would seem... the resolution for alpha would be to check if either of the recheck signals are enabled and if not .. disable the feature gate and possibly warn users that they should employ the pull always admission controller.

7a120ec

if pullImageSecretRecheckPeriod>0 and KubeletEnsureSecretPulledImages is enabled, do recheck related logic.

This will keep the original behavior if we don't set the recheck period even when we enabled the FG by default in later releases or manually enable it in v1.30.

Does this make sense?

I'm actually out of the office today, so I can't give this more time until next week, the more I look at this, I don't see how this can work as expected (reliably make new pods/pulls prove they have access AND not destabilize already running/working pods if a registry goes unavailable) without kubelet keeping track across restarts of images it used pull credentials to fetch.

There's two questions the kubelet has to answer:

did image X which is already pulled to the node require image pull credentials?

if so, are the image pull credentials this pod requesting image X is providing good?

This PR assumes all images that exist at kubelet start don't require credentials, only keeps track of credential-enabled image pulls the kubelet does in-memory, then forgets them on restart.

The "periodically check" approach looks like it will not actually protect images that exist on startup from use by uncredentialed pods. That seems like a fundamental gap.

The "periodically check" approach also looks like it will put existing working credentialed pods at risk of registry outage. Is it actually intended that pods which had image pull credentials and proved their pull credential was valid on their initial start on a node could stop working later because the registry is unavailable? That doesn't seem right.

#94899 (comment) and #94899 (comment) were the feedback on the last attempt of this feature, and looks like we're in essentially the same position now.

The two stories from the kep https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2535-ensure-secret-pulled-images#user-stories.

Story 1: User with multiple tenants will be able to support all image pull policies without concern that one tenant will gain access to an image that they don't have rights to.
2.Story 2: User will will no longer have to inject the Pull Always Image Pull Policy to ensure all tenants have rights to the images that are already present on a host.

The aim of the KEP is to gain an image pull policy that is between Always and current IfNotPresent.

Image pull policyAlways trust nothing.

Image pull policyIfNotPresent trust everything on the disk, but no checking on the image pull secret.

What we want is something likeIfNotPresentWithTrustedAuthPeriod:

we add a recheck/trust period to control the weak level: 24h is like a daily trust credit. 1y may be a long credit trust. Node admin can control its trust for an image pull credential check result.

The PullImageSecretRecheckPeriod naming is somewhat inaccurate, as we didn't recheck the image secret every 12h if it is set to be 12h. The check will happen only when a container startup or restart happen IIUC. It will not kill a pod or container if it found the credential is out-of-dated.

What should be done?

We should keep the behavior not changed even this feature is GAed.

When node admin options in with adding a recheck period, (in another word, this is for those admins who only allow its users usingAlways ImagePullPolicy now), I prefer that restarting kubelet needs recheck, which will be more secure for them. The admin can control the secure/weak level using the recheck period. This is also why I think no checkpoint is acceptable.

pkg/kubelet/images/puller.go

liggitt · 2024-03-07T15:57:23Z

pkg/kubelet/images/image_manager.go

+			ensuredInfo := digest.Auths[hash]
+			if ensuredInfo != nil && ensuredInfo.ensured && ensuredInfo.lastEnsuredDate.Add(m.pullImageSecretRecheckPeriod.Duration).After(time.Now()) {


reading this line, it seems like there's two aspects here:

How often we recheck a previously-valid credential is still valid – pullImageSecretRecheckPeriod makes sense to control this.

Whether we ever check if a pod's pull secrets were valid – using pullImageSecretRecheckPeriod for that is really weird to me... shouldn't the node admin be able to turn on cross-pod checking without also setting a revalidation period?

If we have a boolean configuration for cross pod checking that can be set independently would we then default to some recheck period (cache invalidation) if not set or keep assuming image access for the entry in the cache till the kubelet is running (current in-memory impl)?

If we have a boolean configuration for cross pod checking that can be set independently would we then default to some recheck period (cache invalidation) if not set or keep assuming image access for the entry in the cache till the kubelet is running (current in-memory impl)?

Not sure... I'd probably default both to current behavior and let admins opt into things that could break or disrupt currently working setups, but I hadn't thought about it for long.

good point... let's add another config for the ensured by cross-pod checking option. I like the name.

What do you think about the default for the cross-pod checking option? Same as for the recheck period (defaults to off?)

What do you think about the default for the cross-pod checking option? Same as for the recheck period (defaults to off?)

Yeah, my starting position is always to default to settings that won't make an admin that failed to read 40 pages of release notes come hunt us down because we broke them on upgrade

sounds good. We will have to document that setting the booelan and not the recheck period could have implications :)

If recheck is disabled(with recheck duration is 0s), a new pod with same hash should be checked again.

It sounds a valid corner case.

pkg/kubelet/images/image_manager.go

pkg/kubelet/kuberuntime/kuberuntime_image.go

liggitt · 2024-03-07T16:04:12Z

staging/src/k8s.io/kubelet/config/v1beta1/types.go

+
+	// PullImageSecretRecheckPeriod defines the duration to recheck the pull image secret.
+	// By default, the kubelet will recheck the pull image secret every 24 hours(1d).
+	PullImageSecretRecheckPeriod *metav1.Duration `json:"pullImageSecretRecheckPeriod"`


default change and comment lgtm. traced through to where this is used and had a question about whether the revalidation duration is what we want to decide whether to ever validate a pull secret

mikebrow

see proposed style change to named return vals...

@pacoxu pls ignore the this style change.. diff is to big.. see comment below from Jordan

pkg/kubelet/kuberuntime/kuberuntime_image.go

pkg/kubelet/container/runtime.go

pkg/kubelet/kuberuntime/kuberuntime_image.go

liggitt · 2024-03-07T16:51:07Z

see proposed style change to named return vals...

I'd rather minimize the diff... returning data (imageRef) alongside errors makes me wonder what is using that data in error cases

mikebrow · 2024-03-07T17:16:14Z

see proposed style change to named return vals...

I'd rather minimize the diff... returning data (imageRef) alongside errors makes me wonder what is using that data in error cases

agreed.. I think it was a mistake to return imageRef that error case.

pacoxu · 2024-03-07T17:31:31Z

https://github.com/kubernetes/kubernetes/compare/1f854e8d7b945d95c7444c6d613a1939b8fabf15..3d40dd9a76b085f86e83f5c3242be402d4c77d40
https://github.com/kubernetes/kubernetes/compare/3d40dd9a76b085f86e83f5c3242be402d4c77d40..0ad580e18f54362718f95196453590e7ae216fc7
update for Jordan's comments.

pkg/kubelet/images/image_manager.go

…r IfNotPresent Image

haircommander · 2024-03-08T14:41:18Z

pkg/kubelet/images/image_manager.go

+				// successful pull no auth hash returned, auth was not required so we should reset the hashmap for this
+				// imageref since auth is no longer required for the local image cache, allowing use of the ImageRef
+				// by other pods if it remains cached and pull policy is PullIfNotPresent
+				delete(m.ensureSecretPulledImages, imageRef)


I kinda feel like pullCredentialsHash == "" should be handled in a separate mechansim. As in: if we remove images with an empty credentials hash from this map, then we treat the first pull after kubelet restart and first pull after recheck period as pullCredentialHash == "'". Instead, can this be skipped or even add a "" entry in to the maps, and then we treat not present in map as always recheck?

If the image needs no hash at first and then later needs auth, this would be a problem.

If recheck is enabled, the recheck for no auth image seems to be needed as well.

https://github.com/kubernetes/kubernetes/compare/7a120ec8850900a419db335db37cf7063cba3b3f..51f8822fe18a3b044c76d866a765c8109b32b403 the latest update .

@pacoxu
What do you think about storing an "anonymous" entry instead of starting over (by that delete operation).. when anonymous is successful.. If we go that route we can check for known anonymous success first then if not in the list check for each hash.. With duration checking enabled can also recheck anonymous..

@haircommander agree to your point add a "" entry in to the maps "" or "anonymous" to be more explicit

iow isEnsuredBySecret() needs to be improved to isEnsured() // By Secret || Anonymous .. when anonymous worked for a particular registry that image is now in cache and thus any private pull with auth is moot, at least until another policy says recheck it now..

I'm taking the presumption that we are not going to check if an image was pulled from registry a or registry b.. the image in the k8s.io image cache is still the same bytes either way..

… is enabled, do recheck logic - if recheck is enabeld, recheck no auth image(pullCredentialsHash=="") as well

pacoxu · 2024-03-09T00:31:56Z

/hold
for there are still some things not clear.

/milestone clear

k8s-triage-robot · 2024-06-07T00:37:31Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-ci-robot · 2024-06-07T00:37:40Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-triage-robot · 2024-07-07T00:59:43Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

pacoxu · 2024-07-11T02:09:13Z

/close
ref #125817

k8s-ci-robot · 2024-07-11T02:09:18Z

@pacoxu: Closed this PR.

In response to this:

/close
ref #125817

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot requested a review from mikebrow January 5, 2023 10:11

pacoxu marked this pull request as draft January 5, 2023 10:18

pacoxu marked this pull request as ready for review January 5, 2023 10:20

pacoxu force-pushed the ensure-secret-pulled-image branch from 9f8b7f9 to b618e2e Compare January 5, 2023 10:20

pacoxu force-pushed the ensure-secret-pulled-image branch from b618e2e to 3dbb9d7 Compare January 5, 2023 10:31

pacoxu mentioned this pull request Jan 13, 2023

#2535:ensure secret pulled images - move kep targets; add persist kubernetes/enhancements#3532

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2023

pacoxu force-pushed the ensure-secret-pulled-image branch from 3dbb9d7 to 3dcb3d1 Compare May 8, 2023 07:15

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 8, 2023

liggitt reviewed Mar 7, 2024

View reviewed changes

mikebrow reviewed Mar 7, 2024

View reviewed changes

pacoxu force-pushed the ensure-secret-pulled-image branch 2 times, most recently from 3d40dd9 to 0ad580e Compare March 7, 2024 17:30

pacoxu force-pushed the ensure-secret-pulled-image branch from 0ad580e to 9d2b749 Compare March 7, 2024 17:39

haircommander reviewed Mar 7, 2024

View reviewed changes

pkg/kubelet/images/image_manager.go Outdated Show resolved Hide resolved

haircommander reviewed Mar 7, 2024

View reviewed changes

pkg/kubelet/images/image_manager.go Outdated Show resolved Hide resolved

set PullImageSecretRecheckPeriod default to 0; default not recheck fo…

5b444e4

…r IfNotPresent Image

pacoxu force-pushed the ensure-secret-pulled-image branch from 9d2b749 to 5b444e4 Compare March 8, 2024 08:19

haircommander reviewed Mar 8, 2024

View reviewed changes

if pullImageSecretRecheckPeriod>0 and KubeletEnsureSecretPulledImages…

51f8822

… is enabled, do recheck logic - if recheck is enabeld, recheck no auth image(pullCredentialsHash=="") as well

pacoxu force-pushed the ensure-secret-pulled-image branch from 7a120ec to 51f8822 Compare March 8, 2024 15:16

k8s-ci-robot removed this from the v1.30 milestone Mar 9, 2024

mikebrow mentioned this pull request Mar 11, 2024

blog: ensure secret pulled images kubernetes/website#45293

Closed

mikebrow mentioned this pull request Jun 4, 2024

KEP-2535: retarget alpha in 1.31 and update to readd the disk check kubernetes/enhancements#4693

Merged

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 7, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 7, 2024

sairameshv mentioned this pull request Jul 9, 2024

Implement KEP-2535 - Ensure secret pulled images feature #125817

Closed

k8s-ci-robot closed this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: ensure secret pulled image #114847

kubelet: ensure secret pulled image #114847

pacoxu commented Jan 5, 2023 •

edited

Loading

pacoxu commented Jan 5, 2023

liggitt Mar 7, 2024

mikebrow Mar 7, 2024

liggitt Mar 7, 2024

haircommander Mar 7, 2024

liggitt Mar 7, 2024

mikebrow Mar 7, 2024

mikebrow Mar 7, 2024 •

edited

Loading

pacoxu Mar 8, 2024

liggitt Mar 8, 2024

pacoxu Mar 8, 2024

liggitt Mar 7, 2024

mrunalp Mar 7, 2024

liggitt Mar 7, 2024

mikebrow Mar 7, 2024

liggitt Mar 7, 2024

mrunalp Mar 7, 2024

pacoxu Mar 7, 2024

liggitt Mar 7, 2024

mikebrow left a comment •

edited

Loading

liggitt commented Mar 7, 2024

mikebrow commented Mar 7, 2024

pacoxu commented Mar 7, 2024

haircommander Mar 8, 2024

pacoxu Mar 8, 2024

pacoxu Mar 8, 2024

mikebrow Mar 8, 2024 •

edited

Loading

mikebrow Mar 8, 2024

pacoxu commented Mar 9, 2024

k8s-triage-robot commented Jun 7, 2024

k8s-ci-robot commented Jun 7, 2024

k8s-triage-robot commented Jul 7, 2024

pacoxu commented Jul 11, 2024

k8s-ci-robot commented Jul 11, 2024

		ensuredInfo := digest.Auths[hash]
		if ensuredInfo != nil && ensuredInfo.ensured && ensuredInfo.lastEnsuredDate.Add(m.pullImageSecretRecheckPeriod.Duration).After(time.Now()) {

kubelet: ensure secret pulled image #114847

kubelet: ensure secret pulled image #114847

Conversation

pacoxu commented Jan 5, 2023 • edited Loading

Design Details: track a hash map for the credentials in kubelet

TODO List

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

pacoxu commented Jan 5, 2023

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

mikebrow Mar 7, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow left a comment • edited Loading

Choose a reason for hiding this comment

liggitt commented Mar 7, 2024

mikebrow commented Mar 7, 2024

pacoxu commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pacoxu commented Mar 9, 2024

k8s-triage-robot commented Jun 7, 2024

k8s-ci-robot commented Jun 7, 2024

k8s-triage-robot commented Jul 7, 2024

pacoxu commented Jul 11, 2024

k8s-ci-robot commented Jul 11, 2024

pacoxu commented Jan 5, 2023 •

edited

Loading

mikebrow Mar 7, 2024 •

edited

Loading

mikebrow left a comment •

edited

Loading

mikebrow Mar 8, 2024 •

edited

Loading