From b162d8868c0b14961347c80df834f93f0e7e82a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 20:13:21 +0200 Subject: [PATCH 01/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 070e222ad2..73822eecf2 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 da2d9a5c51..97e7fe1ecd 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) + } + } +} From ff003928b2360304f6b9458d324df090917fab02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 20:24:07 +0200 Subject: [PATCH 02/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 73822eecf2..60c40a40bb 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 97e7fe1ecd..a0b97b1066 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 From 5a5aa6009fdfde0524bc4e551742c366ad0164df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 20:26:28 +0200 Subject: [PATCH 03/27] 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(-) diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index a0b97b1066..38f82ee04d 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) } } } From ad7e5e34f20da1422f79b704fc10a3cfec85e447 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 21:13:33 +0200 Subject: [PATCH 04/27] 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(+) diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index 38f82ee04d..e39a0e041b 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 From d29a4a6d173988b83ac78355e271d0f60e5d2830 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 21:39:26 +0200 Subject: [PATCH 05/27] 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(+) diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index e39a0e041b..ee16d832b5 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 From 1b6bf971306af753e72db9d10f7594ecd0c89785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:05:04 +0200 Subject: [PATCH 06/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 60c40a40bb..8c64368836 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 From 491951d66e1829ad8e847f3049a557dd9d55db68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 21:56:40 +0200 Subject: [PATCH 07/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 8c64368836..7cde6ef5ed 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 ee16d832b5..be86a9cbd2 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{} From 2aeb690d370de9fee15fc7c47b66fb04a30c41d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:15:23 +0200 Subject: [PATCH 08/27] 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(-) diff --git a/pkg/api/handlers/compat/images.go b/pkg/api/handlers/compat/images.go index 4533fddebc..c1cc99da44 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 f85df02e17..2fd5dcccdc 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 3a84b57998..04cad204db 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 e9cc3e2b63..f6ad86a044 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 f2f93434af..6e23845f0f 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 fabdb326bf..518e7cc65a 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 869c83fa35..eb0b6827ff 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 f943fc2402..e6ae9ad180 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 7cde6ef5ed..c0606cf1d4 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 be86a9cbd2..634215acf0 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) - } } } From 7674f2f76b07058aa3bbf44675b7c2482c61811a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:20:26 +0200 Subject: [PATCH 09/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index c0606cf1d4..c19151c911 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 634215acf0..0e6bd42ef8 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) From 6f1a26b04f9a35a649f80c07be2e3372bc65d60a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:22:21 +0200 Subject: [PATCH 10/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index c19151c911..b3fd711847 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 } From 1ecc6ba72852700f748715124bdf98573cc93c0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:28:24 +0200 Subject: [PATCH 11/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index b3fd711847..3ecdd99fec 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 0e6bd42ef8..6acf1f8fb2 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 { From 2946e83493e2628c68819357ebc15bada38d45b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:37:23 +0200 Subject: [PATCH 12/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 3ecdd99fec..8e4cb69592 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) From 1589d70bcb522e49c75632c4e0edff52d2e459c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:39:25 +0200 Subject: [PATCH 13/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 8e4cb69592..9a4e2af858 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): From da86a232851162b584a143efa3c4f3032a480413 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:43:04 +0200 Subject: [PATCH 14/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 9a4e2af858..a95ae47641 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) From 9d56ebb611e008f5ad09048499331f7aac1ceed3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:47:52 +0200 Subject: [PATCH 15/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index a95ae47641..a4d7896a86 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 From 29f40887132709b0d2097bdfe7b45ff90c3f7b47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 22:52:47 +0200 Subject: [PATCH 16/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index a4d7896a86..a17297c7d4 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 From 8155fb5658a3a282550b39b2c3a6cd80bc9653d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 23:00:02 +0200 Subject: [PATCH 17/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index a17297c7d4..d4f356f3db 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 From d073b1275d30b6e7d7b67f71204093dbb283b2de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 23:05:22 +0200 Subject: [PATCH 18/27] 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(-) diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index be6e5ab558..0f73604e11 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 { From 78dd79752044d9ccc812a9ebd5a9c708302c0f26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 23:13:34 +0200 Subject: [PATCH 19/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index d4f356f3db..84b2f8ce62 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 From 3725a34cbf592697d5fca3089d14045c66975fda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 11 Sep 2021 23:15:49 +0200 Subject: [PATCH 20/27] 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(-) diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index 0f73604e11..f643b3c891 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 dfb5007724..74603015cf 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 be21aa5931..c6f20e3e1f 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 2cd7c39977..64a2ae6ae8 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 } From fe1230ef7003e89ad9c92fd11e21494ecc64de85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 21 Oct 2021 19:21:02 +0200 Subject: [PATCH 21/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 84b2f8ce62..b68109429c 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 6acf1f8fb2..e0d2f1ac63 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) From 0e29b89753cc252076d171acdcb5b36aa386ba3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 21 Oct 2021 20:44:15 +0200 Subject: [PATCH 22/27] 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(-) diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index e0d2f1ac63..cbba600a94 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) From 935dcbb0088a667406dd00562ea8fc1fbc00f3ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 21 Oct 2021 19:24:56 +0200 Subject: [PATCH 23/27] 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(-) diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index cbba600a94..ec5c1c9e7e 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) } } } From f9be3262740a2961f5c1a4db24265234a3fc6a5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 21 Oct 2021 21:27:25 +0200 Subject: [PATCH 24/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index b68109429c..006572b092 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 ec5c1c9e7e..a727a9d50e 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 f643b3c891..54c831c369 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 { From d79414c54ff89c3deea84e2ac600525744fc75c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 21 Oct 2021 20:26:05 +0200 Subject: [PATCH 25/27] 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(-) diff --git a/pkg/bindings/images/build.go b/pkg/bindings/images/build.go index 54c831c369..fece5e9d03 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 From 3cfefa1248feb9de8041b9fc67987b508d4c3fcb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 21 Oct 2021 21:31:22 +0200 Subject: [PATCH 26/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 006572b092..2124b53028 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 a727a9d50e..bce488a91b 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 fece5e9d03..7bca431328 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 74603015cf..152ff0cde5 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 c6f20e3e1f..ac583973f0 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 64a2ae6ae8..111a25cac4 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 } From 5bbcfaf4aa4f276423d9fae31aee5945b7f5a9b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 21 Oct 2021 20:52:38 +0200 Subject: [PATCH 27/27] 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(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 2124b53028..f423c011d8 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 bce488a91b..f7e6e4ef64 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)