Skip to content

Conversation

Copy link

Copilot AI commented Aug 30, 2025

  • Initial analysis and planning for multiple principal support
  • Implement multi-principal support for SPNEGO authentication
  • Add comprehensive documentation and tests
  • Merge latest changes from master branch
    • GitHub Actions workflow updates
    • macOS hostname validation fixes
    • DNS blocking fix (changed SERVER_SPN from server.springsource.org to localhost)
    • All tests passing after merge
  • Address PR review comments
    • Remove krb5.conf file (not related to PR, contains dummy hardcoded values)
    • Move MultiPrincipalManager interface to kerb4j-server-common module
    • Remove code duplication by eliminating redundant MultiPrincipalSunJaasKerberosTicketValidator
    • Update imports and references after refactoring
    • Verify all tests still pass after changes

Summary

This PR implements multi-principal support for SPNEGO authentication, allowing a single server to handle authentication requests for multiple service principal names (SPNs) simultaneously. All review comments have been addressed.

Key Features

  • SPN Extraction: Added getServerPrincipalName() methods to extract target service principals from SPNEGO tokens
  • Multi-Principal Management: Created MultiPrincipalManager interface and implementations for both Spring Security and Tomcat
  • Automatic Selection: The library now automatically chooses the correct principal based on the target SPN in incoming tokens
  • Backward Compatibility: All existing single-principal configurations continue to work unchanged
  • Fallback Support: Can combine multi-principal configuration with a default single principal as fallback

Recent Changes

Code Organization Improvements:

  • Moved MultiPrincipalManager interface from kerb4j-server-spring-security to kerb4j-server-common module
  • Removed redundant MultiPrincipalSunJaasKerberosTicketValidator class (functionality already exists in SunJaasKerberosTicketValidator)
  • Made TomcatMultiPrincipalManager implement the MultiPrincipalManager interface
  • Removed unrelated krb5.conf file with dummy hardcoded values
  • All tests continue to pass after refactoring

Usage Example

Spring Security Multi-Principal Configuration:

@Bean
public SimpleMultiPrincipalManager multiPrincipalManager() {
    SimpleMultiPrincipalManager manager = new SimpleMultiPrincipalManager();
    manager.addPrincipal("HTTP/[email protected]", 
                         new FileSystemResource("/etc/keytabs/www1.keytab"));
    manager.addPrincipal("HTTP/[email protected]", 
                         new FileSystemResource("/etc/keytabs/www2.keytab"));
    return manager;
}

@Bean
public SunJaasKerberosTicketValidator kerberosTicketValidator() {
    SunJaasKerberosTicketValidator validator = new SunJaasKerberosTicketValidator();
    validator.setMultiPrincipalManager(multiPrincipalManager());
    return validator;
}

Fixes #67.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copy link
Author

Copilot AI commented Aug 30, 2025

@bedrin 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs.

I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review.

Copilot AI changed the title [WIP] Support different principals on server side based on the spnego token recevied from token Add multi-principal SPNEGO support for handling different service principals Aug 30, 2025
Copilot AI requested a review from bedrin August 30, 2025 08:10
Copilot finished work on behalf of bedrin August 30, 2025 08:10
@bedrin
Copy link
Owner

bedrin commented Aug 30, 2025

@copilot merge latest changes from master

This includes GitHub Actions workflow updates and macOS hostname validation fixes.
Copy link
Author

Copilot AI commented Aug 30, 2025

@copilot merge latest changes from master

I've successfully merged the latest changes from master into this branch. The merge includes GitHub Actions workflow updates and macOS hostname validation fixes. All tests continue to pass after the merge. Commit: 60f735e

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • server.springsource.org
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java -jar /home/REDACTED/work/kerb4j/kerb4j/kerb4j-server/kerb4j-server-spring-security/target/surefire/surefirebooter-20250830141725256_24.jar /home/REDACTED/work/kerb4j/kerb4j/kerb4j-server/kerb4j-server-spring-security/target/surefire 2025-08-30T14-16-51_804-jvmRun1 surefire-20250830141725256_22tmp surefire_4-20250830141725256_23tmp (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot finished work on behalf of bedrin August 30, 2025 14:18
@bedrin
Copy link
Owner

bedrin commented Aug 30, 2025

@copilot merge latest changes from master again

Copy link
Author

Copilot AI commented Aug 30, 2025

@copilot merge latest changes from master again

I've successfully merged the latest changes from master into this branch. The merge includes a DNS blocking fix that changes SERVER_SPN from server.springsource.org to localhost in tests. All tests continue to pass after the merge. Commit: d886f0d

Copilot finished work on behalf of bedrin August 30, 2025 14:47
@bedrin bedrin marked this pull request as ready for review August 30, 2025 15:04
Copilot AI review requested due to automatic review settings August 30, 2025 15:04
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds comprehensive multi-principal SPNEGO support to kerb4j, allowing a single server to authenticate against multiple service principal names (SPNs) by automatically extracting the target SPN from incoming tokens and selecting the appropriate principal for validation.

  • Implements SPN extraction from SPNEGO tokens with new getServerPrincipalName() methods
  • Creates a multi-principal management system with interface and implementations for both Spring Security and Tomcat
  • Maintains full backward compatibility while enabling automatic principal selection based on token contents

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
MultiPrincipalManager.java Core interface defining multi-principal management contract
SimpleMultiPrincipalManager.java Spring Security implementation with keytab resource management
TomcatMultiPrincipalManager.java Tomcat-specific implementation with file path configuration
SunJaasKerberosTicketValidator.java Enhanced validator with multi-principal support and backward compatibility
SpnegoAuthenticator.java Updated Tomcat authenticator with multi-principal token handling
SpnegoInitToken.java Added SPN extraction capability from SPNEGO tokens
SpnegoKerberosMechToken.java Added server principal name extraction from Kerberos tickets
MULTI_PRINCIPAL_SUPPORT.md Comprehensive documentation with usage examples and migration guide

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +40 to +44
try {
SpnegoClient client = SpnegoClient.loginWithKeyTab(principal, keyTabLocation, true);
spnegoClients.put(principal, client);
} catch (Exception e) {
throw new RuntimeException("Failed to initialize principal: " + principal, e);
Copy link

Copilot AI Aug 30, 2025

Choose a reason for hiding this comment

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

The error message should include the keytab location for better debugging. Consider changing to: 'Failed to initialize principal: ' + principal + ' with keytab: ' + keyTabLocation

Suggested change
try {
SpnegoClient client = SpnegoClient.loginWithKeyTab(principal, keyTabLocation, true);
spnegoClients.put(principal, client);
} catch (Exception e) {
throw new RuntimeException("Failed to initialize principal: " + principal, e);
throw new RuntimeException("Failed to initialize principal: " + principal + " with keytab: " + keyTabLocation, e);

Copilot uses AI. Check for mistakes.
Comment on lines +42 to +50
try {
String keyTabPath = keyTabLocation.getURL().toExternalForm();
if (keyTabPath.startsWith("file:")) {
keyTabPath = keyTabPath.substring(5);
}
SpnegoClient client = SpnegoClient.loginWithKeyTab(principal, keyTabPath, acceptOnly);
spnegoClients.put(principal, client);
} catch (Exception e) {
throw new RuntimeException("Failed to initialize principal: " + principal, e);
Copy link

Copilot AI Aug 30, 2025

Choose a reason for hiding this comment

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

The error message should include the keytab location for better debugging. Consider changing to: 'Failed to initialize principal: ' + principal + ' with keytab: ' + keyTabPath

Suggested change
try {
String keyTabPath = keyTabLocation.getURL().toExternalForm();
if (keyTabPath.startsWith("file:")) {
keyTabPath = keyTabPath.substring(5);
}
SpnegoClient client = SpnegoClient.loginWithKeyTab(principal, keyTabPath, acceptOnly);
spnegoClients.put(principal, client);
} catch (Exception e) {
throw new RuntimeException("Failed to initialize principal: " + principal, e);
String keyTabPath = null;
try {
keyTabPath = keyTabLocation.getURL().toExternalForm();
if (keyTabPath.startsWith("file:")) {
keyTabPath = keyTabPath.substring(5);
}
SpnegoClient client = SpnegoClient.loginWithKeyTab(principal, keyTabPath, acceptOnly);
spnegoClients.put(principal, client);
} catch (Exception e) {
throw new RuntimeException("Failed to initialize principal: " + principal + " with keytab: " + keyTabPath, e);

Copilot uses AI. Check for mistakes.
LOG.debug("Using default single principal client for SPN: " + targetSPN);
}
} catch (Kerb4JException e) {
LOG.warn("Failed to extract SPN from token, using default principal", e);
Copy link

Copilot AI Aug 30, 2025

Choose a reason for hiding this comment

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

The log level should be DEBUG instead of WARN since fallback to default principal is expected behavior, not a warning condition.

Suggested change
LOG.warn("Failed to extract SPN from token, using default principal", e);
LOG.debug("Failed to extract SPN from token, using default principal", e);

Copilot uses AI. Check for mistakes.
Comment on lines +248 to +253
if (log.isDebugEnabled()) {
log.debug("Failed to extract SPN from token, using default principal", e);
}
if (spnegoClient == null) {
if (log.isDebugEnabled()) {
log.debug("Failed to extract SPN and no default principal configured");
Copy link

Copilot AI Aug 30, 2025

Choose a reason for hiding this comment

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

Consider logging the exception details even at debug level for troubleshooting. The current implementation only logs the message but not the exception cause.

Suggested change
if (log.isDebugEnabled()) {
log.debug("Failed to extract SPN from token, using default principal", e);
}
if (spnegoClient == null) {
if (log.isDebugEnabled()) {
log.debug("Failed to extract SPN and no default principal configured");
log.debug("Failed to extract SPN and no default principal configured", e);

Copilot uses AI. Check for mistakes.
Copy link
Owner

@bedrin bedrin left a comment

Choose a reason for hiding this comment

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

@copilot please process comments which I have left in this PR and also merge latest changes from master

Copy link
Owner

Choose a reason for hiding this comment

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

Remove this file. It's neither related to this PR nor it makes any sense since it contains dummy hardcoded host and port

Copy link
Author

Choose a reason for hiding this comment

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

Removed the krb5.conf file as requested. It was indeed unrelated to this PR and contained dummy hardcoded values. Commit: 16c314d

Copy link
Owner

Choose a reason for hiding this comment

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

This class isn't specific to Spring framework and should be in kerb4j-server-common module instead

Copy link
Author

Choose a reason for hiding this comment

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

Moved MultiPrincipalManager interface from kerb4j-server-spring-security to kerb4j-server-common module. Updated all imports and made TomcatMultiPrincipalManager implement the interface. Commit: 16c314d

LOG.debug("Using SpnegoClient for SPN: " + targetSPN);

SpnegoContext acceptContext = spnegoClient.createAcceptContext();
byte[] responseToken = acceptContext.acceptToken(token);
Copy link
Owner

Choose a reason for hiding this comment

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

Try to avoid code duplication with SunJaasKerberosTicketValidator.java

Copy link
Author

Choose a reason for hiding this comment

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

Removed MultiPrincipalSunJaasKerberosTicketValidator class to eliminate code duplication. The functionality already exists in SunJaasKerberosTicketValidator through the setMultiPrincipalManager() method. Commit: 16c314d

…le, remove code duplication, and cleanup

Co-authored-by: bedrin <[email protected]>
Copy link
Author

Copilot AI commented Aug 30, 2025

@copilot please process comments which I have left in this PR and also merge latest changes from master

The branch is already up to date with the latest changes from master. No new commits found to merge. All PR review comments have been addressed in commit 16c314d.

Copilot AI requested a review from bedrin August 30, 2025 15:23
Copilot finished work on behalf of bedrin August 30, 2025 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support different principals on server side based on the spnego token recevied from token

2 participants