[c1-squire-dev]

Registers go-mssqldb's krb5 integrated-auth provider so a SQL Server DSN with authenticator=krb5 and keytab params authenticates via Kerberos instead of failing with provider krb5 not found.

What changed

• pkg/database/sqlserver/sqlserver.go: blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5. Its init() registers the krb5 provider. Nothing imported it before, so the driver's registry never had it.
• pkg/database/sqlserver/krb5_test.go: asserts the krb5 registry lookup reaches the provider (fails with provider krb5 not found if the import is dropped).
• docs/docs-info.md: Kerberos/keytab DSN and connect.user/connect.params forms, runtime file mounts, SPN/FQDN and connect.database gotchas.
• go.mod, go.sum, vendor/: adds jcmturner/gokrb5/v8 and its transitive modules. No existing pin moved.

Testing

• Unit test: go test ./pkg/database/sqlserver/.
• End to end against real containers: a Samba AD DC (KDC + DNS, realm CORP.EXAMPLE.COM) and SQL Server 2022 configured with an MSSQLSvc keytab. Login CORP\svcbaton was created FROM WINDOWS. baton-sql ran with only the client keytab, using the ticket's DSN form plus a username and no password.
  • With this change: the sync completes and the session reports auth_scheme=KERBEROS as CORP\svcbaton, with 3 table rows synced. The KDC logs AS-REQ and TGS-REQ for MSSQLSvc/sql1.corp.example.com:1433 as SUCCESS.
  • Same tree with the import removed: validate failed: database "" ping failed: provider krb5 not found. This reproduces the ticket.
  • Using connect.database instead of ?database= fails with KDC_ERR_S_PRINCIPAL_UNKNOWN, because the path is read as an instance name. This is now documented.
• Not covered: Windows AD, and the credcache and password krb5 methods.

Risk

The gokrb5/v8 tree is vendored into every build, including for users who never use Kerberos.

Release Notes

baton-sql now supports Kerberos keytab authentication for SQL Server. Use a DSN with the Kerberos principal as the username (no password) plus authenticator=krb5, krb5-realm, krb5-keytabfile and krb5-configfile.

Resolves CXH-3209

🤖 Generated with Claude Code

[linear-code]

CXH-3209

[github-actions]

Superseded — see the current review report for commit f3ad3ae1eda9

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 5f86dd03c43c.
Review mode: full
View review run

Review Summary

This PR adds one blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go:8. The import's init() registers the krb5 integrated-auth provider. With it, a DSN that has authenticator=krb5 reaches the Kerberos provider instead of failing with provider krb5 not found.

The other changes:

• Tests: a registry test and a default-provider test in krb5_test.go, plus a buildConnectionURL test case showing the krb5-* params pass through unchanged.
• Docs: one line in docs/docs-info.md.
• Dependencies: the gokrb5/v8 module graph is added to go.mod/go.sum/vendor/.

Coverage and checks:

• I scanned the full PR diff for security and correctness issues.
• Docs: I checked the docs line against the vendored krb5.go readKrb5Config/validateKrb5LoginParams, and it is accurate. A keytab login needs a username. If a password is also present, the username+password path is used and the keytab is ignored. The realm falls back to the user@REALM split.
• Username-only DSN: buildConnectionURL already builds a DSN with a username and no password via url.User, so the documented form works.
• Dependencies: go.mod adds only indirect modules required by the newly imported package: gokrb5 v8.4.4 (the current release, past the older gokrb5 CVEs), aescts, dnsutils, gofork, goidentity, rpc, and hashicorp/go-uuid. No existing version changed.

Repo-local criteria: I applied them. No resource builders, sync paths, provisioning, logging, spans, or API response structs changed, so sections A–G and S do not apply. Breaking-change rules B1–B9 do not apply: the change only adds a provider, and existing DSNs behave the same.

Coverage gaps: I did not review the vendored third-party sources line by line. I confirmed they match the modules.txt/go.mod additions. As the PR itself says, a live Kerberos handshake is not tested anywhere.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (confidence: medium) pkg/database/sqlserver/krb5_test.go:46-50: Test_DefaultProviderNameUnchanged expects ntlm but has no build constraint. go-mssqldb sets DefaultProviderName to winsspi on Windows, so the test fails on any Windows test run.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/database/sqlserver/krb5_test.go`:
- Around line 46-50: Test_DefaultProviderNameUnchanged asserts integratedauth.DefaultProviderName == "ntlm", which only holds on unix; go-mssqldb sets it to "winsspi" on Windows. Move this test into a file with a `//go:build !windows` constraint (e.g. krb5_unix_test.go) so it does not fail on Windows test runs.

[github-actions]

Superseded — see the current review report for commit 4c156cadf00b

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 5f86dd03c43c.
Review mode: full
View review run

Review Summary

This PR adds one blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go:8. The import's init() registers the krb5 integrated-auth provider. With it, a DSN that has authenticator=krb5 reaches the Kerberos provider instead of failing with provider krb5 not found.

The other changes:

• Tests: a registry test and a default-provider test in krb5_test.go, plus a buildConnectionURL test case showing the krb5-* params pass through unchanged.
• Docs: one line in docs/docs-info.md.
• Dependencies: the gokrb5/v8 module graph is added to go.mod/go.sum/vendor/.

Coverage and checks:

• I scanned the full PR diff for security and correctness issues.
• Docs: I checked the docs line against the vendored krb5.go readKrb5Config/validateKrb5LoginParams, and it is accurate. A keytab login needs a username. If a password is also present, the username+password path is used and the keytab is ignored. The realm falls back to the user@REALM split.
• Username-only DSN: buildConnectionURL already builds a DSN with a username and no password via url.User, so the documented form works.
• Dependencies: go.mod adds only indirect modules required by the newly imported package: gokrb5 v8.4.4 (the current release, past the older gokrb5 CVEs), aescts, dnsutils, gofork, goidentity, rpc, and hashicorp/go-uuid. No existing version changed.

Repo-local criteria: I applied them. No resource builders, sync paths, provisioning, logging, spans, or API response structs changed, so sections A–G and S do not apply. Breaking-change rules B1–B9 do not apply: the change only adds a provider, and existing DSNs behave the same.

Coverage gaps: I did not review the vendored third-party sources line by line. I confirmed they match the modules.txt/go.mod additions. As the PR itself says, a live Kerberos handshake is not tested anywhere.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (confidence: medium) pkg/database/sqlserver/krb5_test.go:46-50: Test_DefaultProviderNameUnchanged expects ntlm but has no build constraint. go-mssqldb sets DefaultProviderName to winsspi on Windows, so the test fails on any Windows test run.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/database/sqlserver/krb5_test.go`:
- Around line 46-50: Test_DefaultProviderNameUnchanged asserts integratedauth.DefaultProviderName == "ntlm", which only holds on unix; go-mssqldb sets it to "winsspi" on Windows. Move this test into a file with a `//go:build !windows` constraint (e.g. krb5_unix_test.go) so it does not fail on Windows test runs.

---

Reviewed commit: f3ad3ae1eda9

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 4c156cadf00b

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 5f86dd03c43c.
Review mode: incremental since f3ad3ae
View review run

Review Summary

This PR adds a blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go:8. That import registers the driver's krb5 integrated-auth provider. The PR also vendors the gokrb5/v8 module graph (vendor/modules.txt lists integratedauth/krb5 and the jcmturner/* modules) and adds tests and a docs line.

The new commit moves Test_DefaultProviderNameUnchanged out of krb5_test.go into a new krb5_unix_test.go behind a //go:build !windows constraint. krb5_test.go still uses the integratedauth and msdsn imports, so it still compiles. The rest of the change is the same as last time.

I re-scanned the full PR diff for security and correctness and found no issues; the incremental diff was complete, with no dropped paths. I applied the repo-local criteria, but none of the sync-path, logging, provisioning or JSON sections apply: no resource builders, logging, spans, or API structs changed. The dependency changes in go.mod match the new import.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (confidence: high, cosmetic) The test files aren't gofmt-formatted. pkg/database/sqlserver/krb5_test.go:42-44 ends with extra blank lines left behind when the test was moved out, and krb5_unix_test.go:18 has no newline at end of file. golangci-lint has no formatters enabled, so CI won't catch this. Running gofmt -w on both files fixes it.

Resolved prior findings

• Fixed: Test_DefaultProviderNameUnchanged used to fail on Windows, where DefaultProviderName is winsspi. It now lives in pkg/database/sqlserver/krb5_unix_test.go:1 behind //go:build !windows.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/database/sqlserver/krb5_test.go`:
- Around line 42-44: Delete the extra blank lines at the end of the file, left behind when Test_DefaultProviderNameUnchanged was moved out, so the file ends right after the closing brace of Test_Krb5ProviderRegistered. Running `gofmt -w` does this.

In `pkg/database/sqlserver/krb5_unix_test.go`:
- Around line 18: Add a newline at the end of the file. Running `gofmt -w` does this.

[github-actions]

Superseded — see the current review report for commit d8c37cf88432

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 5f86dd03c43c.
Review mode: incremental since f3ad3ae
View review run

Review Summary

This PR adds a blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go:8. That import registers the driver's krb5 integrated-auth provider. The PR also vendors the gokrb5/v8 module graph (vendor/modules.txt lists integratedauth/krb5 and the jcmturner/* modules) and adds tests and a docs line.

The new commit moves Test_DefaultProviderNameUnchanged out of krb5_test.go into a new krb5_unix_test.go behind a //go:build !windows constraint. krb5_test.go still uses the integratedauth and msdsn imports, so it still compiles. The rest of the change is the same as last time.

I re-scanned the full PR diff for security and correctness and found no issues; the incremental diff was complete, with no dropped paths. I applied the repo-local criteria, but none of the sync-path, logging, provisioning or JSON sections apply: no resource builders, logging, spans, or API structs changed. The dependency changes in go.mod match the new import.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (confidence: high, cosmetic) The test files aren't gofmt-formatted. pkg/database/sqlserver/krb5_test.go:42-44 ends with extra blank lines left behind when the test was moved out, and krb5_unix_test.go:18 has no newline at end of file. golangci-lint has no formatters enabled, so CI won't catch this. Running gofmt -w on both files fixes it.

Resolved prior findings

• Fixed: Test_DefaultProviderNameUnchanged used to fail on Windows, where DefaultProviderName is winsspi. It now lives in pkg/database/sqlserver/krb5_unix_test.go:1 behind //go:build !windows.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/database/sqlserver/krb5_test.go`:
- Around line 42-44: Delete the extra blank lines at the end of the file, left behind when Test_DefaultProviderNameUnchanged was moved out, so the file ends right after the closing brace of Test_Krb5ProviderRegistered. Running `gofmt -w` does this.

In `pkg/database/sqlserver/krb5_unix_test.go`:
- Around line 18: Add a newline at the end of the file. Running `gofmt -w` does this.

---

Reviewed commit: 4c156cadf00b

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit d8c37cf88432

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 5f86dd03c43c.
Review mode: incremental since 4c156ca
View review run

Review Summary

The PR adds a blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go. That import registers the driver's krb5 integrated-auth provider. The PR also vendors gokrb5/v8 v8.4.4 and its jcmturner/* and hashicorp/go-uuid dependencies, which every module in the go.mod diff needs because of that import. It adds a DSN round-trip test, a registry test, and one docs line. The new commit only rewords the comments in Test_Krb5ProviderRegistered and adds Test_ProviderMissMessageControl, a negative control that fails if the driver's registry-miss message stops containing "not found". The control is sound, and none of the vendored krb5 error strings contain "not found" either (vendor/.../integratedauth/krb5/krb5.go:33-42).

I scanned the full PR diff for security and correctness, including go.mod. The vendor/ contents only got a reachability check, not a line-by-line read. I applied the repo-local criteria. Sections S (sync data loss), A (log levels), B (error wrapping), C (spans) and J (JSON types) don't apply, because the PR doesn't touch any sync, logging, client or provisioning code. The breaking-change gate (E) isn't triggered either: the change only adds a provider, and DSNs without authenticator=krb5 behave as before.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (confidence: high, cosmetic): the test files are still not gofmt-formatted. pkg/database/sqlserver/krb5_test.go:62-63 has extra blank lines after the last closing brace, and pkg/database/sqlserver/krb5_unix_test.go:18 has no newline at end of file. An open thread already covers this.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/database/sqlserver/krb5_test.go`:
- Around line 62-63: Delete the extra blank lines after the closing brace of Test_ProviderMissMessageControl so the file ends with a single newline. Running `gofmt -w` does this.

In `pkg/database/sqlserver/krb5_unix_test.go`:
- Around line 18: Add a newline at the end of the file. Running `gofmt -w` does this.

[github-actions]

Superseded — see the current review report for commit e34fed7a9a94

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 5f86dd03c43c.
Review mode: incremental since 4c156ca
View review run

Review Summary

The PR adds a blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go. That import registers the driver's krb5 integrated-auth provider. The PR also vendors gokrb5/v8 v8.4.4 and its jcmturner/* and hashicorp/go-uuid dependencies, which every module in the go.mod diff needs because of that import. It adds a DSN round-trip test, a registry test, and one docs line. The new commit only rewords the comments in Test_Krb5ProviderRegistered and adds Test_ProviderMissMessageControl, a negative control that fails if the driver's registry-miss message stops containing "not found". The control is sound, and none of the vendored krb5 error strings contain "not found" either (vendor/.../integratedauth/krb5/krb5.go:33-42).

I scanned the full PR diff for security and correctness, including go.mod. The vendor/ contents only got a reachability check, not a line-by-line read. I applied the repo-local criteria. Sections S (sync data loss), A (log levels), B (error wrapping), C (spans) and J (JSON types) don't apply, because the PR doesn't touch any sync, logging, client or provisioning code. The breaking-change gate (E) isn't triggered either: the change only adds a provider, and DSNs without authenticator=krb5 behave as before.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (confidence: high, cosmetic): the test files are still not gofmt-formatted. pkg/database/sqlserver/krb5_test.go:62-63 has extra blank lines after the last closing brace, and pkg/database/sqlserver/krb5_unix_test.go:18 has no newline at end of file. An open thread already covers this.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/database/sqlserver/krb5_test.go`:
- Around line 62-63: Delete the extra blank lines after the closing brace of Test_ProviderMissMessageControl so the file ends with a single newline. Running `gofmt -w` does this.

In `pkg/database/sqlserver/krb5_unix_test.go`:
- Around line 18: Add a newline at the end of the file. Running `gofmt -w` does this.

---

Reviewed commit: d8c37cf88432

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit e34fed7a9a94

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 5f86dd03c43c.
Review mode: incremental since d8c37cf
View review run

Review Summary

The PR adds one blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go. That import's init() registers the krb5 provider, so a DSN with authenticator=krb5&krb5-* now reaches the Kerberos provider and no longer fails with provider krb5 not found. The PR also adds registry and DSN pass-through tests and one line of docs. The new commit (e34fed7) only runs gofmt on the two test files and shortens a comment. It does not change any behavior.

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 (jcmturner/* and hashicorp/go-uuid) and its go.sum entries. I confirmed vendor/modules.txt lists those modules. No pinned direct dependency changed. I confirmed in the vendored krb5.go that the registry test is sound: with /nonexistent paths the provider returns an os.Open error, which does not contain "not found", while a registry miss does.

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 authenticator=krb5 are unaffected.

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 Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Fixed: the earlier gofmt finding (extra blank lines after the last closing brace in pkg/database/sqlserver/krb5_test.go, and no final newline in pkg/database/sqlserver/krb5_unix_test.go). Commit e34fed7 fixes it: both files now end with a single }\n.

[github-actions]

Superseded — see the current review report for commit fbb69c07ca34

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 5f86dd03c43c.
Review mode: incremental since d8c37cf
View review run

Review Summary

The PR adds one blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go. That import's init() registers the krb5 provider, so a DSN with authenticator=krb5&krb5-* now reaches the Kerberos provider and no longer fails with provider krb5 not found. The PR also adds registry and DSN pass-through tests and one line of docs. The new commit (e34fed7) only runs gofmt on the two test files and shortens a comment. It does not change any behavior.

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 (jcmturner/* and hashicorp/go-uuid) and its go.sum entries. I confirmed vendor/modules.txt lists those modules. No pinned direct dependency changed. I confirmed in the vendored krb5.go that the registry test is sound: with /nonexistent paths the provider returns an os.Open error, which does not contain "not found", while a registry miss does.

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 authenticator=krb5 are unaffected.

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 Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Fixed: the earlier gofmt finding (extra blank lines after the last closing brace in pkg/database/sqlserver/krb5_test.go, and no final newline in pkg/database/sqlserver/krb5_unix_test.go). Commit e34fed7 fixes it: both files now end with a single }\n.

---

Reviewed commit: e34fed7a9a94

[github-actions]

No blocking issues found — see the full review report

[luisina-santos]

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.

[github-actions]

Superseded — see the current review report for commit fbb69c07ca34

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base d744b3bf380b.
Review mode: full
View review run

Review Summary

The only runtime change is a blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go:8. Its init() registers the krb5 provider through integratedauth.SetIntegratedAuthenticationProvider. That fixes the provider krb5 not found failure for DSNs that set authenticator=krb5. Nothing changes for DSNs that don't select it: the unix default stays ntlm, and the tests check this.

I scanned the full PR diff for security and correctness, including go.mod, go.sum and vendor/modules.txt:

• Dependencies: the new modules are jcmturner/gokrb5/v8 v8.4.4 plus its transitive modules and hashicorp/go-uuid. All are marked indirect and match what go-mssqldb v1.8.0's krb5 package needs. No existing pin moved, and there are no replace or exclude directives.
• Vendored code: the vendored krb5.go is the upstream file for the unchanged go-mssqldb version. I did not review the rest of the vendored gokrb5 tree line by line, since it is unmodified third-party source.
• Docs: I checked the claims in docs/docs-info.md against the vendored provider. Keytab login is used only when a username is set and the password is empty (krb5.go:141-157, 188-219), the realm can come from user@REALM (krb5.go:127-134), and the credcache and password methods exist as described.
• Tests: they are sound. With /nonexistent paths the provider errors on loading the config, and that error never contains the registry's provider %v not found text (integratedauth/auth.go:54). The negative control guards against upstream rewording that message.

How I applied the repo-local criteria: most of it doesn't apply here, because the PR touches no resource builders, List, Entitlements or Grants paths, provisioning, logging, spans, or API response structs (sections A–G and S). The breaking-change gate (E) is also not triggered, since Kerberos auth is opt-in through the DSN. The dependency checks were applied as described above.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

Superseded — see the current review report for commit 48f67504db47

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base d744b3bf380b.
Review mode: full
View review run

Review Summary

The only runtime change is a blank import of github.com/microsoft/go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go:8. Its init() registers the krb5 provider through integratedauth.SetIntegratedAuthenticationProvider. That fixes the provider krb5 not found failure for DSNs that set authenticator=krb5. Nothing changes for DSNs that don't select it: the unix default stays ntlm, and the tests check this.

I scanned the full PR diff for security and correctness, including go.mod, go.sum and vendor/modules.txt:

• Dependencies: the new modules are jcmturner/gokrb5/v8 v8.4.4 plus its transitive modules and hashicorp/go-uuid. All are marked indirect and match what go-mssqldb v1.8.0's krb5 package needs. No existing pin moved, and there are no replace or exclude directives.
• Vendored code: the vendored krb5.go is the upstream file for the unchanged go-mssqldb version. I did not review the rest of the vendored gokrb5 tree line by line, since it is unmodified third-party source.
• Docs: I checked the claims in docs/docs-info.md against the vendored provider. Keytab login is used only when a username is set and the password is empty (krb5.go:141-157, 188-219), the realm can come from user@REALM (krb5.go:127-134), and the credcache and password methods exist as described.
• Tests: they are sound. With /nonexistent paths the provider errors on loading the config, and that error never contains the registry's provider %v not found text (integratedauth/auth.go:54). The negative control guards against upstream rewording that message.

How I applied the repo-local criteria: most of it doesn't apply here, because the PR touches no resource builders, List, Entitlements or Grants paths, provisioning, logging, spans, or API response structs (sections A–G and S). The breaking-change gate (E) is also not triggered, since Kerberos auth is opt-in through the DSN. The dependency checks were applied as described above.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: fbb69c07ca34

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 48f67504db47

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base d744b3bf380b.
Review mode: incremental since fbb69c0
View review run

Review Summary

The PR adds a blank import of go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go:8. That import registers the krb5 provider, and the PR also vendors gokrb5/v8 and its transitive modules and documents keytab DSNs. The new commits change tests only. They remove the krb5 DSN round-trip case from database_test.go, the Test_ProviderMissMessageControl negative control, and krb5_unix_test.go. They also expand the comment on Test_Krb5ProviderRegistered. No production code changed since the last review.

I scanned the full PR diff for security and correctness, including go.mod/go.sum. No existing pin moved, and there is no replace or exclude. The added modules match what the krb5 package needs.

The remaining test still holds. With /nonexistent paths the provider fails at krb5.go:124 with Unable to load krb5 config…, which never contains the registry's provider %v not found text (integratedauth/auth.go:54). Without the removed negative control, an upstream rewording of that message would not be caught, but go-mssqldb is pinned and vendored, so the risk is negligible.

I applied the repo-local criteria. Sections A, B, C, D, F and S (logging, error wrapping, spans, JSON types, provisioning, empty-result data loss) do not apply: the PR adds no logging, spans, response structs, provisioning or List/Grants paths. Section E (breaking changes) found nothing breaking, because this only adds an opt-in auth method.

I did not review the vendored gokrb5 tree line by line, since it is unmodified third-party source.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

Connector PR Review: CXH-3209: register go-mssqldb's krb5 authenticator for keytab auth

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base d744b3bf380b.
Review mode: incremental since fbb69c0
View review run

Review Summary

The PR adds a blank import of go-mssqldb/integratedauth/krb5 in pkg/database/sqlserver/sqlserver.go:8. That import registers the krb5 provider, and the PR also vendors gokrb5/v8 and its transitive modules and documents keytab DSNs. The new commits change tests only. They remove the krb5 DSN round-trip case from database_test.go, the Test_ProviderMissMessageControl negative control, and krb5_unix_test.go. They also expand the comment on Test_Krb5ProviderRegistered. No production code changed since the last review.

I scanned the full PR diff for security and correctness, including go.mod/go.sum. No existing pin moved, and there is no replace or exclude. The added modules match what the krb5 package needs.

The remaining test still holds. With /nonexistent paths the provider fails at krb5.go:124 with Unable to load krb5 config…, which never contains the registry's provider %v not found text (integratedauth/auth.go:54). Without the removed negative control, an upstream rewording of that message would not be caught, but go-mssqldb is pinned and vendored, so the risk is negligible.

I applied the repo-local criteria. Sections A, B, C, D, F and S (logging, error wrapping, spans, JSON types, provisioning, empty-result data loss) do not apply: the PR adds no logging, spans, response structs, provisioning or List/Grants paths. Section E (breaking changes) found nothing breaking, because this only adds an opt-in auth method.

I did not review the vendored gokrb5 tree line by line, since it is unmodified third-party source.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: 48f67504db47

[github-actions]

No blocking issues found — see the full review report