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 | Fix certificate validation #2439

Closed
wants to merge 55 commits into from

Conversation

arellegue
Copy link
Contributor

@arellegue arellegue commented Apr 1, 2024

This PR fixes issue #2178.

Problem was with Certificate Chain Link error, which is now resolved by comparing the binary raw data of the client certificate against the server certificate. If both certificates match, we allow it to continue. Prior to this, any certificate policy error, we stopped operations and threw an exception.

@JRahnama
Copy link
Contributor

JRahnama commented Apr 2, 2024

@David-Engel Even if two certificates have identical raw data, they will have different digital signatures if they are issued by different entities and also even if two certificates have identical raw data, they may have been issued to different subjects. The question would be is comparing two certificate raw data is enough to determine if they are equal?

I had an internal conversation and it is proven that I am wrong. Ignore my comment here.

Copy link
Contributor

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

Have you intentionally removed the Address, Circle, Shapes, and Utf8String projects?

@JRahnama
Copy link
Contributor

Also, can you address pipeline failures?


# Update the certificates store
Write-Output "Updating the certificates store..."
update-ca-certificates -v
Copy link
Contributor

Choose a reason for hiding this comment

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

There are couple of things here, generally it is better to use bash on ubuntu, you will have a better control over the commands.

  1. Enabling as system CA certificate is missing here. It could be done by dpkg-reconfigure command
  2. Remember since the server and client are on the same machine you need to configure them for both.
  3. Removing '!' at the beginning of certificate inside ca-certificate is missed. You can test with sudo cat /etc/ca-certificates.conf command to ensure '!' is removed from the certificate name. If '!' shows up Infront of the certificate name it wont be included in update-ca-certificate command.
  4. It is better to provide chmod 755/777 to ensure sql server has access to those files.

KeyAlgorithm = "RSA"
KeyLength = 2048
HashAlgorithm = "SHA256"
TextExtension = "2.5.29.37={text}1.3.6.1.5.5.7.3.1", "2.5.29.17={text}DNS=$fqdn&DNS=$localhost&IPAddress=$LoopBackIPV4&DNS=$sqlAliasName&IPAddress=$LoopBackIPV6"
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simplified by adding DNSName to paramters.

Type = "SSLServerAuthentication"
Subject = "CN=$fqdn"
KeyAlgorithm = "RSA"
KeyLength = 2048
Copy link
Contributor

Choose a reason for hiding this comment

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

Why on Unix you have chosen 4096 and on windows 2048 for key length?

@@ -106,13 +106,15 @@ public void OpenningConnectionWithGoodCertificateTest()

// Test with Mandatory encryption
builder.Encrypt = SqlConnectionEncryptOption.Mandatory;
builder.TrustServerCertificate = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this just mask if the test doesn't set up the server certificate correctly? (Same for line 117.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was addressed in PR #2487.

Comment on lines +45 to +80
string userId = string.Empty;
string password = string.Empty;
SqlConnectionStringBuilder builder = new(DataTestUtility.TCPConnectionString);
userId = builder.UserID;
password = builder.Password;

using TestTdsServer server = TestTdsServer.StartTestServer(enableFedAuth: false, enableLog: false, connectionTimeout: 15,
methodName: "", new X509Certificate2(s_fullPathToPfx, "nopassword", X509KeyStorageFlags.UserKeySet),
encryptionType: connectionTestParameters.TdsEncryptionType);

if (userId != string.Empty)
{
builder = new(server.ConnectionString)
{
UserID = userId,
Password = password,
TrustServerCertificate = connectionTestParameters.TrustServerCertificate,
Encrypt = connectionTestParameters.Encrypt,
};
}
else
{
builder = new(server.ConnectionString)
{
IntegratedSecurity = true,
TrustServerCertificate = connectionTestParameters.TrustServerCertificate,
Encrypt = connectionTestParameters.Encrypt,
};
}

if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && userId == string.Empty)
{
builder.IntegratedSecurity = false;
builder.UserID = "user";
builder.Password = "password";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems strange. Why do we switch between integrated and un/pw here? And why use the un/pw from the config? Doesn't the auth not matter since TestTdsServer doesn't care about auth here? Seems like this could be simplified to just a few lines... No?

Suggested change
string userId = string.Empty;
string password = string.Empty;
SqlConnectionStringBuilder builder = new(DataTestUtility.TCPConnectionString);
userId = builder.UserID;
password = builder.Password;
using TestTdsServer server = TestTdsServer.StartTestServer(enableFedAuth: false, enableLog: false, connectionTimeout: 15,
methodName: "", new X509Certificate2(s_fullPathToPfx, "nopassword", X509KeyStorageFlags.UserKeySet),
encryptionType: connectionTestParameters.TdsEncryptionType);
if (userId != string.Empty)
{
builder = new(server.ConnectionString)
{
UserID = userId,
Password = password,
TrustServerCertificate = connectionTestParameters.TrustServerCertificate,
Encrypt = connectionTestParameters.Encrypt,
};
}
else
{
builder = new(server.ConnectionString)
{
IntegratedSecurity = true,
TrustServerCertificate = connectionTestParameters.TrustServerCertificate,
Encrypt = connectionTestParameters.Encrypt,
};
}
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows) && userId == string.Empty)
{
builder.IntegratedSecurity = false;
builder.UserID = "user";
builder.Password = "password";
}
using TestTdsServer server = TestTdsServer.StartTestServer(enableFedAuth: false, enableLog: false, connectionTimeout: 15,
methodName: "", new X509Certificate2(s_fullPathToPfx, "nopassword", X509KeyStorageFlags.UserKeySet),
encryptionType: connectionTestParameters.TdsEncryptionType);
SqlConnectionStringBuilder builder = new(server.ConnectionString)
{
TrustServerCertificate = connectionTestParameters.TrustServerCertificate,
Encrypt = connectionTestParameters.Encrypt,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was addressed in PR #2487.

@arellegue arellegue removed the request for review from DavoudEshtehari May 3, 2024 15:32
@arellegue arellegue marked this pull request as draft May 3, 2024 15:35
@DavoudEshtehari
Copy link
Contributor

Closing as the rework has been merged with #2487.

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.

7 participants