From b162d8868c0b14961347c80df834f93f0e7e82a6 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 20:13:21 +0200 Subject: Add unit tests for multiAuthHeader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also rename it to parseMultiAuthHeader. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 8 ++++---- pkg/auth/auth_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 4 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 070e222ad..73822eecf 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -21,7 +21,7 @@ type HeaderAuthName string func (h HeaderAuthName) String() string { return string(h) } // XRegistryAuthHeader is the key to the encoded registry authentication configuration in an http-request header. -// This header supports one registry per header occurrence. To support N registries provided N headers, one per registry. +// This header supports one registry per header occurrence. To support N registries provide N headers, one per registry. // As of Docker API 1.40 and Libpod API 1.0.0, this header is supported by all endpoints. const XRegistryAuthHeader HeaderAuthName = "X-Registry-Auth" @@ -108,7 +108,7 @@ func getConfigCredentials(r *http.Request) (*types.DockerAuthConfig, string, err // should be removed after usage. func getAuthCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { // First look for a multi-auth header (i.e., a map). - authConfigs, err := multiAuthHeader(r) + authConfigs, err := parseMultiAuthHeader(r) if err == nil { authfile, err := authConfigsToAuthFile(authConfigs) return nil, authfile, err @@ -327,9 +327,9 @@ func singleAuthHeader(r *http.Request) (map[string]types.DockerAuthConfig, error return authConfigs, nil } -// multiAuthHeader extracts a DockerAuthConfig from the request's header. +// parseMultiAuthHeader extracts a DockerAuthConfig from the request's header. // The header content is a map[string]DockerAuthConfigs. -func multiAuthHeader(r *http.Request) (map[string]types.DockerAuthConfig, error) { +func parseMultiAuthHeader(r *http.Request) (map[string]types.DockerAuthConfig, error) { authHeader := r.Header.Get(string(XRegistryAuthHeader)) // Accept "null" and handle it as empty value for compatibility reason with Docker. // Some java docker clients pass this value, e.g. this one used in Eclipse. diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index da2d9a5c5..97e7fe1ec 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -1,11 +1,14 @@ package auth import ( + "encoding/base64" "io/ioutil" + "net/http" "testing" "github.com/containers/image/v5/types" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestAuthConfigsToAuthFile(t *testing.T) { @@ -64,3 +67,39 @@ func TestAuthConfigsToAuthFile(t *testing.T) { } } } + +func TestParseMultiAuthHeader(t *testing.T) { + for _, tc := range []struct { + input string + shouldErr bool + expected map[string]types.DockerAuthConfig + }{ + // Empty header + {input: "", expected: nil}, + // "null" + {input: "null", expected: nil}, + // Invalid JSON + {input: "@", shouldErr: true}, + // Success + { + input: base64.URLEncoding.EncodeToString([]byte( + `{"https://index.docker.io/v1/":{"username":"u1","password":"p1"},` + + `"quay.io/libpod":{"username":"u2","password":"p2"}}`)), + expected: map[string]types.DockerAuthConfig{ + "https://index.docker.io/v1/": {Username: "u1", Password: "p1"}, + "quay.io/libpod": {Username: "u2", Password: "p2"}, + }, + }, + } { + req, err := http.NewRequest(http.MethodPost, "/", nil) + require.NoError(t, err, tc.input) + req.Header.Set(XRegistryAuthHeader.String(), tc.input) + res, err := parseMultiAuthHeader(req) + if tc.shouldErr { + assert.Error(t, err, tc.input) + } else { + require.NoError(t, err, tc.input) + assert.Equal(t, tc.expected, res, tc.input) + } + } +} -- cgit v1.2.3-54-g00ecf From ff003928b2360304f6b9458d324df090917fab02 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 20:24:07 +0200 Subject: Add unit tests for singleAuthHeader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also rename it to parseSingleAuthHeader Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 6 +++--- pkg/auth/auth_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 3 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 73822eecf..60c40a40b 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -115,7 +115,7 @@ func getAuthCredentials(r *http.Request) (*types.DockerAuthConfig, string, error } // Fallback to looking for a single-auth header (i.e., one config). - authConfigs, err = singleAuthHeader(r) + authConfigs, err = parseSingleAuthHeader(r) if err != nil { return nil, "", err } @@ -309,9 +309,9 @@ func imageAuthToDockerAuth(authConfig types.DockerAuthConfig) dockerAPITypes.Aut } } -// singleAuthHeader extracts a DockerAuthConfig from the request's header. +// parseSingleAuthHeader extracts a DockerAuthConfig from the request's header. // The header content is a single DockerAuthConfig. -func singleAuthHeader(r *http.Request) (map[string]types.DockerAuthConfig, error) { +func parseSingleAuthHeader(r *http.Request) (map[string]types.DockerAuthConfig, error) { authHeader := r.Header.Get(string(XRegistryAuthHeader)) authConfig := dockerAPITypes.AuthConfig{} // Accept "null" and handle it as empty value for compatibility reason with Docker. diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index 97e7fe1ec..a0b97b106 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -68,6 +68,43 @@ func TestAuthConfigsToAuthFile(t *testing.T) { } } +func TestParseSingleAuthHeader(t *testing.T) { + for _, tc := range []struct { + input string + shouldErr bool + expected map[string]types.DockerAuthConfig + }{ + { + input: "", // An empty (or missing) header + expected: map[string]types.DockerAuthConfig{"0": {}}, + }, + { + input: "null", + expected: map[string]types.DockerAuthConfig{"0": {}}, + }, + // Invalid JSON + {input: "@", shouldErr: true}, + // Success + { + input: base64.URLEncoding.EncodeToString([]byte(`{"username":"u1","password":"p1"}`)), + expected: map[string]types.DockerAuthConfig{ + "0": {Username: "u1", Password: "p1"}, + }, + }, + } { + req, err := http.NewRequest(http.MethodPost, "/", nil) + require.NoError(t, err, tc.input) + req.Header.Set(XRegistryAuthHeader.String(), tc.input) + res, err := parseSingleAuthHeader(req) + if tc.shouldErr { + assert.Error(t, err, tc.input) + } else { + require.NoError(t, err, tc.input) + assert.Equal(t, tc.expected, res, tc.input) + } + } +} + func TestParseMultiAuthHeader(t *testing.T) { for _, tc := range []struct { input string -- cgit v1.2.3-54-g00ecf From 5a5aa6009fdfde0524bc4e551742c366ad0164df Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 20:26:28 +0200 Subject: Improve TestAuthConfigsToAuthFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the created temporary file. Use more appropriate assertion calls. Signed-off-by: Miloslav Trmač --- pkg/auth/auth_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index a0b97b106..38f82ee04 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -4,6 +4,7 @@ import ( "encoding/base64" "io/ioutil" "net/http" + "os" "testing" "github.com/containers/image/v5/types" @@ -57,13 +58,14 @@ func TestAuthConfigsToAuthFile(t *testing.T) { filePath, err := authConfigsToAuthFile(configs) if tc.shouldErr { - assert.NotNil(t, err) + assert.Error(t, err) assert.Empty(t, filePath) } else { - assert.Nil(t, err) + assert.NoError(t, err) content, err := ioutil.ReadFile(filePath) - assert.Nil(t, err) + require.NoError(t, err) assert.Contains(t, string(content), tc.expectedContains) + os.Remove(filePath) } } } -- cgit v1.2.3-54-g00ecf From ad7e5e34f20da1422f79b704fc10a3cfec85e447 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 21:13:33 +0200 Subject: Add tests for auth.Header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Just a single function that handles all of Header, headerConfig and headerAuth; we will split that later. Signed-off-by: Miloslav Trmač --- pkg/auth/auth_test.go | 128 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) (limited to 'pkg') diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index 38f82ee04..e39a0e041 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -2,6 +2,8 @@ package auth import ( "encoding/base64" + "encoding/json" + "fmt" "io/ioutil" "net/http" "os" @@ -12,6 +14,132 @@ import ( "github.com/stretchr/testify/require" ) +const largeAuthFile = `{"auths":{ + "docker.io/vendor": {"auth": "ZG9ja2VyOnZlbmRvcg=="}, + "https://index.docker.io/v1": {"auth": "ZG9ja2VyOnRvcA=="}, + "quay.io/libpod": {"auth": "cXVheTpsaWJwb2Q="}, + "quay.io": {"auth": "cXVheTp0b3A="} +}}` + +func TestHeader(t *testing.T) { + for _, tc := range []struct { + headerName HeaderAuthName + name string + fileContents string + username, password string + shouldErr bool + expectedContents string + }{ + { + headerName: XRegistryConfigHeader, + name: "no data", + fileContents: "", + username: "", + password: "", + expectedContents: "", + }, + { + headerName: XRegistryConfigHeader, + name: "invalid JSON", + fileContents: "@invalid JSON", + username: "", + password: "", + shouldErr: true, + }, + { + headerName: XRegistryConfigHeader, + name: "file data", + fileContents: largeAuthFile, + username: "", + password: "", + expectedContents: `{ + "docker.io/vendor": {"username": "docker", "password": "vendor"}, + "docker.io": {"username": "docker", "password": "top"}, + "quay.io/libpod": {"username": "quay", "password": "libpod"}, + "quay.io": {"username": "quay", "password": "top"} + }`, + }, + { + headerName: XRegistryConfigHeader, + name: "file data + override", + fileContents: largeAuthFile, + username: "override-user", + password: "override-pass", + expectedContents: `{ + "docker.io/vendor": {"username": "docker", "password": "vendor"}, + "docker.io": {"username": "docker", "password": "top"}, + "quay.io/libpod": {"username": "quay", "password": "libpod"}, + "quay.io": {"username": "quay", "password": "top"}, + "": {"username": "override-user", "password": "override-pass"} + }`, + }, + { + headerName: XRegistryAuthHeader, + name: "override", + fileContents: "", + username: "override-user", + password: "override-pass", + expectedContents: `{"username": "override-user", "password": "override-pass"}`, + }, + { + headerName: XRegistryAuthHeader, + name: "invalid JSON", + fileContents: "@invalid JSON", + username: "", + password: "", + shouldErr: true, + }, + { + headerName: XRegistryAuthHeader, + name: "file data", + fileContents: largeAuthFile, + username: "", + password: "", + expectedContents: `{ + "docker.io/vendor": {"username": "docker", "password": "vendor"}, + "docker.io": {"username": "docker", "password": "top"}, + "quay.io/libpod": {"username": "quay", "password": "libpod"}, + "quay.io": {"username": "quay", "password": "top"} + }`, + }, + } { + name := fmt.Sprintf("%s: %s", tc.headerName, tc.name) + authFile := "" + if tc.fileContents != "" { + f, err := ioutil.TempFile("", "auth.json") + require.NoError(t, err, name) + defer os.Remove(f.Name()) + authFile = f.Name() + err = ioutil.WriteFile(authFile, []byte(tc.fileContents), 0700) + require.NoError(t, err, name) + } + + res, err := Header(nil, tc.headerName, authFile, tc.username, tc.password) + if tc.shouldErr { + assert.Error(t, err, name) + } else { + require.NoError(t, err, name) + if tc.expectedContents == "" { + assert.Empty(t, res, name) + } else { + require.Len(t, res, 1, name) + header, ok := res[tc.headerName.String()] + require.True(t, ok, name) + decodedHeader, err := base64.URLEncoding.DecodeString(header) + require.NoError(t, err, name) + // Don't test for a specific JSON representation, just for the expected contents. + expected := map[string]interface{}{} + actual := map[string]interface{}{} + err = json.Unmarshal([]byte(tc.expectedContents), &expected) + require.NoError(t, err, name) + err = json.Unmarshal(decodedHeader, &actual) + require.NoError(t, err, name) + assert.Equal(t, expected, actual, name) + } + } + } +} + func TestAuthConfigsToAuthFile(t *testing.T) { for _, tc := range []struct { name string -- cgit v1.2.3-54-g00ecf From d29a4a6d173988b83ac78355e271d0f60e5d2830 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 21:39:26 +0200 Subject: Add TestHeaderGetCredentialsRoundtrip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... as an end-to-end unit test of the header creation/parsing code. Leave the docker.io and docker.io/vendor test cases commented out, because they are currently failing. Signed-off-by: Miloslav Trmač --- pkg/auth/auth_test.go | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) (limited to 'pkg') diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index e39a0e041..ee16d832b 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -9,6 +9,7 @@ import ( "os" "testing" + "github.com/containers/image/v5/pkg/docker/config" "github.com/containers/image/v5/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -21,6 +22,111 @@ const largeAuthFile = `{"auths":{ "quay.io": {"auth": "cXVheTp0b3A="} }}` +// Semantics of largeAuthFile +var largeAuthFileValues = map[string]types.DockerAuthConfig{ + // "docker.io/vendor": {Username: "docker", Password: "vendor"}, + // "docker.io": {Username: "docker", Password: "top"}, + "quay.io/libpod": {Username: "quay", Password: "libpod"}, + "quay.io": {Username: "quay", Password: "top"}, +} + +// Test that GetCredentials() correctly parses what Header() produces +func TestHeaderGetCredentialsRoundtrip(t *testing.T) { + for _, tc := range []struct { + headerName HeaderAuthName + name string + fileContents string + username, password string + expectedOverride *types.DockerAuthConfig + expectedFileValues map[string]types.DockerAuthConfig + }{ + { + headerName: XRegistryConfigHeader, + name: "no data", + fileContents: "", + username: "", + password: "", + expectedOverride: nil, + expectedFileValues: nil, + }, + { + headerName: XRegistryConfigHeader, + name: "file data", + fileContents: largeAuthFile, + username: "", + password: "", + expectedOverride: nil, + expectedFileValues: largeAuthFileValues, + }, + { + headerName: XRegistryConfigHeader, + name: "file data + override", + fileContents: largeAuthFile, + username: "override-user", + password: "override-pass", + expectedOverride: &types.DockerAuthConfig{Username: "override-user", Password: "override-pass"}, + expectedFileValues: largeAuthFileValues, + }, + { + headerName: XRegistryAuthHeader, + name: "override", + fileContents: "", + username: "override-user", + password: "override-pass", + expectedOverride: &types.DockerAuthConfig{Username: "override-user", Password: "override-pass"}, + expectedFileValues: nil, + }, + { + headerName: XRegistryAuthHeader, + name: "file data", + fileContents: largeAuthFile, + username: "", + password: "", + expectedFileValues: largeAuthFileValues, + }, + } { + name := fmt.Sprintf("%s: %s", tc.headerName, tc.name) + inputAuthFile := "" + if tc.fileContents != "" { + f, err := ioutil.TempFile("", "auth.json") + require.NoError(t, err, name) + defer os.Remove(f.Name()) + inputAuthFile = f.Name() + err = ioutil.WriteFile(inputAuthFile, []byte(tc.fileContents), 0700) + require.NoError(t, err, name) + } + + headers, err := Header(nil, tc.headerName, inputAuthFile, tc.username, tc.password) + require.NoError(t, err) + req, err := http.NewRequest(http.MethodPost, "/", nil) + require.NoError(t, err, name) + for k, v := range headers { + req.Header.Set(k, v) + } + + override, resPath, parsedHeader, err := GetCredentials(req) + require.NoError(t, err, name) + defer RemoveAuthfile(resPath) + if tc.expectedOverride == nil { + assert.Nil(t, override, name) + } else { + require.NotNil(t, override, name) + assert.Equal(t, *tc.expectedOverride, *override, name) + } + for key, expectedAuth := range tc.expectedFileValues { + auth, err := config.GetCredentials(&types.SystemContext{AuthFilePath: resPath}, key) + require.NoError(t, err, name) + assert.Equal(t, expectedAuth, auth, "%s, key %s", name, key) + } + if len(headers) != 0 { + assert.Len(t, headers, 1) + assert.Equal(t, tc.headerName, parsedHeader) + } else { + assert.Equal(t, HeaderAuthName(""), parsedHeader) + } + } +} + func TestHeader(t *testing.T) { for _, tc := range []struct { headerName HeaderAuthName -- cgit v1.2.3-54-g00ecf From 1b6bf971306af753e72db9d10f7594ecd0c89785 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:05:04 +0200 Subject: Rename normalize and a few variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... to refer to auth file keys instead of servers and the like. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 60c40a40b..8c6436883 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -258,24 +258,24 @@ func authConfigsToAuthFile(authConfigs map[string]types.DockerAuthConfig) (strin // Now use the c/image packages to store the credentials. It's battle // tested, and we make sure to use the same code as the image backend. sys := types.SystemContext{AuthFilePath: authFilePath} - for server, config := range authConfigs { - server = normalize(server) + for authFileKey, config := range authConfigs { + key := normalizeAuthFileKey(authFileKey) // Note that we do not validate the credentials here. We assume // that all credentials are valid. They'll be used on demand // later. - if err := imageAuth.SetAuthentication(&sys, server, config.Username, config.Password); err != nil { - return "", errors.Wrapf(err, "error storing credentials in temporary auth file (server: %q, user: %q)", server, config.Username) + if err := imageAuth.SetAuthentication(&sys, key, config.Username, config.Password); err != nil { + return "", errors.Wrapf(err, "error storing credentials in temporary auth file (key: %q / %q, user: %q)", authFileKey, key, config.Username) } } return authFilePath, nil } -// normalize takes a server and removes the leading "http[s]://" prefix as well +// normalizeAuthFileKey takes an auth file key and removes the leading "http[s]://" prefix as well // as removes path suffixes from docker registries. -func normalize(server string) string { - stripped := strings.TrimPrefix(server, "http://") +func normalizeAuthFileKey(authFileKey string) string { + stripped := strings.TrimPrefix(authFileKey, "http://") stripped = strings.TrimPrefix(stripped, "https://") /// Normalize docker registries -- cgit v1.2.3-54-g00ecf From 491951d66e1829ad8e847f3049a557dd9d55db68 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 21:56:40 +0200 Subject: Fix normalizeAuthFileKey to use the correct semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 18 +++++++++++------- pkg/auth/auth_test.go | 24 ++++++++++++------------ 2 files changed, 23 insertions(+), 19 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 8c6436883..7cde6ef5e 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -272,20 +272,24 @@ func authConfigsToAuthFile(authConfigs map[string]types.DockerAuthConfig) (strin return authFilePath, nil } -// normalizeAuthFileKey takes an auth file key and removes the leading "http[s]://" prefix as well -// as removes path suffixes from docker registries. +// normalizeAuthFileKey takes an auth file key and converts it into a new-style credential key +// in the canonical format, as interpreted by c/image/pkg/docker/config. func normalizeAuthFileKey(authFileKey string) string { stripped := strings.TrimPrefix(authFileKey, "http://") stripped = strings.TrimPrefix(stripped, "https://") - /// Normalize docker registries - if strings.HasPrefix(stripped, "index.docker.io/") || - strings.HasPrefix(stripped, "registry-1.docker.io/") || - strings.HasPrefix(stripped, "docker.io/") { + if stripped != authFileKey { // URLs are interpreted to mean complete registries stripped = strings.SplitN(stripped, "/", 2)[0] } - return stripped + // Only non-namespaced registry names (or URLs) need to be normalized; repo namespaces + // always use the simple format. + switch stripped { + case "registry-1.docker.io", "index.docker.io": + return "docker.io" + default: + return stripped + } } // dockerAuthToImageAuth converts a docker auth config to one we're using diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index ee16d832b..be86a9cbd 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -24,10 +24,10 @@ const largeAuthFile = `{"auths":{ // Semantics of largeAuthFile var largeAuthFileValues = map[string]types.DockerAuthConfig{ - // "docker.io/vendor": {Username: "docker", Password: "vendor"}, - // "docker.io": {Username: "docker", Password: "top"}, - "quay.io/libpod": {Username: "quay", Password: "libpod"}, - "quay.io": {Username: "quay", Password: "top"}, + "docker.io/vendor": {Username: "docker", Password: "vendor"}, + "docker.io": {Username: "docker", Password: "top"}, + "quay.io/libpod": {Username: "quay", Password: "libpod"}, + "quay.io": {Username: "quay", Password: "top"}, } // Test that GetCredentials() correctly parses what Header() produces @@ -260,28 +260,28 @@ func TestAuthConfigsToAuthFile(t *testing.T) { expectedContains: "{}", }, { - name: "registry with prefix", + name: "registry with a namespace prefix", server: "my-registry.local/username", shouldErr: false, expectedContains: `"my-registry.local/username":`, }, { - name: "normalize https:// prefix", + name: "URLs are interpreted as full registries", server: "http://my-registry.local/username", shouldErr: false, - expectedContains: `"my-registry.local/username":`, + expectedContains: `"my-registry.local":`, }, { - name: "normalize docker registry with https prefix", + name: "the old-style docker registry URL is normalized", server: "http://index.docker.io/v1/", shouldErr: false, - expectedContains: `"index.docker.io":`, + expectedContains: `"docker.io":`, }, { - name: "normalize docker registry without https prefix", - server: "docker.io/v2/", + name: "docker.io vendor namespace", + server: "docker.io/vendor", shouldErr: false, - expectedContains: `"docker.io":`, + expectedContains: `"docker.io/vendor":`, }, } { configs := map[string]types.DockerAuthConfig{} -- cgit v1.2.3-54-g00ecf From 2aeb690d370de9fee15fc7c47b66fb04a30c41d8 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:15:23 +0200 Subject: Don't return a header name from auth.GetCredentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Almost every caller is using it only to wrap an error in exactly the same way, so move that error context into GetCredentials and simplify the users. (The one other caller, build, was even wrapping the error incorrectly talking about query parameters; so let it use the same text as the others.) Signed-off-by: Miloslav Trmač --- pkg/api/handlers/compat/images.go | 4 ++-- pkg/api/handlers/compat/images_build.go | 4 ++-- pkg/api/handlers/compat/images_push.go | 4 ++-- pkg/api/handlers/compat/images_search.go | 4 ++-- pkg/api/handlers/libpod/images.go | 4 ++-- pkg/api/handlers/libpod/images_pull.go | 4 ++-- pkg/api/handlers/libpod/manifests.go | 4 ++-- pkg/api/handlers/libpod/play.go | 4 ++-- pkg/auth/auth.go | 14 ++++++++++---- pkg/auth/auth_test.go | 8 +------- 10 files changed, 27 insertions(+), 27 deletions(-) (limited to 'pkg') diff --git a/pkg/api/handlers/compat/images.go b/pkg/api/handlers/compat/images.go index 4533fddeb..c1cc99da4 100644 --- a/pkg/api/handlers/compat/images.go +++ b/pkg/api/handlers/compat/images.go @@ -270,9 +270,9 @@ func CreateImageFromImage(w http.ResponseWriter, r *http.Request) { return } - authConf, authfile, key, err := auth.GetCredentials(r) + authConf, authfile, err := auth.GetCredentials(r) if err != nil { - utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String())) + utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, err) return } defer auth.RemoveAuthfile(authfile) diff --git a/pkg/api/handlers/compat/images_build.go b/pkg/api/handlers/compat/images_build.go index f85df02e1..2fd5dcccd 100644 --- a/pkg/api/handlers/compat/images_build.go +++ b/pkg/api/handlers/compat/images_build.go @@ -453,10 +453,10 @@ func BuildImage(w http.ResponseWriter, r *http.Request) { } } - creds, authfile, key, err := auth.GetCredentials(r) + creds, authfile, err := auth.GetCredentials(r) if err != nil { // Credential value(s) not returned as their value is not human readable - utils.BadRequest(w, key.String(), "n/a", err) + utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest, err) return } defer auth.RemoveAuthfile(authfile) diff --git a/pkg/api/handlers/compat/images_push.go b/pkg/api/handlers/compat/images_push.go index 3a84b5799..04cad204d 100644 --- a/pkg/api/handlers/compat/images_push.go +++ b/pkg/api/handlers/compat/images_push.go @@ -85,9 +85,9 @@ func PushImage(w http.ResponseWriter, r *http.Request) { return } - authconf, authfile, key, err := auth.GetCredentials(r) + authconf, authfile, err := auth.GetCredentials(r) if err != nil { - utils.Error(w, "Something went wrong.", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String())) + utils.Error(w, "Something went wrong.", http.StatusBadRequest, err) return } defer auth.RemoveAuthfile(authfile) diff --git a/pkg/api/handlers/compat/images_search.go b/pkg/api/handlers/compat/images_search.go index e9cc3e2b6..f6ad86a04 100644 --- a/pkg/api/handlers/compat/images_search.go +++ b/pkg/api/handlers/compat/images_search.go @@ -34,9 +34,9 @@ func SearchImages(w http.ResponseWriter, r *http.Request) { return } - _, authfile, key, err := auth.GetCredentials(r) + _, authfile, err := auth.GetCredentials(r) if err != nil { - utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String())) + utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, err) return } defer auth.RemoveAuthfile(authfile) diff --git a/pkg/api/handlers/libpod/images.go b/pkg/api/handlers/libpod/images.go index f2f93434a..6e23845f0 100644 --- a/pkg/api/handlers/libpod/images.go +++ b/pkg/api/handlers/libpod/images.go @@ -497,9 +497,9 @@ func PushImage(w http.ResponseWriter, r *http.Request) { return } - authconf, authfile, key, err := auth.GetCredentials(r) + authconf, authfile, err := auth.GetCredentials(r) if err != nil { - utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String())) + utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, err) return } defer auth.RemoveAuthfile(authfile) diff --git a/pkg/api/handlers/libpod/images_pull.go b/pkg/api/handlers/libpod/images_pull.go index fabdb326b..518e7cc65 100644 --- a/pkg/api/handlers/libpod/images_pull.go +++ b/pkg/api/handlers/libpod/images_pull.go @@ -68,9 +68,9 @@ func ImagesPull(w http.ResponseWriter, r *http.Request) { } // Do the auth dance. - authConf, authfile, key, err := auth.GetCredentials(r) + authConf, authfile, err := auth.GetCredentials(r) if err != nil { - utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String())) + utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, err) return } defer auth.RemoveAuthfile(authfile) diff --git a/pkg/api/handlers/libpod/manifests.go b/pkg/api/handlers/libpod/manifests.go index 869c83fa3..eb0b6827f 100644 --- a/pkg/api/handlers/libpod/manifests.go +++ b/pkg/api/handlers/libpod/manifests.go @@ -176,9 +176,9 @@ func ManifestPush(w http.ResponseWriter, r *http.Request) { } source := utils.GetName(r) - authconf, authfile, key, err := auth.GetCredentials(r) + authconf, authfile, err := auth.GetCredentials(r) if err != nil { - utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String())) + utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, err) return } defer auth.RemoveAuthfile(authfile) diff --git a/pkg/api/handlers/libpod/play.go b/pkg/api/handlers/libpod/play.go index f943fc240..e6ae9ad18 100644 --- a/pkg/api/handlers/libpod/play.go +++ b/pkg/api/handlers/libpod/play.go @@ -86,9 +86,9 @@ func PlayKube(w http.ResponseWriter, r *http.Request) { utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrap(err, "error closing temporary file")) return } - authConf, authfile, key, err := auth.GetCredentials(r) + authConf, authfile, err := auth.GetCredentials(r) if err != nil { - utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, errors.Wrapf(err, "failed to parse %q header for %s", key, r.URL.String())) + utils.Error(w, "failed to retrieve repository credentials", http.StatusBadRequest, err) return } defer auth.RemoveAuthfile(authfile) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 7cde6ef5e..c0606cf1d 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -32,17 +32,23 @@ const XRegistryConfigHeader HeaderAuthName = "X-Registry-Config" // GetCredentials queries the http.Request for X-Registry-.* headers and extracts // the necessary authentication information for libpod operations -func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, HeaderAuthName, error) { +func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { has := func(key HeaderAuthName) bool { hdr, found := r.Header[string(key)]; return found && len(hdr) > 0 } switch { case has(XRegistryConfigHeader): c, f, err := getConfigCredentials(r) - return c, f, XRegistryConfigHeader, err + if err != nil { + return nil, "", errors.Wrapf(err, "failed to parse %q header for %s", XRegistryConfigHeader, r.URL.String()) + } + return c, f, nil case has(XRegistryAuthHeader): c, f, err := getAuthCredentials(r) - return c, f, XRegistryAuthHeader, err + if err != nil { + return nil, "", errors.Wrapf(err, "failed to parse %q header for %s", XRegistryAuthHeader, r.URL.String()) + } + return c, f, nil } - return nil, "", "", nil + return nil, "", nil } // getConfigCredentials extracts one or more docker.AuthConfig from the request's diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index be86a9cbd..634215acf 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -104,7 +104,7 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { req.Header.Set(k, v) } - override, resPath, parsedHeader, err := GetCredentials(req) + override, resPath, err := GetCredentials(req) require.NoError(t, err, name) defer RemoveAuthfile(resPath) if tc.expectedOverride == nil { @@ -118,12 +118,6 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { require.NoError(t, err, name) assert.Equal(t, expectedAuth, auth, "%s, key %s", name, key) } - if len(headers) != 0 { - assert.Len(t, headers, 1) - assert.Equal(t, tc.headerName, parsedHeader) - } else { - assert.Equal(t, HeaderAuthName(""), parsedHeader) - } } } -- cgit v1.2.3-54-g00ecf From 7674f2f76b07058aa3bbf44675b7c2482c61811a Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:20:26 +0200 Subject: Simplify the interface of parseSingleAuthHeader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don't create a single-element map only for the only caller to laboriously extract an element of that map; just return a single entry. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 18 +++++------------- pkg/auth/auth_test.go | 12 +++++------- 2 files changed, 10 insertions(+), 20 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index c0606cf1d..c19151c91 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -121,17 +121,11 @@ func getAuthCredentials(r *http.Request) (*types.DockerAuthConfig, string, error } // Fallback to looking for a single-auth header (i.e., one config). - authConfigs, err = parseSingleAuthHeader(r) + authConfig, err := parseSingleAuthHeader(r) if err != nil { return nil, "", err } - var conf *types.DockerAuthConfig - for k := range authConfigs { - c := authConfigs[k] - conf = &c - break - } - return conf, "", nil + return &authConfig, "", nil } // Header builds the requested Authentication Header @@ -321,7 +315,7 @@ func imageAuthToDockerAuth(authConfig types.DockerAuthConfig) dockerAPITypes.Aut // parseSingleAuthHeader extracts a DockerAuthConfig from the request's header. // The header content is a single DockerAuthConfig. -func parseSingleAuthHeader(r *http.Request) (map[string]types.DockerAuthConfig, error) { +func parseSingleAuthHeader(r *http.Request) (types.DockerAuthConfig, error) { authHeader := r.Header.Get(string(XRegistryAuthHeader)) authConfig := dockerAPITypes.AuthConfig{} // Accept "null" and handle it as empty value for compatibility reason with Docker. @@ -329,12 +323,10 @@ func parseSingleAuthHeader(r *http.Request) (map[string]types.DockerAuthConfig, if len(authHeader) > 0 && authHeader != "null" { authJSON := base64.NewDecoder(base64.URLEncoding, strings.NewReader(authHeader)) if err := json.NewDecoder(authJSON).Decode(&authConfig); err != nil { - return nil, err + return types.DockerAuthConfig{}, err } } - authConfigs := make(map[string]types.DockerAuthConfig) - authConfigs["0"] = dockerAuthToImageAuth(authConfig) - return authConfigs, nil + return dockerAuthToImageAuth(authConfig), nil } // parseMultiAuthHeader extracts a DockerAuthConfig from the request's header. diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index 634215acf..0e6bd42ef 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -302,24 +302,22 @@ func TestParseSingleAuthHeader(t *testing.T) { for _, tc := range []struct { input string shouldErr bool - expected map[string]types.DockerAuthConfig + expected types.DockerAuthConfig }{ { input: "", // An empty (or missing) header - expected: map[string]types.DockerAuthConfig{"0": {}}, + expected: types.DockerAuthConfig{}, }, { input: "null", - expected: map[string]types.DockerAuthConfig{"0": {}}, + expected: types.DockerAuthConfig{}, }, // Invalid JSON {input: "@", shouldErr: true}, // Success { - input: base64.URLEncoding.EncodeToString([]byte(`{"username":"u1","password":"p1"}`)), - expected: map[string]types.DockerAuthConfig{ - "0": {Username: "u1", Password: "p1"}, - }, + input: base64.URLEncoding.EncodeToString([]byte(`{"username":"u1","password":"p1"}`)), + expected: types.DockerAuthConfig{Username: "u1", Password: "p1"}, }, } { req, err := http.NewRequest(http.MethodPost, "/", nil) -- cgit v1.2.3-54-g00ecf From 6f1a26b04f9a35a649f80c07be2e3372bc65d60a Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:22:21 +0200 Subject: Simplify parseSingleAuthHeader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the "no input" case, return a constant instead of continuing with the decode/convert path, converting empty data. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index c19151c91..b3fd71184 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -317,14 +317,16 @@ func imageAuthToDockerAuth(authConfig types.DockerAuthConfig) dockerAPITypes.Aut // The header content is a single DockerAuthConfig. func parseSingleAuthHeader(r *http.Request) (types.DockerAuthConfig, error) { authHeader := r.Header.Get(string(XRegistryAuthHeader)) - authConfig := dockerAPITypes.AuthConfig{} // Accept "null" and handle it as empty value for compatibility reason with Docker. // Some java docker clients pass this value, e.g. this one used in Eclipse. - if len(authHeader) > 0 && authHeader != "null" { - authJSON := base64.NewDecoder(base64.URLEncoding, strings.NewReader(authHeader)) - if err := json.NewDecoder(authJSON).Decode(&authConfig); err != nil { - return types.DockerAuthConfig{}, err - } + if len(authHeader) == 0 || authHeader == "null" { + return types.DockerAuthConfig{}, nil + } + + authConfig := dockerAPITypes.AuthConfig{} + authJSON := base64.NewDecoder(base64.URLEncoding, strings.NewReader(authHeader)) + if err := json.NewDecoder(authJSON).Decode(&authConfig); err != nil { + return types.DockerAuthConfig{}, err } return dockerAuthToImageAuth(authConfig), nil } -- cgit v1.2.3-54-g00ecf From 1ecc6ba72852700f748715124bdf98573cc93c0b Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:28:24 +0200 Subject: Pass a header value directly to parseSingleAuthHeader and parseMultiAuthHeader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both have a single caller, so there's no point in looking up the header value twice. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 16 ++++++++-------- pkg/auth/auth_test.go | 10 ++-------- 2 files changed, 10 insertions(+), 16 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index b3fd71184..3ecdd99fe 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -113,15 +113,17 @@ func getConfigCredentials(r *http.Request) (*types.DockerAuthConfig, string, err // stored in a temporary auth file (2nd return value). Note that the auth file // should be removed after usage. func getAuthCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { + authHeader := r.Header.Get(XRegistryAuthHeader.String()) + // First look for a multi-auth header (i.e., a map). - authConfigs, err := parseMultiAuthHeader(r) + authConfigs, err := parseMultiAuthHeader(authHeader) if err == nil { authfile, err := authConfigsToAuthFile(authConfigs) return nil, authfile, err } // Fallback to looking for a single-auth header (i.e., one config). - authConfig, err := parseSingleAuthHeader(r) + authConfig, err := parseSingleAuthHeader(authHeader) if err != nil { return nil, "", err } @@ -313,10 +315,9 @@ func imageAuthToDockerAuth(authConfig types.DockerAuthConfig) dockerAPITypes.Aut } } -// parseSingleAuthHeader extracts a DockerAuthConfig from the request's header. +// parseSingleAuthHeader extracts a DockerAuthConfig from an XRegistryAuthHeader value. // The header content is a single DockerAuthConfig. -func parseSingleAuthHeader(r *http.Request) (types.DockerAuthConfig, error) { - authHeader := r.Header.Get(string(XRegistryAuthHeader)) +func parseSingleAuthHeader(authHeader string) (types.DockerAuthConfig, error) { // Accept "null" and handle it as empty value for compatibility reason with Docker. // Some java docker clients pass this value, e.g. this one used in Eclipse. if len(authHeader) == 0 || authHeader == "null" { @@ -331,10 +332,9 @@ func parseSingleAuthHeader(r *http.Request) (types.DockerAuthConfig, error) { return dockerAuthToImageAuth(authConfig), nil } -// parseMultiAuthHeader extracts a DockerAuthConfig from the request's header. +// parseMultiAuthHeader extracts a DockerAuthConfig from an XRegistryAuthHeader value. // The header content is a map[string]DockerAuthConfigs. -func parseMultiAuthHeader(r *http.Request) (map[string]types.DockerAuthConfig, error) { - authHeader := r.Header.Get(string(XRegistryAuthHeader)) +func parseMultiAuthHeader(authHeader string) (map[string]types.DockerAuthConfig, error) { // Accept "null" and handle it as empty value for compatibility reason with Docker. // Some java docker clients pass this value, e.g. this one used in Eclipse. if len(authHeader) == 0 || authHeader == "null" { diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index 0e6bd42ef..6acf1f8fb 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -320,10 +320,7 @@ func TestParseSingleAuthHeader(t *testing.T) { expected: types.DockerAuthConfig{Username: "u1", Password: "p1"}, }, } { - req, err := http.NewRequest(http.MethodPost, "/", nil) - require.NoError(t, err, tc.input) - req.Header.Set(XRegistryAuthHeader.String(), tc.input) - res, err := parseSingleAuthHeader(req) + res, err := parseSingleAuthHeader(tc.input) if tc.shouldErr { assert.Error(t, err, tc.input) } else { @@ -356,10 +353,7 @@ func TestParseMultiAuthHeader(t *testing.T) { }, }, } { - req, err := http.NewRequest(http.MethodPost, "/", nil) - require.NoError(t, err, tc.input) - req.Header.Set(XRegistryAuthHeader.String(), tc.input) - res, err := parseMultiAuthHeader(req) + res, err := parseMultiAuthHeader(tc.input) if tc.shouldErr { assert.Error(t, err, tc.input) } else { -- cgit v1.2.3-54-g00ecf From 2946e83493e2628c68819357ebc15bada38d45b5 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:37:23 +0200 Subject: Beautify GetCredentials.has a bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use separate lines, and use the provided .String() API. Should not change behaivor. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 3ecdd99fe..8e4cb6959 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -33,7 +33,10 @@ const XRegistryConfigHeader HeaderAuthName = "X-Registry-Config" // GetCredentials queries the http.Request for X-Registry-.* headers and extracts // the necessary authentication information for libpod operations func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { - has := func(key HeaderAuthName) bool { hdr, found := r.Header[string(key)]; return found && len(hdr) > 0 } + has := func(key HeaderAuthName) bool { + hdr, found := r.Header[key.String()] + return found && len(hdr) > 0 + } switch { case has(XRegistryConfigHeader): c, f, err := getConfigCredentials(r) -- cgit v1.2.3-54-g00ecf From 1589d70bcb522e49c75632c4e0edff52d2e459c2 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:39:25 +0200 Subject: Use Header.Values in GetCredentials.has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's possibly a bit more expensive, but semantically safer because it does header normalization. And we'll regain the cost by not looking up the value repeatedly. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 8e4cb6959..9a4e2af85 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -34,8 +34,8 @@ const XRegistryConfigHeader HeaderAuthName = "X-Registry-Config" // the necessary authentication information for libpod operations func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { has := func(key HeaderAuthName) bool { - hdr, found := r.Header[key.String()] - return found && len(hdr) > 0 + hdr := r.Header.Values(key.String()) + return len(hdr) > 0 } switch { case has(XRegistryConfigHeader): -- cgit v1.2.3-54-g00ecf From da86a232851162b584a143efa3c4f3032a480413 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:43:04 +0200 Subject: Only look up HTTP header values once in GetCredentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... and have GetCredentials pass the values down to getConfigCredentials and getAuthCredentials. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 9a4e2af85..a95ae4764 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -33,19 +33,18 @@ const XRegistryConfigHeader HeaderAuthName = "X-Registry-Config" // GetCredentials queries the http.Request for X-Registry-.* headers and extracts // the necessary authentication information for libpod operations func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { - has := func(key HeaderAuthName) bool { + nonemptyHeaderValue := func(key HeaderAuthName) ([]string, bool) { hdr := r.Header.Values(key.String()) - return len(hdr) > 0 + return hdr, len(hdr) > 0 } - switch { - case has(XRegistryConfigHeader): - c, f, err := getConfigCredentials(r) + if hdr, ok := nonemptyHeaderValue(XRegistryConfigHeader); ok { + c, f, err := getConfigCredentials(r, hdr) if err != nil { return nil, "", errors.Wrapf(err, "failed to parse %q header for %s", XRegistryConfigHeader, r.URL.String()) } return c, f, nil - case has(XRegistryAuthHeader): - c, f, err := getAuthCredentials(r) + } else if hdr, ok := nonemptyHeaderValue(XRegistryAuthHeader); ok { + c, f, err := getAuthCredentials(hdr) if err != nil { return nil, "", errors.Wrapf(err, "failed to parse %q header for %s", XRegistryAuthHeader, r.URL.String()) } @@ -54,14 +53,14 @@ func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { return nil, "", nil } -// getConfigCredentials extracts one or more docker.AuthConfig from the request's -// header. An empty key will be used as default while a named registry will be +// getConfigCredentials extracts one or more docker.AuthConfig from a request and its +// XRegistryConfigHeader value. An empty key will be used as default while a named registry will be // returned as types.DockerAuthConfig -func getConfigCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { +func getConfigCredentials(r *http.Request, headers []string) (*types.DockerAuthConfig, string, error) { var auth *types.DockerAuthConfig configs := make(map[string]types.DockerAuthConfig) - for _, h := range r.Header[string(XRegistryConfigHeader)] { + for _, h := range headers { param, err := base64.URLEncoding.DecodeString(h) if err != nil { return nil, "", errors.Wrapf(err, "failed to decode %q", XRegistryConfigHeader) @@ -110,13 +109,13 @@ func getConfigCredentials(r *http.Request) (*types.DockerAuthConfig, string, err return auth, authfile, err } -// getAuthCredentials extracts one or more DockerAuthConfigs from the request's -// header. The header could specify a single-auth config in which case the +// getAuthCredentials extracts one or more DockerAuthConfigs from an XRegistryAuthHeader +// value. The header could specify a single-auth config in which case the // first return value is set. In case of a multi-auth header, the contents are // stored in a temporary auth file (2nd return value). Note that the auth file // should be removed after usage. -func getAuthCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { - authHeader := r.Header.Get(XRegistryAuthHeader.String()) +func getAuthCredentials(headers []string) (*types.DockerAuthConfig, string, error) { + authHeader := headers[0] // First look for a multi-auth header (i.e., a map). authConfigs, err := parseMultiAuthHeader(authHeader) -- cgit v1.2.3-54-g00ecf From 9d56ebb611e008f5ad09048499331f7aac1ceed3 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:47:52 +0200 Subject: Consolidate the error handling path in GetCredentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We'll share even more code here in the future. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index a95ae4764..a4d7896a8 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -37,20 +37,23 @@ func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { hdr := r.Header.Values(key.String()) return hdr, len(hdr) > 0 } + var override *types.DockerAuthConfig + var authFile string + var headerName HeaderAuthName + var err error if hdr, ok := nonemptyHeaderValue(XRegistryConfigHeader); ok { - c, f, err := getConfigCredentials(r, hdr) - if err != nil { - return nil, "", errors.Wrapf(err, "failed to parse %q header for %s", XRegistryConfigHeader, r.URL.String()) - } - return c, f, nil + headerName = XRegistryConfigHeader + override, authFile, err = getConfigCredentials(r, hdr) } else if hdr, ok := nonemptyHeaderValue(XRegistryAuthHeader); ok { - c, f, err := getAuthCredentials(hdr) - if err != nil { - return nil, "", errors.Wrapf(err, "failed to parse %q header for %s", XRegistryAuthHeader, r.URL.String()) - } - return c, f, nil + headerName = XRegistryAuthHeader + override, authFile, err = getAuthCredentials(hdr) + } else { + return nil, "", nil + } + if err != nil { + return nil, "", errors.Wrapf(err, "failed to parse %q header for %s", headerName, r.URL.String()) } - return nil, "", nil + return override, authFile, nil } // getConfigCredentials extracts one or more docker.AuthConfig from a request and its -- cgit v1.2.3-54-g00ecf From 29f40887132709b0d2097bdfe7b45ff90c3f7b47 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 22:52:47 +0200 Subject: Move the auth file creation to GetCredentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This shares the code, and makes getConfigCredentials and getAuthCredentials side-effect free and possibly easier to test. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index a4d7896a8..a17297c7d 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -31,48 +31,59 @@ const XRegistryAuthHeader HeaderAuthName = "X-Registry-Auth" const XRegistryConfigHeader HeaderAuthName = "X-Registry-Config" // GetCredentials queries the http.Request for X-Registry-.* headers and extracts -// the necessary authentication information for libpod operations +// the necessary authentication information for libpod operations, possibly +// creating a config file. If that is the case, the caller must call RemoveAuthFile. func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { nonemptyHeaderValue := func(key HeaderAuthName) ([]string, bool) { hdr := r.Header.Values(key.String()) return hdr, len(hdr) > 0 } var override *types.DockerAuthConfig - var authFile string + var fileContents map[string]types.DockerAuthConfig var headerName HeaderAuthName var err error if hdr, ok := nonemptyHeaderValue(XRegistryConfigHeader); ok { headerName = XRegistryConfigHeader - override, authFile, err = getConfigCredentials(r, hdr) + override, fileContents, err = getConfigCredentials(r, hdr) } else if hdr, ok := nonemptyHeaderValue(XRegistryAuthHeader); ok { headerName = XRegistryAuthHeader - override, authFile, err = getAuthCredentials(hdr) + override, fileContents, err = getAuthCredentials(hdr) } else { return nil, "", nil } if err != nil { return nil, "", errors.Wrapf(err, "failed to parse %q header for %s", headerName, r.URL.String()) } + + var authFile string + if fileContents == nil { + authFile = "" + } else { + authFile, err = authConfigsToAuthFile(fileContents) + if err != nil { + return nil, "", errors.Wrapf(err, "failed to parse %q header for %s", headerName, r.URL.String()) + } + } return override, authFile, nil } // getConfigCredentials extracts one or more docker.AuthConfig from a request and its // XRegistryConfigHeader value. An empty key will be used as default while a named registry will be // returned as types.DockerAuthConfig -func getConfigCredentials(r *http.Request, headers []string) (*types.DockerAuthConfig, string, error) { +func getConfigCredentials(r *http.Request, headers []string) (*types.DockerAuthConfig, map[string]types.DockerAuthConfig, error) { var auth *types.DockerAuthConfig configs := make(map[string]types.DockerAuthConfig) for _, h := range headers { param, err := base64.URLEncoding.DecodeString(h) if err != nil { - return nil, "", errors.Wrapf(err, "failed to decode %q", XRegistryConfigHeader) + return nil, nil, errors.Wrapf(err, "failed to decode %q", XRegistryConfigHeader) } ac := make(map[string]dockerAPITypes.AuthConfig) err = json.Unmarshal(param, &ac) if err != nil { - return nil, "", errors.Wrapf(err, "failed to unmarshal %q", XRegistryConfigHeader) + return nil, nil, errors.Wrapf(err, "failed to unmarshal %q", XRegistryConfigHeader) } for k, v := range ac { @@ -108,31 +119,28 @@ func getConfigCredentials(r *http.Request, headers []string) (*types.DockerAuthC } } - authfile, err := authConfigsToAuthFile(configs) - return auth, authfile, err + return auth, configs, nil } // getAuthCredentials extracts one or more DockerAuthConfigs from an XRegistryAuthHeader // value. The header could specify a single-auth config in which case the // first return value is set. In case of a multi-auth header, the contents are -// stored in a temporary auth file (2nd return value). Note that the auth file -// should be removed after usage. -func getAuthCredentials(headers []string) (*types.DockerAuthConfig, string, error) { +// returned in the second return value. +func getAuthCredentials(headers []string) (*types.DockerAuthConfig, map[string]types.DockerAuthConfig, error) { authHeader := headers[0] // First look for a multi-auth header (i.e., a map). authConfigs, err := parseMultiAuthHeader(authHeader) if err == nil { - authfile, err := authConfigsToAuthFile(authConfigs) - return nil, authfile, err + return nil, authConfigs, nil } // Fallback to looking for a single-auth header (i.e., one config). authConfig, err := parseSingleAuthHeader(authHeader) if err != nil { - return nil, "", err + return nil, nil, err } - return &authConfig, "", nil + return &authConfig, nil, nil } // Header builds the requested Authentication Header -- cgit v1.2.3-54-g00ecf From 8155fb5658a3a282550b39b2c3a6cd80bc9653d1 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 23:00:02 +0200 Subject: Turn headerConfig into MakeXRegistryConfigHeader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... which can be called independently. For now, there are no new callers, to test that the behavior has not changed. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index a17297c7d..d4f356f3d 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -153,7 +153,7 @@ func Header(sys *types.SystemContext, headerName HeaderAuthName, authfile, usern case XRegistryAuthHeader: content, err = headerAuth(sys, authfile, username, password) case XRegistryConfigHeader: - content, err = headerConfig(sys, authfile, username, password) + return MakeXRegistryConfigHeader(sys, authfile, username, password) default: err = fmt.Errorf("unsupported authentication header: %q", headerName) } @@ -167,9 +167,9 @@ func Header(sys *types.SystemContext, headerName HeaderAuthName, authfile, usern return nil, nil } -// headerConfig returns a map with the XRegistryConfigHeader set which can +// MakeXRegistryConfigHeader returns a map with the XRegistryConfigHeader set which can // conveniently be used in the http stack. -func headerConfig(sys *types.SystemContext, authfile, username, password string) (string, error) { +func MakeXRegistryConfigHeader(sys *types.SystemContext, authfile, username, password string) (map[string]string, error) { if sys == nil { sys = &types.SystemContext{} } @@ -178,7 +178,7 @@ func headerConfig(sys *types.SystemContext, authfile, username, password string) } authConfigs, err := imageAuth.GetAllCredentials(sys) if err != nil { - return "", err + return nil, err } if username != "" { @@ -189,9 +189,13 @@ func headerConfig(sys *types.SystemContext, authfile, username, password string) } if len(authConfigs) == 0 { - return "", nil + return nil, nil } - return encodeMultiAuthConfigs(authConfigs) + content, err := encodeMultiAuthConfigs(authConfigs) + if err != nil { + return nil, err + } + return map[string]string{XRegistryConfigHeader.String(): content}, nil } // headerAuth returns a base64 encoded map with the XRegistryAuthHeader set which can -- cgit v1.2.3-54-g00ecf From d073b1275d30b6e7d7b67f71204093dbb283b2de Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 23:05:22 +0200 Subject: Call MakeXRegistryConfigHeader instead of Header(..., XRegistryConfigHeader) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All callers hard-code a header value, so this is actually shorter. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/bindings/images/build.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'pkg') diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index be6e5ab55..0f73604e1 100644 --- a/pkg/bindings/images/build.go +++ b/pkg/bindings/images/build.go @@ -294,12 +294,12 @@ func Build(ctx context.Context, containerFiles []string, options entities.BuildO err error ) if options.SystemContext == nil { - headers, err = auth.Header(options.SystemContext, auth.XRegistryConfigHeader, "", "", "") + headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, "", "", "") } else { if options.SystemContext.DockerAuthConfig != nil { headers, err = auth.Header(options.SystemContext, auth.XRegistryAuthHeader, options.SystemContext.AuthFilePath, options.SystemContext.DockerAuthConfig.Username, options.SystemContext.DockerAuthConfig.Password) } else { - headers, err = auth.Header(options.SystemContext, auth.XRegistryConfigHeader, options.SystemContext.AuthFilePath, "", "") + headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, options.SystemContext.AuthFilePath, "", "") } } if err != nil { -- cgit v1.2.3-54-g00ecf From 78dd79752044d9ccc812a9ebd5a9c708302c0f26 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 23:13:34 +0200 Subject: Turn headerAuth into MakeXRegistryAuthHeader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... which can be called independently. For now, there are no new callers, to test that the behavior has not changed. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index d4f356f3d..84b2f8ce6 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -145,26 +145,14 @@ func getAuthCredentials(headers []string) (*types.DockerAuthConfig, map[string]t // Header builds the requested Authentication Header func Header(sys *types.SystemContext, headerName HeaderAuthName, authfile, username, password string) (map[string]string, error) { - var ( - content string - err error - ) switch headerName { case XRegistryAuthHeader: - content, err = headerAuth(sys, authfile, username, password) + return MakeXRegistryAuthHeader(sys, authfile, username, password) case XRegistryConfigHeader: return MakeXRegistryConfigHeader(sys, authfile, username, password) default: - err = fmt.Errorf("unsupported authentication header: %q", headerName) + return nil, fmt.Errorf("unsupported authentication header: %q", headerName) } - if err != nil { - return nil, err - } - - if len(content) > 0 { - return map[string]string{string(headerName): content}, nil - } - return nil, nil } // MakeXRegistryConfigHeader returns a map with the XRegistryConfigHeader set which can @@ -198,11 +186,15 @@ func MakeXRegistryConfigHeader(sys *types.SystemContext, authfile, username, pas return map[string]string{XRegistryConfigHeader.String(): content}, nil } -// headerAuth returns a base64 encoded map with the XRegistryAuthHeader set which can +// MakeXRegistryAuthHeader returns a map with the XRegistryAuthHeader set which can // conveniently be used in the http stack. -func headerAuth(sys *types.SystemContext, authfile, username, password string) (string, error) { +func MakeXRegistryAuthHeader(sys *types.SystemContext, authfile, username, password string) (map[string]string, error) { if username != "" { - return encodeSingleAuthConfig(types.DockerAuthConfig{Username: username, Password: password}) + content, err := encodeSingleAuthConfig(types.DockerAuthConfig{Username: username, Password: password}) + if err != nil { + return nil, err + } + return map[string]string{XRegistryAuthHeader.String(): content}, nil } if sys == nil { @@ -213,9 +205,13 @@ func headerAuth(sys *types.SystemContext, authfile, username, password string) ( } authConfigs, err := imageAuth.GetAllCredentials(sys) if err != nil { - return "", err + return nil, err + } + content, err := encodeMultiAuthConfigs(authConfigs) + if err != nil { + return nil, err } - return encodeMultiAuthConfigs(authConfigs) + return map[string]string{XRegistryAuthHeader.String(): content}, nil } // RemoveAuthfile is a convenience function that is meant to be called in a -- cgit v1.2.3-54-g00ecf From 3725a34cbf592697d5fca3089d14045c66975fda Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Sat, 11 Sep 2021 23:15:49 +0200 Subject: Call MakeXRegistryAuthHeader instead of Header(..., XRegistryAuthHeader) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All callers hard-code a header value, so this is actually shorter. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/bindings/images/build.go | 2 +- pkg/bindings/images/images.go | 4 ++-- pkg/bindings/images/pull.go | 2 +- pkg/bindings/play/play.go | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) (limited to 'pkg') diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index 0f73604e1..f643b3c89 100644 --- a/pkg/bindings/images/build.go +++ b/pkg/bindings/images/build.go @@ -297,7 +297,7 @@ func Build(ctx context.Context, containerFiles []string, options entities.BuildO headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, "", "", "") } else { if options.SystemContext.DockerAuthConfig != nil { - headers, err = auth.Header(options.SystemContext, auth.XRegistryAuthHeader, options.SystemContext.AuthFilePath, options.SystemContext.DockerAuthConfig.Username, options.SystemContext.DockerAuthConfig.Password) + headers, err = auth.MakeXRegistryAuthHeader(options.SystemContext, options.SystemContext.AuthFilePath, options.SystemContext.DockerAuthConfig.Username, options.SystemContext.DockerAuthConfig.Password) } else { headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, options.SystemContext.AuthFilePath, "", "") } diff --git a/pkg/bindings/images/images.go b/pkg/bindings/images/images.go index dfb500772..74603015c 100644 --- a/pkg/bindings/images/images.go +++ b/pkg/bindings/images/images.go @@ -280,7 +280,7 @@ func Push(ctx context.Context, source string, destination string, options *PushO return err } // TODO: have a global system context we can pass around (1st argument) - header, err := auth.Header(nil, auth.XRegistryAuthHeader, options.GetAuthfile(), options.GetUsername(), options.GetPassword()) + header, err := auth.MakeXRegistryAuthHeader(nil, options.GetAuthfile(), options.GetUsername(), options.GetPassword()) if err != nil { return err } @@ -329,7 +329,7 @@ func Search(ctx context.Context, term string, options *SearchOptions) ([]entitie } // TODO: have a global system context we can pass around (1st argument) - header, err := auth.Header(nil, auth.XRegistryAuthHeader, options.GetAuthfile(), "", "") + header, err := auth.MakeXRegistryAuthHeader(nil, options.GetAuthfile(), "", "") if err != nil { return nil, err } diff --git a/pkg/bindings/images/pull.go b/pkg/bindings/images/pull.go index be21aa593..c6f20e3e1 100644 --- a/pkg/bindings/images/pull.go +++ b/pkg/bindings/images/pull.go @@ -42,7 +42,7 @@ func Pull(ctx context.Context, rawImage string, options *PullOptions) ([]string, } // TODO: have a global system context we can pass around (1st argument) - header, err := auth.Header(nil, auth.XRegistryAuthHeader, options.GetAuthfile(), options.GetUsername(), options.GetPassword()) + header, err := auth.MakeXRegistryAuthHeader(nil, options.GetAuthfile(), options.GetUsername(), options.GetPassword()) if err != nil { return nil, err } diff --git a/pkg/bindings/play/play.go b/pkg/bindings/play/play.go index 2cd7c3997..64a2ae6ae 100644 --- a/pkg/bindings/play/play.go +++ b/pkg/bindings/play/play.go @@ -40,7 +40,7 @@ func Kube(ctx context.Context, path string, options *KubeOptions) (*entities.Pla } // TODO: have a global system context we can pass around (1st argument) - header, err := auth.Header(nil, auth.XRegistryAuthHeader, options.GetAuthfile(), options.GetUsername(), options.GetPassword()) + header, err := auth.MakeXRegistryAuthHeader(nil, options.GetAuthfile(), options.GetUsername(), options.GetPassword()) if err != nil { return nil, err } -- cgit v1.2.3-54-g00ecf From fe1230ef7003e89ad9c92fd11e21494ecc64de85 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Thu, 21 Oct 2021 19:21:02 +0200 Subject: Remove pkg/auth.Header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is no longer used. Split the existing tests into MakeXRegistryConfigHeader and MakeXRegistryAuthHeader variants. For now we don't modify the implementations at all, to make review simpler; cleanups will follow. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 13 ------ pkg/auth/auth_test.go | 123 ++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 100 insertions(+), 36 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 84b2f8ce6..b68109429 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -3,7 +3,6 @@ package auth import ( "encoding/base64" "encoding/json" - "fmt" "io/ioutil" "net/http" "os" @@ -143,18 +142,6 @@ func getAuthCredentials(headers []string) (*types.DockerAuthConfig, map[string]t return &authConfig, nil, nil } -// Header builds the requested Authentication Header -func Header(sys *types.SystemContext, headerName HeaderAuthName, authfile, username, password string) (map[string]string, error) { - switch headerName { - case XRegistryAuthHeader: - return MakeXRegistryAuthHeader(sys, authfile, username, password) - case XRegistryConfigHeader: - return MakeXRegistryConfigHeader(sys, authfile, username, password) - default: - return nil, fmt.Errorf("unsupported authentication header: %q", headerName) - } -} - // MakeXRegistryConfigHeader returns a map with the XRegistryConfigHeader set which can // conveniently be used in the http stack. func MakeXRegistryConfigHeader(sys *types.SystemContext, authfile, username, password string) (map[string]string, error) { diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index 6acf1f8fb..e0d2f1ac6 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -3,7 +3,6 @@ package auth import ( "encoding/base64" "encoding/json" - "fmt" "io/ioutil" "net/http" "os" @@ -30,10 +29,9 @@ var largeAuthFileValues = map[string]types.DockerAuthConfig{ "quay.io": {Username: "quay", Password: "top"}, } -// Test that GetCredentials() correctly parses what Header() produces -func TestHeaderGetCredentialsRoundtrip(t *testing.T) { +// Test that GetCredentials() correctly parses what MakeXRegistryConfigHeader() produces +func TestMakeXRegistryConfigHeaderGetCredentialsRoundtrip(t *testing.T) { for _, tc := range []struct { - headerName HeaderAuthName name string fileContents string username, password string @@ -41,7 +39,6 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { expectedFileValues map[string]types.DockerAuthConfig }{ { - headerName: XRegistryConfigHeader, name: "no data", fileContents: "", username: "", @@ -50,7 +47,6 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { expectedFileValues: nil, }, { - headerName: XRegistryConfigHeader, name: "file data", fileContents: largeAuthFile, username: "", @@ -59,7 +55,6 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { expectedFileValues: largeAuthFileValues, }, { - headerName: XRegistryConfigHeader, name: "file data + override", fileContents: largeAuthFile, username: "override-user", @@ -67,8 +62,53 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { expectedOverride: &types.DockerAuthConfig{Username: "override-user", Password: "override-pass"}, expectedFileValues: largeAuthFileValues, }, + } { + name := tc.name + inputAuthFile := "" + if tc.fileContents != "" { + f, err := ioutil.TempFile("", "auth.json") + require.NoError(t, err, name) + defer os.Remove(f.Name()) + inputAuthFile = f.Name() + err = ioutil.WriteFile(inputAuthFile, []byte(tc.fileContents), 0700) + require.NoError(t, err, name) + } + + headers, err := MakeXRegistryConfigHeader(nil, inputAuthFile, tc.username, tc.password) + require.NoError(t, err) + req, err := http.NewRequest(http.MethodPost, "/", nil) + require.NoError(t, err, name) + for k, v := range headers { + req.Header.Set(k, v) + } + + override, resPath, err := GetCredentials(req) + require.NoError(t, err, name) + defer RemoveAuthfile(resPath) + if tc.expectedOverride == nil { + assert.Nil(t, override, name) + } else { + require.NotNil(t, override, name) + assert.Equal(t, *tc.expectedOverride, *override, name) + } + for key, expectedAuth := range tc.expectedFileValues { + auth, err := config.GetCredentials(&types.SystemContext{AuthFilePath: resPath}, key) + require.NoError(t, err, name) + assert.Equal(t, expectedAuth, auth, "%s, key %s", name, key) + } + } +} + +// Test that GetCredentials() correctly parses what MakeXRegistryAuthHeader() produces +func TestMakeXRegistryAuthHeaderGetCredentialsRoundtrip(t *testing.T) { + for _, tc := range []struct { + name string + fileContents string + username, password string + expectedOverride *types.DockerAuthConfig + expectedFileValues map[string]types.DockerAuthConfig + }{ { - headerName: XRegistryAuthHeader, name: "override", fileContents: "", username: "override-user", @@ -77,7 +117,6 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { expectedFileValues: nil, }, { - headerName: XRegistryAuthHeader, name: "file data", fileContents: largeAuthFile, username: "", @@ -85,7 +124,7 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { expectedFileValues: largeAuthFileValues, }, } { - name := fmt.Sprintf("%s: %s", tc.headerName, tc.name) + name := tc.name inputAuthFile := "" if tc.fileContents != "" { f, err := ioutil.TempFile("", "auth.json") @@ -96,7 +135,7 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { require.NoError(t, err, name) } - headers, err := Header(nil, tc.headerName, inputAuthFile, tc.username, tc.password) + headers, err := MakeXRegistryAuthHeader(nil, inputAuthFile, tc.username, tc.password) require.NoError(t, err) req, err := http.NewRequest(http.MethodPost, "/", nil) require.NoError(t, err, name) @@ -121,9 +160,8 @@ func TestHeaderGetCredentialsRoundtrip(t *testing.T) { } } -func TestHeader(t *testing.T) { +func TestMakeXRegistryConfigHeader(t *testing.T) { for _, tc := range []struct { - headerName HeaderAuthName name string fileContents string username, password string @@ -131,7 +169,6 @@ func TestHeader(t *testing.T) { expectedContents string }{ { - headerName: XRegistryConfigHeader, name: "no data", fileContents: "", username: "", @@ -139,7 +176,6 @@ func TestHeader(t *testing.T) { expectedContents: "", }, { - headerName: XRegistryConfigHeader, name: "invalid JSON", fileContents: "@invalid JSON", username: "", @@ -147,7 +183,6 @@ func TestHeader(t *testing.T) { shouldErr: true, }, { - headerName: XRegistryConfigHeader, name: "file data", fileContents: largeAuthFile, username: "", @@ -160,7 +195,6 @@ func TestHeader(t *testing.T) { }`, }, { - headerName: XRegistryConfigHeader, name: "file data + override", fileContents: largeAuthFile, username: "override-user", @@ -173,8 +207,53 @@ func TestHeader(t *testing.T) { "": {"username": "override-user", "password": "override-pass"} }`, }, + } { + name := tc.name + authFile := "" + if tc.fileContents != "" { + f, err := ioutil.TempFile("", "auth.json") + require.NoError(t, err, name) + defer os.Remove(f.Name()) + authFile = f.Name() + err = ioutil.WriteFile(authFile, []byte(tc.fileContents), 0700) + require.NoError(t, err, name) + } + + res, err := MakeXRegistryConfigHeader(nil, authFile, tc.username, tc.password) + if tc.shouldErr { + assert.Error(t, err, name) + } else { + require.NoError(t, err, name) + if tc.expectedContents == "" { + assert.Empty(t, res, name) + } else { + require.Len(t, res, 1, name) + header, ok := res[XRegistryConfigHeader.String()] + require.True(t, ok, name) + decodedHeader, err := base64.URLEncoding.DecodeString(header) + require.NoError(t, err, name) + // Don't test for a specific JSON representation, just for the expected contents. + expected := map[string]interface{}{} + actual := map[string]interface{}{} + err = json.Unmarshal([]byte(tc.expectedContents), &expected) + require.NoError(t, err, name) + err = json.Unmarshal(decodedHeader, &actual) + require.NoError(t, err, name) + assert.Equal(t, expected, actual, name) + } + } + } +} + +func TestMakeXRegistryAuthHeader(t *testing.T) { + for _, tc := range []struct { + name string + fileContents string + username, password string + shouldErr bool + expectedContents string + }{ { - headerName: XRegistryAuthHeader, name: "override", fileContents: "", username: "override-user", @@ -182,7 +261,6 @@ func TestHeader(t *testing.T) { expectedContents: `{"username": "override-user", "password": "override-pass"}`, }, { - headerName: XRegistryAuthHeader, name: "invalid JSON", fileContents: "@invalid JSON", username: "", @@ -190,7 +268,6 @@ func TestHeader(t *testing.T) { shouldErr: true, }, { - headerName: XRegistryAuthHeader, name: "file data", fileContents: largeAuthFile, username: "", @@ -203,7 +280,7 @@ func TestHeader(t *testing.T) { }`, }, } { - name := fmt.Sprintf("%s: %s", tc.headerName, tc.name) + name := tc.name authFile := "" if tc.fileContents != "" { f, err := ioutil.TempFile("", "auth.json") @@ -214,7 +291,7 @@ func TestHeader(t *testing.T) { require.NoError(t, err, name) } - res, err := Header(nil, tc.headerName, authFile, tc.username, tc.password) + res, err := MakeXRegistryAuthHeader(nil, authFile, tc.username, tc.password) if tc.shouldErr { assert.Error(t, err, name) } else { @@ -223,7 +300,7 @@ func TestHeader(t *testing.T) { assert.Empty(t, res, name) } else { require.Len(t, res, 1, name) - header, ok := res[tc.headerName.String()] + header, ok := res[XRegistryAuthHeader.String()] require.True(t, ok, name) decodedHeader, err := base64.URLEncoding.DecodeString(header) require.NoError(t, err, name) -- cgit v1.2.3-54-g00ecf From 0e29b89753cc252076d171acdcb5b36aa386ba3e Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Thu, 21 Oct 2021 20:44:15 +0200 Subject: Consolidate creation of SystemContext with auth.json into a helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change (test) behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth_test.go | 64 +++++++++++++++++++-------------------------------- 1 file changed, 24 insertions(+), 40 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index e0d2f1ac6..cbba600a9 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -29,6 +29,22 @@ var largeAuthFileValues = map[string]types.DockerAuthConfig{ "quay.io": {Username: "quay", Password: "top"}, } +// tempAuthFilePath returns a non-empty path pointing +// to a temporary file with fileContents, or "" if fileContents is empty; and a cleanup +// function the caller must arrange to call. +func tempAuthFilePath(t *testing.T, fileContents string) (string, func()) { + if fileContents == "" { + return "", func() {} + } + + f, err := ioutil.TempFile("", "auth.json") + require.NoError(t, err) + path := f.Name() + err = ioutil.WriteFile(path, []byte(fileContents), 0700) + require.NoError(t, err) + return path, func() { os.Remove(path) } +} + // Test that GetCredentials() correctly parses what MakeXRegistryConfigHeader() produces func TestMakeXRegistryConfigHeaderGetCredentialsRoundtrip(t *testing.T) { for _, tc := range []struct { @@ -64,16 +80,8 @@ func TestMakeXRegistryConfigHeaderGetCredentialsRoundtrip(t *testing.T) { }, } { name := tc.name - inputAuthFile := "" - if tc.fileContents != "" { - f, err := ioutil.TempFile("", "auth.json") - require.NoError(t, err, name) - defer os.Remove(f.Name()) - inputAuthFile = f.Name() - err = ioutil.WriteFile(inputAuthFile, []byte(tc.fileContents), 0700) - require.NoError(t, err, name) - } - + inputAuthFile, cleanup := tempAuthFilePath(t, tc.fileContents) + defer cleanup() headers, err := MakeXRegistryConfigHeader(nil, inputAuthFile, tc.username, tc.password) require.NoError(t, err) req, err := http.NewRequest(http.MethodPost, "/", nil) @@ -125,16 +133,8 @@ func TestMakeXRegistryAuthHeaderGetCredentialsRoundtrip(t *testing.T) { }, } { name := tc.name - inputAuthFile := "" - if tc.fileContents != "" { - f, err := ioutil.TempFile("", "auth.json") - require.NoError(t, err, name) - defer os.Remove(f.Name()) - inputAuthFile = f.Name() - err = ioutil.WriteFile(inputAuthFile, []byte(tc.fileContents), 0700) - require.NoError(t, err, name) - } - + inputAuthFile, cleanup := tempAuthFilePath(t, tc.fileContents) + defer cleanup() headers, err := MakeXRegistryAuthHeader(nil, inputAuthFile, tc.username, tc.password) require.NoError(t, err) req, err := http.NewRequest(http.MethodPost, "/", nil) @@ -209,16 +209,8 @@ func TestMakeXRegistryConfigHeader(t *testing.T) { }, } { name := tc.name - authFile := "" - if tc.fileContents != "" { - f, err := ioutil.TempFile("", "auth.json") - require.NoError(t, err, name) - defer os.Remove(f.Name()) - authFile = f.Name() - err = ioutil.WriteFile(authFile, []byte(tc.fileContents), 0700) - require.NoError(t, err, name) - } - + authFile, cleanup := tempAuthFilePath(t, tc.fileContents) + defer cleanup() res, err := MakeXRegistryConfigHeader(nil, authFile, tc.username, tc.password) if tc.shouldErr { assert.Error(t, err, name) @@ -281,16 +273,8 @@ func TestMakeXRegistryAuthHeader(t *testing.T) { }, } { name := tc.name - authFile := "" - if tc.fileContents != "" { - f, err := ioutil.TempFile("", "auth.json") - require.NoError(t, err, name) - defer os.Remove(f.Name()) - authFile = f.Name() - err = ioutil.WriteFile(authFile, []byte(tc.fileContents), 0700) - require.NoError(t, err, name) - } - + authFile, cleanup := tempAuthFilePath(t, tc.fileContents) + defer cleanup() res, err := MakeXRegistryAuthHeader(nil, authFile, tc.username, tc.password) if tc.shouldErr { assert.Error(t, err, name) -- cgit v1.2.3-54-g00ecf From 935dcbb0088a667406dd00562ea8fc1fbc00f3ef Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Thu, 21 Oct 2021 19:24:56 +0200 Subject: Remove no-longer-useful name variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit which used to contain more context, but now are just a pointless copy. Should not change (test) behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth_test.go | 68 ++++++++++++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 36 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index cbba600a9..ec5c1c9e7 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -79,30 +79,29 @@ func TestMakeXRegistryConfigHeaderGetCredentialsRoundtrip(t *testing.T) { expectedFileValues: largeAuthFileValues, }, } { - name := tc.name inputAuthFile, cleanup := tempAuthFilePath(t, tc.fileContents) defer cleanup() headers, err := MakeXRegistryConfigHeader(nil, inputAuthFile, tc.username, tc.password) require.NoError(t, err) req, err := http.NewRequest(http.MethodPost, "/", nil) - require.NoError(t, err, name) + require.NoError(t, err, tc.name) for k, v := range headers { req.Header.Set(k, v) } override, resPath, err := GetCredentials(req) - require.NoError(t, err, name) + require.NoError(t, err, tc.name) defer RemoveAuthfile(resPath) if tc.expectedOverride == nil { - assert.Nil(t, override, name) + assert.Nil(t, override, tc.name) } else { - require.NotNil(t, override, name) - assert.Equal(t, *tc.expectedOverride, *override, name) + require.NotNil(t, override, tc.name) + assert.Equal(t, *tc.expectedOverride, *override, tc.name) } for key, expectedAuth := range tc.expectedFileValues { auth, err := config.GetCredentials(&types.SystemContext{AuthFilePath: resPath}, key) - require.NoError(t, err, name) - assert.Equal(t, expectedAuth, auth, "%s, key %s", name, key) + require.NoError(t, err, tc.name) + assert.Equal(t, expectedAuth, auth, "%s, key %s", tc.name, key) } } } @@ -132,30 +131,29 @@ func TestMakeXRegistryAuthHeaderGetCredentialsRoundtrip(t *testing.T) { expectedFileValues: largeAuthFileValues, }, } { - name := tc.name inputAuthFile, cleanup := tempAuthFilePath(t, tc.fileContents) defer cleanup() headers, err := MakeXRegistryAuthHeader(nil, inputAuthFile, tc.username, tc.password) require.NoError(t, err) req, err := http.NewRequest(http.MethodPost, "/", nil) - require.NoError(t, err, name) + require.NoError(t, err, tc.name) for k, v := range headers { req.Header.Set(k, v) } override, resPath, err := GetCredentials(req) - require.NoError(t, err, name) + require.NoError(t, err, tc.name) defer RemoveAuthfile(resPath) if tc.expectedOverride == nil { - assert.Nil(t, override, name) + assert.Nil(t, override, tc.name) } else { - require.NotNil(t, override, name) - assert.Equal(t, *tc.expectedOverride, *override, name) + require.NotNil(t, override, tc.name) + assert.Equal(t, *tc.expectedOverride, *override, tc.name) } for key, expectedAuth := range tc.expectedFileValues { auth, err := config.GetCredentials(&types.SystemContext{AuthFilePath: resPath}, key) - require.NoError(t, err, name) - assert.Equal(t, expectedAuth, auth, "%s, key %s", name, key) + require.NoError(t, err, tc.name) + assert.Equal(t, expectedAuth, auth, "%s, key %s", tc.name, key) } } } @@ -208,30 +206,29 @@ func TestMakeXRegistryConfigHeader(t *testing.T) { }`, }, } { - name := tc.name authFile, cleanup := tempAuthFilePath(t, tc.fileContents) defer cleanup() res, err := MakeXRegistryConfigHeader(nil, authFile, tc.username, tc.password) if tc.shouldErr { - assert.Error(t, err, name) + assert.Error(t, err, tc.name) } else { - require.NoError(t, err, name) + require.NoError(t, err, tc.name) if tc.expectedContents == "" { - assert.Empty(t, res, name) + assert.Empty(t, res, tc.name) } else { - require.Len(t, res, 1, name) + require.Len(t, res, 1, tc.name) header, ok := res[XRegistryConfigHeader.String()] - require.True(t, ok, name) + require.True(t, ok, tc.name) decodedHeader, err := base64.URLEncoding.DecodeString(header) - require.NoError(t, err, name) + require.NoError(t, err, tc.name) // Don't test for a specific JSON representation, just for the expected contents. expected := map[string]interface{}{} actual := map[string]interface{}{} err = json.Unmarshal([]byte(tc.expectedContents), &expected) - require.NoError(t, err, name) + require.NoError(t, err, tc.name) err = json.Unmarshal(decodedHeader, &actual) - require.NoError(t, err, name) - assert.Equal(t, expected, actual, name) + require.NoError(t, err, tc.name) + assert.Equal(t, expected, actual, tc.name) } } } @@ -272,30 +269,29 @@ func TestMakeXRegistryAuthHeader(t *testing.T) { }`, }, } { - name := tc.name authFile, cleanup := tempAuthFilePath(t, tc.fileContents) defer cleanup() res, err := MakeXRegistryAuthHeader(nil, authFile, tc.username, tc.password) if tc.shouldErr { - assert.Error(t, err, name) + assert.Error(t, err, tc.name) } else { - require.NoError(t, err, name) + require.NoError(t, err, tc.name) if tc.expectedContents == "" { - assert.Empty(t, res, name) + assert.Empty(t, res, tc.name) } else { - require.Len(t, res, 1, name) + require.Len(t, res, 1, tc.name) header, ok := res[XRegistryAuthHeader.String()] - require.True(t, ok, name) + require.True(t, ok, tc.name) decodedHeader, err := base64.URLEncoding.DecodeString(header) - require.NoError(t, err, name) + require.NoError(t, err, tc.name) // Don't test for a specific JSON representation, just for the expected contents. expected := map[string]interface{}{} actual := map[string]interface{}{} err = json.Unmarshal([]byte(tc.expectedContents), &expected) - require.NoError(t, err, name) + require.NoError(t, err, tc.name) err = json.Unmarshal(decodedHeader, &actual) - require.NoError(t, err, name) - assert.Equal(t, expected, actual, name) + require.NoError(t, err, tc.name) + assert.Equal(t, expected, actual, tc.name) } } } -- cgit v1.2.3-54-g00ecf From f9be3262740a2961f5c1a4db24265234a3fc6a5f Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Thu, 21 Oct 2021 21:27:25 +0200 Subject: Remove the authfile parameter of MakeXRegistryConfigHeader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Having a parameter that modifies the provides types.SystemContext seems rather unexpected and risky to have around - and the only user of that is actually a no-op; so, remove that option and simplify. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 5 +---- pkg/auth/auth_test.go | 4 ++-- pkg/bindings/images/build.go | 4 ++-- 3 files changed, 5 insertions(+), 8 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index b68109429..006572b09 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -144,13 +144,10 @@ func getAuthCredentials(headers []string) (*types.DockerAuthConfig, map[string]t // MakeXRegistryConfigHeader returns a map with the XRegistryConfigHeader set which can // conveniently be used in the http stack. -func MakeXRegistryConfigHeader(sys *types.SystemContext, authfile, username, password string) (map[string]string, error) { +func MakeXRegistryConfigHeader(sys *types.SystemContext, username, password string) (map[string]string, error) { if sys == nil { sys = &types.SystemContext{} } - if authfile != "" { - sys.AuthFilePath = authfile - } authConfigs, err := imageAuth.GetAllCredentials(sys) if err != nil { return nil, err diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index ec5c1c9e7..a727a9d50 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -81,7 +81,7 @@ func TestMakeXRegistryConfigHeaderGetCredentialsRoundtrip(t *testing.T) { } { inputAuthFile, cleanup := tempAuthFilePath(t, tc.fileContents) defer cleanup() - headers, err := MakeXRegistryConfigHeader(nil, inputAuthFile, tc.username, tc.password) + headers, err := MakeXRegistryConfigHeader(&types.SystemContext{AuthFilePath: inputAuthFile}, tc.username, tc.password) require.NoError(t, err) req, err := http.NewRequest(http.MethodPost, "/", nil) require.NoError(t, err, tc.name) @@ -208,7 +208,7 @@ func TestMakeXRegistryConfigHeader(t *testing.T) { } { authFile, cleanup := tempAuthFilePath(t, tc.fileContents) defer cleanup() - res, err := MakeXRegistryConfigHeader(nil, authFile, tc.username, tc.password) + res, err := MakeXRegistryConfigHeader(&types.SystemContext{AuthFilePath: authFile}, tc.username, tc.password) if tc.shouldErr { assert.Error(t, err, tc.name) } else { diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index f643b3c89..54c831c36 100644 --- a/pkg/bindings/images/build.go +++ b/pkg/bindings/images/build.go @@ -294,12 +294,12 @@ func Build(ctx context.Context, containerFiles []string, options entities.BuildO err error ) if options.SystemContext == nil { - headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, "", "", "") + headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, "", "") } else { if options.SystemContext.DockerAuthConfig != nil { headers, err = auth.MakeXRegistryAuthHeader(options.SystemContext, options.SystemContext.AuthFilePath, options.SystemContext.DockerAuthConfig.Username, options.SystemContext.DockerAuthConfig.Password) } else { - headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, options.SystemContext.AuthFilePath, "", "") + headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, "", "") } } if err != nil { -- cgit v1.2.3-54-g00ecf From d79414c54ff89c3deea84e2ac600525744fc75c5 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Thu, 21 Oct 2021 20:26:05 +0200 Subject: Simplify the header decision in pkg/bindings/images.Build a bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... now that two of the three cases are the same. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/bindings/images/build.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) (limited to 'pkg') diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index 54c831c36..fece5e9d0 100644 --- a/pkg/bindings/images/build.go +++ b/pkg/bindings/images/build.go @@ -293,14 +293,10 @@ func Build(ctx context.Context, containerFiles []string, options entities.BuildO headers map[string]string err error ) - if options.SystemContext == nil { - headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, "", "") + if options.SystemContext != nil && options.SystemContext.DockerAuthConfig != nil { + headers, err = auth.MakeXRegistryAuthHeader(options.SystemContext, options.SystemContext.AuthFilePath, options.SystemContext.DockerAuthConfig.Username, options.SystemContext.DockerAuthConfig.Password) } else { - if options.SystemContext.DockerAuthConfig != nil { - headers, err = auth.MakeXRegistryAuthHeader(options.SystemContext, options.SystemContext.AuthFilePath, options.SystemContext.DockerAuthConfig.Username, options.SystemContext.DockerAuthConfig.Password) - } else { - headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, "", "") - } + headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, "", "") } if err != nil { return nil, err -- cgit v1.2.3-54-g00ecf From 3cfefa1248feb9de8041b9fc67987b508d4c3fcb Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Thu, 21 Oct 2021 21:31:22 +0200 Subject: Remove the authfile parameter of MakeXRegistryAuthHeader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Having a parameter that modifies the provides types.SystemContext seems rather unexpected and risky to have around - and the only user of that is actually a no-op, others only provide a nil SystemContext; so, remove that option and simplify (well, somewhat; many callers now have extra &types.SystemContext{AuthFilePath} boilerplate; at least that's consistent with that code carrying a TODO to create a larger-scope SystemContext). Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 5 +---- pkg/auth/auth_test.go | 28 ++++++++++++++-------------- pkg/bindings/images/build.go | 2 +- pkg/bindings/images/images.go | 5 +++-- pkg/bindings/images/pull.go | 3 ++- pkg/bindings/play/play.go | 3 ++- 6 files changed, 23 insertions(+), 23 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 006572b09..2124b5302 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -172,7 +172,7 @@ func MakeXRegistryConfigHeader(sys *types.SystemContext, username, password stri // MakeXRegistryAuthHeader returns a map with the XRegistryAuthHeader set which can // conveniently be used in the http stack. -func MakeXRegistryAuthHeader(sys *types.SystemContext, authfile, username, password string) (map[string]string, error) { +func MakeXRegistryAuthHeader(sys *types.SystemContext, username, password string) (map[string]string, error) { if username != "" { content, err := encodeSingleAuthConfig(types.DockerAuthConfig{Username: username, Password: password}) if err != nil { @@ -184,9 +184,6 @@ func MakeXRegistryAuthHeader(sys *types.SystemContext, authfile, username, passw if sys == nil { sys = &types.SystemContext{} } - if authfile != "" { - sys.AuthFilePath = authfile - } authConfigs, err := imageAuth.GetAllCredentials(sys) if err != nil { return nil, err diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index a727a9d50..bce488a91 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -29,12 +29,12 @@ var largeAuthFileValues = map[string]types.DockerAuthConfig{ "quay.io": {Username: "quay", Password: "top"}, } -// tempAuthFilePath returns a non-empty path pointing -// to a temporary file with fileContents, or "" if fileContents is empty; and a cleanup -// function the caller must arrange to call. -func tempAuthFilePath(t *testing.T, fileContents string) (string, func()) { +// systemContextForAuthFile returns a types.SystemContext with AuthFilePath pointing +// to a temporary file with fileContents, or nil if fileContents is empty; and a cleanup +// function the calle rmust arrange to call. +func systemContextForAuthFile(t *testing.T, fileContents string) (*types.SystemContext, func()) { if fileContents == "" { - return "", func() {} + return nil, func() {} } f, err := ioutil.TempFile("", "auth.json") @@ -42,7 +42,7 @@ func tempAuthFilePath(t *testing.T, fileContents string) (string, func()) { path := f.Name() err = ioutil.WriteFile(path, []byte(fileContents), 0700) require.NoError(t, err) - return path, func() { os.Remove(path) } + return &types.SystemContext{AuthFilePath: path}, func() { os.Remove(path) } } // Test that GetCredentials() correctly parses what MakeXRegistryConfigHeader() produces @@ -79,9 +79,9 @@ func TestMakeXRegistryConfigHeaderGetCredentialsRoundtrip(t *testing.T) { expectedFileValues: largeAuthFileValues, }, } { - inputAuthFile, cleanup := tempAuthFilePath(t, tc.fileContents) + sys, cleanup := systemContextForAuthFile(t, tc.fileContents) defer cleanup() - headers, err := MakeXRegistryConfigHeader(&types.SystemContext{AuthFilePath: inputAuthFile}, tc.username, tc.password) + headers, err := MakeXRegistryConfigHeader(sys, tc.username, tc.password) require.NoError(t, err) req, err := http.NewRequest(http.MethodPost, "/", nil) require.NoError(t, err, tc.name) @@ -131,9 +131,9 @@ func TestMakeXRegistryAuthHeaderGetCredentialsRoundtrip(t *testing.T) { expectedFileValues: largeAuthFileValues, }, } { - inputAuthFile, cleanup := tempAuthFilePath(t, tc.fileContents) + sys, cleanup := systemContextForAuthFile(t, tc.fileContents) defer cleanup() - headers, err := MakeXRegistryAuthHeader(nil, inputAuthFile, tc.username, tc.password) + headers, err := MakeXRegistryAuthHeader(sys, tc.username, tc.password) require.NoError(t, err) req, err := http.NewRequest(http.MethodPost, "/", nil) require.NoError(t, err, tc.name) @@ -206,9 +206,9 @@ func TestMakeXRegistryConfigHeader(t *testing.T) { }`, }, } { - authFile, cleanup := tempAuthFilePath(t, tc.fileContents) + sys, cleanup := systemContextForAuthFile(t, tc.fileContents) defer cleanup() - res, err := MakeXRegistryConfigHeader(&types.SystemContext{AuthFilePath: authFile}, tc.username, tc.password) + res, err := MakeXRegistryConfigHeader(sys, tc.username, tc.password) if tc.shouldErr { assert.Error(t, err, tc.name) } else { @@ -269,9 +269,9 @@ func TestMakeXRegistryAuthHeader(t *testing.T) { }`, }, } { - authFile, cleanup := tempAuthFilePath(t, tc.fileContents) + sys, cleanup := systemContextForAuthFile(t, tc.fileContents) defer cleanup() - res, err := MakeXRegistryAuthHeader(nil, authFile, tc.username, tc.password) + res, err := MakeXRegistryAuthHeader(sys, tc.username, tc.password) if tc.shouldErr { assert.Error(t, err, tc.name) } else { diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index fece5e9d0..7bca43132 100644 --- a/pkg/bindings/images/build.go +++ b/pkg/bindings/images/build.go @@ -294,7 +294,7 @@ func Build(ctx context.Context, containerFiles []string, options entities.BuildO err error ) if options.SystemContext != nil && options.SystemContext.DockerAuthConfig != nil { - headers, err = auth.MakeXRegistryAuthHeader(options.SystemContext, options.SystemContext.AuthFilePath, options.SystemContext.DockerAuthConfig.Username, options.SystemContext.DockerAuthConfig.Password) + headers, err = auth.MakeXRegistryAuthHeader(options.SystemContext, options.SystemContext.DockerAuthConfig.Username, options.SystemContext.DockerAuthConfig.Password) } else { headers, err = auth.MakeXRegistryConfigHeader(options.SystemContext, "", "") } diff --git a/pkg/bindings/images/images.go b/pkg/bindings/images/images.go index 74603015c..152ff0cde 100644 --- a/pkg/bindings/images/images.go +++ b/pkg/bindings/images/images.go @@ -8,6 +8,7 @@ import ( "net/url" "strconv" + imageTypes "github.com/containers/image/v5/types" "github.com/containers/podman/v3/pkg/api/handlers/types" "github.com/containers/podman/v3/pkg/auth" "github.com/containers/podman/v3/pkg/bindings" @@ -280,7 +281,7 @@ func Push(ctx context.Context, source string, destination string, options *PushO return err } // TODO: have a global system context we can pass around (1st argument) - header, err := auth.MakeXRegistryAuthHeader(nil, options.GetAuthfile(), options.GetUsername(), options.GetPassword()) + header, err := auth.MakeXRegistryAuthHeader(&imageTypes.SystemContext{AuthFilePath: options.GetAuthfile()}, options.GetUsername(), options.GetPassword()) if err != nil { return err } @@ -329,7 +330,7 @@ func Search(ctx context.Context, term string, options *SearchOptions) ([]entitie } // TODO: have a global system context we can pass around (1st argument) - header, err := auth.MakeXRegistryAuthHeader(nil, options.GetAuthfile(), "", "") + header, err := auth.MakeXRegistryAuthHeader(&imageTypes.SystemContext{AuthFilePath: options.GetAuthfile()}, "", "") if err != nil { return nil, err } diff --git a/pkg/bindings/images/pull.go b/pkg/bindings/images/pull.go index c6f20e3e1..ac583973f 100644 --- a/pkg/bindings/images/pull.go +++ b/pkg/bindings/images/pull.go @@ -10,6 +10,7 @@ import ( "os" "strconv" + "github.com/containers/image/v5/types" "github.com/containers/podman/v3/pkg/auth" "github.com/containers/podman/v3/pkg/bindings" "github.com/containers/podman/v3/pkg/domain/entities" @@ -42,7 +43,7 @@ func Pull(ctx context.Context, rawImage string, options *PullOptions) ([]string, } // TODO: have a global system context we can pass around (1st argument) - header, err := auth.MakeXRegistryAuthHeader(nil, options.GetAuthfile(), options.GetUsername(), options.GetPassword()) + header, err := auth.MakeXRegistryAuthHeader(&types.SystemContext{AuthFilePath: options.GetAuthfile()}, options.GetUsername(), options.GetPassword()) if err != nil { return nil, err } diff --git a/pkg/bindings/play/play.go b/pkg/bindings/play/play.go index 64a2ae6ae..111a25cac 100644 --- a/pkg/bindings/play/play.go +++ b/pkg/bindings/play/play.go @@ -6,6 +6,7 @@ import ( "os" "strconv" + "github.com/containers/image/v5/types" "github.com/containers/podman/v3/pkg/auth" "github.com/containers/podman/v3/pkg/bindings" "github.com/containers/podman/v3/pkg/domain/entities" @@ -40,7 +41,7 @@ func Kube(ctx context.Context, path string, options *KubeOptions) (*entities.Pla } // TODO: have a global system context we can pass around (1st argument) - header, err := auth.MakeXRegistryAuthHeader(nil, options.GetAuthfile(), options.GetUsername(), options.GetPassword()) + header, err := auth.MakeXRegistryAuthHeader(&types.SystemContext{AuthFilePath: options.GetAuthfile()}, options.GetUsername(), options.GetPassword()) if err != nil { return nil, err } -- cgit v1.2.3-54-g00ecf From 5bbcfaf4aa4f276423d9fae31aee5945b7f5a9b8 Mon Sep 17 00:00:00 2001 From: Miloslav Trmač Date: Thu, 21 Oct 2021 20:52:38 +0200 Subject: Make XRegistryAuthHeader and XRegistryConfigHeader private MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... now that they have no public users. Also remove the HeaderAuthName type, we don't need the type-safety so much for private constants, and using plain strings results in less visual noise. Should not change behavior. Signed-off-by: Miloslav Trmač --- pkg/auth/auth.go | 52 ++++++++++++++++++++++++--------------------------- pkg/auth/auth_test.go | 4 ++-- 2 files changed, 26 insertions(+), 30 deletions(-) (limited to 'pkg') diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 2124b5302..f423c011d 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -15,37 +15,33 @@ import ( "github.com/sirupsen/logrus" ) -type HeaderAuthName string - -func (h HeaderAuthName) String() string { return string(h) } - -// XRegistryAuthHeader is the key to the encoded registry authentication configuration in an http-request header. +// xRegistryAuthHeader is the key to the encoded registry authentication configuration in an http-request header. // This header supports one registry per header occurrence. To support N registries provide N headers, one per registry. // As of Docker API 1.40 and Libpod API 1.0.0, this header is supported by all endpoints. -const XRegistryAuthHeader HeaderAuthName = "X-Registry-Auth" +const xRegistryAuthHeader = "X-Registry-Auth" -// XRegistryConfigHeader is the key to the encoded registry authentication configuration in an http-request header. +// xRegistryConfigHeader is the key to the encoded registry authentication configuration in an http-request header. // This header supports N registries in one header via a Base64 encoded, JSON map. // As of Docker API 1.40 and Libpod API 2.0.0, this header is supported by build endpoints. -const XRegistryConfigHeader HeaderAuthName = "X-Registry-Config" +const xRegistryConfigHeader = "X-Registry-Config" // GetCredentials queries the http.Request for X-Registry-.* headers and extracts // the necessary authentication information for libpod operations, possibly // creating a config file. If that is the case, the caller must call RemoveAuthFile. func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { - nonemptyHeaderValue := func(key HeaderAuthName) ([]string, bool) { - hdr := r.Header.Values(key.String()) + nonemptyHeaderValue := func(key string) ([]string, bool) { + hdr := r.Header.Values(key) return hdr, len(hdr) > 0 } var override *types.DockerAuthConfig var fileContents map[string]types.DockerAuthConfig - var headerName HeaderAuthName + var headerName string var err error - if hdr, ok := nonemptyHeaderValue(XRegistryConfigHeader); ok { - headerName = XRegistryConfigHeader + if hdr, ok := nonemptyHeaderValue(xRegistryConfigHeader); ok { + headerName = xRegistryConfigHeader override, fileContents, err = getConfigCredentials(r, hdr) - } else if hdr, ok := nonemptyHeaderValue(XRegistryAuthHeader); ok { - headerName = XRegistryAuthHeader + } else if hdr, ok := nonemptyHeaderValue(xRegistryAuthHeader); ok { + headerName = xRegistryAuthHeader override, fileContents, err = getAuthCredentials(hdr) } else { return nil, "", nil @@ -67,7 +63,7 @@ func GetCredentials(r *http.Request) (*types.DockerAuthConfig, string, error) { } // getConfigCredentials extracts one or more docker.AuthConfig from a request and its -// XRegistryConfigHeader value. An empty key will be used as default while a named registry will be +// xRegistryConfigHeader value. An empty key will be used as default while a named registry will be // returned as types.DockerAuthConfig func getConfigCredentials(r *http.Request, headers []string) (*types.DockerAuthConfig, map[string]types.DockerAuthConfig, error) { var auth *types.DockerAuthConfig @@ -76,13 +72,13 @@ func getConfigCredentials(r *http.Request, headers []string) (*types.DockerAuthC for _, h := range headers { param, err := base64.URLEncoding.DecodeString(h) if err != nil { - return nil, nil, errors.Wrapf(err, "failed to decode %q", XRegistryConfigHeader) + return nil, nil, errors.Wrapf(err, "failed to decode %q", xRegistryConfigHeader) } ac := make(map[string]dockerAPITypes.AuthConfig) err = json.Unmarshal(param, &ac) if err != nil { - return nil, nil, errors.Wrapf(err, "failed to unmarshal %q", XRegistryConfigHeader) + return nil, nil, errors.Wrapf(err, "failed to unmarshal %q", xRegistryConfigHeader) } for k, v := range ac { @@ -112,16 +108,16 @@ func getConfigCredentials(r *http.Request, headers []string) (*types.DockerAuthC if auth == nil { logrus.Debugf("%q header found in request, but \"registry=%v\" query parameter not provided", - XRegistryConfigHeader, registries) + xRegistryConfigHeader, registries) } else { - logrus.Debugf("%q header found in request for username %q", XRegistryConfigHeader, auth.Username) + logrus.Debugf("%q header found in request for username %q", xRegistryConfigHeader, auth.Username) } } return auth, configs, nil } -// getAuthCredentials extracts one or more DockerAuthConfigs from an XRegistryAuthHeader +// getAuthCredentials extracts one or more DockerAuthConfigs from an xRegistryAuthHeader // value. The header could specify a single-auth config in which case the // first return value is set. In case of a multi-auth header, the contents are // returned in the second return value. @@ -142,7 +138,7 @@ func getAuthCredentials(headers []string) (*types.DockerAuthConfig, map[string]t return &authConfig, nil, nil } -// MakeXRegistryConfigHeader returns a map with the XRegistryConfigHeader set which can +// MakeXRegistryConfigHeader returns a map with the "X-Registry-Config" header set, which can // conveniently be used in the http stack. func MakeXRegistryConfigHeader(sys *types.SystemContext, username, password string) (map[string]string, error) { if sys == nil { @@ -167,10 +163,10 @@ func MakeXRegistryConfigHeader(sys *types.SystemContext, username, password stri if err != nil { return nil, err } - return map[string]string{XRegistryConfigHeader.String(): content}, nil + return map[string]string{xRegistryConfigHeader: content}, nil } -// MakeXRegistryAuthHeader returns a map with the XRegistryAuthHeader set which can +// MakeXRegistryAuthHeader returns a map with the "X-Registry-Auth" header set, which can // conveniently be used in the http stack. func MakeXRegistryAuthHeader(sys *types.SystemContext, username, password string) (map[string]string, error) { if username != "" { @@ -178,7 +174,7 @@ func MakeXRegistryAuthHeader(sys *types.SystemContext, username, password string if err != nil { return nil, err } - return map[string]string{XRegistryAuthHeader.String(): content}, nil + return map[string]string{xRegistryAuthHeader: content}, nil } if sys == nil { @@ -192,7 +188,7 @@ func MakeXRegistryAuthHeader(sys *types.SystemContext, username, password string if err != nil { return nil, err } - return map[string]string{XRegistryAuthHeader.String(): content}, nil + return map[string]string{xRegistryAuthHeader: content}, nil } // RemoveAuthfile is a convenience function that is meant to be called in a @@ -309,7 +305,7 @@ func imageAuthToDockerAuth(authConfig types.DockerAuthConfig) dockerAPITypes.Aut } } -// parseSingleAuthHeader extracts a DockerAuthConfig from an XRegistryAuthHeader value. +// parseSingleAuthHeader extracts a DockerAuthConfig from an xRegistryAuthHeader value. // The header content is a single DockerAuthConfig. func parseSingleAuthHeader(authHeader string) (types.DockerAuthConfig, error) { // Accept "null" and handle it as empty value for compatibility reason with Docker. @@ -326,7 +322,7 @@ func parseSingleAuthHeader(authHeader string) (types.DockerAuthConfig, error) { return dockerAuthToImageAuth(authConfig), nil } -// parseMultiAuthHeader extracts a DockerAuthConfig from an XRegistryAuthHeader value. +// parseMultiAuthHeader extracts a DockerAuthConfig from an xRegistryAuthHeader value. // The header content is a map[string]DockerAuthConfigs. func parseMultiAuthHeader(authHeader string) (map[string]types.DockerAuthConfig, error) { // Accept "null" and handle it as empty value for compatibility reason with Docker. diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index bce488a91..f7e6e4ef6 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -217,7 +217,7 @@ func TestMakeXRegistryConfigHeader(t *testing.T) { assert.Empty(t, res, tc.name) } else { require.Len(t, res, 1, tc.name) - header, ok := res[XRegistryConfigHeader.String()] + header, ok := res[xRegistryConfigHeader] require.True(t, ok, tc.name) decodedHeader, err := base64.URLEncoding.DecodeString(header) require.NoError(t, err, tc.name) @@ -280,7 +280,7 @@ func TestMakeXRegistryAuthHeader(t *testing.T) { assert.Empty(t, res, tc.name) } else { require.Len(t, res, 1, tc.name) - header, ok := res[XRegistryAuthHeader.String()] + header, ok := res[xRegistryAuthHeader] require.True(t, ok, tc.name) decodedHeader, err := base64.URLEncoding.DecodeString(header) require.NoError(t, err, tc.name) -- cgit v1.2.3-54-g00ecf