SPNEGO: add RFC 4178 multi-leg auth + RFC 4121 sealed wrap tokens#15
SPNEGO: add RFC 4178 multi-leg auth + RFC 4121 sealed wrap tokens#15smnsjas wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive multi-leg SPNEGO authentication support and RFC 4121 sealed wrap tokens to enable compatibility with Windows WinRM/WSMan servers. The changes introduce a stateful HTTP client that handles complex RFC 4178 negotiation flows, client-side security contexts with proper state management, and encrypted (sealed) GSS-API wrap tokens with DCE-style rotation support.
Changes:
- Implements a stateful NegotiateClient with multi-leg SPNEGO flow handling, including multiple WWW-Authenticate headers and server token processing
- Adds ClientContext for managing security context state, sequence numbers, mutual authentication, and per-message cryptographic operations
- Introduces RFC 4121 sealed wrap tokens with confidentiality support, DCE-style ciphertext rotation, and auto-detect unwrap functionality
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spnego/negotiate_client.go | New stateful HTTP client implementing RFC 4178 multi-leg SPNEGO negotiation with state machine |
| spnego/client_ctx.go | Client-side security context with state tracking, sequence numbers, and per-message crypto operations |
| spnego/negotiation_token.go | Enhanced NegTokenInit/Resp with raw MechTypes DER preservation for mechListMIC and AP-REP processing |
| spnego/krb5_token.go | Updated KRB5 token creation to return sequence number and added AP-REP verification for mutual auth |
| gssapi/wrap_token.go | Implemented sealed wrap tokens with encryption, DCE-style rotation, and unified Unwrap API |
| spnego/negotiate_client_test.go | Comprehensive tests for state machine, multi-leg flows, and context operations |
| spnego/krb5_token_test.go | Updated tests to handle KRB5TokenResult structure |
| gssapi/wrap_token_test.go | Extensive tests for sealed tokens, rotation functions, and auto-detect unwrap |
| REFERENCE.md | Added documentation note about DCE-style wrap token rotation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if req.GetBody != nil { | ||
| body, err := req.GetBody() | ||
| if err == nil { | ||
| newReq.Body = body | ||
| } | ||
| } |
There was a problem hiding this comment.
The cloneRequest function doesn't handle errors from req.GetBody(). If GetBody returns an error, the cloned request will have a nil Body even though the original had one. This could lead to silent failures where request bodies are unexpectedly dropped during authentication retries. Consider logging the error or returning it to the caller.
| func (c *NegotiateClient) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| // Make the initial request. | ||
| resp, err := c.transport.RoundTrip(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Check if we need to authenticate. | ||
| if resp.StatusCode != http.StatusUnauthorized { | ||
| return resp, nil | ||
| } | ||
|
|
||
| // Check for WWW-Authenticate: Negotiate header(s). | ||
| // HTTP allows multiple WWW-Authenticate headers (e.g., Negotiate and Kerberos). | ||
| challenge := findNegotiateChallenge(resp.Header) | ||
| if challenge == nil { | ||
| return resp, nil | ||
| } | ||
|
|
||
| // Drain and close the response body to reuse the connection. | ||
| _, _ = io.Copy(io.Discard, resp.Body) | ||
| resp.Body.Close() | ||
|
|
||
| if !challenge.hasToken { | ||
| // Bare Negotiate header - start authentication. | ||
| return c.handleInitialChallenge(req) | ||
| } | ||
|
|
||
| // Handle "Negotiate <token>" - process server response. | ||
| return c.handleServerToken(req, challenge.token) | ||
| } |
There was a problem hiding this comment.
The RoundTrip method holds the mutex (c.mu) for the entire duration of potentially multiple HTTP round trips, including network I/O. This can cause significant contention if multiple goroutines attempt to use the same NegotiateClient concurrently. Consider redesigning to hold the lock only when accessing shared state, not during network operations.
| c.ctx.SetFailed() | ||
| return nil, fmt.Errorf("failed to build MIC response: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
When handling NegStateRequestMIC, the context is never transitioned to the Established state. After sending the mechListMIC response, the context remains in InProgress state. This could cause issues if the server doesn't send another response after receiving the MIC. Consider transitioning to Established after successfully building and sending the MIC response token, or handling this in the sendFinalRequest/handleSuccessResponse flow.
| // After successfully building the MIC response, the context can be considered established. | |
| if err := c.ctx.SetEstablished(); err != nil { | |
| c.ctx.SetFailed() | |
| return nil, fmt.Errorf("failed to establish context after MIC response: %w", err) | |
| } |
|
|
||
| // handleSuccessResponse processes a successful response, potentially with a final token. | ||
| func (c *NegotiateClient) handleSuccessResponse(resp *http.Response) (*http.Response, error) { | ||
| if err := c.processFinalAuthToken(resp); err != nil { |
There was a problem hiding this comment.
When handleSuccessResponse returns an error due to processFinalAuthToken failure, the response body is not closed before returning. This can lead to resource leaks (unclosed HTTP response bodies). Consider closing resp.Body before returning an error, or document that the caller is responsible for closing the body even on error.
| if err := c.processFinalAuthToken(resp); err != nil { | |
| if err := c.processFinalAuthToken(resp); err != nil { | |
| if resp.Body != nil { | |
| resp.Body.Close() | |
| } |
| krbErr, _ := krb5Token.GetKRBError() | ||
|
|
||
| c.ctx.SetFailed() | ||
|
|
||
| return fmt.Errorf("server returned KRB_ERROR: %s", krbErr.EText) |
There was a problem hiding this comment.
The error returned by GetKRBError() is being ignored (using _). If GetKRBError returns an error (which it shouldn't since IsKRBError() returned true, but defensively), the code will panic on the next line trying to access krbErr.EText. Consider handling the error or at least checking if krbErr is nil before accessing its fields.
Implement RFC 4121/4178 multi-leg Negotiate authentication for WinRM/SSPI: - Add ClientContext state machine (Initial → InProgress → Established) - Add NegotiateClient/NegotiatingRoundTripper for HTTP SPNEGO flows - Implement AP-REP verification for mutual authentication - Preserve raw MechTypes DER bytes for mechListMIC computation per RFC 4178 - Add KRB5Token.VerifyAPRep() and GetKRBError() methods - Add NegTokenResp.GetKRB5Token() and HasMechListMIC() methods - Gate Wrap/GetMIC operations on context establishment - Add comprehensive unit tests (coverage: 26.4% → 34.7%)
…ence number support
…icate headers and improve token handling
…t and EC configuration
smnsjas
force-pushed
the
feat/negotiate-context
branch
from
9828694 to
0a7be1f
Compare
smnsjas
force-pushed
the
feat/negotiate-context
branch
from
0a7be1f to
c767a76
Compare
Summary
WWW-Authenticateheaders and server token processing.Why
Some SPNEGO servers (notably Windows/WinRM) require multi‑leg RFC 4178 negotiation and only complete the context after a follow‑on token. They also use RFC 4121 sealed wrap tokens with DCE‑style rotation for per‑message security. The existing flow handled only the simplest case, which breaks against those stacks. This update adds the missing negotiation and wrap/unwarp behavior needed for compatibility.