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

feat: add e2e test for docker auth with _json_key #132

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

quixoten
Copy link
Contributor

@quixoten quixoten commented Aug 3, 2024

Overview

@csatib02, i've add the e2e tests from bank-vaults/vault-secrets-webhook#494. however, they're currently failing:

expected: "X2pzb25fa2V5OiB7CiAgInR5cGUiOiAic2VydmljZV9hY2NvdW50IiwKICAicHJvamVjdF9pZCI6ICJ0ZXN0Igp9Cg=="
actual  : "dmF1bHQ6c2VjcmV0L2RhdGEvZG9ja2VycmVwbyNET0NLRVJfUkVQT19KU09OX0tFWQ=="

or

expected: "_json_key: {\n  "type": "service_account",\n  "project_id": "test"\n}"
actual  : "vault:secret/data/dockerrepo#DOCKER_REPO_JSON_KEY"

So it's not currently unwrapping the value. I can look into this more later, but figured you might know immediately what the issue is.

@quixoten quixoten requested a review from a team as a code owner August 3, 2024 16:34
@quixoten quixoten requested review from csatib02 and removed request for a team August 3, 2024 16:34
@github-actions github-actions bot added the size/S Denotes a PR that changes 10-99 lines label Aug 3, 2024
@quixoten quixoten force-pushed the dc/docker-auth-generic branch from 182a1bf to ba2ddbe Compare August 3, 2024 16:46
@ramizpolic ramizpolic changed the title add e2e test for docker auth with _json_key feat: add e2e test for docker auth with _json_key Aug 4, 2024
Copy link
Member

@csatib02 csatib02 left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, the reason why the test fails, is because you have to add: secrets-webhook.security.bank-vaults.io/provider: vault to the annotations.

e2e/test/secret-docker-json-key.yaml Outdated Show resolved Hide resolved
e2e/webhook_test.go Outdated Show resolved Hide resolved
e2e/test/secret-docker-json-key.yaml Outdated Show resolved Hide resolved
@csatib02 csatib02 added the kind/test Categorizes issue or PR as related to testing. label Aug 5, 2024
- added correct annotations to the secret
- pulled duplicated types out to shared context

Signed-off-by: Devin Christensen <[email protected]>
@quixoten quixoten force-pushed the dc/docker-auth-generic branch from 8ac8c92 to d247832 Compare August 5, 2024 19:16
@quixoten
Copy link
Contributor Author

quixoten commented Aug 5, 2024

@csatib02, test is still failing because the unwrapped value from vault is not being base64 encoded:

--- Expected
+++ Actual
@@ -1 +1,5 @@
-X2pzb25fa2V5OiB7CiAgInR5cGUiOiAic2VydmljZV9hY2NvdW50IiwKICAicHJvamVjdF9pZCI6ICJ0ZXN0Igp9Cg==
+_json_key: {
+  "type": "service_account",
+  "project_id": "test"
+}
+

i was looking in the docker source code to try to confirm that they're always treating the value of auth as a base64 encoded string, but didn't find anything definitive.

@csatib02
Copy link
Member

csatib02 commented Aug 6, 2024

Visiting the code I noticed a mistake in the AssembleDockerAuthConfig() function. Encoding of the auth field was always overwritten with the retrieved data, when the authType was a JSON or a single Vault path.

If you change the two functions to the following, then the tests will pass.
(The function signature was also changed in case of the last one here!)

// assembleCredentialData assembles the credential data that will be retrieved from Vault
func AssembleCredentialData(authCreds map[string]string) (map[string]string, error) {
	if username, ok := authCreds["username"]; ok {
		if password, ok := authCreds["password"]; ok {
			return map[string]string{
				"username": username,
				"password": password,
			}, nil
		}
	}

	if auth, ok := authCreds["auth"]; ok {
		return map[string]string{
			"auth": auth,
		}, nil
	}

	return nil, fmt.Errorf("no valid credentials found")
}

// assembleDockerAuthConfig assembles the DockerAuthConfig from the retrieved data from Vault
func AssembleDockerAuthConfig(dcCreds map[string]string) DockerAuthConfig {
	if username, ok := dcCreds["username"]; ok {
		if password, ok := dcCreds["password"]; ok {
			return DockerAuthConfig{
				Username: username,
				Password: password,
				Auth:     base64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", username, password))),
			}
		}
	}

	if auth, ok := dcCreds["auth"]; ok {
		return DockerAuthConfig{
			Auth: base64.StdEncoding.EncodeToString([]byte(auth)),
		}
	}

	return DockerAuthConfig{}
}

@quixoten quixoten force-pushed the dc/docker-auth-generic branch from 61c85ed to 17e4aef Compare August 6, 2024 15:47
@github-actions github-actions bot added size/M Denotes a PR that changes 100-499 lines and removed size/S Denotes a PR that changes 10-99 lines labels Aug 6, 2024
Copy link
Member

@csatib02 csatib02 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for working on this! 🚀

@csatib02 csatib02 merged commit 61784d9 into bank-vaults:main Aug 7, 2024
24 checks passed
@quixoten quixoten deleted the dc/docker-auth-generic branch August 7, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/test Categorizes issue or PR as related to testing. size/M Denotes a PR that changes 100-499 lines
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants