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: support generic secret in docker auth #494

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

Conversation

quixoten
Copy link

@quixoten quixoten commented Aug 1, 2024

Overview

currently, the webhook only supports unwrapping the value of the auth key if it's in a specific user:pass format. this limitation prevents the webhook from working correctly for other valid formats of the auth value, e.g., _json_key.

@quixoten quixoten requested a review from a team as a code owner August 1, 2024 13:40
@quixoten quixoten requested review from sagikazarmark and removed request for a team August 1, 2024 13:40
@github-actions github-actions bot added the size/M Denotes a PR that changes 100-499 lines label Aug 1, 2024
@quixoten quixoten changed the title support generic secret in docker auth feat: support generic secret in docker auth Aug 1, 2024
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.

e2e/webhook_test.go Outdated Show resolved Hide resolved
@csatib02
Copy link
Member

csatib02 commented Aug 2, 2024

Hey @quixoten,
First of all thank you for your contribution!
Vault-Secrets-Webhook, will no longer be maintained in the near future, because it will be replaced with our new Webhook project.
I already added this feature to that one, but adding an e2e test-case for json_key handling seems reasonable, may I ask you to please take a look at Secrets-Webhook and if you have the time for it incorporate your changes regarding the e2e tests on that project?

In the meanwhile I left a few comments.
Also, please sign your commits, there are instructions if you click on the failed DCO check.

@quixoten quixoten force-pushed the dc/docker-auth-generic branch 2 times, most recently from 52dd1b0 to c0338a9 Compare August 3, 2024 15:06
currently, we only support unwrapping the auth key if its value supports
a specific format. this change will fallback to generic unwrapping of
the auth value when it doesn't match the user:pass format.

Signed-off-by: Devin Christensen <[email protected]>
e2e/webhook_test.go Outdated Show resolved Hide resolved
Signed-off-by: Devin Christensen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 100-499 lines
Projects
Status: 🏗 In progress
3 participants