Fix #197 - AWSEKSResourceDetector fails to detect resources due to exception "The SSL connection could not be established" #208

swetharavichandrancisco · 2022-02-17T22:40:34Z

Issue Description:
With the current code, it requires an additional step of installing kubernetes self-signed certificate to the trusted store as .NET core expects them to the trusted store to make a secure connection.

Fix Provided:
Handler - Moved the creation of HttpClientHandler with client certificate into a separate class
ServerCertificationValidationProvider - Safely loads the certificate to the trusted collection along with ServerSideValidation and passed to HttpClient. This would avoid the unnecessary step of installing the certificate manually.

iskiselev · 2022-03-01T04:27:02Z

src/OpenTelemetry.Contrib.Extensions.AWSXRay/AWSXRayEventSource.cs

@@ -56,6 +56,12 @@ public void FailedToInjectActivityContext(string format, string error)
            this.WriteEvent(2, format, error);
        }

+        [Event(2, Message = "Failed to validate certificate in format: '{0}', error: '{1}'.", Level = EventLevel.Warning)]


Should event id here be 4 (same as few lines bellow)?

iskiselev · 2022-03-01T04:32:56Z

src/OpenTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/Handler.cs

+            {
+                var clientHandler = new HttpClientHandler()
+                {
+                    ClientCertificateOptions = ClientCertificateOption.Manual,


I don't think we still need it. ClientCertificateOptions (with ClientCertificates)used in case if client need to be authenticated on server with SSL certificate (mutual SSL) - and not in case if we need to validate server certificate only.

iskiselev · 2022-03-02T21:20:13Z

...enTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/ServerCertificateValidationProvider.cs

+            {
+                foreach (var certificate in collection)
+                {
+                    if (chainElement.Certificate.GetPublicKey().SequenceEqual(certificate.GetPublicKey()))


We can use LINQ instead

Enumerable.SequenceEqual(chainElement.Certificate.GetPublicKey(), certificate.GetPublicKey())

agree. And that way we can remove the extension method OpenTelemetry.Contrib.Extensions.AWSXRay.Resources.Http.X509Extensions.SequenceEqual(this byte[] first, byte[] second)

iskiselev · 2022-03-02T21:25:34Z

...enTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/ServerCertificateValidationProvider.cs

+        }
+
+        private bool ValidateCertificate(X509Certificate2 cert, X509Chain chain, SslPolicyErrors errors)
+        {


With this function we will try do full validation of certificate chain - it will still apply standard checks that certificate is not expired, is not revoked, created for proper domain.
It may be overkill if we would like just trust certificate provided in file on disk. So, we can discuss if we can validate just that server public key match public key on disk.

@srprash What's your thoughts on this?

Yeah, it may be a bit overkill, but since this validation will be done only 1 time on TraceProvider initialization, it should be okay. BTW, do you by any chance know what's the performance impact of this validation?

We have not done performance measurement, but it should be neglectable as we are not doing much more than standard certificate validation will do - we try to build second chain (one chain is already built by normal check), validate results and scan certificate keys to match certificate on disk. I'd expect it may be up to 3 time slower then normal SSL certificate check, but it never will be on hot path - and in our case, it will be only at init time.

Sounds good. Thanks!

src/OpenTelemetry.Contrib.Extensions.AWSXRay/OpenTelemetry.Contrib.Extensions.AWSXRay.csproj

iskiselev · 2022-03-02T21:34:58Z

...enTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/ServerCertificateValidationProvider.cs

+        {
+            try
+            {
+                collection.Import(certFileName, null, (X509KeyStorageFlags)32);


32 means EphemeralKey. If provided certificate will have private key to prevent creating temporary files on disk. I believe it is not required if we guarantee to pass certificate only file, not pfx.

otherwise if we have to use EphemeralKeySet flag I would suggest checking for the runtimes lower than net472 or lower than netstandard2.1 where X509KeyStorageFlags.EphemeralKeySet (32) doesn't exist. Otherwise we will eventually throw here. If we need I'd recommend doing this check much earlier so we can fallback gracefully

Thanks for pointing this out. I have removed EphemeralKey.

alexeypukhov

I think this code should work as expected. Just few comments for improvements

alexeypukhov · 2022-03-02T21:07:38Z

src/OpenTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/Handler.cs

+                };
+                clientHandler.ClientCertificates.Add(new X509Certificate2(certificateFile));
+
+                ServerCertificateValidationProvider serverCertificateValidationProvider =


should you check if serverCertificateValidationProvider is not ServerCertificateValidationProvider.InvalidProvider before adding a validation callback? If the cert file doesn't exist or cannot be parsed you cannot use it for validation

I don't think it is required. If serverCertificateValidationProvider is ServerCertificateValidationProvider.InvalidProvider, ValidationCallback will be null and no call will be made for ServerCertificateCustomValidationCallback.

alexeypukhov · 2022-03-02T21:10:35Z

src/OpenTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/X509Extensions.cs

+{
+    internal static class X509Extensions
+    {
+        public static bool SequenceEqual(this byte[] first, byte[] second)


do you really need this extension method? It should be available in .net starting from 3.5

As suggested, used LINQ instead

alexeypukhov · 2022-03-02T21:20:34Z

...enTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/ServerCertificateValidationProvider.cs

+        {
+            try
+            {
+                collection.Import(certFileName, null, (X509KeyStorageFlags)32);


Do we expect that the certFileName always contains just one certificate? Because if it has multiple certificates than only the first one will be added to the collection. Maybe still worth mentioning in the comment

Certificate File will be loaded from Kubernetes machine ("/var/run/secrets/kubernetes.io/serviceaccount/ca.crt") which is hardcoded. There is not gonna be multiple certificates being loaded.

alexeypukhov · 2022-03-02T23:57:05Z

src/OpenTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/Handler.cs

+                ServerCertificateValidationProvider serverCertificateValidationProvider =
+                    ServerCertificateValidationProvider.FromCertificateFile(certificateFile);
+
+                clientHandler.ServerCertificateCustomValidationCallback =


you can simplify this assignment to

clientHandler.ServerCertificateCustomValidationCallback = serverCertificateValidationProvider.ValidationCallback;

if you change the signature of the method in ServerCertificateValidationProvider class:

bool ValidationCallback(HttpRequestMessage requestMessage, X509Certificate2 certificate, X509Chain chain, SslPolicyErrors sslErrors) { ... }

alexeypukhov · 2022-03-03T00:00:41Z

...enTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/ServerCertificateValidationProvider.cs

+        {
+            try
+            {
+                collection.Import(certFileName, null, (X509KeyStorageFlags)32);


otherwise if we have to use EphemeralKeySet flag I would suggest checking for the runtimes lower than net472 or lower than netstandard2.1 where X509KeyStorageFlags.EphemeralKeySet (32) doesn't exist. Otherwise we will eventually throw here. If we need I'd recommend doing this check much earlier so we can fallback gracefully

alexeypukhov · 2022-03-03T00:06:19Z

...enTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/ServerCertificateValidationProvider.cs

+            {
+                foreach (var certificate in collection)
+                {
+                    if (chainElement.Certificate.GetPublicKey().SequenceEqual(certificate.GetPublicKey()))


agree. And that way we can remove the extension method OpenTelemetry.Contrib.Extensions.AWSXRay.Resources.Http.X509Extensions.SequenceEqual(this byte[] first, byte[] second)

iskiselev · 2022-03-08T04:08:12Z

It is fix #197

codecov · 2022-03-30T17:49:01Z

Codecov Report

Merging #208 (7f76cf5) into main (3883de8) will increase coverage by 0.89%.
The diff coverage is 61.44%.

@@            Coverage Diff             @@
##             main     #208      +/-   ##
==========================================
+ Coverage   24.35%   25.25%   +0.89%     
==========================================
  Files         124      126       +2     
  Lines        3703     3778      +75     
==========================================
+ Hits          902      954      +52     
- Misses       2801     2824      +23

Impacted Files	Coverage Δ
...nsions.AWSXRay/Resources/AWSEKSResourceDetector.cs	`0.00% <0.00%> (ø)`
...ources/Http/ServerCertificateValidationProvider.cs	`61.29% <61.29%> (ø)`
...ntrib.Extensions.AWSXRay/Resources/Http/Handler.cs	`73.33% <73.33%> (ø)`
...y.Contrib.Extensions.AWSXRay/AWSXRayEventSource.cs	`14.28% <100.00%> (+14.28%)`	⬆️

linux-foundation-easycla · 2022-04-04T19:06:59Z

The committers listed above are authorized under a signed CLA.

✅ login: swetharavichandrancisco / name: Swetha Ravichandran (5083e87, 18d9906, 363d5ce)

srprash · 2022-04-20T16:13:12Z

src/OpenTelemetry.Contrib.Extensions.AWSXRay/Resources/Http/Handler.cs

+
+using System;
+using System.Net.Http;
+using System.Security.Cryptography.X509Certificates;


This is an unused import. Can be removed.

srprash · 2022-04-20T16:15:44Z

@ievgen-platonov I think you'll need to complete CLA authorization.

… due to exception "The SSL connection could not be established" Issue Description: With the current code, it requires an additional step of installing kubernetes self-signed certificate to the trusted store as .NET core expects them to the trsuted store to make a secure connection. Fix Provided: ServerCertificationValidationProvider - Safely loads the certificate to the trusted collection along with ServerSideValidation. This would avoid the unnecessary step of installing the certificate manually. Handler - Creates HttpClientHandler with client certificate. (cherry picked from commit 41e68d3)

(cherry picked from commit 0b5d9a9)

(cherry picked from commit 4aa1c62)

swetharavichandrancisco · 2022-04-21T19:10:55Z

@ievgen-platonov I think you'll need to complete CLA authorization.

@srprash, It was my mistake as I had fetched the main branch in the GITHUB UI where main and my commits were not in sync. I fixed it and it should not fail in CLA authorization

linux-foundation-easycla · 2022-05-06T16:32:02Z

The committers listed above are authorized under a signed CLA.

✅ login: swetharavichandrancisco / name: Swetha Ravichandran (5083e87, 18d9906, 363d5ce, 7a68013, c5e8860, 5cba7e7, d0ee9a2)
✅ login: srprash / name: Prashant Srivastava (705ceb5)
✅ login: iskiselev / name: Igor Kiselev (4fb6c27)

iskiselev · 2022-05-06T19:42:25Z

test/OpenTelemetry.Contrib.Extensions.AWSXRay.Tests/Resources/Http/CertificateUploader.cs

+
+        public void Dispose()
+        {
+            for (int tries = 0; tries < 3; tries++)


Should it be:

for (int tries = 0; ; tries++)

You already limit amount of tries in when on catch.

Fixed For loop tries

This reverts commit b982325.

srprash

Thanks a lot! :)

swetharavichandrancisco requested a review from a team February 17, 2022 22:40

swetharavichandrancisco requested a review from srprash as a code owner February 17, 2022 22:40

iskiselev reviewed Mar 2, 2022

View reviewed changes

alexeypukhov reviewed Mar 3, 2022

View reviewed changes

swetharavichandrancisco changed the title ~~#197 - AWSEKSResourceDetector fails to detect resources due to exception "The SSL connection could not be established"~~ Fix #197 - AWSEKSResourceDetector fails to detect resources due to exception "The SSL connection could not be established" Mar 28, 2022

github-actions bot assigned CodeBlanch, SergeyKanzhelev, srprash and vishweshbankwar Apr 4, 2022

srprash reviewed Apr 20, 2022

View reviewed changes

swetharavichandrancisco added 3 commits April 21, 2022 02:38

PR Review Comments

18d9906

(cherry picked from commit 0b5d9a9)

Address PR comments

363d5ce

(cherry picked from commit 4aa1c62)

swetharavichandrancisco force-pushed the bug/eksssl branch from 8567907 to 363d5ce Compare April 20, 2022 21:11

Remove unused import

7a68013

srprash and others added 5 commits April 21, 2022 12:37

Merge branch 'main' into bug/eksssl

705ceb5

Added Unit Tests

c5e8860

Removed unused directive

5cba7e7

Added Trait for codecov build

d0ee9a2

Use temporary files to store test certificate

4fb6c27

swetharavichandrancisco added 2 commits May 6, 2022 22:27

Fixed File Access Issue in Unit Test

d4bfbb9

Use TempFileName for Unit Tests

6c57745

iskiselev reviewed May 6, 2022

View reviewed changes

Merge branch 'main' into bug/eksssl

4b85e19

swetharavichandrancisco added 3 commits May 9, 2022 23:28

Added Debug logging for build test

b982325

Fixed For loop tries

Revert "Added Debug logging for build test"

7f34dce

This reverts commit b982325.

Fixed tries logic in for loop

cf49b8e

iskiselev approved these changes May 12, 2022

View reviewed changes

Merge branch 'main' into bug/eksssl

7f76cf5

srprash approved these changes May 12, 2022

View reviewed changes

srprash merged commit 5608616 into open-telemetry:main May 12, 2022

iskiselev mentioned this pull request May 31, 2023

REQUEST: New membership for iskiselev open-telemetry/community#1517

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #197 - AWSEKSResourceDetector fails to detect resources due to exception "The SSL connection could not be established" #208

Fix #197 - AWSEKSResourceDetector fails to detect resources due to exception "The SSL connection could not be established" #208

swetharavichandrancisco commented Feb 17, 2022

iskiselev Mar 1, 2022

swetharavichandrancisco Apr 4, 2022

iskiselev Mar 1, 2022

iskiselev Mar 2, 2022

alexeypukhov Mar 3, 2022

iskiselev Mar 2, 2022

swetharavichandrancisco Apr 4, 2022

srprash Apr 20, 2022

iskiselev Apr 20, 2022 •

edited

Loading

srprash Apr 20, 2022

iskiselev Mar 2, 2022

alexeypukhov Mar 3, 2022

swetharavichandrancisco Mar 30, 2022

alexeypukhov left a comment

alexeypukhov Mar 2, 2022

swetharavichandrancisco Mar 30, 2022

alexeypukhov Mar 2, 2022

swetharavichandrancisco Mar 30, 2022 •

edited

Loading

alexeypukhov Mar 2, 2022

swetharavichandrancisco Mar 30, 2022

alexeypukhov Mar 2, 2022

alexeypukhov Mar 3, 2022

alexeypukhov Mar 3, 2022

iskiselev commented Mar 8, 2022

codecov bot commented Mar 30, 2022 •

edited

Loading

linux-foundation-easycla bot commented Apr 4, 2022 •

edited

Loading

srprash Apr 20, 2022

swetharavichandrancisco Apr 21, 2022

srprash commented Apr 20, 2022

swetharavichandrancisco commented Apr 21, 2022

linux-foundation-easycla bot commented May 6, 2022 •

edited

Loading

iskiselev May 6, 2022 •

edited

Loading

srprash left a comment

Fix #197 - AWSEKSResourceDetector fails to detect resources due to exception "The SSL connection could not be established" #208

Fix #197 - AWSEKSResourceDetector fails to detect resources due to exception "The SSL connection could not be established" #208

Conversation

swetharavichandrancisco commented Feb 17, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iskiselev Apr 20, 2022 • 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

alexeypukhov left a 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

swetharavichandrancisco Mar 30, 2022 • 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

iskiselev commented Mar 8, 2022

codecov bot commented Mar 30, 2022 • edited Loading

Codecov Report

linux-foundation-easycla bot commented Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srprash commented Apr 20, 2022

swetharavichandrancisco commented Apr 21, 2022

linux-foundation-easycla bot commented May 6, 2022 • edited Loading

iskiselev May 6, 2022 • edited Loading

Choose a reason for hiding this comment

srprash left a comment

Choose a reason for hiding this comment

iskiselev Apr 20, 2022 •

edited

Loading

swetharavichandrancisco Mar 30, 2022 •

edited

Loading

codecov bot commented Mar 30, 2022 •

edited

Loading

linux-foundation-easycla bot commented Apr 4, 2022 •

edited

Loading

linux-foundation-easycla bot commented May 6, 2022 •

edited

Loading

iskiselev May 6, 2022 •

edited

Loading