From 1af760de335dddd2750cea5d30bf31134ea11fb2 Mon Sep 17 00:00:00 2001 From: Somtochi Onyekwere Date: Fri, 19 May 2023 17:52:25 +0100 Subject: [PATCH 1/3] Use instance region for aws region instead of parsing region from ecr registry url. This commits removes the code that parses the region from the ecr registry and uses it to set the config. By default, LoadDefaultConfig will get the region of the instance from the metadata service which is the correct region that should be used when requesting for a token. Signed-off-by: Somtochi Onyekwere --- oci/auth/aws/auth.go | 25 ++++++++++++++----------- oci/auth/aws/auth_test.go | 12 ++++-------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/oci/auth/aws/auth.go b/oci/auth/aws/auth.go index fa088ee42..d73b6739e 100644 --- a/oci/auth/aws/auth.go +++ b/oci/auth/aws/auth.go @@ -27,6 +27,7 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/feature/ec2/imds" "github.com/aws/aws-sdk-go-v2/service/ecr" "github.com/google/go-containerregistry/pkg/authn" ctrl "sigs.k8s.io/controller-runtime" @@ -78,7 +79,7 @@ func (c *Client) WithConfig(cfg *aws.Config) { // be the case if it's running in EKS, and may need additional setup // otherwise (visit https://aws.github.io/aws-sdk-go-v2/docs/configuring-sdk/ // as a starting point). -func (c *Client) getLoginAuth(ctx context.Context, awsEcrRegion string) (authn.AuthConfig, error) { +func (c *Client) getLoginAuth(ctx context.Context) (authn.AuthConfig, error) { // No caching of tokens is attempted; the quota for getting an // auth token is high enough that getting a token every time you // scan an image is viable for O(500) images per region. See @@ -91,11 +92,20 @@ func (c *Client) getLoginAuth(ctx context.Context, awsEcrRegion string) (authn.A cfg = c.config.Copy() } else { var err error - cfg, err = config.LoadDefaultConfig(ctx, config.WithRegion(awsEcrRegion)) + cfg, err = config.LoadDefaultConfig(ctx) if err != nil { c.mu.Unlock() return authConfig, fmt.Errorf("failed to load default configuration: %w", err) } + // Query the current region from IMDS if it's not set yet. + if cfg.Region == "" { + client := imds.NewFromConfig(cfg) + resp, err := client.GetRegion(ctx, &imds.GetRegionInput{}) + if err != nil { + return authConfig, err + } + cfg.Region = resp.Region + } c.config = &cfg } c.mu.Unlock() @@ -132,18 +142,11 @@ func (c *Client) getLoginAuth(ctx context.Context, awsEcrRegion string) (authn.A return authConfig, nil } -// Login attempts to get the authentication material for ECR. It extracts -// the account and region information from the image URI. The caller can ensure -// that the passed image is a valid ECR image using ParseRegistry(). +// Login attempts to get the authentication material for ECR. func (c *Client) Login(ctx context.Context, autoLogin bool, image string) (authn.Authenticator, error) { if autoLogin { ctrl.LoggerFrom(ctx).Info("logging in to AWS ECR for " + image) - _, awsEcrRegion, ok := ParseRegistry(image) - if !ok { - return nil, errors.New("failed to parse AWS ECR image, invalid ECR image") - } - - authConfig, err := c.getLoginAuth(ctx, awsEcrRegion) + authConfig, err := c.getLoginAuth(ctx) if err != nil { return nil, err } diff --git a/oci/auth/aws/auth_test.go b/oci/auth/aws/auth_test.go index d06f76c3d..e9b2d5527 100644 --- a/oci/auth/aws/auth_test.go +++ b/oci/auth/aws/auth_test.go @@ -155,10 +155,13 @@ func TestGetLoginAuth(t *testing.T) { cfg.EndpointResolverWithOptions = aws.EndpointResolverWithOptionsFunc(func(service, region string, options ...interface{}) (aws.Endpoint, error) { return aws.Endpoint{URL: srv.URL}, nil }) + // set the region in the config since we are not using the `LoadDefaultConfig` function that sets the region + // by querying the instance metadata service(IMDS) + cfg.Region = "us-east-1" cfg.Credentials = credentials.NewStaticCredentialsProvider("x", "y", "z") ec.WithConfig(cfg) - a, err := ec.getLoginAuth(context.TODO(), "us-east-1") + a, err := ec.getLoginAuth(context.TODO()) g.Expect(err != nil).To(Equal(tt.wantErr)) if tt.statusCode == http.StatusOK { g.Expect(a).To(Equal(tt.wantAuthConfig)) @@ -195,13 +198,6 @@ func TestLogin(t *testing.T) { statusCode: http.StatusInternalServerError, wantErr: true, }, - { - name: "non ECR image", - autoLogin: true, - image: "gcr.io/foo/bar:v1", - statusCode: http.StatusOK, - wantErr: true, - }, } for _, tt := range tests { From 48ad0ebbf390cb7a81073b96c46648874027631a Mon Sep 17 00:00:00 2001 From: Somtochi Onyekwere Date: Fri, 19 May 2023 18:01:14 +0100 Subject: [PATCH 2/3] Implement new OIDC Login method and update old Login method to ignore ref argument The Login function in oci/auth no longer uses the name.Reference argument and instead gets the registry host from the image argument and passes that to the Login method of the different region. This is to allow login to consumers of this function that don't have a repository name and just a registry host. Signed-off-by: Somtochi Onyekwere --- oci/auth/aws/auth.go | 11 +++++++++ oci/auth/aws/auth_test.go | 8 +++++++ oci/auth/azure/auth.go | 28 ++++++++++++++++++----- oci/auth/azure/auth_test.go | 19 ++++++++-------- oci/auth/azure/exchanger.go | 17 +++++++++----- oci/auth/gcp/auth.go | 12 ++++++++++ oci/auth/gcp/auth_test.go | 8 +++++++ oci/auth/login/login.go | 45 +++++++++++++++++++++++++++++++++++-- oci/go.mod | 2 +- 9 files changed, 127 insertions(+), 23 deletions(-) diff --git a/oci/auth/aws/auth.go b/oci/auth/aws/auth.go index d73b6739e..6b221cc52 100644 --- a/oci/auth/aws/auth.go +++ b/oci/auth/aws/auth.go @@ -156,3 +156,14 @@ func (c *Client) Login(ctx context.Context, autoLogin bool, image string) (authn } return nil, fmt.Errorf("ECR authentication failed: %w", oci.ErrUnconfiguredProvider) } + +// OIDCLogin attempts to get the authentication material for ECR. +func (c *Client) OIDCLogin(ctx context.Context) (authn.Authenticator, error) { + authConfig, err := c.getLoginAuth(ctx) + if err != nil { + return nil, err + } + + auth := authn.FromConfig(authConfig) + return auth, nil +} diff --git a/oci/auth/aws/auth_test.go b/oci/auth/aws/auth_test.go index e9b2d5527..b78019d30 100644 --- a/oci/auth/aws/auth_test.go +++ b/oci/auth/aws/auth_test.go @@ -176,6 +176,7 @@ func TestLogin(t *testing.T) { autoLogin bool image string statusCode int + testOIDC bool wantErr bool }{ { @@ -190,12 +191,14 @@ func TestLogin(t *testing.T) { autoLogin: true, image: testValidECRImage, statusCode: http.StatusOK, + testOIDC: true, }, { name: "login failure", autoLogin: true, image: testValidECRImage, statusCode: http.StatusInternalServerError, + testOIDC: true, wantErr: true, }, } @@ -224,6 +227,11 @@ func TestLogin(t *testing.T) { _, err := ecrClient.Login(context.TODO(), tt.autoLogin, tt.image) g.Expect(err != nil).To(Equal(tt.wantErr)) + + if tt.testOIDC { + _, err = ecrClient.OIDCLogin(context.TODO()) + g.Expect(err != nil).To(Equal(tt.wantErr)) + } }) } } diff --git a/oci/auth/azure/auth.go b/oci/auth/azure/auth.go index 61a63b36f..7a11ce97c 100644 --- a/oci/auth/azure/auth.go +++ b/oci/auth/azure/auth.go @@ -58,8 +58,9 @@ func (c *Client) WithScheme(scheme string) *Client { } // getLoginAuth returns authentication for ACR. The details needed for authentication -// are gotten from environment variable so there is not need to mount a host path. -func (c *Client) getLoginAuth(ctx context.Context, ref name.Reference) (authn.AuthConfig, error) { +// are gotten from environment variable so there is no need to mount a host path. +// The endpoint is the registry server and will be queried for OAuth authorization token. +func (c *Client) getLoginAuth(ctx context.Context, registryURL string) (authn.AuthConfig, error) { var authConfig authn.AuthConfig // Use default credentials if no token credential is provided. @@ -83,8 +84,7 @@ func (c *Client) getLoginAuth(ctx context.Context, ref name.Reference) (authn.Au } // Obtain ACR access token using exchanger. - endpoint := fmt.Sprintf("%s://%s", c.scheme, ref.Context().RegistryStr()) - ex := newExchanger(endpoint) + ex := newExchanger(registryURL) accessToken, err := ex.ExchangeACRAccessToken(string(armToken.Token)) if err != nil { return authConfig, fmt.Errorf("error exchanging token: %w", err) @@ -114,7 +114,10 @@ func ValidHost(host string) bool { func (c *Client) Login(ctx context.Context, autoLogin bool, image string, ref name.Reference) (authn.Authenticator, error) { if autoLogin { ctrl.LoggerFrom(ctx).Info("logging in to Azure ACR for " + image) - authConfig, err := c.getLoginAuth(ctx, ref) + // get registry host from image + strArr := strings.SplitN(image, "/", 2) + endpoint := fmt.Sprintf("%s://%s", c.scheme, strArr[0]) + authConfig, err := c.getLoginAuth(ctx, endpoint) if err != nil { ctrl.LoggerFrom(ctx).Info("error logging into ACR " + err.Error()) return nil, err @@ -125,3 +128,18 @@ func (c *Client) Login(ctx context.Context, autoLogin bool, image string, ref na } return nil, fmt.Errorf("ACR authentication failed: %w", oci.ErrUnconfiguredProvider) } + +// OIDCLogin attempts to get an Authenticator for the provided ACR registry URL endpoint. +// +// If you want to construct an Authenticator based on an image reference, +// you may want to use Login instead. +func (c *Client) OIDCLogin(ctx context.Context, registryUrl string) (authn.Authenticator, error) { + authConfig, err := c.getLoginAuth(ctx, registryUrl) + if err != nil { + ctrl.LoggerFrom(ctx).Info("error logging into ACR " + err.Error()) + return nil, err + } + + auth := authn.FromConfig(authConfig) + return auth, nil +} diff --git a/oci/auth/azure/auth_test.go b/oci/auth/azure/auth_test.go index 6482b9743..21e1f9186 100644 --- a/oci/auth/azure/auth_test.go +++ b/oci/auth/azure/auth_test.go @@ -78,19 +78,12 @@ func TestGetAzureLoginAuth(t *testing.T) { srv.Close() }) - // Construct an image repo name against the test server. - u, err := url.Parse(srv.URL) - g.Expect(err).ToNot(HaveOccurred()) - image := path.Join(u.Host, "foo/bar:v1") - ref, err := name.ParseReference(image) - g.Expect(err).ToNot(HaveOccurred()) - // Configure new client with test token credential. c := NewClient(). WithTokenCredential(tt.tokenCredential). WithScheme("http") - auth, err := c.getLoginAuth(context.TODO(), ref) + auth, err := c.getLoginAuth(context.TODO(), srv.URL) g.Expect(err != nil).To(Equal(tt.wantErr)) if tt.statusCode == http.StatusOK { g.Expect(auth).To(Equal(tt.wantAuthConfig)) @@ -125,6 +118,7 @@ func TestLogin(t *testing.T) { name string autoLogin bool statusCode int + testOIDC bool wantErr bool }{ { @@ -136,12 +130,14 @@ func TestLogin(t *testing.T) { { name: "with auto login", autoLogin: true, + testOIDC: true, statusCode: http.StatusOK, }, { name: "login failure", autoLogin: true, statusCode: http.StatusInternalServerError, + testOIDC: true, wantErr: true, }, } @@ -171,8 +167,13 @@ func TestLogin(t *testing.T) { WithTokenCredential(&FakeTokenCredential{Token: "foo"}). WithScheme("http") - _, err = ac.Login(context.TODO(), tt.autoLogin, image, ref) + _, err = ac.Login(context.TODO(), tt.autoLogin, u.Host, ref) g.Expect(err != nil).To(Equal(tt.wantErr)) + + if tt.testOIDC { + _, err = ac.OIDCLogin(context.TODO(), srv.URL) + g.Expect(err != nil).To(Equal(tt.wantErr)) + } }) } } diff --git a/oci/auth/azure/exchanger.go b/oci/auth/azure/exchanger.go index 00dccacc3..9ab07ea4e 100644 --- a/oci/auth/azure/exchanger.go +++ b/oci/auth/azure/exchanger.go @@ -47,6 +47,7 @@ package azure import ( "encoding/json" "fmt" + "io" "net/http" "net/url" "path" @@ -95,24 +96,28 @@ func (e *exchanger) ExchangeACRAccessToken(armToken string) (string, error) { if err != nil { return "", fmt.Errorf("failed to send token exchange request: %w", err) } + defer resp.Body.Close() + b, err := io.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("failed to read the body of the response: %w", err) + } if resp.StatusCode != http.StatusOK { // Parse the error response. var errors []acrError - decoder := json.NewDecoder(resp.Body) - if err = decoder.Decode(&errors); err == nil { + if err = json.Unmarshal(b, &errors); err == nil { return "", fmt.Errorf("unexpected status code %d from exchange request: %s", resp.StatusCode, errors) } // Error response could not be parsed, return a generic error. - return "", fmt.Errorf("unexpected status code %d from exchange request", resp.StatusCode) + return "", fmt.Errorf("unexpected status code %d from exchange request, response body: %s", + resp.StatusCode, string(b)) } var tokenResp tokenResponse - decoder := json.NewDecoder(resp.Body) - if err = decoder.Decode(&tokenResp); err != nil { - return "", fmt.Errorf("failed to decode the response: %w", err) + if err = json.Unmarshal(b, &tokenResp); err != nil { + return "", fmt.Errorf("failed to decode the response: %w, response body: %s", err, string(b)) } return tokenResp.RefreshToken, nil } diff --git a/oci/auth/gcp/auth.go b/oci/auth/gcp/auth.go index 6876f2193..52bed87d7 100644 --- a/oci/auth/gcp/auth.go +++ b/oci/auth/gcp/auth.go @@ -117,3 +117,15 @@ func (c *Client) Login(ctx context.Context, autoLogin bool, image string, ref na } return nil, fmt.Errorf("GCR authentication failed: %w", oci.ErrUnconfiguredProvider) } + +// OIDCLogin attempts to get the authentication material for GCR from the token url set in the client. +func (c *Client) OIDCLogin(ctx context.Context) (authn.Authenticator, error) { + authConfig, err := c.getLoginAuth(ctx) + if err != nil { + ctrl.LoggerFrom(ctx).Info("error logging into GCP " + err.Error()) + return nil, err + } + + auth := authn.FromConfig(authConfig) + return auth, nil +} diff --git a/oci/auth/gcp/auth_test.go b/oci/auth/gcp/auth_test.go index d6fb6ffa8..afddfb0a0 100644 --- a/oci/auth/gcp/auth_test.go +++ b/oci/auth/gcp/auth_test.go @@ -111,6 +111,7 @@ func TestLogin(t *testing.T) { autoLogin bool image string statusCode int + testOIDC bool wantErr bool }{ { @@ -124,6 +125,7 @@ func TestLogin(t *testing.T) { name: "with auto login", autoLogin: true, image: testValidGCRImage, + testOIDC: true, statusCode: http.StatusOK, }, { @@ -131,6 +133,7 @@ func TestLogin(t *testing.T) { autoLogin: true, image: testValidGCRImage, statusCode: http.StatusInternalServerError, + testOIDC: true, wantErr: true, }, { @@ -161,6 +164,11 @@ func TestLogin(t *testing.T) { _, err = gc.Login(context.TODO(), tt.autoLogin, tt.image, ref) g.Expect(err != nil).To(Equal(tt.wantErr)) + + if tt.testOIDC { + _, err = gc.OIDCLogin(context.TODO()) + g.Expect(err != nil).To(Equal(tt.wantErr)) + } }) } } diff --git a/oci/auth/login/login.go b/oci/auth/login/login.go index 7a72d531c..cc5db7b5f 100644 --- a/oci/auth/login/login.go +++ b/oci/auth/login/login.go @@ -18,10 +18,13 @@ package login import ( "context" + "fmt" + "net/url" "strings" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" + ctrl "sigs.k8s.io/controller-runtime" "github.com/fluxcd/pkg/oci" "github.com/fluxcd/pkg/oci/auth/aws" @@ -103,8 +106,8 @@ func (m *Manager) WithACRClient(c *azure.Client) *Manager { return m } -// Login performs authentication against a registry and returns the -// authentication material. For generic registry provider, it is no-op. +// Login performs authentication against a registry and returns the Authenticator. +// For generic registry provider, it is no-op. func (m *Manager) Login(ctx context.Context, url string, ref name.Reference, opts ProviderOptions) (authn.Authenticator, error) { switch ImageRegistryProvider(url, ref) { case oci.ProviderAWS: @@ -116,3 +119,41 @@ func (m *Manager) Login(ctx context.Context, url string, ref name.Reference, opt } return nil, nil } + +// OIDCLogin attempts to get an Authenticator for the provided URL endpoint. +// +// If you want to construct an Authenticator based on an image reference, +// you may want to use Login instead. +func (m *Manager) OIDCLogin(ctx context.Context, registryURL string, opts ProviderOptions) (authn.Authenticator, error) { + u, err := url.Parse(registryURL) + if err != nil { + return nil, fmt.Errorf("unable to parse registry url: %w", err) + } + + provider := ImageRegistryProvider(u.Host, nil) + if err != nil { + return nil, fmt.Errorf("unable to set up provider: %w", err) + } + + switch provider { + case oci.ProviderAWS: + if !opts.AwsAutoLogin { + return nil, fmt.Errorf("ECR authentication failed: %w", oci.ErrUnconfiguredProvider) + } + ctrl.LoggerFrom(ctx).Info("logging in to AWS ECR for " + u.Host) + return m.ecr.OIDCLogin(ctx) + case oci.ProviderGCP: + if !opts.GcpAutoLogin { + return nil, fmt.Errorf("GCR authentication failed: %w", oci.ErrUnconfiguredProvider) + } + ctrl.LoggerFrom(ctx).Info("logging in to GCP GCR for " + u.Host) + return m.gcr.OIDCLogin(ctx) + case oci.ProviderAzure: + if !opts.AzureAutoLogin { + return nil, fmt.Errorf("ACR authentication failed: %w", oci.ErrUnconfiguredProvider) + } + ctrl.LoggerFrom(ctx).Info("logging in to Azure ACR for " + u.Host) + return m.acr.OIDCLogin(ctx, fmt.Sprintf("%s://%s", u.Scheme, u.Host)) + } + return nil, nil +} diff --git a/oci/go.mod b/oci/go.mod index d7ef5a4a6..ea2d231eb 100644 --- a/oci/go.mod +++ b/oci/go.mod @@ -15,6 +15,7 @@ require ( github.com/aws/aws-sdk-go-v2 v1.18.0 github.com/aws/aws-sdk-go-v2/config v1.18.25 github.com/aws/aws-sdk-go-v2/credentials v1.13.24 + github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.13.3 github.com/aws/aws-sdk-go-v2/service/ecr v1.18.11 github.com/distribution/distribution/v3 v3.0.0-20230519140516-983358f8e250 github.com/fluxcd/pkg/sourceignore v0.3.3 @@ -32,7 +33,6 @@ require ( github.com/AzureAD/microsoft-authentication-library-for-go v1.0.0 // indirect github.com/Shopify/logrus-bugsnag v0.0.0-20171204204709-577dee27f20d // indirect github.com/acomagu/bufpipe v1.0.4 // indirect - github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.13.3 // indirect github.com/aws/aws-sdk-go-v2/internal/configsources v1.1.33 // indirect github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.4.27 // indirect github.com/aws/aws-sdk-go-v2/internal/ini v1.3.34 // indirect From 1c2956bb08383f20eedbbcf98710a304d41adace Mon Sep 17 00:00:00 2001 From: Somtochi Onyekwere Date: Tue, 23 May 2023 12:38:19 +0100 Subject: [PATCH 3/3] Update oci integration tests to test OIDCLogin It also updates the Makefile to build the testapp for linux/amd64 by default. This can be changed by setting GOARCH and GOOS variables. Signed-off-by: Somtochi Onyekwere --- oci/tests/integration/Makefile | 4 +++- oci/tests/integration/repo_list_test.go | 22 ++++++++++++++++++++++ oci/tests/integration/testapp/main.go | 13 ++++++++++--- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/oci/tests/integration/Makefile b/oci/tests/integration/Makefile index 7f90367db..33acd06ff 100644 --- a/oci/tests/integration/Makefile +++ b/oci/tests/integration/Makefile @@ -1,12 +1,14 @@ GO_TEST_ARGS ?= PROVIDER_ARG ?= TEST_TIMEOUT ?= 30m +GOARCH ?= amd64 +GOOS ?= linux TEST_IMG ?= fluxcd/testapp:test .PHONY: app app: - CGO_ENABLED=0 go build -v -o app ./testapp + CGO_ENABLED=0 GOARCH=$(GOARCH) GOOS=$(GOOS) go build -v -o app ./testapp docker-build: app docker buildx build -t $(TEST_IMG) --load . diff --git a/oci/tests/integration/repo_list_test.go b/oci/tests/integration/repo_list_test.go index 12aaac274..0247320e9 100644 --- a/oci/tests/integration/repo_list_test.go +++ b/oci/tests/integration/repo_list_test.go @@ -53,6 +53,28 @@ func TestRepositoryRootLoginListTags(t *testing.T) { } } +func TestOIDCLoginListTags(t *testing.T) { + for name, repo := range testRepos { + t.Run(name, func(t *testing.T) { + // Registry only. + parts := strings.SplitN(repo, "/", 2) + args := []string{ + "-oidc-login=true", + fmt.Sprintf("-registry=%s", parts[0]), + fmt.Sprintf("-repo=%s", parts[1]), + } + testImageRepositoryListTags(t, args) + + // Registry + repo. + args = []string{ + "-oidc-login=true", + fmt.Sprintf("-repo=%s", repo), + } + testImageRepositoryListTags(t, args) + }) + } +} + func testImageRepositoryListTags(t *testing.T, args []string) { g := NewWithT(t) ctx := context.TODO() diff --git a/oci/tests/integration/testapp/main.go b/oci/tests/integration/testapp/main.go index c7df88bbc..7a0ae7b8c 100644 --- a/oci/tests/integration/testapp/main.go +++ b/oci/tests/integration/testapp/main.go @@ -19,6 +19,7 @@ package main import ( "context" "flag" + "fmt" "log" "strings" "time" @@ -38,8 +39,9 @@ import ( // - when the repository contains only the repository name and registry name // is provided separately, e.g. registry: foo.azurecr.io, repo: bar. var ( - registry = flag.String("registry", "", "registry of the repository") - repo = flag.String("repo", "", "repository to list") + registry = flag.String("registry", "", "registry of the repository") + repo = flag.String("repo", "", "repository to list") + oidcLogin = flag.Bool("oidc-login", false, "login with OIDCLogin function") ) func main() { @@ -77,7 +79,12 @@ func main() { panic(err) } - auth, err = login.NewManager().Login(ctx, loginURL, ref, opts) + if *oidcLogin { + auth, err = login.NewManager().OIDCLogin(ctx, fmt.Sprintf("https://%s", loginURL), opts) + } else { + auth, err = login.NewManager().Login(ctx, loginURL, ref, opts) + } + if err != nil { panic(err) }