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.

Sign up for GitHub

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

Ensure secret pulled images #94899

Closed
wants to merge 1 commit into from
Closed

Ensure secret pulled images #94899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions pkg/features/kube_features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,14 @@ const (
//
// Stop auto-generation of secret-based service account tokens.
LegacyServiceAccountTokenNoAutoGeneration featuregate.Feature = "LegacyServiceAccountTokenNoAutoGeneration"

// owner: @mikebrow
// kep: http://kep.k8s.io/2535
// alpha: v1.24
//
// Enables kubelet to ensure images pulled with pod imagePullSecrets are authenticated
// by other pods that do not have the same credentials.
Comment on lines +834 to +835
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what this means and that I can't explain it to other people. I could read the code; I haven't, because I'm checking whether the docs part makes sense.

What effect does this feature gate have?

Copy link
Member Author

@mikebrow mikebrow Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Described in more detail in the KEP. The sentence presumes the reader knows what kubelet is, container images, pods, image pull secrets (credentials), and authentication to registries using existing image pull policies and said credentials... and yes, that is a lot of assumptions. There are existing descriptions for the pull if not present, pull always policies, and warnings for using mitigations to force the pull always policy, because kubelet does not currently "ensure images pulled with pod imagePullSecrets are authenticated by other pods that do not have the same credentials" when pull always is not requested. We could adds links. This is a somewhat complex topic regarding currently known potential security vulnerabilities and subsequent performance issues with current mitigations.

Suggestions, to simplify the description are more than welcome.

Looks like we will not be merging in this release... Instead we will add in additional phase II suggested items, such as persistence of the ensure metadata, a gc sync process with container runtime image cache, (possibly) a solution for identifying which container images are loaded to be used as never pull images for the pod pull never container image policy, and a few other TODO items that were proposed for the next phase.

Copy link
Member Author

@mikebrow mikebrow Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

overly simple explanation:
feature gate off (current behavior)
pod spec A has a private image that requires credentials (aka image pull secrets)
pod spec B uses the private image (This pod doesn't need credentials because the image is already cached)

feature gate on (desired behavior)
pod spec B also needs credentials to use the private image

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we switch to a revised comment that explains this clearly enough?
(I'm afraid I'm convinced that the current text falls some way short).

Copy link
Member Author

Choose a reason for hiding this comment

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

will do

KubeletEnsureSecretPulledImages featuregate.Feature = "KubeletEnsureSecretPulledImages"
)

func init() {
Expand Down Expand Up @@ -947,6 +955,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
RecoverVolumeExpansionFailure: {Default: false, PreRelease: featuregate.Alpha},
GRPCContainerProbe: {Default: false, PreRelease: featuregate.Alpha},
LegacyServiceAccountTokenNoAutoGeneration: {Default: true, PreRelease: featuregate.Beta},
KubeletEnsureSecretPulledImages: {Default: false, PreRelease: featuregate.Alpha},

// inherited features from generic apiserver, relisted here to get a conflict if it is changed
// unintentionally on either side:
Expand Down
9 changes: 9 additions & 0 deletions pkg/kubelet/container/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"encoding/json"
"fmt"
"hash/fnv"
"strconv"
"strings"

"k8s.io/klog/v2"
Expand Down Expand Up @@ -107,6 +108,14 @@ func HashContainer(container *v1.Container) uint64 {
return uint64(hash.Sum32())
}

// HashAuth - returns a hash code for a CRI pull image auth
func HashAuth(auth *runtimeapi.AuthConfig) string {
mikebrow marked this conversation as resolved.
Show resolved Hide resolved
hash := fnv.New64a()
Copy link
Member

Choose a reason for hiding this comment

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

fnv is a non-cryptographic hash algorithm... squinting at whether that matters here, but non-cryptographic + credentials + security make me squint

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we switch to sha256 so when we add persistence it is less of a worry.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could add a TODO if you like

Copy link
Contributor

Choose a reason for hiding this comment

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

I think changing to sha256 would be ideal (or even making this configurable to future-proof this?)

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe opencontainers/go-digest

authJSON, _ := json.Marshal(auth)
hashutil.DeepHashObject(hash, authJSON)
return strconv.FormatUint(hash.Sum64(), 16)
}

// envVarsToMap constructs a map of environment name to value from a slice
// of env vars.
func envVarsToMap(envs []EnvVar) map[string]string {
Expand Down
50 changes: 50 additions & 0 deletions pkg/kubelet/container/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
utilfeature "k8s.io/apiserver/pkg/util/feature"
featuregatetesting "k8s.io/component-base/featuregate/testing"
runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1"
"k8s.io/kubernetes/pkg/features"
)

Expand Down Expand Up @@ -699,3 +700,52 @@ func TestShouldRecordEvent(t *testing.T) {
_, actual = innerEventRecorder.shouldRecordEvent(nilObj)
assert.Equal(t, false, actual, "should not panic if the typed nil was used, see https://github.com/kubernetes/kubernetes/issues/95552")
}

func TestHashAuth(t *testing.T) {
testUser := "username"
testPasswd := "password"
testAuth := testUser + ":" + testPasswd
testServer := "https://registry-1.io"
testIdentityToken := "kas9Da81Dfa8"
testRegistryToken :=
`eyJ0eXAiOiJKV1QiLCJhbGciOiJFUzI1NiIsImtpZCI6IlBZWU86VEVXVTpWN0pIOjI2SlY6QVFUWjpMSkMzOlNYVko6WEdIQTozNEYyOjJMQVE6WlJNSzpaN1E2In0.eyJpc3MiOiJhdXRoLmRvY2tlci5jb20iLCJzdWIiOiJqbGhhd24iLCJhdWQiOiJyZWdpc3RyeS5kb2NrZXIuY29tIiwiZXhwIjoxNDE1Mzg3MzE1LCJuYmYiOjE0MTUzODcwMTUsImlhdCI6MTQxNTM4NzAxNSwianRpIjoidFlKQ08xYzZjbnl5N2tBbjBjN3JLUGdiVjFIMWJGd3MiLCJhY2Nlc3MiOlt7InR5cGUiOiJyZXBvc2l0b3J5IiwibmFtZSI6InNhbWFsYmEvbXktYXBwIiwiYWN0aW9ucyI6WyJwdXNoIl19XX0.QhflHPfbd6eVF4lM9bwYpFZIV0PfikbyXuLx959ykRTBpe3CYnzs6YBK8FToVb5R47920PVLrh8zuLzdCr9t3w`

testCases := []struct {
auth *runtimeapi.AuthConfig
expectedHash string
}{
{
auth: &runtimeapi.AuthConfig{
IdentityToken: testIdentityToken},
expectedHash: "4d3e035376aaa6fe",
},
{
auth: &runtimeapi.AuthConfig{
Username: testUser,
Password: testPasswd,
ServerAddress: testServer},
expectedHash: "deed1b4ea7ff40f9",
},
{
auth: &runtimeapi.AuthConfig{
Auth: testAuth,
ServerAddress: testServer},
expectedHash: "f5017bcbcc0fbffc",
},
{
auth: &runtimeapi.AuthConfig{
ServerAddress: testServer,
RegistryToken: testRegistryToken},
expectedHash: "28256605b2151d68",
},
{
auth: &runtimeapi.AuthConfig{},
expectedHash: "42a21bc689278e90",
},
}

for _, tc := range testCases {
hashVal := HashAuth(tc.auth)
assert.Equal(t, tc.expectedHash, hashVal, "the hash value here should not be changed.")
}
}
5 changes: 3 additions & 2 deletions pkg/kubelet/container/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,9 @@ type StreamingRuntime interface {
// ImageService interfaces allows to work with image service.
type ImageService interface {
// PullImage pulls an image from the network to local storage using the supplied
// secrets if necessary. It returns a reference (digest or ID) to the pulled image.
PullImage(image ImageSpec, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) (string, error)
// secrets if necessary. It returns a reference (digest or ID) to the pulled image
// and if applicable a hash for the auth used to successfully pull the image.
PullImage(image ImageSpec, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) (string, string, error)
// GetImageRef gets the reference (digest or ID) of the image which has already been in
// the local storage. It returns ("", nil) if the image isn't in the local storage.
GetImageRef(image ImageSpec) (string, error)
Expand Down
4 changes: 2 additions & 2 deletions pkg/kubelet/container/testing/fake_runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ func (f *FakeRuntime) GetContainerLogs(_ context.Context, pod *v1.Pod, container
return f.Err
}

func (f *FakeRuntime) PullImage(image kubecontainer.ImageSpec, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) (string, error) {
func (f *FakeRuntime) PullImage(image kubecontainer.ImageSpec, pullSecrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) (string, string, error) {
f.Lock()
defer f.Unlock()

Expand All @@ -305,7 +305,7 @@ func (f *FakeRuntime) PullImage(image kubecontainer.ImageSpec, pullSecrets []v1.
}
f.ImageList = append(f.ImageList, i)
}
return image.Image, f.Err
return image.Image, "", f.Err
}

func (f *FakeRuntime) GetImageRef(image kubecontainer.ImageSpec) (string, error) {
Expand Down
30 changes: 24 additions & 6 deletions pkg/kubelet/container/testing/runtime_mock.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions pkg/kubelet/cri/remote/remote_runtime_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ func TestVersion(t *testing.T) {
r := createRemoteRuntimeService(endpoint, t)
version, err := r.Version(apitest.FakeVersion)
assert.NoError(t, err)
assert.Equal(t, apitest.FakeVersion, version.Version)
assert.Equal(t, apitest.FakeRuntimeName, version.RuntimeName)
if version != nil { // TODO: (Mike Brown) refactor for startup timing issues
assert.Equal(t, apitest.FakeVersion, version.Version)
assert.Equal(t, apitest.FakeRuntimeName, version.RuntimeName)
}
}
4 changes: 2 additions & 2 deletions pkg/kubelet/images/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ type throttledImageService struct {
limiter flowcontrol.RateLimiter
}

func (ts throttledImageService) PullImage(image kubecontainer.ImageSpec, secrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) (string, error) {
func (ts throttledImageService) PullImage(image kubecontainer.ImageSpec, secrets []v1.Secret, podSandboxConfig *runtimeapi.PodSandboxConfig) (string, string, error) {
if ts.limiter.TryAccept() {
return ts.ImageService.PullImage(image, secrets, podSandboxConfig)
}
return "", fmt.Errorf("pull QPS exceeded")
return "", "", fmt.Errorf("pull QPS exceeded")
}
Loading