-
Notifications
You must be signed in to change notification settings - Fork 21
Add multi-principal SPNEGO support for handling different service principals #68
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
@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. |
Co-authored-by: bedrin <[email protected]>
Co-authored-by: bedrin <[email protected]>
Co-authored-by: bedrin <[email protected]>
|
@copilot merge latest changes from master |
This includes GitHub Actions workflow updates and macOS hostname validation fixes.
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:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot merge latest changes from master again |
There was a problem hiding this 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.
| try { | ||
| SpnegoClient client = SpnegoClient.loginWithKeyTab(principal, keyTabLocation, true); | ||
| spnegoClients.put(principal, client); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Failed to initialize principal: " + principal, e); |
Copilot
AI
Aug 30, 2025
There was a problem hiding this comment.
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
| 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); |
| 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); |
Copilot
AI
Aug 30, 2025
There was a problem hiding this comment.
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
| 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); |
| 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); |
Copilot
AI
Aug 30, 2025
There was a problem hiding this comment.
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.
| LOG.warn("Failed to extract SPN from token, using default principal", e); | |
| LOG.debug("Failed to extract SPN from token, using default principal", e); |
| 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"); |
Copilot
AI
Aug 30, 2025
There was a problem hiding this comment.
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.
| 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); |
bedrin
left a comment
There was a problem hiding this 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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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]>
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
getServerPrincipalName()methods to extract target service principals from SPNEGO tokensMultiPrincipalManagerinterface and implementations for both Spring Security and TomcatRecent Changes
Code Organization Improvements:
MultiPrincipalManagerinterface fromkerb4j-server-spring-securitytokerb4j-server-commonmoduleMultiPrincipalSunJaasKerberosTicketValidatorclass (functionality already exists inSunJaasKerberosTicketValidator)TomcatMultiPrincipalManagerimplement theMultiPrincipalManagerinterfacekrb5.conffile with dummy hardcoded valuesUsage Example
Spring Security Multi-Principal Configuration:
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.