🔥 feat: Add SPNEGO Authentication Middleware#1836
Conversation
Move configuration and utility functions from config package to main package, optimize code organization Add new error types and utility functions, improve test coverage Simplify documentation content, remove redundant information
- Update English README document structure, add badges and document ID - Synchronize update of Chinese translation version - Unify keytab lookup implementation in example code
test: Update test cases to use temporary directory instead of current directory refactor: Remove unused config package
- Migrate spnego middleware to v3 directory - Update package name to github.com/gofiber/contrib/v3/spnego - Remove support for Fiber v2 entirely - Update Fiber dependency to v3.0.0-rc.2 - Update README.md and README.zh-CN.md documentation - Update Go version requirement to 1.25 or higher
1. add file close error handing in utils/mock_keytab.go 2. format go file 3. Updated example comments in example.go with additional usage instructions
…in example.go This commit: - Removed Chinese documentation (README.zh-CN.md) - Added comprehensive instructions in example.go
…lify context wrapping - Remove dependency on utils adapter package by directly implementing wrapContext struct in spnego.go - Implement wrapContext struct with methods to adapt Fiber context to http.ResponseWriter interface - add test-spnego.yaml workflows - add error check in mock keytab close file - rename creating function to New
…dleware conversion
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ReneWerner87
requested review from
efectn,
gaby and
sixcolors
and removed request for
a team
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 59 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (14)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new SPNEGO Kerberos authentication middleware for Fiber v3. The implementation includes a flexible keytab lookup system, identity storage within the Fiber context, and a suite of testing utilities for mocking keytabs. Feedback focuses on correcting the Go version in the module file to a currently stable release and optimizing the keytab file lookup function to cache results instead of performing disk I/O on every request.
| @@ -0,0 +1,36 @@ | |||
| module github.com/gofiber/contrib/v3/spnego | |||
|
|
|||
| go 1.25.0 | |||
There was a problem hiding this comment.
The Go version is set to 1.25.0, which is a future version not yet released. Since the code uses features like maps.Values and range-over-iterators (introduced in Go 1.23), this should be updated to 1.23.0 to ensure compatibility with current stable Go releases.
| return func() (*keytab.Keytab, error) { | ||
| var mergeKeytab keytab.Keytab | ||
| for _, keytabFile := range keytabFiles { | ||
| kt, err := keytab.Load(keytabFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err) | ||
| } | ||
| mergeKeytab.Entries = append(mergeKeytab.Entries, kt.Entries...) | ||
| } | ||
| return &mergeKeytab, nil | ||
| }, nil |
There was a problem hiding this comment.
The current implementation of NewKeytabFileLookupFunc returns a closure that performs disk I/O and parses keytab files on every HTTP request, which will significantly degrade performance.
It is recommended to load and merge the keytab files once during initialization and return the cached result. Additionally, using keytab.New() is preferred over a zero-value struct to ensure internal fields (like the keytab version) are correctly initialized.
| return func() (*keytab.Keytab, error) { | |
| var mergeKeytab keytab.Keytab | |
| for _, keytabFile := range keytabFiles { | |
| kt, err := keytab.Load(keytabFile) | |
| if err != nil { | |
| return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err) | |
| } | |
| mergeKeytab.Entries = append(mergeKeytab.Entries, kt.Entries...) | |
| } | |
| return &mergeKeytab, nil | |
| }, nil | |
| ktMerged := keytab.New() | |
| for _, keytabFile := range keytabFiles { | |
| kt, err := keytab.Load(keytabFile) | |
| if err != nil { | |
| return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err) | |
| } | |
| ktMerged.Entries = append(ktMerged.Entries, kt.Entries...) | |
| } | |
| return func() (*keytab.Keytab, error) { | |
| return ktMerged, nil | |
| }, nil |
There was a problem hiding this comment.
Pull request overview
Adds a new Fiber v3 middleware module (v3/spnego) that implements SPNEGO/Kerberos authentication, plus supporting utilities, tests, docs, and CI coverage for the new module.
Changes:
- Introduces SPNEGO auth middleware with keytab lookup support and context identity helpers.
- Adds keytab utility helpers (mock keytab creation + keytab inspection) and test suite.
- Registers the new module in
go.workand adds a dedicated GitHub Actions workflow to run its tests.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
v3/spnego/spnego.go |
New SPNEGO middleware implementation for Fiber v3. |
v3/spnego/config.go |
Middleware config + helper to build a keytab lookup function from files. |
v3/spnego/error.go |
Error definitions for config/load/lookup failures. |
v3/spnego/identity.go |
Context helper functions to set/get authenticated identity. |
v3/spnego/utils/mock_keytab.go |
Test utility to generate mock keytabs (optionally written to disk). |
v3/spnego/utils/keytab.go |
Utility to extract/summarize keytab entries. |
v3/spnego/*_test.go |
Unit tests covering middleware config, identity helpers, and keytab utilities. |
v3/spnego/README.md |
Documentation and usage guidance for the middleware. |
v3/spnego/doc.go |
Package-level documentation stub. |
v3/spnego/example/example.go |
Example program demonstrating middleware setup. |
v3/spnego/go.mod / v3/spnego/go.sum |
New module dependencies. |
go.work |
Adds ./v3/spnego to the workspace. |
.github/workflows/test-spnego.yml |
Adds CI job to run go test -race for the new module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _, err := NewKeytabFileLookupFunc() | ||
| require.ErrorIs(t, err, ErrConfigInvalidOfAtLeastOneKeytabFileRequired) | ||
| }) | ||
| t.Run("test not found keytab file", func(t *testing.T) { |
There was a problem hiding this comment.
This subtest name says "not found keytab file" but it actually creates a file with invalid contents. Renaming the test to reflect the real failure mode (invalid/unparseable keytab) would make intent clearer.
| t.Run("test not found keytab file", func(t *testing.T) { | |
| t.Run("test invalid keytab file", func(t *testing.T) { |
| } | ||
| var clean = func() {} | ||
| if len(opt.Filename) > 0 { | ||
| file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o666) |
There was a problem hiding this comment.
NewMockKeytab writes keytab files with mode 0o666. Keytabs contain secret key material, so even in test utilities it’s safer to default to 0o600 (and rely on umask if needed) to avoid accidentally creating world-readable keytabs in examples/CI.
| file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o666) | |
| file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600) |
| "time" | ||
|
|
||
| "maps" | ||
| "sort" |
There was a problem hiding this comment.
Standard-library imports are split into two groups (time vs maps/sort) even though they’re all stdlib. This will be reformatted by gofmt/goimports; consider grouping stdlib imports together to match the repo’s usual import style.
| "time" | |
| "maps" | |
| "sort" | |
| "maps" | |
| "sort" | |
| "time" |
| for _, keytabFile := range keytabFiles { | ||
| kt, err := keytab.Load(keytabFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err) |
There was a problem hiding this comment.
fmt.Errorf format string uses two %w verbs, which is invalid and will panic at runtime when a keytab file fails to load. Wrap only one error with %w (e.g., wrap ErrLoadKeytabFileFailed and include the underlying error with %v), or use errors.Join to preserve both errors for errors.Is checks.
| return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err) | |
| return nil, fmt.Errorf("%w: file %s load failed: %v", ErrLoadKeytabFileFailed, keytabFile, err) |
| require ( | ||
| github.com/gofiber/fiber/v3 v3.0.0-rc.2 | ||
| github.com/jcmturner/goidentity/v6 v6.0.1 | ||
| github.com/jcmturner/gokrb5/v8 v8.4.4 | ||
| github.com/stretchr/testify v1.11.1 | ||
| github.com/valyala/fasthttp v1.65.0 |
There was a problem hiding this comment.
This module pins fiber/v3 to v3.0.0-rc.2 while the rest of the v3 workspace modules are on v3.1.0 (e.g., v3/jwt/go.mod:7). Consider aligning the Fiber version (and related indirect deps) to avoid inconsistent dependency graphs across the go.work workspace.
| // KeytabInfo represents information about a principal in a Kerberos keytab | ||
| // It contains the principal name, realm, and associated encryption type pairs | ||
| type KeytabInfo struct { | ||
| PrincipalName string // The Kerberos principal name (e.g., HTTP/service.example.com) | ||
| Realm string // The Kerberos realm (e.g., EXAMPLE.COM) | ||
| Pairs []EncryptTypePair // List of encryption type pairs for this principal |
There was a problem hiding this comment.
KeytabInfo.PrincipalName is populated with entry.Principal.String(), which includes the realm (e.g., "HTTP/host@REALM"), but the field comment/examples describe it as just "HTTP/service.example.com". Please update the docs (or store only the name without realm) so the API matches its documentation.
| name: "Test spnego" | ||
|
|
||
| on: | ||
| push: | ||
| branches: | ||
| - master | ||
| - main | ||
| paths: | ||
| - 'v3/spnego/**/*.go' | ||
| - 'v3/spnego/go.mod' | ||
| - 'v3/spnego/go.sum' | ||
| pull_request: | ||
| paths: | ||
| - 'v3/spnego/**/*.go' | ||
| - 'v3/spnego/go.mod' | ||
| - 'v3/spnego/go.sum' | ||
|
|
There was a problem hiding this comment.
All other test-* workflows in this repo include a workflow_dispatch trigger (e.g., .github/workflows/test-jwt.yml:16), but this one doesn’t. Adding workflow_dispatch would keep workflow behavior consistent and allow manually rerunning SPNEGO tests from the GitHub UI.
| "fmt" | ||
| "net/http" | ||
| "path" | ||
| "testing" | ||
| "time" |
There was a problem hiding this comment.
path.Join is intended for slash-separated paths (URL paths). For filesystem paths (TempDir + filename), prefer filepath.Join for portability, especially on Windows runners.
|
|
||
| import ( | ||
| "os" | ||
| "path" |
There was a problem hiding this comment.
path.Join is intended for slash-separated paths (URL paths). For filesystem paths (TempDir + filename), prefer filepath.Join for portability, especially on Windows runners.
| "path" | |
| path "path/filepath" |
Reopened from #1368 — original was closed due to a broken force-push that orphaned main's history (now restored).