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

Implement credsStore in credential config files #1179

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

Implement credsStore in credential config files #1179

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e12be70
Use credstore form docker config
matejvasek Aug 23, 2021
1045cb0
Fix test reading non-testing config
matejvasek Aug 23, 2021
f448235
Add test for credStore
matejvasek Aug 23, 2021
7ed9a4b
Fix style
matejvasek Aug 23, 2021
ac05c2c
revert withTmpHome
matejvasek Aug 31, 2021
174d302
fixup
matejvasek Aug 31, 2021
7cac62b
add tests
matejvasek Sep 14, 2021
e5c4f94
more error handling
matejvasek Sep 14, 2021
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
25 changes: 25 additions & 0 deletions pkg/docker/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ type dockerAuthConfig struct {
type dockerConfigFile struct {
AuthConfigs map[string]dockerAuthConfig `json:"auths"`
CredHelpers map[string]string `json:"credHelpers,omitempty"`
CredsStore string `json:"credsStore,omitempty"`
}

type authPath struct {
Expand Down Expand Up @@ -85,6 +86,9 @@ func SetCredentials(sys *types.SystemContext, key, username, password string) (s
}
return false, setAuthToCredHelper(ch, key, username, password)
}
if auths.CredsStore != "" {
return false, setAuthToCredHelper(auths.CredsStore, key, username, password)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep the order of handling CredHelpers/CredsStore/AuthConfigs consistent throughout the package, to make it easier to check that all operations handle the same data. (I guess in the order above, unless there’s a specific reason to do something else.)

creds := base64.StdEncoding.EncodeToString([]byte(username + ":" + password))
newCreds := dockerAuthConfig{Auth: creds}
auths.AuthConfigs[key] = newCreds
Expand Down Expand Up @@ -156,6 +160,13 @@ func GetAllCredentials(sys *types.SystemContext) (map[string]types.DockerAuthCon
for registry := range auths.AuthConfigs {
addRegistry(registry)
}
if auths.CredsStore != "" {
if creds, err := listAuthsFromCredHelper(auths.CredsStore); err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it OK to completely ignore errors here? I’d expect this to work similarly to a helper returned by CredentialHelpers.

for registry := range creds {
addRegistry(registry)
}
}
}
}
// External helpers.
default:
Expand Down Expand Up @@ -440,6 +451,14 @@ func RemoveAllAuthentication(sys *types.SystemContext) error {
}
auths.CredHelpers = make(map[string]string)
auths.AuthConfigs = make(map[string]dockerAuthConfig)
if auths.CredsStore != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

RemoveAuthentication also needs adding the support.

if creds, err := listAuthsFromCredHelper(auths.CredsStore); err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it OK to completely ignore errors here? I’d expect this to work similarly to a helper returned by CredentialHelpers.

for registry := range creds {
_ = deleteAuthFromCredHelper(auths.CredsStore, registry)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

}
}

}
return true, nil
})
// External helpers.
Expand Down Expand Up @@ -646,6 +665,12 @@ func findAuthentication(ref reference.Named, registry, path string, legacyFormat
return getAuthFromCredHelper(ch, registry)
}

if auths.CredsStore != "" {
if cred, err := getAuthFromCredHelper(auths.CredsStore, registry); err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it OK to completely ignore errors here?

return cred, err
}
}

// Support for different paths in auth.
// (This is not a feature of ~/.docker/config.json; we support it even for
// those files as an extension.)
Expand Down
37 changes: 37 additions & 0 deletions pkg/docker/config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,7 @@ func TestGetAuthFailsOnBadInput(t *testing.T) {
}

func TestGetAllCredentials(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add tests for the other modified functions as well.

Copy link
Author

Choose a reason for hiding this comment

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

Please add tests for the other modified functions as well.

Are those functions properly tested right now anyway? I believe they are not tested with respect to helpers right now.

Copy link
Author

Choose a reason for hiding this comment

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

I believe they are not tested with respect to helpers right now.

For instance the mock helper does noting for the store operation so you can really assert it written anything when calling the SetCredentials function.

defer withTmpHome(t)()
// Create a temporary authentication file.
tmpFile, err := ioutil.TempFile("", "auth.json.")
require.NoError(t, err)
Expand Down Expand Up @@ -636,6 +637,42 @@ func TestGetAllCredentials(t *testing.T) {
require.Equal(t, d.password, conf.Password, "%v", d)
}

// test "credStore"
f, err := os.OpenFile(authFilePath, os.O_RDWR|os.O_TRUNC, 0644)
require.NoError(t, err)
_, err = f.Write([]byte(`{ "credsStore": "helper-registry" }`))
require.NoError(t, err)
err = f.Close()
require.NoError(t, err)
sys = types.SystemContext{
AuthFilePath: authFilePath,
SystemRegistriesConfPath: "",
SystemRegistriesConfDirPath: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is (sadly?) the same as leaving the values unset, in particular it doesn’t disable any ordinary users’ configuration. So if this data matters, it should point at real files.

Copy link
Author

Choose a reason for hiding this comment

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

@mtrmac Do I need to set it? I think that for this test AuthFilePath is enough, isn't it?

}
authConfigs, err = GetAllCredentials(&sys)
require.NoError(t, err)
require.Equal(t, 1, len(authConfigs))
require.Equal(t, "foo", authConfigs["registry-a.com"].Username)
require.Equal(t, "bar", authConfigs["registry-a.com"].Password)
}

func withTmpHome(t *testing.T) func() {
t.Helper()
dir, err := ioutil.TempDir("", "test-home")
if err != nil {
t.Fatal(err)
return func() {}
}
oldHome, oldHomeExists := os.LookupEnv("HOME")
os.Setenv("HOME", dir)
return func() {
if oldHomeExists {
os.Setenv("HOME", oldHome)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t immediately see that this is necessary (the existing GetAllCredentials test got by without), and anyway, AFAIK it doesn’t work; containers/storage/pkg/homedir.Get() caches the value throughout its lifetime, so a single-test override is either completely ineffective, or is effective only if that test happens to run first.

So, to make some of the code testable, we had to introduce getCredentialsWithHomeDir and I suspect something similar would be necessary in other cases that tried to override HOME.

Copy link
Author

Choose a reason for hiding this comment

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

@mtrmac without this the test is failing on my machine because the GetAllCredentials function also returns credentials from my docker config.

Copy link
Author

Choose a reason for hiding this comment

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

I you don't have $HOME/.docker/config.json on your machine it probably works. But for me it's failing.

Copy link
Author

Choose a reason for hiding this comment

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

caches the value throughout its lifetime, so a single-test override is either completely ineffective, or is effective only if that test happens to run first.

I don't what to do about this.

Copy link
Author

@matejvasek matejvasek Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The test is touching not only testing config files but also actual user config files, that's not good.

} else {
os.Unsetenv("HOME")
}
os.RemoveAll(dir)
}
}

func TestAuthKeysForRef(t *testing.T) {
Expand Down