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

Expose SqlAuthenticationParameters on SSPIContextProvider #2454

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

twsouthwick
Copy link
Member

@twsouthwick twsouthwick commented Apr 8, 2024

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

Copy link

codecov bot commented May 8, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 7 lines in your changes missing coverage. Please review.

Project coverage is 66.09%. Comparing base (1e59b88) to head (72f1c99).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...crosoft/Data/SqlClient/SSPI/SSPIContextProvider.cs 83.87% 5 Missing ⚠️
...t/Data/SqlClient/SSPI/NativeSSPIContextProvider.cs 66.66% 1 Missing ⚠️
...ata/SqlClient/SSPI/NegotiateSSPIContextProvider.cs 87.50% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (1e59b88) and HEAD (72f1c99). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (1e59b88) HEAD (72f1c99)
addons 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2454      +/-   ##
==========================================
- Coverage   72.58%   66.09%   -6.49%     
==========================================
  Files         289      282       -7     
  Lines       59503    59309     -194     
==========================================
- Hits        43188    39199    -3989     
- Misses      16315    20110    +3795     
Flag Coverage Δ
addons ?
netcore 68.88% <75.00%> (-6.24%) ⬇️
netfx 65.02% <65.62%> (-6.19%) ⬇️

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 twsouthwick force-pushed the auth-params branch 3 times, most recently from 86512cb to ff9a983 Compare February 3, 2025 19:29
@twsouthwick twsouthwick force-pushed the auth-params branch 3 times, most recently from 96b5fb5 to 2f6ddef Compare March 3, 2025 19:19
@twsouthwick twsouthwick requested a review from mdaigle March 3, 2025 19:21
@twsouthwick
Copy link
Member Author

@mdaigle next step for sspi :)

@twsouthwick twsouthwick marked this pull request as ready for review March 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.
@cheenamalhotra cheenamalhotra requested a review from a team March 6, 2025 16:03
@twsouthwick
Copy link
Member Author

ping @mdaigle @cheenamalhotra

@mdaigle
Copy link
Contributor

mdaigle commented Mar 13, 2025

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

@twsouthwick
Copy link
Member Author

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

@mdaigle mdaigle 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.

Verified

This commit was signed with the committer’s verified signature.
jkuester Joshua Kuestersteffen
Copy link
Contributor

@mdaigle mdaigle left a comment

Choose a reason for hiding this comment

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

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

{
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.

var authParamsBuilder = new SqlAuthenticationParameters.Builder(

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.

Copy link
Member Author

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 existing SqlAuthenticationParameters

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants