Skip to content

Fix Heimdall principal claims pattern for client IDs (LFXV2-922) #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
emsearcy wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −6
Open

Fix Heimdall principal claims pattern for client IDs (LFXV2-922) #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ LOG_LEVEL=debug make run
- **Heimdall Integration**: All messages validated against JWT service
- **Multiple Audiences**: Configurable audience validation
- **Principal Parsing**: Authorization header and X-On-Behalf-Of delegation support
- **Machine User Detection**: `clients@` prefix identification
- **Machine User Detection**: `@clients` suffix identification

**Input Validation:**

Expand Down
6 changes: 3 additions & 3 deletions internal/infrastructure/auth/auth_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,12 @@ func (r *AuthRepository) ParsePrincipals(ctx context.Context, headers map[string
}
authorizedPrincipals = append(authorizedPrincipals, principalEntity)

if strings.HasPrefix(principal, constants.MachineUserPrefix) {
if strings.HasSuffix(principal, constants.MachineUserSuffix) {
isMachineUser = true
r.logger.Info("Machine user detected in authorization header",
"auth_id", authID,
"principal", r.safePrincipalLog(principal),
"machine_user_prefix", constants.MachineUserPrefix)
"machine_user_suffix", constants.MachineUserSuffix)
Comment on lines +176 to +181
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the change to suffix-based machine user detection, machine user principals now follow the pattern {client_id}@clients. However, the safePrincipalLog function treats any string containing @ as an email address and redacts it to {prefix}@***. This means machine user principals will be logged as {client_id}@*** instead of showing the full identifier.

Consider updating the safePrincipalLog function to check if a principal is a machine user (using isMachineUser()) before applying email redaction logic, since machine user identifiers are not personal information and should be logged in full for debugging purposes.

Copilot uses AI. Check for mistakes.
}

r.logger.Debug("Authorization principal parsed successfully",
Expand Down Expand Up @@ -491,5 +491,5 @@ func (r *AuthRepository) safePrincipalLog(principal string) string {

// isMachineUser checks if a principal is a machine user
func (r *AuthRepository) isMachineUser(principal string) bool {
return strings.HasPrefix(principal, constants.MachineUserPrefix)
return strings.HasSuffix(principal, constants.MachineUserSuffix)
}
2 changes: 1 addition & 1 deletion internal/infrastructure/auth/auth_repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ func TestAuthRepository_HelperMethods(t *testing.T) {

t.Run("isMachineUser", func(t *testing.T) {
// Test machine user
result := repo.isMachineUser(constants.MachineUserPrefix + "test-machine")
result := repo.isMachineUser("test-machine" + constants.MachineUserSuffix)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test updates correctly verify the machine user detection logic, but there's missing test coverage for how safePrincipalLog handles machine user principals with the new @clients suffix pattern.

Since machine users now end with @clients, they will be treated as email addresses by safePrincipalLog and redacted. Consider adding a test case in the safePrincipalLog test suite to verify the expected behavior when logging machine user principals (e.g., whether they should be redacted or logged in full).

Copilot uses AI. Check for mistakes.
assert.True(t, result)

// Test regular user
Expand Down
2 changes: 1 addition & 1 deletion pkg/constants/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const (
BearerPrefix = "bearer "

// Machine user identifier
MachineUserPrefix = "clients@"
MachineUserSuffix = "@clients"

// Error messages for authentication
ErrInvalidToken = "invalid token"
Expand Down