Repository navigation
CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth - #156
c1-squire-dev[bot] wants to merge 6 commits into
Conversation
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
…nix_test.go DefaultProviderName is set by go-mssqldb's platform-specific init (ntlm on unix, winsspi on Windows), so the unix-default assertion needs //go:build !windows. Keeps Test_Krb5ProviderRegistered compiling on every GOOS. Addresses judge review comment 4380239426 on PR #156. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
![c1-squire-dev[bot]](https://avatars.githubusercontent.com/in/2846581?s=60&v=4){kind=small}
… clarify comments Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
- gofmt -w both test files (trailing blank lines at EOF in krb5_test.go; missing final newline in krb5_unix_test.go) — G1 - shrink the inline comment in Test_Krb5ProviderRegistered to one line that adds only what the doc comment does not (deterministic error via nonexistent config paths); the doc comment remains the full explanation — G2 Co-authored-by: Resolver3 <resolver3@connector-farm.local> Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Superseded — see the current review report for commit
|
Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab authBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe PR adds one blank import of I scanned the full PR diff for security and correctness issues. I also checked the go.mod and go.sum changes, which add only the gokrb5/v8 dependency tree ( The connector criteria were applied: client and pagination (C/R), sync data loss (S), logging (L), spans (T), JSON safety (J), provisioning (P) and breaking changes (B/BP). None apply here, because the change touches no sync, provisioning, resource or logging code. The registration only adds an opt-in driver provider, and DSNs without Coverage gap: I did not review the vendored gokrb5 source line by line (vendored code is excluded). I only checked that it matches go.mod. As the PR itself says, the actual Kerberos handshake with a real KDC is not tested anywhere. Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Resolved prior findings
Reviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
luisina-santos
left a comment
luisina-santos
left a comment
There was a problem hiding this comment.
A few nits inline.
- nit (optional, general): Not needed for this ticket. Importing the package also enables the credential-cache (
krb5-credcachefile) and user/password Kerberos methods. Mention them in one line, or leave a follow-up to document them. - nit (PR description): The title has the issue ID, good. The body is very long. Trim it to what changed, why, and how it was tested.
| // Test_Krb5ProviderRegistered proves the blank import in sqlserver.go registers | ||
| // the "krb5" integrated-auth provider. Normative wording (no specific upstream | ||
| // error label): the assertion is err != nil AND the error message does not | ||
| // contain "not found". Any non-"not found" error from GetIntegratedAuthenticator | ||
| // proves the registry lookup reached the krb5 provider rather than the miss | ||
| // `provider %v not found`; upstream may reword its config-load or keytab error | ||
| // messages, so no specific upstream string is asserted here. |
There was a problem hiding this comment.
nit: This comment is long and reads like review notes. Cut it to one or two lines, e.g. // A non-"not found" error proves the lookup reached the krb5 provider.
| // Test_DefaultProviderNameUnchanged guards that registering the krb5 provider | ||
| // does not change the platform default authenticator, which stays "ntlm" on | ||
| // unix (per auth_unix.go init() being its only unix writer). |
There was a problem hiding this comment.
nit: Drop "(per auth_unix.go init() being its only unix writer)". // Default provider stays ntlm on unix. is enough.
| }, | ||
| want: "postgres://testuser:testpass@[::1]:5432/testdb", | ||
| }, | ||
| { |
There was a problem hiding this comment.
nit: Add User: "svc-account" (no Password) to this row so it pins the form the docs tell users to use: sqlserver://svc-account@host… with no password. As written it pins params only, and upstream rejects a keytab login without a username (ErrUsernameRequiredWithKeytab).
| > - PostgreSQL: `postgres://username:password@host:port/database` | ||
| > - Oracle: `oracle://username:password@host:port/service` | ||
| > - SQL Server: `sqlserver://username:password@host:port?database=dbname` | ||
| > - SQL Server (Kerberos/keytab): use a DSN with a username but no password, e.g. `sqlserver://<user>@host:port?database=dbname&authenticator=krb5&krb5-realm=<REALM>&krb5-keytabfile=<path>&krb5-configfile=<path>` (with both username and password present the keytab is ignored; the realm may also be encoded as `<user>@<REALM>`) |
There was a problem hiding this comment.
nit: Consider also covering connect.user with no connect.password plus connect.params (the README's recommended style), since most users configure through params, not a raw DSN. Also note that the keytab and krb5.conf paths must exist inside the connector's runtime (container mount for K8s).
baton-sql can now authenticate to SQL Server with Kerberos: a DSN carrying
authenticator=krb5with keytab parameters reaches the driver's Kerberos provider instead of dying withprovider krb5 not found. go-mssqldb ships its Kerberos support as an optional package that only registers itself when something imports it, and nothing in this repo imported it — so the provider name existed in the ticket and in the driver's docs, but never in the driver's registry.What was asked
CXH-3209 (GitHub issue ConductorOne/baton-sql#155): configure a SQL Server DSN like
sqlserver://host:port?database=db&authenticator=krb5&krb5-realm=REALM&krb5-keytabfile=/etc/krb5/svc.keytab&krb5-configfile=/etc/krb5/krb5.confand run baton-sql against it. Expected: Kerberos keytab authentication works "the same way other supported authenticators work". Actual: connection fails with
provider krb5 not found. Customer impact: blocks self-hosted/Kubernetes SQL Server deployments that require keytab auth.What changed
go-mssqldb v1.8.0 already contains everything needed: the optional
integratedauth/krb5package registers akrb5provider from itsinit()and reads thekrb5-*DSN parameters itself. The connector's own plumbing already forwards everyconnect.paramsentry into the DSN verbatim. The only gap was the import. This PR closes that gap and adds the tests and docs that pin it.1.
pkg/database/sqlserver/sqlserver.goWhere: import block, line 8
What changed: one blank import —
_ "github.com/microsoft/go-mssqldb/integratedauth/krb5" // registers the "krb5" integrated-auth provider.Why: this is the driver's own registration seam (upstream uses the same pattern to register
ntlmon unix inauth_unix.go). Importing the package runs itsinit(), which putskrb5into the driver's provider registry. Nothing else in the file moves;Connectstill opens the DSN untouched.2.
pkg/database/sqlserver/krb5_test.go(new)Where: whole file, ~50 lines;
Test_Krb5ProviderRegistered,Test_DefaultProviderNameUnchangedWhat changed: two tests. The first calls the driver's exported
integratedauth.GetIntegratedAuthenticatorwithauthenticator=krb5and nonexistent keytab/config paths and requires an error that does NOT contain "not found" — the registry missprovider %v not foundis the exact failure the ticket reports, so any other error proves the lookup landed in the krb5 provider. The test deliberately does not blank-import the krb5 package itself, so it fails loudly if someone ever drops the production import insqlserver.go(counterfactually verified during review: with the import removed the test fails withprovider krb5 not found). The second assertsintegratedauth.DefaultProviderNamestaysntlmon unix, pinning that the new registration changes nothing about DSNs that don't ask forauthenticator=krb5.Why: the bug was invisible precisely because nothing exercised provider registration; these two tests convert the failure class into a red unit test.
3.
pkg/database/database_test.goWhere:
Test_buildConnectionURLtable, new row "sqlserver with krb5 integrated-auth params round-trips verbatim" (~L775)What changed: one case proving the four krb5 parameters survive
buildConnectionURLinto the built DSN query byte-for-byte (URL-encoded paths, sorted keys).Why: pins that the connector passes
authenticator=krb5&krb5-*through to the driver without allowlisting or mutation — the behavior the feature depends on.4.
docs/docs-info.mdWhere: supported-DSN-forms list, line 53
What changed: one line documenting the Kerberos/keytab DSN form: a username with no password (upstream's
validateKrb5LoginParamsrejects a username-less keytab login with "user id is required to login with krb5 when using krb5-keytabfile" — worth knowing because the ticket's example DSN omits the username), the three requiredkrb5-*parameters, and the note that a password alongside the keytab makes the keytab ignored.Why: the repro DSN in the ticket would fail even after registration because it carries no username; the doc line teaches the form that actually connects.
5.
go.mod,go.sum,vendor/What changed:
go mod tidy -v && go mod vendor(the repo's ownmake add-deprecipe) pullsgithub.com/jcmturner/gokrb5/v8plus its transitive modules (sixjcmturner/*modules andhashicorp/go-uuid, all required by the gokrb5 graph) into go.mod/go.sum and vendors the tree. No pinned version moved:baton-sdk v0.42.4,go-mssqldb v1.8.0,.versions.yaml, and the Go pin are untouched.vendor/is excluded from review per BUGBOT.md.How far this is verified
go test ./pkg/database/... ./pkg/database/sqlserver/...(includes the registry, default-provider, negative-control and pass-through tests), andvalidate-changes.sh(targeted build/vet/test of the changed packages) is green. The connector rulebook checks (check-kind-rules.sh) are clean: zero violations, zero hints.e34fed7a. All repository workflows pass (test with live postgres sync, verify test/lint/docs, pr-review, connector-docs, check-versions). The repository's review bot reviewed this PR and accepted it after one fix pass: its first review suggestedTest_DefaultProviderNameUnchangedneeded a Windows build constraint (the default provider iswinsspithere); the test moved to a//go:build !windowsfile, the thread was answered and resolved, and the bot's ruling at the current head is clean.readKrb5Config/validateKrb5LoginParams, and raised four findings (a self-checking negative control for the registry-miss wording; a comment deduplication; gofmt whitespace in the two new test files; the comment nit only half-fixed on the first pass). All four are fixed on the current head with the fixes quoted as evidence in the resolved threads. An audit confirms every bot/deep-review comment on the PR is replied and resolved.provider krb5 not foundwithout it), the PR's tests pass, the binary builds and starts, the DSN param round-trip row passes, the default provider staysntlm. UNTESTABLE: actual Kerberos keytab authentication against a real SQL Server/KDC — no SQL Server, KDC or keytab exists in this environment (attempted and blocked; recorded). The tester ran in a clean checkout but on the batch's shared VM, not a clean room.cmd/baton-sqlbinary starts (--helpruns). This repo declares no mock, so no sync was exercised — the change is unproven against a live or mocked upstream.Risk — what to look at hardest
integratedauthwill fail that test loudly rather than silently.Linear requirements
CXH-3209
This PR meets the ticket's acceptance criterion at the level CI and unit tests can reach; the live-authentication half of it needs a real KDC and is marked accordingly.
krb5authenticator is registered with the driverpkg/database/sqlserver/sqlserver.goL8 blank import; vendoredintegratedauth/krb5/krb5.goinit()→SetIntegratedAuthenticationProvider("krb5", …); pinned byTest_Krb5ProviderRegisteredauthenticator=krb5+ keytab params reaches the provider instead of failingprovider krb5 not foundTest_buildConnectionURLkrb5 row; provider routing pinned by the registry test. Actual Kerberos authentication needs a real KDC/SQL Server and keytab — no CI environment here has oneTest_DefaultProviderNameUnchanged(default staysntlm)pkg/bsql/config.go,pkg/database/database.go(non-test),.versions.yaml,config_schema.json,baton_capabilities.json: zero diff; additive registry entry onlyRelease Notes
baton-sql now supports Kerberos keytab authentication for SQL Server. A DSN with
authenticator=krb5,krb5-realm,krb5-keytabfileandkrb5-configfile— and the Kerberos principal as the DSN username with no password — authenticates via the driver's Kerberos provider instead of failing withprovider krb5 not found. Self-hosted and Kubernetes SQL Server deployments that require keytab auth can connect without a custom build.