Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract Kerberos functionality from SQL Server engine #51352

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tener
Copy link
Contributor

@Tener Tener commented Jan 22, 2025

Changes:

  • Moved the Kerberos functionality built around kinit into db/common
  • Extracted generic Kerberos glue code to new type kerberos.ClientProvider.
  • Updated SQL Server engine to use kerberos.ClientProvider

I've tried very hard to keep this change minimal. There is a number of things I'd like to change in the current code, but those changes can happen in future PRs while keeping this one as small as possible.

This is a prerequisite to using Kerberos in other db engines, in particular Oracle.

@Tener Tener added the no-changelog Indicates that a PR does not require a changelog entry label Jan 22, 2025
@github-actions github-actions bot added database-access Database access related issues and PRs size/sm labels Jan 22, 2025
@Tener Tener requested a review from gabrielcorado January 22, 2025 14:10

type clientProvider struct {
AuthClient windows.AuthInterface
DataDir string
Copy link
Contributor

@greedy52 greedy52 Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had some offline discussion on the cache dir. we can look into that afterwards. Here are just some notes but they should not block this PR:

kinit does refresh the output every connection, but there could be a race that two connections trying to output to the same file then read it.

On the other hand, it seems credentials.LoadCCache finishes reading that file immediately and has no use of it afterwards. We could experiment if we could just use tmp dir instead of data-dir.

Another consideration is if we want to add real caching to avoid kinit on every connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 database-access Database access related issues and PRs no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants