Add Windows Kerberos/Negotiate proxy authentication (SSPI) - #180
Conversation
| // instrumentedBasic wraps a basicAuthenticator with a call counter so a test | ||
| // can assert Basic was not invoked when Negotiate should have won. | ||
| type instrumentedBasic struct { | ||
| *basicAuthenticator | ||
| calls atomic.Int32 | ||
| } | ||
|
|
||
| func (b *instrumentedBasic) do(req *http.Request, rt http.RoundTripper) (*http.Response, error) { | ||
| b.calls.Add(1) | ||
| return b.basicAuthenticator.do(req, rt) | ||
| } | ||
|
|
||
| func newInstrumentedBasic(creds string) *instrumentedBasic { | ||
| return &instrumentedBasic{basicAuthenticator: newBasicAuthenticator(creds)} | ||
| } |
There was a problem hiding this comment.
this looks the same as the instrumentedBasic type in kerberos_darwin_integration_test.go. i wonder if this could go into basicauth_test.go, or maybe even just merged with basicAuthServer there? the basic auth tests could then have an additional assertion.
| // backend. Kerberos/Negotiate is implemented on macOS (GSS.framework, | ||
| // kerberos_darwin.go) and Windows (SSPI, kerberos_windows.go). |
There was a problem hiding this comment.
let's remove the second sentence of this comment, so that it doesn't churn every time we add a new platform?
| // Kerberos/Negotiate is auto-detected on macOS (GSS.framework) and | ||
| // Windows (SSPI): if a valid ticket is present it is added to the | ||
| // chain, and applicableTo re-checks per 407 so a ticket that arrives | ||
| // later is honoured without a restart. No flag needed for the common | ||
| // already-signed-in case; pass --no-kerberos to opt out. |
There was a problem hiding this comment.
i can already see a lot of code churn due to Claude constantly mentioning which operating systems are supported, over and over again. We don't need to talk about it in main.go; we can probably just delete this entire paragraph?
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| //go:build darwin || windows |
There was a problem hiding this comment.
the various kerberos files were a bit confusing to me at first glance, can we rename them to:
- kerberos.go -> kerberos_stub.go
- kerberos_common.go -> kerberos.go
| t.Skip("integration: ALPACA_IT_PROXY not set; run via the " + | ||
| "testdata/kerberos-windows-integration harness on a domain-joined host") |
There was a problem hiding this comment.
i'm a bit confused by what "run via the testdata/kerberos-windows-integration harness" means. is this just saying to run it with the right env vars and test tags set?
| // Unlike the macOS fixture, this test cannot create its own environment: | ||
| // SSPI requires the host to be domain-joined with a real Kerberos credential, | ||
| // which can't be containerised or run on the CI matrix. The test therefore | ||
| // reads its connection details from the environment and self-skips when they | ||
| // (or a real ticket) are absent, so it never fails on a developer machine or | ||
| // in CI. testdata/kerberos-windows-integration/README.md documents the | ||
| // domain-joined environment used to exercise it (a Samba AD DC and a | ||
| // Negotiate-advertising Squid) and how to reproduce it locally. |
There was a problem hiding this comment.
It's a shame that we can't automate the harness setup for Windows, but it is what it is. The actual test logic (the sub-tests) themselves look more-or-less the same as Darwin though?
Is there a reason we can't have the test logic in some sort of platform-independent file (e.g. kerberos_integration_test.go) but it calls some platform-specific setup function (e.g. with separate implementations in kerberos_darwin_integration_test.go and kerberos_windows_integration_test.go)?
Alternatively, could the darwin test setup just set up the ALPACA_IT_* env vars and call these tests?
That way, we can reuse the tests for new platform implementations such as Linux.
| @@ -0,0 +1,96 @@ | |||
| # negotiate-authentication Specification | |||
There was a problem hiding this comment.
Overall, I like the idea of SDD and I think we'll need to figure out a process for Alpaca, but I don't think we're quite there yet, so let's leave the specs out of this PR? Let's find some time to discuss AI-assisted development processes once this is merged.
For future reference, one of the things I think we need to do is ensure that specs reference any applicable RFCs (or other standards). We've seen a few times now (outside of this PR) cases where Claude has hallucinated and implemented protocol-violating behaviour, and getting the relevant RFCs into its context is the least we should to do try to avoid that.
| @@ -158,7 +163,7 @@ Both are enforced in CI. | |||
|
|
|||
| ### Style | |||
There was a problem hiding this comment.
Something I'm noticing is that Claude is adding a lot of documentation in both markdown files and in comments, and often even repeating comments in multiple places. This is creating a few problems:
- General noise - the actual code that we're interested in is buried in comments (if we want to preserve the original intent, that's already in the spec files, so these code-level comments are redundant). This is distracting to humans like me, but I also imagine it is going to eat into the context window for coding agents too.
- Churn - e.g. every PR has to update all the comments that talk about which platforms are supported, and this makes even the diffs noisy. I mentioned this elsewhere in this PR review (as well as the previous one) but I might not have caught everything.
So there are a few things we should instruct coding agents to do, with regards to comments:
- Prioritise writing readable, self-documenting code with descriptive variable and function names over adding comments.
- Avoid writing comments that simply explain what the code is doing. Prefer writing comments to explain why a non-obvious approach was taken, such as working around a known library bug or optimising a critical bottleneck.
- Avoid repeating the same information in multiple comments.
- README.md contains all user-facing documentation. If there is detailed documentation for specific features that only apply to a subset of users, it should link to a file in the doc/ subdirectory.
- Documentation should be as close to the code as possible. This means that code-level (or source-file-level) comments are preferred over markdown docs, unless there is too much information for a comment. In that case, the comment should reference a file in the doc/ subdirectory.
If you've got any other prompts, let's discuss adding them too. Are you able to ask Claude to remove comments based on these guidelines and see if and how much this helps?
Also I'm not sure where this belongs, maybe in another section...
- Logs and error messages need to have enough detail to troubleshoot what went wrong.
- Error messages also need to include information on how users can fix the error.
Builds on #178 (macOS Kerberos) to add the Windows half of Negotiate proxy authentication, so a domain-joined Windows user gets the same automatic, no-flag Kerberos experience macOS already has.
What it does
negotiateAuthenticatorintokerberos_common.go(darwin || windows); each platform supplies justcheckKerberosTicket()andgenerateSPNEGOToken().kerberos_windows.go, an SSPI backend viagithub.com/alexbrainman/sspi/negotiate(pure Go, no cgo). It requests theHTTP/<host>SPN that Active Directory registers.--no-kerberosopts out. macOS behaviour is unchanged.Testing
integration && windows) that self-skips unless it runs on a domain-joined host with a ticket, so it never fails in CI. It mirrors the macOS integration test's assertions.alpaca.exe(Negotiate-only, no NTLM/Basic configured) returns200through the proxy.testdata/kerberos-windows-integration/README.mddocuments how to reproduce it.Also in here
e2eintegration test to a darwin-explicit name (integration && darwin,TestKerberosDarwinIntegration,testdata/kerberos-darwin-integration/), completing the rename discussed in Multi-method proxy auth with macOS Kerberos: Builds on #168 #178 and establishing oneintegration && <os>convention across both platforms. Adds the darwin unit test that the shared-code extraction would otherwise have dropped.CLAUDE.md->AGENTS.md(a tool-neutral contributor guide).openspec/. Happy to remove these from the PR if we don't want them just yet - say the word and I'll strip them in a follow-up commit.Notes
alexbrainman/sspiis pinned by hash ingo.sum; happy to vendor it or request a tagged release if you'd prefer.