CLI for local users MFA #2979

tmalikconfluent · 2024-12-20T18:08:32Z

Release Notes

Breaking Changes

PLACEHOLDER

New Features

Add Multi-Factor Authentication (MFA) for users logging in using the CLI.

Bug Fixes

PLACEHOLDER

Checklist

What

Add Multi-Factor Authentication (MFA) for users logging in using the CLI.

Blast Radius

This will impact all users trying to log into the confluent CLI whose organization has enabled Multi-Factor Authentication (MFA).

References

https://confluentinc.atlassian.net/browse/CLI-3318

Test & Review

Testing Doc: https://docs.google.com/document/d/1woyiPp9a4cPlECj0tfgxl5QRzdkxH-4KDj89MMDQbFM/edit?tab=t.0

confluent-cla-assistant · 2024-12-20T18:08:42Z

🎉 All Contributor License Agreements have been signed. Ready to merge.
_{Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.}

sgagniere

Thanks! I left some comments/questions

pkg/auth/auth_token_handler.go

pkg/auth/mfa/auth_server.go

pkg/auth/auth_token_handler.go

pkg/auth/login_credentials_manager.go

channingdong · 2025-01-31T21:56:43Z

pkg/auth/auth.go

@@ -112,7 +112,7 @@ func PersistCCloudCredentialsToConfig(config *config.Config, client *ccloudv1.Cl
 	return ctx.CurrentEnvironment, user.GetOrganization(), nil
 }

-func addOrUpdateContext(cfg *config.Config, isCloud bool, credentials *Credentials, ctxName, url string, state *config.ContextState, caCertPath, organizationId string, save bool) error {
+func addOrUpdateContext(cfg *config.Config, isCloud bool, credentials *Credentials, ctxName, url string, state *config.ContextState, caCertPath, organizationId string, save, IsMfa bool) error {


For go, please follow the acronyms convention to use isMFA instead.

For local variable including the ones in the arguments, the first letter should be non capitalized.

channingdong · 2025-01-31T21:59:48Z

pkg/config/config.go

@@ -494,7 +494,7 @@ func (c *Config) FindContext(name string) (*Context, error) {
 	return context, nil
 }

-func (c *Config) AddContext(name, platformName, credentialName string, kafkaClusters map[string]*KafkaClusterConfig, kafka string, state *ContextState, organizationId, environmentId string) error {
+func (c *Config) AddContext(name, platformName, credentialName string, kafkaClusters map[string]*KafkaClusterConfig, kafka string, state *ContextState, organizationId, environmentId string, IsMfa bool) error {


Same here, please update the argument names wherever applies.

channingdong · 2025-01-31T22:04:55Z

pkg/auth/auth_token_handler.go

@@ -40,11 +42,16 @@ func (a *AuthTokenHandlerImpl) GetCCloudTokens(clientFactory CCloudClientFactory
 			if token, refreshToken, err := a.refreshCCloudSSOToken(client, credentials.AuthRefreshToken, organizationId); err == nil {
 				return token, refreshToken, nil
 			}
+		} else if credentials.IsMfa {


refreshCCloudMFAToken

channingdong · 2025-01-31T22:13:51Z

pkg/auth/auth_token_handler.go

+		}
+
+		client = clientFactory.JwtHTTPClientFactory(context.Background(), token, url)
+		err = a.checkMFAEmailMatchesLogin(client, credentials.Username)


If the function returns an error: “Validate” or “verify” implies the function will produce an error if the emails do not match.

validateMFAEmailMatchesLogin()

channingdong · 2025-01-31T22:16:50Z

pkg/auth/auth_token_handler.go

@@ -146,6 +209,24 @@ func (a *AuthTokenHandlerImpl) refreshCCloudSSOToken(client *ccloudv1.Client, re

 	return res.GetToken(), refreshToken, err
 }
+func (a *AuthTokenHandlerImpl) refreshCCloudMfaToken(client *ccloudv1.Client, refreshToken, organizationId, email string) (string, string, error) {


Should we check for things as we did in getCCloudMfaToken()?

channingdong · 2025-01-31T22:19:44Z

pkg/auth/login_credentials_manager.go

@@ -108,6 +109,9 @@ func (h *LoginCredentialsManagerImpl) getCredentialsFromEnvVarFunc(envVars envir
 		if h.isSSOUser(email, organizationId) {
 			log.CliLogger.Debugf("%s=%s belongs to an SSO user.", ConfluentCloudEmail, email)
 			return &Credentials{Username: email, IsSSO: true}, nil
+		} else if h.isMFARequired(email, organizationId) {
+			log.CliLogger.Debugf("%s=%s belongs to an MFA user.", ConfluentCloudEmail, email)


a MFA user.

channingdong · 2025-01-31T22:21:55Z

pkg/auth/login_credentials_manager.go

+	res, err := h.client.User.LoginRealm(req)
+	// Fine to ignore non-nil err for this request: e.g. what if this fails due to invalid/malicious
+	// email, we want to silently continue and give the illusion of password prompt.
+	return err == nil && res.GetMfaRequired()


It seems the comments indicate we can ignore the err, but line 303 doesn't can you please confirm?

we are ignoring the non-nil error. We still want to catch the nil error. This is very similar to the SSO case where we do the similar thing. This should be good as is

channingdong · 2025-01-31T22:27:29Z

pkg/auth/mfa/auth_state.go

+	MFAProviderIDToken            string
+	MFAProviderRefreshToken       string
+	MFAProviderState              string
+	email                         string


If we want to export it we should use Email

we are not exporting email, it is only for MFA

channingdong · 2025-01-31T22:30:02Z

pkg/auth/sso/auth_server.go

 	return &authServer{
 		wait:  make(chan bool),
 		State: state,
 	}
 }

 // Start begins the server including attempting to bind the desired TCP port
-func (s *authServer) startServer() error {
+func (s *authServer) StartServer() error {


Why are we making this change, while the StartServer() inside mfa folder is non-capitalized?

yes i will revert this, there were some changes made for testing purpose, i will fix all the capitalization/non-capitaloizatiom

channingdong · 2025-01-31T22:32:40Z

pkg/auth/sso/auth_sso_flow.go

@@ -13,7 +13,7 @@ import (
 )

 func Login(authURL string, noBrowser bool, connectionName string) (string, string, error) {
-	state, err := newState(authURL, noBrowser)
+	state, err := NewState(authURL, noBrowser)


Uppercase first letter → Exported (visible outside the package).
Lowercase first letter → Unexported (internal to the package).
Acronyms (e.g., URL, HTTP, ID) remain uppercase as per Go style guidelines.

sgagniere reviewed Jan 6, 2025

View reviewed changes

pkg/auth/auth_token_handler.go Outdated Show resolved Hide resolved

pkg/auth/auth_token_handler.go Outdated Show resolved Hide resolved

pkg/auth/auth_token_handler.go Outdated Show resolved Hide resolved

pkg/auth/mfa/auth_server.go Show resolved Hide resolved

airlock-confluentinc bot force-pushed the CLI-3318 branch from fcdb80b to 63e09be Compare January 13, 2025 16:16

tmalikconfluent marked this pull request as ready for review January 13, 2025 19:49

tmalikconfluent requested a review from a team as a code owner January 13, 2025 19:49

tmalikconfluent requested a review from sgagniere January 13, 2025 20:10

tmalikconfluent added 10 commits January 15, 2025 10:57

CLI for local users MFA

59ad79d

Remove copy paste step

1c9622f

refractor field

72de955

Code changes

a0f0b46

Connection name update

a6dddcc

Review changes

37f3688

Lint

6252ffc

Integration tests

c65c9e8

Change date format

2a4c39d

Rebase

8a5734c

airlock-confluentinc bot force-pushed the CLI-3318 branch from e76e75f to 8a5734c Compare January 15, 2025 17:02

tmalikconfluent added 5 commits January 15, 2025 11:10

Lint

fb53c2b

server test

d5847c0

Test Change

cd46360

Lint

69c77fd

add mfa_from_cli

1e8da53

sgagniere reviewed Jan 29, 2025

View reviewed changes

pkg/auth/auth_token_handler.go Show resolved Hide resolved

pkg/auth/login_credentials_manager.go Outdated Show resolved Hide resolved

tmalikconfluent added 2 commits January 29, 2025 10:05

Add log message

9695fb6

Merge branch 'refs/heads/main' into CLI-3318

7f0cbb0

sgagniere previously approved these changes Jan 30, 2025

View reviewed changes

Refresh token logic

0124cbe

airlock-confluentinc bot dismissed sgagniere’s stale review via 0124cbe January 30, 2025 21:03

channingdong reviewed Jan 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI for local users MFA #2979

CLI for local users MFA #2979

tmalikconfluent commented Dec 20, 2024 •

edited

Loading

confluent-cla-assistant bot commented Dec 20, 2024

sgagniere left a comment

channingdong Jan 31, 2025

channingdong Jan 31, 2025

channingdong Jan 31, 2025

channingdong Jan 31, 2025

channingdong Jan 31, 2025

channingdong Jan 31, 2025

channingdong Jan 31, 2025

tmalikconfluent Jan 31, 2025

channingdong Jan 31, 2025

tmalikconfluent Jan 31, 2025

channingdong Jan 31, 2025

tmalikconfluent Jan 31, 2025

channingdong Jan 31, 2025

CLI for local users MFA #2979

Are you sure you want to change the base?

CLI for local users MFA #2979

Conversation

tmalikconfluent commented Dec 20, 2024 • edited Loading

Release Notes

Checklist

What

Blast Radius

References

Test & Review

confluent-cla-assistant bot commented Dec 20, 2024

sgagniere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmalikconfluent commented Dec 20, 2024 •

edited

Loading