feat: auth login/logout commands #180

bernielomax · 2025-08-26T20:29:40Z

Summary

Enhances authentication and configuration commands with better namespace handling and code quality improvements. Builds on the abctl config init foundation to add OAuth/OIDC authentication capabilities.

Note: Per @bgroff request, auth/config commands are initially under the airbox command to keep this logic separate during development, with plans to possibly merge into the main project later.

What's New

Authentication Package Design

The auth package is designed as a reusable library for multiple service contexts (not just CLI). Some of the design was borrowed from #176:

Thread-safe client: Uses mutex protection for future concurrent API calls from multiple goroutines
Async OAuth flow: Channels/goroutines handle browser-based callbacks while enforcing timeouts
Auto-refresh: Client automatically refreshes expired tokens transparently
Persistent storage: Credentials stored in K8s secrets for cross-session persistence

While some patterns (mutex, channels) may seem complex for CLI usage, they're intentional for when this package is reused by API servers and background services in future PRs.

Authentication Commands

abctl auth login - OAuth/OIDC authentication flow with configurable callback port
abctl auth logout - Clear stored authentication credentials
Automatic namespace detection from current kubeconfig context when -n not specified
Secure credential storage in Kubernetes secrets
Support for PKCE (Proof Key for Code Exchange) flow

Configuration Improvements

Fixed namespace detection - abctl config init now uses current kubeconfig namespace when -n flag not provided
Consistent namespace handling across all commands

Command Structure

After this PR:

airbox auth login - Authenticate with Airbyte
airbox auth logout - Clear authentication
airbox config init - Initialize configuration from existing Airbyte installation

Authentication Flow

Auto-detects namespace from kubeconfig context
Loads Airbyte configuration (API endpoints, auth server)
Launches OAuth/OIDC flow with local callback server
Stores credentials securely in Kubernetes secret

Test Coverage

Comprehensive auth flow testing with proper mocks
HTTP client testing with gomock-generated mocks
All existing functionality verified

Implements OAuth2/OIDC authentication flow with PKCE for secure CLI authentication. Includes provider discovery, token exchange, refresh handling, and comprehensive tests. Uses fixed port 51004 for OAuth callbacks to work with Keycloak redirect URI validation.

- Create HTTP client that handles base URL resolution - Support any HTTPDoer implementation for flexibility - Preserve request context, headers, and body - Add comprehensive test coverage (100%)

- Create API client with typed methods for dataplane operations - Support Get, List, Create, and Delete dataplane operations - Use HTTP constants and path constants for maintainability - Organize dataplane code in separate files following Go conventions - Add comprehensive table-driven tests (82.8% coverage)

- Add auth command group following resource-oriented pattern - Implement auth login with OAuth/OIDC flow support - Implement auth logout to clear stored credentials - Store credentials in Kubernetes secrets for security

bernielomax · 2025-08-27T17:13:21Z

internal/auth/refresh.go

+// EnsureValidAuth loads credentials and refreshes if expired
+func EnsureValidAuth(ctx context.Context, k8sClient k8s.Client, namespace string) (*Credentials, error) {
+	// Load credentials from secret
+	secret, err := k8sClient.SecretGet(ctx, namespace, "abctl-auth")


Initially these commands were under abctl. I was asked to move them to airbox. Not sure what to do with these kind of secret/configmap names, if the intention is that the commands could be moved back under abctl? Do I change them, or leave them?

@bgroff do you have a preference? Or thoughts on this?

internal/auth/auth.go

colesnodgrass · 2025-08-27T17:46:08Z

internal/auth/auth.go

+	client := &http.Client{
+		Timeout: 30 * time.Second,
+	}


Should this be defined outside of this function? At minimal for testing purposes?

yeah. It should probably be configurable.

I've added a fixup so that all commands share a default client with timeout. I think that should be enough for now. Testing coverage should be ample as-is.

colesnodgrass · 2025-08-27T17:46:43Z

internal/auth/auth.go

+	client := &http.Client{
+		Timeout: 30 * time.Second,
+	}


Same comment as previous client.

colesnodgrass · 2025-08-27T17:49:05Z

internal/auth/auth.go

+			Error            string `json:"error"`
+			ErrorDescription string `json:"error_description"`
+		}
+		json.NewDecoder(resp.Body).Decode(&errResp)


Should do something with the error response from this. Maybe populate the errResp struct with data indicating that the response couldn't be decoded?

I have added a fixup that should return the error with status code for context. 9deb541

internal/auth/client.go

colesnodgrass · 2025-08-27T17:53:52Z

internal/auth/client.go

+	if tokenType == "" {
+		tokenType = "Bearer" // Default token type
+	}


Why not set this to the default value in the ensureValidToken call when we update/set the credentials information?

Good call. I'll add fixup.

Fixup - d2d256d

colesnodgrass · 2025-08-27T18:07:53Z

internal/auth/client.go

+	}
+
+	// Refresh the token
+	tokens, err := RefreshAccessToken(ctx, c.provider, c.clientID, c.credentials.RefreshToken)


Do we also need to check again (checked first on line 97) if we have a RefreshToken?

If I understand this correctly.. Refresh tokens can never become empty once set - they only get updated to new non-empty values. The double-check isn't needed since the race condition described cannot actually occur in this codebase.

internal/auth/client.go

colesnodgrass · 2025-08-27T18:19:52Z

internal/auth/flow.go

+	return base64.RawURLEncoding.EncodeToString(hash[:])
+}
+
+const successHTML = `<!DOCTYPE html>


For code-cleanliness reasons I wonder if we should move this to a separate file and then embed it into this file.

Fixup - 2573a23

colesnodgrass · 2025-08-27T18:26:24Z

internal/cmd/auth/logout.go

+	// Delete the auth secret by type (this will delete all Opaque secrets, but in practice
+	// this namespace should only contain the abctl-auth secret for auth purposes)


How can we know this for sure? We don't have any checks in place to ensure this is true. If no namespace is provided then we fetch the current namespace, and if that namespace happened to contain an abctl-auth secret then we going to remove all the secrets.

I might be being a bit alarmist but anytime there is a potential delete more than we created my spidey-sense goes off.

This was a good catch. I was just using the already defined interface, but it in hindsight it was not very safe. I have added fixup c6497a9 which will patch the secret with more safety and prescision.

Fix docs to reflect what IsExpired is actually doing.

Declare a default HTTP client for all airbox commands to use.

Plug in default HTTP client into airbox commands.

Return any decode errors for non-200 responses.

Remove runtime TokenType defaulting in favor of setting defaults during credential creation/update.

Embed success HTML

Patch auth secret rather than delete for saftey.

bernielomax · 2025-08-28T00:33:03Z

Thanks for the review @colesnodgrass . I've added fixups. It should be good for a second pass.

colesnodgrass

One question regarding the potential for an extra / in a request path.

colesnodgrass · 2025-08-28T05:45:03Z

internal/auth/auth.go

+// DiscoverProvider fetches OIDC provider configuration from well-known endpoint
+func DiscoverProvider(ctx context.Context, issuerURL string) (*Provider, error) {
+	// Construct well-known URL
+	wellKnownURL := fmt.Sprintf("%s/.well-known/openid-configuration", issuerURL)


Do we need to ensure the issuerURL doesn't end with a /?

colesnodgrass · 2025-08-28T05:46:20Z

internal/auth/success.html

bernielomax requested review from bgroff and colesnodgrass August 26, 2025 20:29

bernielomax requested a review from a team as a code owner August 26, 2025 20:29

bernielomax marked this pull request as draft August 26, 2025 21:25

bernielomax force-pushed the bernielomax/feat/auth-login-logout branch 4 times, most recently from 774ff74 to 248f7f5 Compare August 27, 2025 16:49

bernielomax added 6 commits August 27, 2025 09:55

feat: add HTTP client package with base URL resolution

7e088b5

- Create HTTP client that handles base URL resolution - Support any HTTPDoer implementation for flexibility - Preserve request context, headers, and body - Add comprehensive test coverage (100%)

feat: auth login/logout commands

646f4dc

- Add auth command group following resource-oriented pattern - Implement auth login with OAuth/OIDC flow support - Implement auth logout to clear stored credentials - Store credentials in Kubernetes secrets for security

fix: config init uses current namespace from kubeconfig by default

c05272a

feat: airbox command for easy dataplane installs

7f22ea2

bernielomax force-pushed the bernielomax/feat/auth-login-logout branch from 248f7f5 to 7f22ea2 Compare August 27, 2025 16:55

bernielomax marked this pull request as ready for review August 27, 2025 17:00

bernielomax commented Aug 27, 2025

View reviewed changes

colesnodgrass reviewed Aug 27, 2025

View reviewed changes

bernielomax added 7 commits August 27, 2025 17:13

fixup! feat: add OAuth/OIDC authentication package

b253484

Fix docs to reflect what IsExpired is actually doing.

fixup! feat: add HTTP client package with base URL resolution

aac766f

Declare a default HTTP client for all airbox commands to use.

fixup! feat: add OAuth/OIDC authentication package

f57dd56

Plug in default HTTP client into airbox commands.

fixup! feat: add OAuth/OIDC authentication package

9deb541

Return any decode errors for non-200 responses.

fixup! feat: add OAuth/OIDC authentication package

d2d256d

Remove runtime TokenType defaulting in favor of setting defaults during credential creation/update.

fixup! feat: add OAuth/OIDC authentication package

2573a23

Embed success HTML

fixup! feat: add OAuth/OIDC authentication package

c6497a9

Patch auth secret rather than delete for saftey.

colesnodgrass approved these changes Aug 28, 2025

View reviewed changes

bernielomax merged commit 93dc6e0 into main Aug 28, 2025
2 checks passed

bernielomax deleted the bernielomax/feat/auth-login-logout branch August 28, 2025 18:58

		// Delete the auth secret by type (this will delete all Opaque secrets, but in practice
		// this namespace should only contain the abctl-auth secret for auth purposes)

feat: auth login/logout commands #180

feat: auth login/logout commands #180

Uh oh!

Conversation

bernielomax commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's New

Authentication Package Design

Authentication Commands

Configuration Improvements

Command Structure

Authentication Flow

Test Coverage

Uh oh!

bernielomax Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernielomax Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernielomax commented Aug 28, 2025

Uh oh!

colesnodgrass left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bernielomax commented Aug 26, 2025 •

edited

Loading

bernielomax Aug 27, 2025 •

edited

Loading

bernielomax Aug 28, 2025 •

edited

Loading