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

Fix | Skip the CRL check during authenticaiton #1559

Merged

Conversation

DavoudEshtehari
Copy link
Contributor

@DavoudEshtehari DavoudEshtehari commented Mar 24, 2022

Passing the true as the last parameter in AuthenticateAsClient(String, X509CertificateCollection, SslProtocols, Boolean) method enables the Certificate Revocation List (CRL) check during authentication. If the CRL wasn't accessible from where the request was sent, it will fail to send the request to the SQL Server.

@DavoudEshtehari DavoudEshtehari added the Area\Managed SNI Issues that are targeted to the Managed SNI codebase. label Mar 24, 2022
@DavoudEshtehari DavoudEshtehari marked this pull request as ready for review March 25, 2022 17:16
@DavoudEshtehari DavoudEshtehari added this to the 5.0.0-preview2 milestone Mar 25, 2022
Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

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

I'd note in the description that passing false instead of true to that API changes the behavior of the call back to the way it was prior to the changes in #1168.

@roji
Copy link
Member

roji commented Apr 2, 2022

It may be worth adding a connection string parameter to allow opting into the CRL check, for users which do want it (CRLs can be a very important security thing).

@David-Engel
Copy link
Contributor

It may be worth adding a connection string parameter to allow opting into the CRL check, for users which do want it (CRLs can be a very important security thing).

@roji
I don't think that is necessary. The previous method we were calling, AuthenticateAsClient(string targetHost), just called the new method with a false parameter:
AuthenticateAsClient(targetHost, null, SecurityProtocol.SystemDefaultSecurityProtocols, false);
So by changing false to true in the previous PR, I believe we are forcing an immediate CRL check on every connection (bad for performance). I also assume, that by passing false, as the previous method was, we are only suppressing the CRL check for the immediate connection. The underlying platform can still update the CRL list when it deems it is necessary/appropriate.

@DavoudEshtehari DavoudEshtehari merged commit e597ff4 into dotnet:main Apr 5, 2022
@roji
Copy link
Member

roji commented Apr 7, 2022

So by changing false to true in the previous PR, I believe we are forcing an immediate CRL check on every connection (bad for performance).

A CRL check on every (physical) connection can be critical for security, otherwise an application may accept certificates that have been revoked. In the common case, I don't expect the performance of physical connection opening to be critical - that's why pooling is there, and SSL/TLS already typically involved multiple round trips between the client and server.

Note that I'm not advocating CRL necessarily be enabled by default - just to allow users to opt into it if they so wish, for added security (FWIW this is what Npgsql does, see docs. After all, SslStream does expose this bool flag, so it's just a matter of allowing users to determine what SqlClient passes into it. If that makes sense, I can open a new issue for that.

DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Aug 11, 2022
Kaur-Parminder pushed a commit that referenced this pull request Aug 11, 2022
Skip the CRL check during authentication.
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Aug 15, 2022
DavoudEshtehari added a commit to DavoudEshtehari/SqlClient that referenced this pull request Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area\Managed SNI Issues that are targeted to the Managed SNI codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants