From ccc1be2e7d0e4029e70ae7749eca67fcea0ca45d Mon Sep 17 00:00:00 2001 From: jsilverio22 <115576434+jsilverio22@users.noreply.github.com> Date: Mon, 28 Nov 2022 11:40:45 -0700 Subject: [PATCH] Fix and improve acorn login validation check (#922) - Add --skip-checks as an escape hatch for trying to authenticate during `acorn login - Rename `acorn install --checks` to `acorn install --skip-checks` for symmetry - Fix the check itself so that authing against quay.io works (by removing the pull/push scopes from the authentication call) - Move the check server side Signed-off-by: Joshua Silverio Signed-off-by: Craig Jellick Co-authored-by: Craig Jellick --- .../01-command-line/acorn_credential_login.md | 1 + .../01-command-line/acorn_install.md | 2 +- .../01-command-line/acorn_login.md | 1 + .../client/credentials/credentials_test.go | 22 +++++------ pkg/apis/api.acorn.io/v1/types.go | 1 + pkg/cli/credential_login.go | 5 ++- pkg/cli/install.go | 4 +- pkg/client/client.go | 4 +- pkg/client/credentials.go | 38 ++----------------- pkg/client/ignore.go | 8 ++-- pkg/install/install.go | 24 +++++++----- pkg/openapi/generated/openapi_generated.go | 6 +++ pkg/server/registry/credentials/strategy.go | 14 +++++++ .../registry/credentials/strategy_validate.go | 28 ++++++++++++++ 14 files changed, 92 insertions(+), 66 deletions(-) create mode 100644 pkg/server/registry/credentials/strategy_validate.go diff --git a/docs/docs/100-reference/01-command-line/acorn_credential_login.md b/docs/docs/100-reference/01-command-line/acorn_credential_login.md index 96846747a..4c2982457 100644 --- a/docs/docs/100-reference/01-command-line/acorn_credential_login.md +++ b/docs/docs/100-reference/01-command-line/acorn_credential_login.md @@ -22,6 +22,7 @@ acorn login ghcr.io -h, --help help for login -p, --password string Password --password-stdin Take the password from stdin + --skip-checks Bypass login validation checks -u, --username string Username ``` diff --git a/docs/docs/100-reference/01-command-line/acorn_install.md b/docs/docs/100-reference/01-command-line/acorn_install.md index dfea4a077..0a74fdd50 100644 --- a/docs/docs/100-reference/01-command-line/acorn_install.md +++ b/docs/docs/100-reference/01-command-line/acorn_install.md @@ -23,7 +23,6 @@ acorn install --acorn-dns-endpoint string The URL to access the Acorn DNS service --api-server-replicas int acorn-api deployment replica count --auto-upgrade-interval string For apps configured with automatic upgrades enabled, the interval at which to check for new versions. Upgrade intervals configured at the application level cannot be smaller than this. (default '5m' - 5 minutes) - --checks Disable preflight checks with --checks=false --cluster-domain strings The externally addressable cluster domain (default .on-acorn.io) --controller-replicas int acorn-controller deployment replica count --default-publish-mode string If no publish mode is set default to this value (default user) @@ -37,6 +36,7 @@ acorn install -o, --output string Output manifests instead of applying them (json, yaml) --pod-security-enforce-profile string The name of the PodSecurity profile to set (default baseline) --set-pod-security-enforce-profile Set the PodSecurity profile on created namespaces (default true) + --skip-checks Bypass installation checks ``` ### Options inherited from parent commands diff --git a/docs/docs/100-reference/01-command-line/acorn_login.md b/docs/docs/100-reference/01-command-line/acorn_login.md index 5f68c83cc..c1f7b8980 100644 --- a/docs/docs/100-reference/01-command-line/acorn_login.md +++ b/docs/docs/100-reference/01-command-line/acorn_login.md @@ -22,6 +22,7 @@ acorn login ghcr.io -h, --help help for login -p, --password string Password --password-stdin Take the password from stdin + --skip-checks Bypass login validation checks -u, --username string Username ``` diff --git a/integration/client/credentials/credentials_test.go b/integration/client/credentials/credentials_test.go index 7cfcfcea6..f45fd888b 100644 --- a/integration/client/credentials/credentials_test.go +++ b/integration/client/credentials/credentials_test.go @@ -28,7 +28,7 @@ func TestCredentialCreate(t *testing.T) { t.Fatal(err) } - cred, err := c.CredentialCreate(ctx, reg, "user", "pass") + cred, err := c.CredentialCreate(ctx, reg, "user", "pass", false) if err != nil { t.Fatal(err) } @@ -38,7 +38,7 @@ func TestCredentialCreate(t *testing.T) { assert.Equal(t, "user", cred.Username) assert.Nil(t, cred.Password) - cred1, err := c.CredentialCreate(ctx, reg1, "user2", "pass2") + cred1, err := c.CredentialCreate(ctx, reg1, "user2", "pass2", false) if err != nil { t.Fatal(err) } @@ -74,12 +74,12 @@ func TestCredentialList(t *testing.T) { t.Fatal(err) } - cred1, err := c.CredentialCreate(ctx, reg, "user", "pass") + cred1, err := c.CredentialCreate(ctx, reg, "user", "pass", false) if err != nil { t.Fatal(err) } - cred2, err := c.CredentialCreate(ctx, reg1, "user2", "pass2") + cred2, err := c.CredentialCreate(ctx, reg1, "user2", "pass2", false) if err != nil { t.Fatal(err) } @@ -115,12 +115,12 @@ func TestCredentialGet(t *testing.T) { t.Fatal(err) } - _, err = c.CredentialCreate(ctx, reg, "user", "pass") + _, err = c.CredentialCreate(ctx, reg, "user", "pass", false) if err != nil { t.Fatal(err) } - cred1, err := c.CredentialCreate(ctx, reg1, "user2", "pass2") + cred1, err := c.CredentialCreate(ctx, reg1, "user2", "pass2", false) if err != nil { t.Fatal(err) } @@ -151,17 +151,17 @@ func TestCredentialUpdate(t *testing.T) { t.Fatal(err) } - _, err = c.CredentialCreate(ctx, reg, "user", "pass") + _, err = c.CredentialCreate(ctx, reg, "user", "pass", false) if err != nil { t.Fatal(err) } - _, err = c.CredentialCreate(ctx, reg1, "user2", "pass2") + _, err = c.CredentialCreate(ctx, reg1, "user2", "pass2", false) if err != nil { t.Fatal(err) } - cred1New, err := c.CredentialUpdate(ctx, reg1, "user3", "pass3") + cred1New, err := c.CredentialUpdate(ctx, reg1, "user3", "pass3", false) if err != nil { t.Fatal(err) } @@ -194,12 +194,12 @@ func TestCredentialDelete(t *testing.T) { t.Fatal(err) } - _, err = c.CredentialCreate(ctx, reg, "user", "pass") + _, err = c.CredentialCreate(ctx, reg, "user", "pass", false) if err != nil { t.Fatal(err) } - _, err = c.CredentialCreate(ctx, reg1, "user2", "pass2") + _, err = c.CredentialCreate(ctx, reg1, "user2", "pass2", false) if err != nil { t.Fatal(err) } diff --git a/pkg/apis/api.acorn.io/v1/types.go b/pkg/apis/api.acorn.io/v1/types.go index 64d2afdc2..dfb17aaec 100644 --- a/pkg/apis/api.acorn.io/v1/types.go +++ b/pkg/apis/api.acorn.io/v1/types.go @@ -242,6 +242,7 @@ type Credential struct { ServerAddress string `json:"serverAddress,omitempty"` Username string `json:"username,omitempty"` Password *string `json:"password,omitempty"` + SkipChecks bool `json:"skipChecks,omitempty"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/pkg/cli/credential_login.go b/pkg/cli/credential_login.go index c34afd50c..2f3b10462 100644 --- a/pkg/cli/credential_login.go +++ b/pkg/cli/credential_login.go @@ -30,6 +30,7 @@ acorn login ghcr.io`, } type CredentialLogin struct { + SkipChecks bool `usage:"Bypass login validation checks"` PasswordStdin bool `usage:"Take the password from stdin"` Password string `usage:"Password" short:"p"` Username string `usage:"Username" short:"u"` @@ -71,7 +72,7 @@ func (a *CredentialLogin) Run(cmd *cobra.Command, args []string) error { existing, err := client.CredentialGet(cmd.Context(), args[0]) if apierror.IsNotFound(err) { - cred, err := client.CredentialCreate(cmd.Context(), args[0], a.Username, a.Password) + cred, err := client.CredentialCreate(cmd.Context(), args[0], a.Username, a.Password, a.SkipChecks) if err != nil { return err } @@ -82,7 +83,7 @@ func (a *CredentialLogin) Run(cmd *cobra.Command, args []string) error { existing.Username = a.Username existing.Password = &a.Password - cred, err := client.CredentialUpdate(cmd.Context(), args[0], a.Username, a.Password) + cred, err := client.CredentialUpdate(cmd.Context(), args[0], a.Username, a.Password, a.SkipChecks) if err != nil { return err } diff --git a/pkg/cli/install.go b/pkg/cli/install.go index eaf8c4b5a..58240411a 100644 --- a/pkg/cli/install.go +++ b/pkg/cli/install.go @@ -20,7 +20,7 @@ acorn install`, } type Install struct { - Checks *bool `usage:"Disable preflight checks with --checks=false"` + SkipChecks bool `usage:"Bypass installation checks"` Image string `usage:"Override the default image used for the deployment"` Output string `usage:"Output manifests instead of applying them (json, yaml)" short:"o"` @@ -38,7 +38,7 @@ func (i *Install) Run(cmd *cobra.Command, args []string) error { } return install.Install(cmd.Context(), image, &install.Options{ - Checks: i.Checks, + SkipChecks: i.SkipChecks, OutputFormat: i.Output, Config: i.Config, APIServerReplicas: i.APIServerReplicas, diff --git a/pkg/client/client.go b/pkg/client/client.go index d8432d1d0..d071c8cc1 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -194,10 +194,10 @@ type Client interface { AppLog(ctx context.Context, name string, opts *LogOptions) (<-chan apiv1.LogMessage, error) AppConfirmUpgrade(ctx context.Context, name string) error - CredentialCreate(ctx context.Context, serverAddress, username, password string) (*apiv1.Credential, error) + CredentialCreate(ctx context.Context, serverAddress, username, password string, skipChecks bool) (*apiv1.Credential, error) CredentialList(ctx context.Context) ([]apiv1.Credential, error) CredentialGet(ctx context.Context, serverAddress string) (*apiv1.Credential, error) - CredentialUpdate(ctx context.Context, serverAddress, username, password string) (*apiv1.Credential, error) + CredentialUpdate(ctx context.Context, serverAddress, username, password string, skipChecks bool) (*apiv1.Credential, error) CredentialDelete(ctx context.Context, serverAddress string) (*apiv1.Credential, error) SecretCreate(ctx context.Context, name, secretType string, data map[string][]byte) (*apiv1.Secret, error) diff --git a/pkg/client/credentials.go b/pkg/client/credentials.go index ea03d6622..b80b7052f 100644 --- a/pkg/client/credentials.go +++ b/pkg/client/credentials.go @@ -2,23 +2,15 @@ package client import ( "context" - "net/http" "sort" apiv1 "github.com/acorn-io/acorn/pkg/apis/api.acorn.io/v1" - "github.com/google/go-containerregistry/pkg/authn" - "github.com/google/go-containerregistry/pkg/name" - "github.com/google/go-containerregistry/pkg/v1/remote/transport" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" kclient "sigs.k8s.io/controller-runtime/pkg/client" ) -func (c *client) CredentialCreate(ctx context.Context, serverAddress, username, password string) (*apiv1.Credential, error) { - if err := credentialValidate(ctx, username, password, serverAddress); err != nil { - return nil, err - } - +func (c *client) CredentialCreate(ctx context.Context, serverAddress, username, password string, skipChecks bool) (*apiv1.Credential, error) { credential := &apiv1.Credential{ ObjectMeta: metav1.ObjectMeta{ Name: serverAddress, @@ -27,6 +19,7 @@ func (c *client) CredentialCreate(ctx context.Context, serverAddress, username, ServerAddress: serverAddress, Username: username, Password: &password, + SkipChecks: skipChecks, } return credential, c.Client.Create(ctx, credential) } @@ -39,11 +32,7 @@ func (c *client) CredentialGet(ctx context.Context, serverAddress string) (*apiv }, credential) } -func (c *client) CredentialUpdate(ctx context.Context, serverAddress, username, password string) (*apiv1.Credential, error) { - if err := credentialValidate(ctx, username, password, serverAddress); err != nil { - return nil, err - } - +func (c *client) CredentialUpdate(ctx context.Context, serverAddress, username, password string, skipChecks bool) (*apiv1.Credential, error) { credential := &apiv1.Credential{} err := c.Client.Get(ctx, kclient.ObjectKey{ Name: serverAddress, @@ -55,6 +44,7 @@ func (c *client) CredentialUpdate(ctx context.Context, serverAddress, username, credential.Username = username credential.Password = &password + credential.SkipChecks = skipChecks return credential, c.Client.Update(ctx, credential) } @@ -96,23 +86,3 @@ func (c *client) CredentialDelete(ctx context.Context, serverAddress string) (*a } return credential, err } - -// credentialValidate takes a username, password and serverAddress string to validate -// whether their combination is valid and will succeed login for pushes/pulls. -func credentialValidate(ctx context.Context, username, password, serverAddress string) error { - // Build a registry struct for the host - reg, err := name.NewRegistry(serverAddress) - if err != nil { - return err - } - - // Build a new transport for the registry which validates authentication - scopes := []string{transport.PullScope, transport.PushScope} - auth := &authn.Basic{Username: username, Password: password} - _, err = transport.NewWithContext(ctx, reg, auth, http.DefaultTransport, scopes) - if err != nil { - return err - } - - return nil -} diff --git a/pkg/client/ignore.go b/pkg/client/ignore.go index ff61e7c69..a130a94ee 100644 --- a/pkg/client/ignore.go +++ b/pkg/client/ignore.go @@ -204,9 +204,9 @@ func (c IgnoreUninstalled) BuilderRegistryDialer(ctx context.Context) (func(ctx }) } -func (c IgnoreUninstalled) CredentialCreate(ctx context.Context, serverAddress, username, password string) (*apiv1.Credential, error) { +func (c IgnoreUninstalled) CredentialCreate(ctx context.Context, serverAddress, username, password string, skipChecks bool) (*apiv1.Credential, error) { return promptInstall(ctx, func() (*apiv1.Credential, error) { - return c.client.CredentialCreate(ctx, serverAddress, username, password) + return c.client.CredentialCreate(ctx, serverAddress, username, password, skipChecks) }) } @@ -218,8 +218,8 @@ func (c IgnoreUninstalled) CredentialGet(ctx context.Context, serverAddress stri return c.client.CredentialGet(ctx, serverAddress) } -func (c IgnoreUninstalled) CredentialUpdate(ctx context.Context, serverAddress, username, password string) (*apiv1.Credential, error) { - return c.client.CredentialUpdate(ctx, serverAddress, username, password) +func (c IgnoreUninstalled) CredentialUpdate(ctx context.Context, serverAddress, username, password string, skipChecks bool) (*apiv1.Credential, error) { + return c.client.CredentialUpdate(ctx, serverAddress, username, password, skipChecks) } func (c IgnoreUninstalled) CredentialDelete(ctx context.Context, serverAddress string) (*apiv1.Credential, error) { diff --git a/pkg/install/install.go b/pkg/install/install.go index 5f081a530..b3b9220c8 100644 --- a/pkg/install/install.go +++ b/pkg/install/install.go @@ -56,7 +56,7 @@ var ( type Mode string type Options struct { - Checks *bool + SkipChecks bool OutputFormat string APIServerReplicas *int ControllerReplicas *int @@ -167,7 +167,7 @@ func Install(ctx context.Context, image string, opts *Options) error { } checkOpts := CheckOptions{RuntimeImage: image} - if opts.Checks == nil || *opts.Checks { + if !opts.SkipChecks { s := opts.Progress.New("Running Pre-install Checks") checkResults := PreInstallChecks(ctx, checkOpts) if IsFailed(checkResults) { @@ -233,16 +233,20 @@ func Install(ctx context.Context, image string, opts *Options) error { return err } - s = opts.Progress.New("Running Post-install Checks") - checkResults := PostInstallChecks(ctx, checkOpts) - if IsFailed(checkResults) { - msg := "Post-install checks failed. Use `acorn check` to debug or `acorn install --checks=false` to skip" - for _, result := range checkResults { - if !result.Passed { - msg += fmt.Sprintf("\n%s: %s", result.Name, result.Message) + if !opts.SkipChecks { + s = opts.Progress.New("Running Post-install Checks") + checkResults := PostInstallChecks(ctx, checkOpts) + if IsFailed(checkResults) { + msg := "Post-install checks failed. Use `acorn check` to debug or `acorn install --checks=false` to skip" + for _, result := range checkResults { + if !result.Passed { + msg += fmt.Sprintf("\n%s: %s", result.Name, result.Message) + } } + s.SuccessWithWarning(msg) + } else { + s.Success() } - s.SuccessWithWarning(msg) } else { s.Success() } diff --git a/pkg/openapi/generated/openapi_generated.go b/pkg/openapi/generated/openapi_generated.go index 97b4b5a8f..cfbfcead4 100644 --- a/pkg/openapi/generated/openapi_generated.go +++ b/pkg/openapi/generated/openapi_generated.go @@ -1229,6 +1229,12 @@ func schema_pkg_apis_apiacornio_v1_Credential(ref common.ReferenceCallback) comm Format: "", }, }, + "skipChecks": { + SchemaProps: spec.SchemaProps{ + Type: []string{"boolean"}, + Format: "", + }, + }, }, }, }, diff --git a/pkg/server/registry/credentials/strategy.go b/pkg/server/registry/credentials/strategy.go index 99cd39ccb..6030e501a 100644 --- a/pkg/server/registry/credentials/strategy.go +++ b/pkg/server/registry/credentials/strategy.go @@ -10,6 +10,7 @@ import ( "github.com/acorn-io/mink/pkg/strategy/translation" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/registry/rest" kclient "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -37,3 +38,16 @@ func (s *Strategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { cred := obj.(*apiv1.Credential) cred.ServerAddress = normalizeDockerIO(cred.ServerAddress) } +func (s *Strategy) Validate(ctx context.Context, obj runtime.Object) (result field.ErrorList) { + params := obj.(*apiv1.Credential) + if !params.SkipChecks { + if err := s.credentialValidate(ctx, params.Username, *params.Password, params.ServerAddress); err != nil { + result = append(result, field.Forbidden(field.NewPath("username/password"), err.Error())) + } + } + return result +} +func (s *Strategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) (result field.ErrorList) { + params := obj.(*apiv1.Credential) + return s.Validate(ctx, params) +} diff --git a/pkg/server/registry/credentials/strategy_validate.go b/pkg/server/registry/credentials/strategy_validate.go new file mode 100644 index 000000000..f4076591e --- /dev/null +++ b/pkg/server/registry/credentials/strategy_validate.go @@ -0,0 +1,28 @@ +package credentials + +import ( + "context" + "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/name" + "github.com/google/go-containerregistry/pkg/v1/remote/transport" + "net/http" +) + +// credentialValidate takes a username, password and serverAddress string to validate +// whether their combination is valid and will succeed login for pushes/pulls. +func (s *Strategy) credentialValidate(ctx context.Context, username, password, serverAddress string) error { + // Build a registry struct for the host + reg, err := name.NewRegistry(serverAddress) + if err != nil { + return err + } + + // Build a new transport for the registry which validates authentication + auth := &authn.Basic{Username: username, Password: password} + _, err = transport.NewWithContext(ctx, reg, auth, http.DefaultTransport, nil) + if err != nil { + return err + } + + return nil +}