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

Jump to bottom

feat: support generic secret in docker auth #494

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: support generic secret in docker auth #494

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
68004ba
support generic secret in docker auth
quixoten Aug 1, 2024
4ce88b6
simplify testing types
quixoten Aug 8, 2024
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
5 changes: 5 additions & 0 deletions e2e/deploy/vault/vault.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,11 @@ spec:
data:
DOCKER_REPO_USER: dockerrepouser
DOCKER_REPO_PASSWORD: dockerrepopassword
DOCKER_REPO_JSON_KEY: |
_json_key: {
"type": "service_account",
"project_id": "test"
}
- type: kv
path: secret/data/mysql
data:
Expand Down
20 changes: 20 additions & 0 deletions e2e/test/secret-docker-json-key.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
apiVersion: v1
kind: Secret
metadata:
name: test-secret-docker-json-key
annotations:
vault.security.banzaicloud.io/vault-addr: "https://vault.default.svc.cluster.local:8200"
vault.security.banzaicloud.io/vault-role: "default"
vault.security.banzaicloud.io/vault-tls-secret: vault-tls
# vault.security.banzaicloud.io/vault-skip-verify: "true"
vault.security.banzaicloud.io/vault-path: "kubernetes"
type: kubernetes.io/dockerconfigjson
stringData:
.dockerconfigjson: |
{
"auths": {
"https://index.docker.io/v1/": {
"auth": "dmF1bHQ6c2VjcmV0L2RhdGEvZG9ja2VycmVwbyNET0NLRVJfUkVQT19KU09OX0tFWQ=="
}
}
}
50 changes: 49 additions & 1 deletion e2e/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,62 @@ func TestSecretValueInjection(t *testing.T) {
err = json.Unmarshal(secret.Data[".dockerconfigjson"], &dockerconfigjson)
require.NoError(t, err)

dockerrepoauth := base64.StdEncoding.EncodeToString([]byte("dockerrepouser:dockerrepopassword"))
assert.Equal(t, "dockerrepouser", dockerconfigjson.Auths.V1.Username)
assert.Equal(t, "dockerrepopassword", dockerconfigjson.Auths.V1.Password)
assert.Equal(t, dockerrepoauth, dockerconfigjson.Auths.V1.Auth)
assert.Equal(t, "Inline: secretId AWS_ACCESS_KEY_ID", string(secret.Data["inline"]))

return ctx
}).
Feature()

secretDockerJsonKey := applyResource(features.New("secret"), "secret-docker-json-key.yaml").
Assess("object created", func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context {
secrets := &v1.SecretList{
Items: []v1.Secret{
{
ObjectMeta: metav1.ObjectMeta{Name: "test-secret-docker-json-key", Namespace: cfg.Namespace()},
},
},
}

// wait for the secret to become available
err := wait.For(conditions.New(cfg.Client().Resources()).ResourcesFound(secrets), wait.WithTimeout(1*time.Minute))
require.NoError(t, err)

return ctx
}).
Assess("secret values are injected", func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context {
var secret v1.Secret

err := cfg.Client().Resources(cfg.Namespace()).Get(ctx, "test-secret-docker-json-key", cfg.Namespace(), &secret)
require.NoError(t, err)

type v1 struct {
Auth string `json:"auth"`
}

type auths struct {
V1 v1 `json:"https://index.docker.io/v1/"`
}

type dockerconfig struct {
Auths auths `json:"auths"`
}
quixoten marked this conversation as resolved.
Show resolved Hide resolved

var dockerconfigjson dockerconfig

err = json.Unmarshal(secret.Data[".dockerconfigjson"], &dockerconfigjson)
require.NoError(t, err)

dockerrepoauth := base64.StdEncoding.EncodeToString([]byte("_json_key: {\n \"type\": \"service_account\",\n \"project_id\": \"test\"\n}\n"))
assert.Equal(t, dockerrepoauth, dockerconfigjson.Auths.V1.Auth)

return ctx
}).
Feature()

configMap := applyResource(features.New("configmap"), "configmap.yaml").
Assess("object created", func(ctx context.Context, t *testing.T, cfg *envconf.Config) context.Context {
configMaps := &v1.ConfigMapList{
Expand Down Expand Up @@ -120,7 +168,7 @@ func TestSecretValueInjection(t *testing.T) {
}).
Feature()

testenv.Test(t, secret, configMap)
testenv.Test(t, secret, secretDockerJsonKey, configMap)
}

func TestPodMutation(t *testing.T) {
Expand Down
54 changes: 33 additions & 21 deletions pkg/webhook/secret.go
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

This approach is rather forgiving, as it treats all cases where the authentication is not via username and password, as _json_key right away, without verification. This is quite error-prone.

Copy link
Author

Choose a reason for hiding this comment

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

I probably should have avoided using the specific _json_key as the example. The implementation should actually cover any case. Let me see if i can explain better given the following example:

    {
        "auths": {
            "https://index.docker.io/v1/": {
                "auth": "vault:secret/data/dockerrepo#ANY_VALUE"
            }
        }
    }

(showing the auth value base64 decoded for clarity)

previously, the webhook would make no attempt to unwrap this auth value, because it doesn't match the user:pass format it expects, i.e., vault:secret/data/creds#USER:vault:secret/data/creds#PW. instead, it would fail with the splitting auth credentials failed error. there are certainly issues with the previous implementation. one being that it is restrictive, and another being that it doesn't support transit values, i.e., vault:v1:aGkK:vault:v1:YnllCg== could be a valid username:password but will fail with the same splitting auth credentials failed error.

my goal with this change is to leave the previous functionality (including assumptions) in tact, but opening up the possibility for the user to put whatever they need to inside of the auth value. instead of failing with the splitting auth credentials failed error, it will now make an attempt to unwrap the value (we've already verified it starts with the vault prefix at this point) and, if successful, swap it in. this gives the user full control of the auth value. it could be a _json_key or it could be user:pass or anything else.

Copy link
Member

Choose a reason for hiding this comment

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

Your observation is astute. Indeed, the main issue here is that using len(split) for control flow can be quite misleading for someone reading the code. It's not immediately clear how the length of the split array relates to the logic being implemented, which can make the code harder to understand and maintain. A more explicit and self-explanatory approach to control flow would likely improve code readability and reduce potential confusion for other developers working with this code.

The thing I wrote about verification, is for the same reason. If someone sees a verification function being called e.g.:

json.Valid(auth) // verifying JSON format
isValidPrefix(string(auth)) // verifying whether a string is a valid Vault path that can be consumed by the injector.

Immediately knows what is happening, and what values can be used.

Original file line number Diff line number Diff line change
Expand Up @@ -138,30 +138,42 @@ func (mw *MutatingWebhook) mutateDockerCreds(secret *corev1.Secret, dc *dockerCr
auth := string(authBytes)
if common.HasVaultPrefix(auth) {
split := strings.Split(auth, ":")
if len(split) != 4 {
return errors.New("splitting auth credentials failed")
}
username := fmt.Sprintf("%s:%s", split[0], split[1])
password := fmt.Sprintf("%s:%s", split[2], split[3])
if len(split) == 4 {
username := fmt.Sprintf("%s:%s", split[0], split[1])
password := fmt.Sprintf("%s:%s", split[2], split[3])

credentialData := map[string]string{
"username": username,
"password": password,
}
credentialData := map[string]string{
"username": username,
"password": password,
}

dcCreds, err := secretInjector.GetDataFromVault(credentialData)
if err != nil {
return err
}
auth = fmt.Sprintf("%s:%s", dcCreds["username"], dcCreds["password"])
dockerAuth := dockerAuthConfig{
Auth: base64.StdEncoding.EncodeToString([]byte(auth)),
}
if creds.Username != "" && creds.Password != "" {
dockerAuth.Username = dcCreds["username"]
dockerAuth.Password = dcCreds["password"]
dcCreds, err := secretInjector.GetDataFromVault(credentialData)
if err != nil {
return err
}
auth = fmt.Sprintf("%s:%s", dcCreds["username"], dcCreds["password"])
dockerAuth := dockerAuthConfig{
Auth: base64.StdEncoding.EncodeToString([]byte(auth)),
}
if creds.Username != "" && creds.Password != "" {
dockerAuth.Username = dcCreds["username"]
dockerAuth.Password = dcCreds["password"]
}
assembled.Auths[key] = dockerAuth
} else {
credentialData := map[string]string{
"auth": auth,
}

dcCreds, err := secretInjector.GetDataFromVault(credentialData)
if err != nil {
return err
}
dockerAuth := dockerAuthConfig{
Auth: base64.StdEncoding.EncodeToString([]byte(dcCreds["auth"])),
}
assembled.Auths[key] = dockerAuth
}
assembled.Auths[key] = dockerAuth
}
}

Expand Down
Loading