Otlp grpc tls certificates #4731

dhhoang · 2023-07-29T01:27:18Z

Fixes #2009
Design discussion issue #2009

Changes

Implement the configurations according to specs:

Certificate file. OTEL_EXPORTER_OTLP_CERTIFICATE
Client key file. OTEL_EXPORTER_OTLP_CLIENT_KEY
Client certificate file. OTEL_EXPORTER_OTLP_CLIENT_CERTIFICATE

Notes

Some known issues:

When using HTTP/Protobuf protocol, the options only work for NET6_0_OR_GREATER targets. Older .NET lacks the required APIs to load PEM certificates. Implementing this would require essentially porting those APIs back from .NET 6. Given that HttpClientFactory can be used for HTTP/Protobuf I don't think it's worth the effort.
Testing: I'm not currently satisfied especially regarding GRPC flow. I'd love to have some end-to-end tests where the test case can communicate with a GRPC server to verify the mTLS functionalities. That's the only way to know whether the code works, because some of the options here passed to native C bindings (for GRPC in older .NET) and hence not possible to do any meaningful unit test. I'd like to hear recommendations on how these kinds of test could be done.

Merge requirement checklist

CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

linux-foundation-easycla · 2023-07-29T01:27:22Z

The committers listed above are authorized under a signed CLA.

✅ login: dhhoang / name: hoangh (3e773a1, 22e80db, bcb71f3, 48b1081, 4249037, a4bab30, 3250ab0, 0cdac9b, d8d2d70, 1183adb, 12014f0, 9a9cc91, b8064a6, 5f4346f, ef6e2b8)

alanwest · 2023-07-31T20:51:36Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

@@ -84,6 +88,21 @@ public OtlpExporterOptions()
            this.TimeoutMilliseconds = timeout;
        }

+        if (configuration.TryGetStringValue(ClientCertificateFileEnvVarName, out var clientCertificateFile))


It might make sense to add a TryGetFilePath method. Similar to the TryGetUriValue it could validate that the string is a file path for an existing file and log a message if it isn't.

opentelemetry-dotnet/src/Shared/Options/ConfigurationExtensions.cs

Lines 69 to 73 in 8f38400

if (!Uri.TryCreate(stringValue, UriKind.Absolute, out value))

{

LogInvalidEnvironmentVariable?.Invoke(key, stringValue!);

return false;

}

After trying this I think a TryGetFilePath() method doesn't make much sense here. The TryGet... methods simply check if the value is syntactically valid and not whether the resource actually exists (e.g. URI).
If the certificate file with specified path doesn't exist then TryGetFilePath() would return false and the option would be set to null and treated as if no option has been set by the developer.
I think from a developer PoV, if the file doesn't exist I'd prefer to be notified with a FileNotFoundException.

Syntactic validation is what I had in mind - using something like Path.GetInvalidPathChars or something. Though file paths may be tricky to validate syntactically in a meaningful/helpful way, so I'm fine with not doing anything more in this PR.

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

alanwest · 2023-07-31T21:01:58Z

...porter.OpenTelemetryProtocol.Tests/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests.csproj

@@ -22,6 +22,12 @@
    <PackageReference Include="Microsoft.Extensions.Http" />
  </ItemGroup>

+  <ItemGroup>
+    <Content Include="Certificates\**">


In #4732 I tried to avoid adding test certs/keys to the repo and generated them on the fly. See: https://github.com/open-telemetry/opentelemetry-dotnet/pull/4732/files#diff-5b358d0b47462dc9c3919270476736612be64b9db3362961d5df3d8b6defc257R4-R6.

The tests you have in this PR aren't run in docker of course, but I'm wondering if we can do something similar to avoid adding certs and keys.

One way to do it I guess would be to add an MSBuild Task to run the scripts to generate the certs (sh on Linux, Powershell on Windows).
Gonna take a crack at this.

Hey @dhhoang, apologies, I haven't had a chance yet to circle back and give this PR another pass. I will look at it more closely again tomorrow.

Though, quick thought about generating these certs, another idea may be to perform these tests using the integration test framework I landed in #4732. It's a minor bummer to have the tests you have so far depend on docker-compose to run them, but I figure you'll want to expand the integration tests anyways to test mTLS end to end.

It just occurred to me a readme for how to run the integration test locally might be useful. See #4741.

Giving this a try right now, I'll mark the PR as draft in the mean time.

alanwest · 2023-07-31T21:06:54Z

I'd love to have some end-to-end tests where the test case can communicate with a GRPC server to verify the mTLS functionalities.

💯 yes this would be great!

Once we can get #4732 landed, then I think you could expand on it and test mTLS. You could probably add an additional OTLP receiver with the necessary mTLS config. See: https://github.com/open-telemetry/opentelemetry-dotnet/pull/4732/files#diff-836987be2117c46a9d22eb1a86fe30cee3eee98299b46abceb7fcc6e9e84e1c9R14

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

…dd unit tests to verify exceptions

codecov · 2023-08-03T01:52:29Z

Codecov Report

Merging #4731 (3250ab0) into main (e294312) will decrease coverage by 0.16%.
Report is 11 commits behind head on main.
The diff coverage is 86.66%.

❗ Current head 3250ab0 differs from pull request most recent head ef6e2b8. Consider uploading reports for the commit ef6e2b8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4731      +/-   ##
==========================================
- Coverage   82.13%   81.97%   -0.16%     
==========================================
  Files         313      313              
  Lines       12783    12773      -10     
==========================================
- Hits        10499    10471      -28     
- Misses       2284     2302      +18

Files Changed	Coverage Δ
...TelemetryProtocol/OtlpExporterOptionsExtensions.cs	`92.52% <60.00%> (-4.29%)`	⬇️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs	`97.61% <100.00%> (+1.25%)`	⬆️

... and 33 files with indirect coverage changes

…ests

dhhoang · 2023-08-12T10:25:35Z

Finally got the time to work on this! I've finished the test cases:

Added scripts to generate test certificates for unit tests on Windows (pwsh) and Unix (bash)
Added the logic needed for integration test (Linux/Docker):
- Trusted certificates with TLS/mTLS.
- Untrusted certificates with custom trust store.
- Negative validation for cases above.
Manual testing on Windows for net6.0 net7.0 and net462 (i.e. gRPC using C binding).

Marking this PR as ready for review.

alanwest · 2023-08-14T20:53:47Z

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

+            handler.ServerCertificateCustomValidationCallback = (message, serverCert, chain, policyErrs) =>
+            {
+                chain.ChainPolicy.TrustMode = X509ChainTrustMode.CustomRootTrust;
+                chain.ChainPolicy.CustomTrustStore.Add(trustedCA);
+                var buildResult = chain.Build(serverCert);
+                return buildResult;
+            };


I'm not an expert on these things, so still need to do some of my own research to understand if this is sufficient.

One thing that leaves me a bit unsure here is that policyErrs is ignored. My understanding is the "out-of-the-box" validations would be reflected in policyErrs (e.g., is the cert signed by a trusted CA, is the cert expired, etc). I suspect we want the custom validation here to be in addition to the out-of-the-box validations.

You're correct that policyErrs would contain the error that the "default" validation method returns. The default validator would trust the certificates in the root trust store (aka certificates "installed" on the machine).
If user is manually trusting a certificate, it means the certificate might not be in the machine root store and we need to at least ignore that error.

The other validation criteria (e.g. expiration, revocation) is encoded in the X509Chain parameter. In this implementation I keep them as default settings (other than TrustMode of course). This means that other failure criteria would still work (e.g. the trusted cert still fails if it's expired).

alanwest · 2023-08-21T21:32:41Z

Hey @dhhoang, thanks for your patience. I've had limited time to give this PR a thorough review, so I appreciate you bearing with me 😄 - I am looking forward to getting this support landed!

Since this is your first PR to this repo, would you mind landing it in a few iterations? If you land a small PR first, then your "first-time contributor" status will go away and then on subsequent PRs the CI will just run automatically.

Here's a suggestion for breaking this PR up into multiple PRs:

First PR I really like what you're doing by generating the necessary certificates/keys via MSBuild. First PR could be to improve the existing integration test to use this new method. I like that it stores the certs in the build output directory. We should just be able to configure the docker compose volume to point to the output directory. Also, I think we could then do away with the create-cert container that spins up via docker compose since now you have the cert generated at build time.
Second PR mTLS support. This seems to be the more straight-forward piece to implement from the standpoint of the APIs used to add client certificates to the handler.
Third PR Server certificate support. I think the validation you're doing is pretty close to what we need, but I'm keen on splitting this into its own PR because I think we'll be able to solicit a review from a security expert at Microsoft. I've researched the validation a bit and found a few examples that I believe are closer to what we'll want. Here's just one example Add TrustedRootCertificates property to HttpClientHandler dotnet/runtime#39835 (comment).

github-actions · 2023-08-29T03:16:40Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2023-09-06T03:16:29Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2023-09-13T03:16:35Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

sandy2008 · 2024-09-09T02:56:27Z

@alanwest Hi! #5818 We made the first PR accordingly, would you mind taking a look?

dhhoang mentioned this pull request Jul 29, 2023

Add OTLP Exporter TLS/mTLS configuration options #2009

Open

dhhoang force-pushed the otlp-grpc-tls-certificates branch from fcb7ec6 to 53e4c43 Compare July 29, 2023 03:44

[OTLP] Implement client certificate options

3e773a1

dhhoang force-pushed the otlp-grpc-tls-certificates branch from 53e4c43 to 3e773a1 Compare July 29, 2023 04:02

dhhoang added 2 commits July 31, 2023 00:01

[OTLP] Attempt to load X509 PEM for old .NET

22e80db

[OTLP] Implement OTEL_EXPORTER_OTLP_CERTIFICATE option

bcb71f3

dhhoang marked this pull request as ready for review July 31, 2023 09:43

dhhoang requested a review from a team July 31, 2023 09:43

dhhoang added 2 commits July 31, 2023 21:35

[OTLP] Update Http/protobuf certificate test

48b1081

[OTLP] Remove certificate options for Http/Protobuf in old .NET

4249037

alanwest mentioned this pull request Jul 31, 2023

Add OTLP exporter TLS integration test #4732

Merged

alanwest reviewed Jul 31, 2023

View reviewed changes

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs Show resolved Hide resolved

dhhoang added 2 commits August 1, 2023 21:51

[OTLP] throws PlatformNotSupportedException on unsupported targets, a…

a4bab30

…dd unit tests to verify exceptions

[OTLP] Fix using directive code style error

3250ab0

dhhoang marked this pull request as draft August 4, 2023 15:16

dhhoang added 6 commits August 8, 2023 14:22

[OTLP] Pass certificate to GrpcChannelOptions.HttpHandler, add unit t…

0cdac9b

…ests

[OTLP] remove static test Certificates

d8d2d70

Merge branch 'main' into otlp-grpc-tls-certificates

1183adb

[OTLP] add mTLS receiver for otel collector for integration test

12014f0

[OTLP] Implement TLS certificates integration tests

9a9cc91

[OTLP] generate certificates for unit tests on Windows

b8064a6

dhhoang marked this pull request as ready for review August 12, 2023 10:26

Merge branch 'main' into otlp-grpc-tls-certificates

5f4346f

alanwest reviewed Aug 14, 2023

View reviewed changes

[OTLP] Fix 'Using directive is unnecessary' error when building test

ef6e2b8

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 29, 2023

alanwest removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Aug 30, 2023

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Sep 6, 2023

github-actions bot closed this Sep 13, 2023

reyang mentioned this pull request Apr 19, 2024

Configure GrpcChannel #5555

Closed

4 tasks

sandy2008 mentioned this pull request Sep 9, 2024

Improve Integration Test by Generating TLS/mTLS Certificates via MSBuild #5818

Merged

4 tasks

sandy2008 mentioned this pull request Oct 23, 2024

[otlp] Add mTLS Support for OTLP Exporter #5918

Draft

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Otlp grpc tls certificates #4731

Otlp grpc tls certificates #4731

dhhoang commented Jul 29, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jul 29, 2023 •

edited

Loading

alanwest Jul 31, 2023

dhhoang Aug 1, 2023

alanwest Aug 14, 2023

alanwest Jul 31, 2023

dhhoang Aug 1, 2023

alanwest Aug 4, 2023

alanwest Aug 4, 2023 •

edited

Loading

dhhoang Aug 4, 2023

alanwest commented Jul 31, 2023

codecov bot commented Aug 3, 2023 •

edited

Loading

dhhoang commented Aug 12, 2023

alanwest Aug 14, 2023

dhhoang Aug 15, 2023 •

edited

Loading

alanwest commented Aug 21, 2023

github-actions bot commented Aug 29, 2023

github-actions bot commented Sep 6, 2023

github-actions bot commented Sep 13, 2023

sandy2008 commented Sep 9, 2024

	if (!Uri.TryCreate(stringValue, UriKind.Absolute, out value))
	{
	LogInvalidEnvironmentVariable?.Invoke(key, stringValue!);
	return false;
	}

Otlp grpc tls certificates #4731

Otlp grpc tls certificates #4731

Conversation

dhhoang commented Jul 29, 2023 • edited Loading

Changes

Notes

Merge requirement checklist

linux-foundation-easycla bot commented Jul 29, 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

alanwest Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanwest commented Jul 31, 2023

codecov bot commented Aug 3, 2023 • edited Loading

Codecov Report

dhhoang commented Aug 12, 2023

Choose a reason for hiding this comment

dhhoang Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

alanwest commented Aug 21, 2023

github-actions bot commented Aug 29, 2023

github-actions bot commented Sep 6, 2023

github-actions bot commented Sep 13, 2023

sandy2008 commented Sep 9, 2024

dhhoang commented Jul 29, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jul 29, 2023 •

edited

Loading

alanwest Aug 4, 2023 •

edited

Loading

codecov bot commented Aug 3, 2023 •

edited

Loading

dhhoang Aug 15, 2023 •

edited

Loading