From 1afbf1737b32ebf2a5b05ffffc97d68b334393ba Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 2 Nov 2026 15:10:58 +0100 Subject: [PATCH 1/4] fix(cli): verify the token with the control plane on auth login auth login reported success for any well-formed JWT, also an expired one or one from a different instance. The CLI now asks the control plane for the session of the token before it saves it, and fails the login when the control plane rejects it. Closes #3769 Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 8a55816c-d854-40a9-9865-967f64a2b376 --- app/cli/cmd/auth_login.go | 55 ++++++++++++++++++++++++++++--- app/cli/cmd/auth_login_test.go | 59 ++++++++++++++++++++++++++++++++++ app/cli/cmd/root.go | 42 ++++++++++++++---------- 3 files changed, 134 insertions(+), 22 deletions(-) diff --git a/app/cli/cmd/auth_login.go b/app/cli/cmd/auth_login.go index 698cc7cd9..e46a6e508 100644 --- a/app/cli/cmd/auth_login.go +++ b/app/cli/cmd/auth_login.go @@ -30,10 +30,13 @@ import ( pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1" "github.com/chainloop-dev/chainloop/internal/oauth" + "github.com/chainloop-dev/chainloop/pkg/grpcconn" jwt "github.com/golang-jwt/jwt/v5" "github.com/spf13/cobra" "github.com/spf13/viper" "golang.org/x/oauth2" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) type app struct { @@ -212,22 +215,58 @@ func headlessAuth(loginURL *url.URL) error { return fmt.Errorf("retrieving password from stdin: %w", err) } - // We just want to check that it is a token, the actual verification will happen when it is sent to the server - // To be clear, this is just a best effort sanity check - if _, _, err := new(jwt.Parser).ParseUnverified(string(token), &jwt.MapClaims{}); err != nil { - return errors.New("invalid token") + fmt.Println("") + if err := checkAuthToken(context.Background(), string(token), verifyAuthToken); err != nil { + return err } if err := saveAuthToken(string(token)); err != nil { return fmt.Errorf("storing token in config file: %w", err) } - fmt.Println("") logger.Info().Msg("login successful!") return nil } +// verifyTimeout limits the time to wait for the control plane to check a token +const verifyTimeout = 30 * time.Second + +// checkAuthToken makes sure that the token is a JWT and that the control plane accepts it, +// so that the login only reports success for a token that works +func checkAuthToken(ctx context.Context, token string, verify func(ctx context.Context, token string) error) error { + // A quick local check before the call to the control plane + if _, _, err := new(jwt.Parser).ParseUnverified(token, &jwt.MapClaims{}); err != nil { + return errors.New("invalid token") + } + + if err := verify(ctx, token); err != nil { + if status.Code(err) == codes.Unauthenticated { + return errors.New("invalid token: the control plane rejected it, it can be expired or come from a different instance") + } + + return fmt.Errorf("verifying the token with the control plane: %w", err) + } + + return nil +} + +// verifyAuthToken asks the control plane for the session of the token. The call +// does not need an organization, so it works for a user that is not a member of one +func verifyAuthToken(ctx context.Context, token string) error { + ctx, cancel := context.WithTimeout(ctx, verifyTimeout) + defer cancel() + + conn, err := grpcconn.New(viper.GetString(confOptions.controlplaneAPI.viperKey), token, controlPlaneConnOpts()...) + if err != nil { + return err + } + defer conn.Close() + + _, err = pb.NewContextServiceClient(conn).Current(ctx, &pb.ContextServiceCurrentRequest{}) + return err +} + // Save token to config file func saveAuthToken(token string) error { viper.Set(confOptions.authToken.viperKey, token) @@ -247,6 +286,12 @@ func (a *app) handleCallback(w http.ResponseWriter, r *http.Request) { return } + if err = checkAuthToken(r.Context(), token, verifyAuthToken); err != nil { + w.WriteHeader(http.StatusExpectationFailed) + fmt.Fprintln(w, "login failed, please check the terminal") + return + } + if err = saveAuthToken(token); err != nil { w.WriteHeader(http.StatusInternalServerError) return diff --git a/app/cli/cmd/auth_login_test.go b/app/cli/cmd/auth_login_test.go index fe4265d9e..06a2bd241 100644 --- a/app/cli/cmd/auth_login_test.go +++ b/app/cli/cmd/auth_login_test.go @@ -27,6 +27,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/oauth2" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) func TestAddCodeChallenge(t *testing.T) { @@ -160,3 +162,60 @@ func TestLoginCallbackToken(t *testing.T) { }) } } + +func TestCheckAuthToken(t *testing.T) { + // A well-formed JWT, the CLI does not check its signature + const wellFormed = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0In0.c2lnbmF0dXJl" + + testCases := []struct { + name string + token string + verifyErr error + wantVerify bool + wantErr string + }{ + { + name: "token accepted by the control plane", + token: wellFormed, + wantVerify: true, + }, + { + name: "not a JWT", + token: "not-a-token", + wantErr: "invalid token", + }, + { + name: "token rejected by the control plane", + token: wellFormed, + verifyErr: status.Error(codes.Unauthenticated, "token is invalid"), + wantVerify: true, + wantErr: "invalid token: the control plane rejected it", + }, + { + name: "control plane error", + token: wellFormed, + verifyErr: errors.New("user not found"), + wantVerify: true, + wantErr: "verifying the token with the control plane: user not found", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var verified bool + err := checkAuthToken(context.Background(), tc.token, func(_ context.Context, token string) error { + verified = true + assert.Equal(t, tc.token, token) + return tc.verifyErr + }) + + assert.Equal(t, tc.wantVerify, verified) + if tc.wantErr != "" { + assert.ErrorContains(t, err, tc.wantErr) + return + } + + assert.NoError(t, err) + }) + } +} diff --git a/app/cli/cmd/root.go b/app/cli/cmd/root.go index 5a46c90df..42bbef317 100644 --- a/app/cli/cmd/root.go +++ b/app/cli/cmd/root.go @@ -170,23 +170,7 @@ func ensureActionOpts(cmd *cobra.Command) error { } } - var opts = []grpcconn.Option{ - grpcconn.WithInsecure(apiInsecure()), - grpcconn.WithCLIVersion(fullVersion()), - } - - if maxRecv := apiMaxRecvMsgSize(); maxRecv > 0 { - opts = append(opts, grpcconn.WithMaxRecvMsgSize(maxRecv)) - } - - if caValue := viper.GetString(confOptions.controlplaneCA.viperKey); caValue != "" { - // Check if the value is a file path, if it is we read the content and encode it to base64, if not we assume it's the content already - if _, err := os.Stat(caValue); err == nil { - opts = append(opts, grpcconn.WithCAFile(caValue)) - } else { - opts = append(opts, grpcconn.WithCAContent(caValue)) - } - } + opts := controlPlaneConnOpts() controlplaneURL := viper.GetString(confOptions.controlplaneAPI.viperKey) @@ -235,6 +219,30 @@ func ensureActionOpts(cmd *cobra.Command) error { return nil } +// controlPlaneConnOpts returns the gRPC options for a control plane connection, +// without the organization +func controlPlaneConnOpts() []grpcconn.Option { + opts := []grpcconn.Option{ + grpcconn.WithInsecure(apiInsecure()), + grpcconn.WithCLIVersion(fullVersion()), + } + + if maxRecv := apiMaxRecvMsgSize(); maxRecv > 0 { + opts = append(opts, grpcconn.WithMaxRecvMsgSize(maxRecv)) + } + + if caValue := viper.GetString(confOptions.controlplaneCA.viperKey); caValue != "" { + // Check if the value is a file path, if it is we read the content and encode it to base64, if not we assume it's the content already + if _, err := os.Stat(caValue); err == nil { + opts = append(opts, grpcconn.WithCAFile(caValue)) + } else { + opts = append(opts, grpcconn.WithCAContent(caValue)) + } + } + + return opts +} + func NewRootCmd(l zerolog.Logger) *cobra.Command { // Seed the root logger here and not only in PersistentPreRunE: cobra // rejects unknown subcommands and invalid flags before that hook runs, and From 58c9dd3041d483d38451db73afe5621276d8c1c7 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 2 Nov 2026 16:58:46 +0100 Subject: [PATCH 2/4] refactor(cli): let the control plane check the login token The login no longer parses the token without verification, the control plane checks it and its authentication errors are rendered by the shared CLI message. Both login paths verify and save the token through one function, and the check reuses the current context action. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 8a55816c-d854-40a9-9865-967f64a2b376 --- app/cli/cmd/auth_login.go | 62 +++++++++++++++------------------- app/cli/cmd/auth_login_test.go | 32 +++++++++++------- 2 files changed, 46 insertions(+), 48 deletions(-) diff --git a/app/cli/cmd/auth_login.go b/app/cli/cmd/auth_login.go index e46a6e508..9452bf938 100644 --- a/app/cli/cmd/auth_login.go +++ b/app/cli/cmd/auth_login.go @@ -28,15 +28,13 @@ import ( "runtime" "time" + "github.com/chainloop-dev/chainloop/app/cli/pkg/action" pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1" "github.com/chainloop-dev/chainloop/internal/oauth" "github.com/chainloop-dev/chainloop/pkg/grpcconn" - jwt "github.com/golang-jwt/jwt/v5" "github.com/spf13/cobra" "github.com/spf13/viper" "golang.org/x/oauth2" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) type app struct { @@ -216,35 +214,38 @@ func headlessAuth(loginURL *url.URL) error { } fmt.Println("") - if err := checkAuthToken(context.Background(), string(token), verifyAuthToken); err != nil { + if err := verifyAndSaveAuthToken(string(token)); err != nil { return err } - if err := saveAuthToken(string(token)); err != nil { - return fmt.Errorf("storing token in config file: %w", err) - } - logger.Info().Msg("login successful!") return nil } -// verifyTimeout limits the time to wait for the control plane to check a token -const verifyTimeout = 30 * time.Second +// verifyAndSaveAuthToken saves the token to the config file only when the control plane +// accepts it, so that the login only reports success for a token that works +func verifyAndSaveAuthToken(token string) error { + if err := checkAuthToken(token, verifyAuthToken); err != nil { + return err + } -// checkAuthToken makes sure that the token is a JWT and that the control plane accepts it, -// so that the login only reports success for a token that works -func checkAuthToken(ctx context.Context, token string, verify func(ctx context.Context, token string) error) error { - // A quick local check before the call to the control plane - if _, _, err := new(jwt.Parser).ParseUnverified(token, &jwt.MapClaims{}); err != nil { - return errors.New("invalid token") + if err := saveAuthToken(token); err != nil { + return fmt.Errorf("storing token in config file: %w", err) } - if err := verify(ctx, token); err != nil { - if status.Code(err) == codes.Unauthenticated { - return errors.New("invalid token: the control plane rejected it, it can be expired or come from a different instance") - } + return nil +} +// checkAuthToken makes sure that verify accepts the token. The control plane checks the +// token, so the CLI does not parse it. A rejected token keeps its gRPC status, so the CLI +// shows its usual authentication message +func checkAuthToken(token string, verify func(token string) error) error { + if token == "" { + return errors.New("no token was entered") + } + + if err := verify(token); err != nil { return fmt.Errorf("verifying the token with the control plane: %w", err) } @@ -253,17 +254,14 @@ func checkAuthToken(ctx context.Context, token string, verify func(ctx context.C // verifyAuthToken asks the control plane for the session of the token. The call // does not need an organization, so it works for a user that is not a member of one -func verifyAuthToken(ctx context.Context, token string) error { - ctx, cancel := context.WithTimeout(ctx, verifyTimeout) - defer cancel() - +func verifyAuthToken(token string) error { conn, err := grpcconn.New(viper.GetString(confOptions.controlplaneAPI.viperKey), token, controlPlaneConnOpts()...) if err != nil { return err } defer conn.Close() - _, err = pb.NewContextServiceClient(conn).Current(ctx, &pb.ContextServiceCurrentRequest{}) + _, err = action.NewConfigCurrentContext(newActionOpts(logger, conn, token)).Run() return err } @@ -280,23 +278,16 @@ func (a *app) handleCallback(w http.ResponseWriter, r *http.Request) { }() token, err := a.callback.token(r) - if err != nil { - w.WriteHeader(http.StatusExpectationFailed) - fmt.Fprintln(w, "login failed, please check the terminal") - return + if err == nil { + err = verifyAndSaveAuthToken(token) } - if err = checkAuthToken(r.Context(), token, verifyAuthToken); err != nil { + if err != nil { w.WriteHeader(http.StatusExpectationFailed) fmt.Fprintln(w, "login failed, please check the terminal") return } - if err = saveAuthToken(token); err != nil { - w.WriteHeader(http.StatusInternalServerError) - return - } - logger.Info().Msg("login successful!") fmt.Fprintln(w, "login successful, you can now close this window and go back to the terminal") } @@ -304,6 +295,7 @@ func (a *app) handleCallback(w http.ResponseWriter, r *http.Request) { func openbrowser(url string) error { var cmd *exec.Cmd + //nolint:gosec // no shell is involved, the login URL from the control plane is a single argument switch runtime.GOOS { case "linux": cmd = exec.Command("xdg-open", url) diff --git a/app/cli/cmd/auth_login_test.go b/app/cli/cmd/auth_login_test.go index 06a2bd241..cd0c7344d 100644 --- a/app/cli/cmd/auth_login_test.go +++ b/app/cli/cmd/auth_login_test.go @@ -23,6 +23,7 @@ import ( "net/url" "testing" + "github.com/chainloop-dev/chainloop/app/cli/pkg/action" "github.com/chainloop-dev/chainloop/internal/oauth" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -164,8 +165,8 @@ func TestLoginCallbackToken(t *testing.T) { } func TestCheckAuthToken(t *testing.T) { - // A well-formed JWT, the CLI does not check its signature - const wellFormed = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0In0.c2lnbmF0dXJl" + // The CLI does not parse the token, only the control plane checks it + const token = "a-token" testCases := []struct { name string @@ -173,27 +174,30 @@ func TestCheckAuthToken(t *testing.T) { verifyErr error wantVerify bool wantErr string + // whether the error renders as the CLI authentication message + wantAuthErr bool }{ { name: "token accepted by the control plane", - token: wellFormed, + token: token, wantVerify: true, }, { - name: "not a JWT", - token: "not-a-token", - wantErr: "invalid token", + name: "no token", + token: "", + wantErr: "no token was entered", }, { - name: "token rejected by the control plane", - token: wellFormed, - verifyErr: status.Error(codes.Unauthenticated, "token is invalid"), - wantVerify: true, - wantErr: "invalid token: the control plane rejected it", + name: "token rejected by the control plane", + token: token, + verifyErr: status.Error(codes.Unauthenticated, "token is invalid"), + wantVerify: true, + wantErr: "verifying the token with the control plane", + wantAuthErr: true, }, { name: "control plane error", - token: wellFormed, + token: token, verifyErr: errors.New("user not found"), wantVerify: true, wantErr: "verifying the token with the control plane: user not found", @@ -203,7 +207,7 @@ func TestCheckAuthToken(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { var verified bool - err := checkAuthToken(context.Background(), tc.token, func(_ context.Context, token string) error { + err := checkAuthToken(tc.token, func(token string) error { verified = true assert.Equal(t, tc.token, token) return tc.verifyErr @@ -212,6 +216,8 @@ func TestCheckAuthToken(t *testing.T) { assert.Equal(t, tc.wantVerify, verified) if tc.wantErr != "" { assert.ErrorContains(t, err, tc.wantErr) + _, isAuthErr := action.AuthErrorMessage(err) + assert.Equal(t, tc.wantAuthErr, isAuthErr) return } From 3c4cd2bc610596c1682f6873b7b80a12a69929a3 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 2 Nov 2026 21:52:37 +0100 Subject: [PATCH 3/4] fix(cli): bound the login token check with a timeout ConfigCurrentContext.Run now takes a context, so the login can limit the token check to 30 seconds and a stalled control plane does not block it. The other callers pass the command context. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 8a55816c-d854-40a9-9865-967f64a2b376 --- app/cli/cmd/auth_delete_account.go | 4 ++-- app/cli/cmd/auth_login.go | 5 ++++- app/cli/cmd/organization_describe.go | 2 +- app/cli/cmd/root.go | 2 +- app/cli/pkg/action/config_current_context.go | 4 ++-- app/cli/pkg/action/trace_attestation.go | 4 ++-- 6 files changed, 12 insertions(+), 9 deletions(-) diff --git a/app/cli/cmd/auth_delete_account.go b/app/cli/cmd/auth_delete_account.go index 982066d13..4d6d00380 100644 --- a/app/cli/cmd/auth_delete_account.go +++ b/app/cli/cmd/auth_delete_account.go @@ -1,5 +1,5 @@ // -// Copyright 2023 The Chainloop Authors. +// Copyright 2023-2026 The Chainloop Authors. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -31,7 +31,7 @@ func newAuthDeleteAccountCmd() *cobra.Command { Short: "delete your account", RunE: func(cmd *cobra.Command, args []string) error { // Get user information to make sure the user knows the account she is deleting - contextResp, err := action.NewConfigCurrentContext(ActionOpts).Run() + contextResp, err := action.NewConfigCurrentContext(ActionOpts).Run(cmd.Context()) if err != nil { return err } diff --git a/app/cli/cmd/auth_login.go b/app/cli/cmd/auth_login.go index 9452bf938..fce90ce66 100644 --- a/app/cli/cmd/auth_login.go +++ b/app/cli/cmd/auth_login.go @@ -261,7 +261,10 @@ func verifyAuthToken(token string) error { } defer conn.Close() - _, err = action.NewConfigCurrentContext(newActionOpts(logger, conn, token)).Run() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + _, err = action.NewConfigCurrentContext(newActionOpts(logger, conn, token)).Run(ctx) return err } diff --git a/app/cli/cmd/organization_describe.go b/app/cli/cmd/organization_describe.go index 4153e5ee1..aef1a3464 100644 --- a/app/cli/cmd/organization_describe.go +++ b/app/cli/cmd/organization_describe.go @@ -31,7 +31,7 @@ func newOrganizationDescribeCmd() *cobra.Command { Aliases: []string{"current-context"}, Short: "Describe the current organization", RunE: func(cmd *cobra.Command, args []string) error { - res, err := action.NewConfigCurrentContext(ActionOpts).Run() + res, err := action.NewConfigCurrentContext(ActionOpts).Run(cmd.Context()) if err != nil { return err } diff --git a/app/cli/cmd/root.go b/app/cli/cmd/root.go index 42bbef317..d2e34f15b 100644 --- a/app/cli/cmd/root.go +++ b/app/cli/cmd/root.go @@ -182,7 +182,7 @@ func ensureActionOpts(cmd *cobra.Command) error { return err } - currentContext, err := action.NewConfigCurrentContext(newActionOpts(logger, conn, authToken)).Run() + currentContext, err := action.NewConfigCurrentContext(newActionOpts(logger, conn, authToken)).Run(cmd.Context()) if err == nil && currentContext.CurrentMembership != nil { if err := setLocalOrganization(currentContext.CurrentMembership.Org.Name); err != nil { return fmt.Errorf("writing config file: %w", err) diff --git a/app/cli/pkg/action/config_current_context.go b/app/cli/pkg/action/config_current_context.go index 7cb7bf05c..aca29af32 100644 --- a/app/cli/pkg/action/config_current_context.go +++ b/app/cli/pkg/action/config_current_context.go @@ -74,9 +74,9 @@ func (u *UserItem) PrintUserProfileWithEmail() string { return result } -func (action *ConfigCurrentContext) Run() (*ConfigContextItem, error) { +func (action *ConfigCurrentContext) Run(ctx context.Context) (*ConfigContextItem, error) { client := pb.NewContextServiceClient(action.cfg.CPConnection) - resp, err := client.Current(context.Background(), &pb.ContextServiceCurrentRequest{}) + resp, err := client.Current(ctx, &pb.ContextServiceCurrentRequest{}) if err != nil { return nil, err } diff --git a/app/cli/pkg/action/trace_attestation.go b/app/cli/pkg/action/trace_attestation.go index 91507c905..739e657e0 100644 --- a/app/cli/pkg/action/trace_attestation.go +++ b/app/cli/pkg/action/trace_attestation.go @@ -159,12 +159,12 @@ func casCAPath() string { return viper.GetString("artifact-cas.api-ca") } func casInsecure() bool { return viper.GetBool("api-insecure") } // CheckAuth verifies the CLI can reach the control plane. -func (e *AttestationExecutor) CheckAuth(_ context.Context) error { +func (e *AttestationExecutor) CheckAuth(ctx context.Context) error { _, err := NewConfigCurrentContext(&ActionsOpts{ CPConnection: e.actionOpts.CPConnection, Logger: e.actionOpts.Logger, AuthTokenRaw: e.actionOpts.AuthTokenRaw, - }).Run() + }).Run(ctx) if err != nil { // The wrap keeps the gRPC status reachable with errors.As, so an // authentication failure still renders through AuthErrorMessage; From 7ac8a23a3fb4bc4763ef4a9cd1f3b934262835b6 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Mon, 2 Nov 2026 21:59:22 +0100 Subject: [PATCH 4/4] refactor(cli): share the current context lookup between login and startup fetchCurrentContext connects to the control plane without an organization, asks who the token belongs to and closes the connection. The login check and the organization lookup at startup both use it, and the login check takes its context from the caller. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 8a55816c-d854-40a9-9865-967f64a2b376 --- app/cli/cmd/auth_login.go | 48 +++++++++++++++------------------- app/cli/cmd/auth_login_test.go | 30 +++++++++------------ app/cli/cmd/root.go | 21 ++++++++++----- 3 files changed, 49 insertions(+), 50 deletions(-) diff --git a/app/cli/cmd/auth_login.go b/app/cli/cmd/auth_login.go index fce90ce66..6897a687e 100644 --- a/app/cli/cmd/auth_login.go +++ b/app/cli/cmd/auth_login.go @@ -28,10 +28,8 @@ import ( "runtime" "time" - "github.com/chainloop-dev/chainloop/app/cli/pkg/action" pb "github.com/chainloop-dev/chainloop/app/controlplane/api/controlplane/v1" "github.com/chainloop-dev/chainloop/internal/oauth" - "github.com/chainloop-dev/chainloop/pkg/grpcconn" "github.com/spf13/cobra" "github.com/spf13/viper" "golang.org/x/oauth2" @@ -91,7 +89,7 @@ func newAuthLoginCmd() *cobra.Command { Use: "login", Short: "authenticate the CLI with the Control Plane", RunE: func(cmd *cobra.Command, args []string) error { - return interactiveAuth(forceHeadlessLogin) + return interactiveAuth(cmd.Context(), forceHeadlessLogin) }, } @@ -99,7 +97,7 @@ func newAuthLoginCmd() *cobra.Command { return cmd } -func interactiveAuth(forceHeadless bool) error { +func interactiveAuth(ctx context.Context, forceHeadless bool) error { var a app listener, callbackURL, err := localListenerAndCallbackURL() @@ -123,7 +121,7 @@ func interactiveAuth(forceHeadless bool) error { serverLoginURL.RawQuery = q.Encode() if forceHeadless || isRemoteSession() { - return headlessAuth(serverLoginURL) + return headlessAuth(ctx, serverLoginURL) } a.callback = &loginCallback{ @@ -141,7 +139,7 @@ func interactiveAuth(forceHeadless bool) error { err = openbrowser(serverLoginURL.String()) if err != nil { logger.Debug().Err(err).Msg("falling back to manual login") - return headlessAuth(serverLoginURL) + return headlessAuth(ctx, serverLoginURL) } a.serverChan = make(chan error) @@ -203,7 +201,7 @@ func toHeadlessLoginURL(loginURL *url.URL) { loginURL.RawQuery = q.Encode() } -func headlessAuth(loginURL *url.URL) error { +func headlessAuth(ctx context.Context, loginURL *url.URL) error { toHeadlessLoginURL(loginURL) fmt.Printf("To authenticate, click on the following link and paste the result back here\n\n %s\n\n", loginURL.String()) @@ -214,7 +212,7 @@ func headlessAuth(loginURL *url.URL) error { } fmt.Println("") - if err := verifyAndSaveAuthToken(string(token)); err != nil { + if err := verifyAndSaveAuthToken(ctx, string(token)); err != nil { return err } @@ -223,10 +221,14 @@ func headlessAuth(loginURL *url.URL) error { return nil } +// loginVerifyTimeout bounds the call that checks the login token, so a stalled +// control plane does not block the login +const loginVerifyTimeout = 30 * time.Second + // verifyAndSaveAuthToken saves the token to the config file only when the control plane // accepts it, so that the login only reports success for a token that works -func verifyAndSaveAuthToken(token string) error { - if err := checkAuthToken(token, verifyAuthToken); err != nil { +func verifyAndSaveAuthToken(ctx context.Context, token string) error { + if err := checkAuthToken(ctx, token, verifyAuthToken); err != nil { return err } @@ -237,34 +239,26 @@ func verifyAndSaveAuthToken(token string) error { return nil } -// checkAuthToken makes sure that verify accepts the token. The control plane checks the -// token, so the CLI does not parse it. A rejected token keeps its gRPC status, so the CLI -// shows its usual authentication message -func checkAuthToken(token string, verify func(token string) error) error { +// checkAuthToken makes sure that verify accepts the token. A rejected token keeps its +// gRPC status, so the CLI shows its usual authentication message +func checkAuthToken(ctx context.Context, token string, verify func(ctx context.Context, token string) error) error { if token == "" { return errors.New("no token was entered") } - if err := verify(token); err != nil { + if err := verify(ctx, token); err != nil { return fmt.Errorf("verifying the token with the control plane: %w", err) } return nil } -// verifyAuthToken asks the control plane for the session of the token. The call -// does not need an organization, so it works for a user that is not a member of one -func verifyAuthToken(token string) error { - conn, err := grpcconn.New(viper.GetString(confOptions.controlplaneAPI.viperKey), token, controlPlaneConnOpts()...) - if err != nil { - return err - } - defer conn.Close() - - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +// verifyAuthToken asks the control plane for the session of the token +func verifyAuthToken(ctx context.Context, token string) error { + ctx, cancel := context.WithTimeout(ctx, loginVerifyTimeout) defer cancel() - _, err = action.NewConfigCurrentContext(newActionOpts(logger, conn, token)).Run(ctx) + _, err := fetchCurrentContext(ctx, token) return err } @@ -282,7 +276,7 @@ func (a *app) handleCallback(w http.ResponseWriter, r *http.Request) { token, err := a.callback.token(r) if err == nil { - err = verifyAndSaveAuthToken(token) + err = verifyAndSaveAuthToken(r.Context(), token) } if err != nil { diff --git a/app/cli/cmd/auth_login_test.go b/app/cli/cmd/auth_login_test.go index cd0c7344d..38e208d7e 100644 --- a/app/cli/cmd/auth_login_test.go +++ b/app/cli/cmd/auth_login_test.go @@ -165,22 +165,19 @@ func TestLoginCallbackToken(t *testing.T) { } func TestCheckAuthToken(t *testing.T) { - // The CLI does not parse the token, only the control plane checks it const token = "a-token" testCases := []struct { - name string - token string - verifyErr error - wantVerify bool - wantErr string + name string + token string + verifyErr error + wantErr string // whether the error renders as the CLI authentication message wantAuthErr bool }{ { - name: "token accepted by the control plane", - token: token, - wantVerify: true, + name: "token accepted by the control plane", + token: token, }, { name: "no token", @@ -191,29 +188,28 @@ func TestCheckAuthToken(t *testing.T) { name: "token rejected by the control plane", token: token, verifyErr: status.Error(codes.Unauthenticated, "token is invalid"), - wantVerify: true, wantErr: "verifying the token with the control plane", wantAuthErr: true, }, { - name: "control plane error", - token: token, - verifyErr: errors.New("user not found"), - wantVerify: true, - wantErr: "verifying the token with the control plane: user not found", + name: "control plane error", + token: token, + verifyErr: errors.New("user not found"), + wantErr: "verifying the token with the control plane: user not found", }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { var verified bool - err := checkAuthToken(tc.token, func(token string) error { + err := checkAuthToken(context.Background(), tc.token, func(_ context.Context, token string) error { verified = true assert.Equal(t, tc.token, token) return tc.verifyErr }) - assert.Equal(t, tc.wantVerify, verified) + // Only a token that was entered goes to the control plane + assert.Equal(t, tc.token != "", verified) if tc.wantErr != "" { assert.ErrorContains(t, err, tc.wantErr) _, isAuthErr := action.AuthErrorMessage(err) diff --git a/app/cli/cmd/root.go b/app/cli/cmd/root.go index d2e34f15b..5a2e6eeb8 100644 --- a/app/cli/cmd/root.go +++ b/app/cli/cmd/root.go @@ -16,6 +16,7 @@ package cmd import ( + "context" "crypto/sha256" "encoding/hex" "errors" @@ -177,12 +178,8 @@ func ensureActionOpts(cmd *cobra.Command) error { // If no organization is selected, load it from the server and save it. orgName := effectiveOrganization(cmd) if orgName == "" { - conn, err := grpcconn.New(controlplaneURL, authToken, opts...) - if err != nil { - return err - } - - currentContext, err := action.NewConfigCurrentContext(newActionOpts(logger, conn, authToken)).Run(cmd.Context()) + // A connection error here also fails the connection below, which returns it + currentContext, err := fetchCurrentContext(cmd.Context(), authToken) if err == nil && currentContext.CurrentMembership != nil { if err := setLocalOrganization(currentContext.CurrentMembership.Org.Name); err != nil { return fmt.Errorf("writing config file: %w", err) @@ -243,6 +240,18 @@ func controlPlaneConnOpts() []grpcconn.Option { return opts } +// fetchCurrentContext asks the control plane who the token belongs to, on a connection +// without an organization, so it also works for a user that is not a member of one +func fetchCurrentContext(ctx context.Context, token string) (*action.ConfigContextItem, error) { + conn, err := grpcconn.New(viper.GetString(confOptions.controlplaneAPI.viperKey), token, controlPlaneConnOpts()...) + if err != nil { + return nil, err + } + defer conn.Close() + + return action.NewConfigCurrentContext(newActionOpts(logger, conn, token)).Run(ctx) +} + func NewRootCmd(l zerolog.Logger) *cobra.Command { // Seed the root logger here and not only in PersistentPreRunE: cobra // rejects unknown subcommands and invalid flags before that hook runs, and