Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Expose SspiAuthenticationParameters on SspiContextProvider#2454

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
twsouthwick merged 13 commits intodotnet:mainfromtwsouthwick:auth-params
May 13, 2025

Conversation

twsouthwick
Copy link
Member

@twsouthwicktwsouthwick commentedApr 8, 2024
edited
Loading

This change updates the SSPI context provider to surface information to implementers via SqlAuthenticationParameters.

This also expands#2559 to loop through all SPNs in the base SSPIContextProvider rather than just a specific implementation.

Part of#2253

@codecovCodecov
Copy link

codecovbot commentedMay 8, 2024
edited
Loading

Codecov Report

Attention: Patch coverage is79.31034% with12 lines in your changes missing coverage. Please review.

Project coverage is 61.43%. Comparing base(17621da) to head(f7c90b4).
Report is 1 commits behind head on main.

Files with missing linesPatch %Lines
...crosoft/Data/SqlClient/SSPI/SspiContextProvider.cs75.00%8 Missing⚠️
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs0.00%1 Missing⚠️
...t/Data/SqlClient/SSPI/NativeSspiContextProvider.cs66.66%1 Missing⚠️
...ata/SqlClient/SSPI/NegotiateSspiContextProvider.cs88.88%1 Missing⚠️
...ata/SqlClient/SSPI/SspiAuthenticationParameters.cs88.88%1 Missing⚠️

❗ There is a different number of reports uploaded between BASE (17621da) and HEAD (f7c90b4). Click for more details.

HEAD has 1 upload less than BASE
FlagBASE (17621da)HEAD (f7c90b4)
addons10
Additional details and impacted files
@@            Coverage Diff             @@##             main    #2454      +/-   ##==========================================- Coverage   67.03%   61.43%   -5.61%==========================================  Files         299      294       -5       Lines       65497    65207     -290     ==========================================- Hits        43907    40057    -3850- Misses      21590    25150    +3560
FlagCoverage Δ
addons?
netcore66.63% <73.21%> (-5.58%)⬇️
netfx59.57% <65.21%> (-5.55%)⬇️

Flags with carried forward coverage won't be shown.Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

🚀 New features to boost your workflow:
  • ❄️Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@twsouthwick
Copy link
MemberAuthor

@mdaigle next step for sspi :)

@twsouthwicktwsouthwick marked this pull request as ready for reviewMarch 3, 2025 19:28
As part of this change, the SSPIContextProvider base class now iterates through all the server names similar to what NegotiateSSPIContextProvider did.
@twsouthwick
Copy link
MemberAuthor

ping@mdaigle@cheenamalhotra

@mdaigle
Copy link
Contributor

Hey@twsouthwick, I'll try to take a look at this today!

twsouthwick reacted with heart emoji

@twsouthwick
Copy link
MemberAuthor

The errors appear to also be on main and are an "Access denied" for something related to certificates. It doesn't appear to be caused by my change, but let me know if you believe it is.

Copy link
Contributor

@mdaiglemdaigle left a comment

Choose a reason for hiding this comment

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

I think this all makes sense to me. I'll take another look tomorrow and also see why the tests are failing.

Copy link
Contributor

@mdaiglemdaigle left a comment

Choose a reason for hiding this comment

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

Testsshould be green now. We had a build agent issue that was resolved yesterday.

twsouthwick reacted with hooray emoji
{
var auth = new SqlAuthenticationParameters.Builder(
authenticationMethod: connection.ConnectionOptions.Authentication,
resource: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to think of the best way to populate this. In other usage, I see serverSpn getting set as the resource and dataSource getting set as the serverName. I think it makes sense to continue that pattern here given that serverName and server SPN may be different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also not sure how to feel about authority. In federated auth, it's assumed that authority will be present, but here we don't set a value.

Let me chat with the team to see if we'd prefer to create a new type to better represent this set of credentials.

twsouthwick reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sounds good. I refactored the builder so that the same pattern can be had wherever it's used if we want to use the existingSqlAuthenticationParameters

@paulmedynskipaulmedynski self-assigned thisApr 2, 2025
@twsouthwicktwsouthwick changed the titleExpose SqlAuthenticationParameters on SSPIContextProviderExpose SspiAuthenticationParameters on SSPIContextProviderMay 1, 2025
@twsouthwicktwsouthwick changed the titleExpose SspiAuthenticationParameters on SSPIContextProviderExpose SspiAuthenticationParameters on SspiContextProviderMay 1, 2025
@mdaiglemdaigle requested a review froma teamMay 12, 2025 22:44
@twsouthwicktwsouthwick merged commit46b88bf intodotnet:mainMay 13, 2025
237 checks passed
@twsouthwicktwsouthwick deleted the auth-params branchMay 13, 2025 14:31
{
NegotiateAuthenticationStatusCode statusCode = NegotiateAuthenticationStatusCode.UnknownCredentials;

_negotiateAuth ??= new(new NegotiateAuthenticationClientOptions { Package = "Negotiate", TargetName = authParams.Resource });

Choose a reason for hiding this comment

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

I'm probably missing something, but from my understandingSspiContextProvider attempts to generate sspi context by iterating over eachserverSpns (which is passed viaauthParams.Resource). But because_negotiateAuth is cached here, if it fails for the first spn, it's also going to fail for the others since it's going to be the exact same spn.

Copy link
Contributor

@mdaiglemdaigleMay 13, 2025
edited
Loading

Choose a reason for hiding this comment

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

You're right, nice catch! The implementation used to reset _negotiateAuth to null if authentication failed, but that's missing now.@twsouthwick can we correct it in a follow up PR?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Will do - good catch. I'll open a separate PR for that.

The looping was added independently of the work I've been doing, so I'm not fully aware of how it's expected to work.

It opens a question as to the design I've moved this to - if we have a list of SPNs, what's the expected outcome? I think things will be just fine if it's the first SPN that completes the SSPI handshake. However if it's the second, on subsequent attempts it will be trying the first again. I can update the SspiContextProvider base to keep track of what was the successfull SPN to only submit that on subsequent calls.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Here's the PR to add that back:#3347

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@mdaiglemdaiglemdaigle approved these changes

@vonzshikvonzshikvonzshik left review comments

@paulmedynskipaulmedynskipaulmedynski approved these changes

@David-EngelDavid-EngelAwaiting requested review from David-Engel

@benrr101benrr101Awaiting requested review from benrr101

Assignees

@paulmedynskipaulmedynski

Labels
None yet
Projects
None yet
Milestone
6.1-preview2
Development

Successfully merging this pull request may close these issues.

6 participants
@twsouthwick@mdaigle@benrr101@vonzshik@David-Engel@paulmedynski

[8]ページ先頭

©2009-2025 Movatter.jp