[DRAFT] [Issue 305] Introduce a hook in Microsoft.Data.SqlClient to allow different mechanisms to acquire a Kerberos ticket#1379
Conversation
|
|
|
resolves #305 |
| internal byte[] _accessTokenInBytes; | ||
|
|
||
| // Kerberos ticket provided by OnRetrieveKerberosTicket | ||
| internal byte[] _kerberosTicketInBytes; |
There was a problem hiding this comment.
I realise that this follow the naming convention from the line above, the same logic will apply to that field as well if someone added it today.
Do we need the InBytes suffix? Do we have any other form of the same data (string etc) that is used? If not then it could just be called _kerberosTicket
There was a problem hiding this comment.
Agree with the comment. As you've noticed correctly the idea was to follow the established convention. Team may consider name refactoring.
| string instanceName = null; | ||
|
|
||
| // ReadOnlySpan is not supported in netstandard 2.0, but installing System.Memory solves the issue | ||
| ReadOnlySpan<char> input = dataSource.AsSpan().TrimStart(); |
There was a problem hiding this comment.
Thanks for using spans, almost always better for perf 😁
There was a problem hiding this comment.
This is c/p of a class from netcore path. This needs some refactoring and unification as it's useful for both necore and netfx.
|
|
||
| private bool InferConnectionDetails() | ||
| { | ||
| string[] tokensByCommaAndSlash = _dataSourceAfterTrimmingProtocol.Split(BackSlashCharacter, CommaSeparator); |
There was a problem hiding this comment.
Could this use span to avoid the string and array allocations? We had some interesting back and forth on using spans while parsing in #1237 that might be useful to review.
There was a problem hiding this comment.
It's definitely worth exploring. Everything said above applies.
| /// A callback to provide kerberos ticket for given service principal name (SPN) on demand. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public delegate byte[] KerberosTicketRetrievalCallback(string serverPrincipalName); |
There was a problem hiding this comment.
Does this need to be a new type, or is there a reason to use a type over a Func<byte[],string> ?
I'm also no sure about the naming. Is it really a callback? it feels more like a provider or an adapter. The user will set the this property of the connection to and it will be called by the connection to get the ticket to use. So we're not really calling back to anything instead we're providing a way to get an external value by some means. KerberosTicketProvider ? something like that?
There was a problem hiding this comment.
This follows conventions for this library ( https://github.com/dotnet/SqlClient/search?q=callback ). Team is also exploring changing this into adapter pattern design already used in FedAuth flow.
|
3rd anniversary of this PR! Congratulations :) |
|
Closing the PR for now as stale, please fix conflicts and re-open when ready for review. |
5 participants
No description provided.