Fix "tsh app login" prompting for user login when multiple AWS roles are present

lib/client/api.go

@@ -796,8 +796,43 @@ func WithMakeCurrentProfile(makeCurrentProfile bool) RetryWithReloginOption {
 	}
 }
 
+// NewNonRetryableError wraps an error to indicate that the error should fail
+// IsErrorResolvableWithRelogin. This wrapper is used to workaround the false
+// positives like trace.IsBadParameter check in IsErrorResolvableWithRelogin.
+func NewNonRetryableError(err error) error {
+	return &nonRetryableError{
+		Err: err,
+	}
+}
+
+type nonRetryableError struct {
+	Err error
+}
+
+func (e *nonRetryableError) Error() string {
+	if e == nil || e.Err == nil {
+		return "error"
+	}
+	return e.Err.Error()
+}
+
+func (e *nonRetryableError) Unwrap() error {
+	if e == nil {
+		return nil
+	}
+	return e.Err
+}
+
+func isNonRetryableError(err error) bool {
+	return errors.As(err, new(*nonRetryableError))
+}
+
 // IsErrorResolvableWithRelogin returns true if relogin is attempted on `err`.
 func IsErrorResolvableWithRelogin(err error) bool {
+	if isNonRetryableError(err) {
+		return false
+	}
+
 	// Private key policy errors indicate that the user must login with an
 	// unexpected private key policy requirement satisfied. This can occur
 	// in the following cases:
@@ -833,6 +868,8 @@ func IsErrorResolvableWithRelogin(err error) bool {
 	// TODO(codingllama): Retrying BadParameter is a terrible idea.
 	//  We should fix this and remove the RemoteError condition above as well.
 	//  Any retriable error should be explicitly marked as such.
+	//  Once trace.IsBadParameter check is removed, the nonRetryableError
+	//  workaround can also be removed.
 	return trace.IsBadParameter(err) ||
 		trace.IsTrustError(err) ||
 		utils.IsCertExpiredError(err) ||

lib/client/api_test.go

@@ -1265,6 +1265,16 @@ func TestIsErrorResolvableWithRelogin(t *testing.T) {
 			},
 			expectResolvable: true,
 		},
+		{
+			name:             "trace.BadParameter should be resolvable",
+			err:              trace.BadParameter("bad"),
+			expectResolvable: true,
+		},
+		{
+			name:             "nonRetryableError should not be resolvable",
+			err:              trace.Wrap(NewNonRetryableError(trace.BadParameter("bad"))),
+			expectResolvable: false,
+		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			resolvable := IsErrorResolvableWithRelogin(tt.err)
@@ -1365,3 +1375,11 @@ func TestGetTargetNodes(t *testing.T) {
 		})
 	}
 }
+
+func TestNonRetryableError(t *testing.T) {
+	err := NewNonRetryableError(trace.AccessDenied("do not enter"))
+	require.Error(t, err)
+	require.Equal(t, "do not enter", err.Error())
+	require.True(t, isNonRetryableError(err))
+	require.True(t, trace.IsAccessDenied(err))
+}

tool/tsh/common/app.go

@@ -526,7 +526,8 @@ func getAppInfo(cf *CLIConf, clt authclient.ClientI, profile *client.ProfileStat
 			isActive:   true,
 		}, nil
 	} else if !trace.IsNotFound(err) {
-		return nil, trace.Wrap(err)
+		// pickActiveApp errors are hon-retryable.
+		return nil, trace.Wrap(client.NewNonRetryableError(err))
 	}
 
 	// If we didn't find an active profile for the app, get info from server.
@@ -550,33 +551,45 @@ func getAppInfo(cf *CLIConf, clt authclient.ClientI, profile *client.ProfileStat
 		app: app,
 	}
 
+	// When getAppInfo gets called inside RetryWithRelogin, it will relogin on
+	// trace.BadParameter errors. Wrap errors from pickCloudAppLogin as they
+	// are not retryable.
+	if err := appInfo.pickCloudAppLogin(cf, logins); err != nil {
+		return nil, trace.Wrap(client.NewNonRetryableError(err))
+	}
+	return appInfo, nil
+}
+
+// pickCloudAppLogin picks the cloud identity for the app based on provided CLI
+// flags and/or available logins of the Teleport user.
+func (a *appInfo) pickCloudAppLogin(cf *CLIConf, logins []string) error {
 	// If this is a cloud app, set additional applicable fields from CLI flags or roles.
 	switch {
-	case app.IsAWSConsole():
-		awsRoleARN, err := getARNFromFlags(cf, app, logins)
+	case a.app.IsAWSConsole():
+		awsRoleARN, err := getARNFromFlags(cf, a.app, logins)
 		if err != nil {
-			return nil, trace.Wrap(err)
+			return trace.Wrap(err)
 		}
-		appInfo.AWSRoleARN = awsRoleARN
+		a.AWSRoleARN = awsRoleARN
 
-	case app.IsAzureCloud():
-		azureIdentity, err := getAzureIdentityFromFlags(cf, profile)
+	case a.app.IsAzureCloud():
+		azureIdentity, err := getAzureIdentityFromFlags(cf, a.profile)
 		if err != nil {
-			return nil, trace.Wrap(err)
+			return trace.Wrap(err)
 		}
 		log.Debugf("Azure identity is %q", azureIdentity)
-		appInfo.AzureIdentity = azureIdentity
+		a.AzureIdentity = azureIdentity
 
-	case app.IsGCP():
-		gcpServiceAccount, err := getGCPServiceAccountFromFlags(cf, profile)
+	case a.app.IsGCP():
+		gcpServiceAccount, err := getGCPServiceAccountFromFlags(cf, a.profile)
 		if err != nil {
-			return nil, trace.Wrap(err)
+			return trace.Wrap(err)
 		}
 		log.Debugf("GCP service account is %q", gcpServiceAccount)
-		appInfo.GCPServiceAccount = gcpServiceAccount
+		a.GCPServiceAccount = gcpServiceAccount
 	}
 
-	return appInfo, nil
+	return nil
 }
 
 // appInfo wraps a RouteToApp and the corresponding app.