-
Notifications
You must be signed in to change notification settings - Fork 2k
Use DOCKER_AUTH_CONFIG
env as credential store
#6008
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6008 +/- ##
==========================================
+ Coverage 55.03% 55.13% +0.09%
==========================================
Files 361 362 +1
Lines 30153 30271 +118
==========================================
+ Hits 16596 16690 +94
- Misses 12599 12620 +21
- Partials 958 961 +3 🚀 New features to boost your workflow:
|
866c70a
to
eb43744
Compare
e7c6f4d
to
b3aa07f
Compare
de90f77
to
b7770d7
Compare
d801989
to
0417f99
Compare
0417f99
to
5e95a08
Compare
This patch enables the CLI to natively pick up the `DOCKER_AUTH_CONFIG` environment variable and use it as a credential store. The `DOCKER_AUTH_CONFIG` value should be a JSON object and must store the credentials in a base64 encoded string under the `auth` key. For example: `printf "username:pat" | openssl base64 -A` `export DOCKER_AUTH_CONFIG='{ "auths": { "https://index.docker.io/v1/": { "auth": "aGk6KTpkY2tyX3BhdF9oZWxsbw==" } } }'` Credentials stored in `DOCKER_AUTH_CONFIG` would take precedence over any credential stored in the file store (`~/.docker/config.json`) or native store (credential helper). Destructive actions, such as deleting a credential would result in a noop if found in the environment credential. Credentials found in the file or native store would get removed. Signed-off-by: Alano Terblanche <[email protected]>
5e95a08
to
63c990c
Compare
@@ -46,6 +48,49 @@ type ConfigFile struct { | |||
Experimental string `json:"experimental,omitempty"` | |||
} | |||
|
|||
type configEnvAuth struct { | |||
Auth string `json:"auth"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably keep the omitempty
to match the "actual" types.AuthConfig
;
cli/cli/config/types/authconfig.go
Line 7 in 9e50654
Auth string `json:"auth,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we can't here. This is enforced if the environment variable is set.
if c.AuthConfigs == nil { | ||
c.AuthConfigs = make(map[string]configEnvAuth) | ||
} | ||
if err := json.NewDecoder(strings.NewReader(v)).Decode(c); err != nil && !errors.Is(err, io.EOF) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using json.NewDecoder
because it was designed to handle JSON streams (multiple records). We're doing that wrong in various places (some were probably intended as optimisation), but in this case we only expect a single JSON document, so better to not use;
https://go.dev/play/p/ehHfvnCkGDL
package main
import (
"encoding/json"
"errors"
"fmt"
"io"
"strings"
)
type Foo struct {
Name string
}
func main() {
const data = `{"Name":"foo"}{"Name":"bar"}{"Name":"baz"}`
decoder := json.NewDecoder(strings.NewReader(data))
var p Foo
for {
err := decoder.Decode(&p)
if err != nil {
if errors.Is(err, io.EOF) {
fmt.Println("end of stream")
break
}
fmt.Println(err)
return
}
fmt.Println(p)
}
err := json.Unmarshal([]byte(data), &p)
if err != nil {
fmt.Println(err)
return
}
fmt.Println(p)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copied it from the current implementation -
cli/cli/config/configfile/file.go
Lines 117 to 119 in 63c990c
if err := json.NewDecoder(configData).Decode(configFile); err != nil && !errors.Is(err, io.EOF) { | |
return err | |
} |
authConfigs := make(map[string]types.AuthConfig) | ||
for addr, envAuth := range c.AuthConfigs { | ||
if envAuth.Auth == "" { | ||
return authConfigs, fmt.Errorf("DOCKER_AUTH_CONFIG environment variable is missing auth for %s", addr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just ignore these; when using credential-stores, these will always be an empty struct;
cat ~/.docker/config.json
{
"auths": {
"https://index.docker.io/v1/": {},
"https://index.docker.io/v1/access-token": {},
"https://index.docker.io/v1/refresh-token": {}
},
"credsStore": "desktop"
}
I think it's fine to ignore those; while we probably don't expect mixed credentials-store vs "plain-text" entries, I guess technically this file is valid;
{
"auths": {
"env.example.test": {
"auth": "ZW52X3VzZXI6ZW52X3Bhc3M="
},
"https://index.docker.io/v1/": {},
"https://index.docker.io/v1/access-token": {},
"https://index.docker.io/v1/refresh-token": {}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't agree, this is a very explicit thing to set and would be up to the user to set it correctly. I think returning an error when the config isn't what we expect would be better.
v := os.Getenv("DOCKER_AUTH_CONFIG") | ||
if v == "" { | ||
return errDockerAuthConfigNotSet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... so we're always returning an error unless the env-var is set.
Wondering if we should keep the logic to construct a memory store in the first place "conditional", and only try to use these options if it's either set, or set and non-empty
envConfig := &configEnv{} | ||
err := envConfig.LoadFromEnv() | ||
if err != nil { | ||
// ignore if DOCKER_AUTH_CONFIG is not set | ||
if errors.Is(err, errDockerAuthConfigNotSet) { | ||
return nil | ||
} | ||
return err | ||
} | ||
|
||
authConfigs, err := envConfig.GetAuthConfigs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit convoluted; in the end, what we need is;
- A string containing a (valid) JSON-encoded authconfig
- That's decoded to a
map[string]types.AuthConfig
i.e., this could look something like;
authConfigs, err := unmarshalAuthConfig(v)
if err != nil {
return err
}
return memorystore.WithAuthConfig(authConfigs)(c)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me try to draw this up quickly; will post later
"auths": { | ||
"env.example.test": { | ||
"auth": "ZW52X3VzZXI6ZW52X3Bhc3M=" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment about "empty" values here; perhaps we should have that as a test-case to verify that behavior;
"auths": { | |
"env.example.test": { | |
"auth": "ZW52X3VzZXI6ZW52X3Bhc3M=" | |
} | |
} | |
"auths": { | |
"env.example.test": { | |
"auth": "ZW52X3VzZXI6ZW52X3Bhc3M=" | |
}, | |
"https://index.docker.io/v1/": {}, | |
"https://index.docker.io/v1/access-token": {}, | |
"https://index.docker.io/v1/refresh-token": {} | |
} |
|
This patch enables the CLI to natively pick up the
DOCKER_AUTH_CONFIG
environment variable and use it as a credential store.
The
DOCKER_AUTH_CONFIG
value should be a JSON object and must storethe credentials in a base64 encoded string under the
auth
key.Credentials stored in
DOCKER_AUTH_CONFIG
would take precedence over anycredential stored in the file store (
~/.docker/config.json
) or native store(credential helper).
Destructive actions, such as deleting a credential would result in a noop if
found in the environment credential. Credentials found in the file or
native store would get removed.
- What I did
- How I did it
- How to verify it
printf "username:pat" | openssl base64 -A
Setup the
DOCKER_AUTH_CONFIG
environment variable- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)