Validate domain of Srv endpoints #489

RichardChen820 · 2023-11-03T09:39:20Z

Validate domain of Srv endpoints @jimmyca15

jimmyca15 · 2023-11-06T23:45:18Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

+
+        public static async Task<string> GetValidDomain(Uri originEndpoint, string srvHostName)
+        {
+            if (string.IsNullOrEmpty(srvHostName))


That seems like argument exception. Why would empty or null be passed?

. is a valid target host in SRV DNS, if we can't trust the SRV DNS lookup result, we probably can get such a host name, and after trimming the tailing '.', we get an empty host name.

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

jimmyca15 · 2023-11-06T23:49:34Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

+                }
+            }
+
+            // Get the Common Name (CN) from the Subject Name if Subject Alternative Name is not available


All of our certs use SAN right?

They should.

then we can remove this.

This is a way recommended by a RFC doc, but given we ensure we use SAN, we can remove this.

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

jimmyca15 · 2023-11-06T23:54:49Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

+        private const string CommonName = "CN=";
+        private const string MultiDomainWildcard = "*.";
+
+        public static async Task<string> GetValidDomain(Uri originEndpoint, string srvHostName)


This should be able to return multiple valid domains if there are multiple matches, for future proofing.

Make sense, updated

jimmyca15 · 2023-11-06T23:58:04Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/ConfigurationClientManager.cs

+
+        private async Task<bool> IsValidEndpoint(string hostName)
+        {
+            if (string.IsNullOrWhiteSpace(hostName))


Why would we have null or whitespace here?

Explained in another comment

jimmyca15 · 2023-11-06T23:58:19Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/ConfigurationClientManager.cs

+                return false;
+            }
+
+            if (string.IsNullOrEmpty(_validDomain))


I was expecting valid domains to be an enumerable.

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

jimmyca15 · 2023-11-07T00:09:54Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

+                // Initiate the connection, so it will download the server certificate
+                await sslStream.AuthenticateAsClientAsync(endpoint).ConfigureAwait(false);
+
+                var serverCertificate = sslStream.RemoteCertificate;


This will throw for untrusted cert right? Trying to figure out the code path of untrusted endpoint and untrusted endpoint with untrusted cert.

I think not, this doesn't verify whether it's a trusted cert, it won't verify the certificate chain, the expiration time, the issuer or the root cert is trusted or not, etc. And seems doing such verification is overkill here because we just aim to verify the domain of srv endpoint, not really need to check the legality of that certificate. We just leverage the cert to ensure the acquired SRV endpoints are in the same domain as the original endpoint would be fine.

No way ! We shouldn't be using any info from a certificate we haven't validated.

In that case, someone controlling a bad endpoint could mint a certificate with whatever they want.

Then we should validate the certificate, what kind of validation should be done against the certificate? This is what we can get which can help to do the validation
https://learn.microsoft.com/en-us/dotnet/api/system.net.security.sslpolicyerrors?view=net-7.0

Can we make an anonymous http call to the endpoint? I think http call has built in validation.

https://learn.microsoft.com/en-us/dotnet/api/system.net.security.remotecertificatevalidationcallback?view=net-7.0
Such Example

Confirmed, it does throw for untrusted cert as expected.

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

jimmyca15 · 2023-11-07T23:27:04Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/ConfigurationClientManager.cs

@@ -36,7 +38,8 @@ internal class ConfigurationClientManager : IConfigurationClientManager
        private readonly bool _replicaDiscoveryEnabled;

        private IList<ConfigurationClientWrapper> _dynamicClients;
-        private Lazy<SrvLookupClient> _srvLookupClient;
+        private SrvLookupClient _srvLookupClient;


Suggested change

private SrvLookupClient _srvLookupClient;

private readonly SrvLookupClient _srvLookupClient;

jimmyca15 · 2023-11-08T00:02:46Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

+                    return validDomains;
+                }
+
+                cert = new X509Certificate2(serverCertificate);


That's disposable.

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

jimmyca15 · 2023-11-14T23:58:31Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs

+                return validDomains;
+            }
+
+            using (cert)


It's a bit unfortunate the using is separated from the insantiation of the disposable object.

Could you use a private method to avoid separating the two?

using (var cert = new X509Certificate2(serverCertificate)) { return GetValidDomains(cert); }

* Validate domain of Srv endpoints * Resolve comments * Resolve comments * Create object in using

* Automatic http failover base on Srv Dns * update failover condition * Logging messages for auto failover * Not use generic method * Remove useless constructor * Failover to TCP when response is truncated * Error handling of TCP failover * Resolve comments and add thrid party notice * Fix the tests * Fix the tess * Fix the test * Update to use DnsClient.NET * Remove unecessary code * Resolve conflict * Stop once it has been auto failovered * Update GetAvaliableClient method * Resolve comments * Resolve comments * Internalize getting dynamic client in ConfigurationClientManager * Fix format * Fix format * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Not await RefreshFallbackClients when startup * Revert non-await RefreshFallbackClients * Validate domain of Srv endpoints (#489) * Validate domain of Srv endpoints * Resolve comments * Resolve comments * Create object in using * Add the origin host to the srv lookup result * Validate endpoint by domain name (#499) * Validate endpoint by domain name * Resolve comments * Remove *-test * Fix formatResolve conflict * Use await using for IAsyncDisposible * Replace IAsyncEumerable with ValueTask<IEnumerable> * Resolve comments * Remove cancallation token * Resolve comments

* Automatic http failover base on Srv Dns (#442) * Automatic http failover base on Srv Dns * update failover condition * Logging messages for auto failover * Not use generic method * Remove useless constructor * Failover to TCP when response is truncated * Error handling of TCP failover * Resolve comments and add thrid party notice * Fix the tests * Fix the tess * Fix the test * Update to use DnsClient.NET * Remove unecessary code * Resolve conflict * Stop once it has been auto failovered * Update GetAvaliableClient method * Resolve comments * Resolve comments * Internalize getting dynamic client in ConfigurationClientManager * Fix format * Fix format * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Resolve comments * Not await RefreshFallbackClients when startup * Revert non-await RefreshFallbackClients * Validate domain of Srv endpoints (#489) * Validate domain of Srv endpoints * Resolve comments * Resolve comments * Create object in using * Add the origin host to the srv lookup result * Validate endpoint by domain name (#499) * Validate endpoint by domain name * Resolve comments * Remove *-test * Fix formatResolve conflict * Use await using for IAsyncDisposible * Replace IAsyncEumerable with ValueTask<IEnumerable> * Resolve comments * Remove cancallation token * Resolve comments * Refactor and move backoff logic to AzureAppConfigurationProvider (#508) * move backoff logic to provider, can't access backoffendtime from manager for dynamic clients * add condition before calling getclients in refreshasync, add refreshclients method * fix logic to check if backoff ended for client * fix indents * check client backoff before refresh, fix tests in progress * remove tolist * comment changes, fix refresh tests * reduce minimum backoff when testing * fix tests, fix typo with failover logic * update comments in tests * resolve PR comments * resolve small comments * fix tests backoff duration * fix tests backoff duration * fix error from merging main branch in * add back fix for attempts backoff calculation --------- Co-authored-by: Richard chen <99175581+RichardChen820@users.noreply.github.com>

RichardChen820 force-pushed the user/junbchen/domainVerification2 branch 2 times, most recently from 1ddedad to c35748b Compare November 6, 2023 10:10

Validate domain of Srv endpoints

ec6a431

RichardChen820 force-pushed the user/junbchen/domainVerification2 branch from c35748b to ec6a431 Compare November 6, 2023 14:30

jimmyca15 reviewed Nov 6, 2023

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs Show resolved Hide resolved

jimmyca15 reviewed Nov 6, 2023

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs Show resolved Hide resolved

jimmyca15 reviewed Nov 6, 2023

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs Outdated Show resolved Hide resolved

jimmyca15 reviewed Nov 6, 2023

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs Outdated Show resolved Hide resolved

jimmyca15 reviewed Nov 6, 2023

View reviewed changes

jimmyca15 reviewed Nov 7, 2023

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs Outdated Show resolved Hide resolved

jimmyca15 reviewed Nov 7, 2023

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs Outdated Show resolved Hide resolved

Resolve comments

b4c41e0

RichardChen820 force-pushed the user/junbchen/domainVerification2 branch from 62d66e4 to b4c41e0 Compare November 7, 2023 09:58

jimmyca15 reviewed Nov 7, 2023

View reviewed changes

jimmyca15 reviewed Nov 8, 2023

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/DomainVerifier.cs Show resolved Hide resolved

Resolve comments

523733e

RichardChen820 force-pushed the user/junbchen/domainVerification2 branch from 9d7f7c2 to 523733e Compare November 8, 2023 03:47

jimmyca15 reviewed Nov 14, 2023

View reviewed changes

jimmyca15 approved these changes Nov 14, 2023

View reviewed changes

Create object in using

647b1f1

RichardChen820 merged commit 9479d35 into user/junbchen/srvDnsClient Nov 15, 2023
2 checks passed

RichardChen820 added a commit that referenced this pull request Nov 15, 2023

Validate domain of Srv endpoints (#489)

0d67bc9

* Validate domain of Srv endpoints * Resolve comments * Resolve comments * Create object in using

RichardChen820 added a commit that referenced this pull request Nov 28, 2023

Validate domain of Srv endpoints (#489)

cc5e5ba

* Validate domain of Srv endpoints * Resolve comments * Resolve comments * Create object in using

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate domain of Srv endpoints #489

Validate domain of Srv endpoints #489

RichardChen820 commented Nov 3, 2023 •

edited

Loading

jimmyca15 Nov 6, 2023

RichardChen820 Nov 7, 2023 •

edited

Loading

jimmyca15 Nov 6, 2023

jimmyca15 Nov 6, 2023

jimmyca15 Nov 7, 2023

RichardChen820 Nov 7, 2023 •

edited

Loading

jimmyca15 Nov 6, 2023 •

edited

Loading

RichardChen820 Nov 7, 2023

jimmyca15 Nov 6, 2023

RichardChen820 Nov 7, 2023

jimmyca15 Nov 6, 2023

RichardChen820 Nov 7, 2023

jimmyca15 Nov 7, 2023 •

edited

Loading

RichardChen820 Nov 7, 2023 •

edited

Loading

jimmyca15 Nov 7, 2023

jimmyca15 Nov 7, 2023

RichardChen820 Nov 8, 2023

jimmyca15 Nov 8, 2023

RichardChen820 Nov 8, 2023

RichardChen820 Nov 8, 2023 •

edited

Loading

jimmyca15 Nov 7, 2023

jimmyca15 Nov 8, 2023

jimmyca15 Nov 14, 2023 •

edited

Loading

	private SrvLookupClient _srvLookupClient;
	private readonly SrvLookupClient _srvLookupClient;

Validate domain of Srv endpoints #489

Validate domain of Srv endpoints #489

Conversation

RichardChen820 commented Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

RichardChen820 Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

jimmyca15 Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmyca15 Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

RichardChen820 Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichardChen820 Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmyca15 Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

RichardChen820 commented Nov 3, 2023 •

edited

Loading

RichardChen820 Nov 7, 2023 •

edited

Loading

RichardChen820 Nov 7, 2023 •

edited

Loading

jimmyca15 Nov 6, 2023 •

edited

Loading

jimmyca15 Nov 7, 2023 •

edited

Loading

RichardChen820 Nov 7, 2023 •

edited

Loading

RichardChen820 Nov 8, 2023 •

edited

Loading

jimmyca15 Nov 14, 2023 •

edited

Loading