-
Notifications
You must be signed in to change notification settings - Fork 859
[OTLP] Add mTLS Support for OTLP Exporter #6343
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
Conversation
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsHttpClientFactory.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsHttpClientFactory.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMtlsOptions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Outdated
Show resolved
Hide resolved
bebeff9 to
4b2dc0b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6343 +/- ##
==========================================
- Coverage 86.88% 86.85% -0.03%
==========================================
Files 259 262 +3
Lines 12066 12414 +348
==========================================
+ Hits 10483 10782 +299
- Misses 1583 1632 +49
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…ostics Co-authored-by: Martin Costello <[email protected]>
…null reference Co-authored-by: Martin Costello <[email protected]>
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsHttpClientFactory.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsHttpClientFactory.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsHttpClientFactory.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMtlsOptions.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsHttpClientFactory.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsHttpClientFactory.cs
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsCertificateManager.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsHttpClientFactory.cs
Outdated
Show resolved
Hide resolved
|
@sandy2008 I have finished my review and provided feedback. This is almost done; once the feedback is addressed, we can plan to merge before the end of next week. |
… via a new event source entry
… server validation callback
@rajkumar-rangaraj Hi! All the comments are resolved, please take a look and merge it! |
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/OtlpMtlsHttpClientFactory.cs
Outdated
Show resolved
Hide resolved
rajkumar-rangaraj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM — left a minor comment on event source logging.
Thanks a lot for your patience and perseverance on this PR.
You’ve waited far too long for reviews, and every time we shared feedback you came back with thoughtful fixes and improvements. I really appreciate your persistence, responsiveness, and the quality of changes throughout this process.
Approved — thanks again for hanging in there and contributing to the project!
@rajkumar-rangaraj Thank you! Do you know when can we get it merged? |
Within a week, Thanks again for your contributions! |
Kielek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a chance that you have discussed it already, but some CHANGELOG entry can be beneficial.. If I missed the comment, feel free to merge as is.
|
|
||
| #if NET | ||
| // Apply mTLS configuration from environment variables | ||
| this.ApplyMtlsConfiguration(configuration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not want to block this PR, consider adding also support for OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE OTEL_EXPORTER_OTLP_METRICS_CERTIFICATE OTEL_EXPORTER_OTLP_LOGS_CERTIFICATE and other signal specific vars.
There is a chance that cover all cases will required breaking changes (signal specific OTLP options), but the best effort should be also possible.
I have seen at least one method guess which env. var. is appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend considering this as a follow-up, given the length of the PR. If the core part changes again, we'll need to review it from the beginning.
Thanks for catching it. @sandy2008 Could you please update CHANGELOG entry? |
Done! Thank you! |
alanwest
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple small comments. Thanks for your immense patience @sandy2008!
|
|
||
| | Environment variable | `OtlpMtlsOptions` property | Description | | ||
| | -------------------------------------------------| ----------------------------- | ------------------------------------- | | ||
| | `OTEL_EXPORTER_OTLP_CERTIFICATE` | `CaCertificatePath` | Path to CA certificate file (PEM) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OTEL_EXPORTER_OTLP_CERTIFICATE configuration option does not strictly apply to mTLS.
mTLS is an authentication system in which both the client and server authenticate each other.
The OTEL_EXPORTER_OTLP_CERTIFICATE option should be able to be used independently of an mTLS scenario. Common use case is where a server has a self-signed certificate that has not been verified by a third-party certificate authority. In this scenario you can use the OTEL_EXPORTER_OTLP_CERTIFICATE setting to enable the client to trust the certificate. This is not considered mTLS.
This is not a blocker for this PR since all this code is internal, but for clarity I'd suggest a small refactor reorganizing the code to decouple the the OTEL_EXPORTER_OTLP_CERTIFICATE from any classes with mTLS in their name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alanwest Thx! I will submit another PR once this one is merged :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracked under: #6764
|
Thank you for your contribution @sandy2008! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
Fixes #2009
Design discussion issue #5918
Changes
This PR adds support for mutual TLS (mTLS) configuration to the OTLP exporter on .NET 8 and later. This allows users to specify certificate paths, enable validation, and inject custom server certificate validation logic.
Key changes include:
OtlpMtlsOptionsclass with properties likeClientCertificatePath,ClientKeyPath,CaCertificatePath,EnableFilePermissionChecks, etc.OtlpMtlsHttpClientFactoryfor creatingHttpClientinstances configured for mTLS.OtlpMtlsCertificateManagerfor certificate loading, validation, and file permission checks.OpenTelemetryProtocolExporterEventSourcefor mTLS events.mTLS support is only enabled for builds targeting
NET8_0_OR_GREATER.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesLicense Information
THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE APACHE LICENSE V.2.0. YOU MAY OBTAIN A COPY OF THE LICENSE AT https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/LICENSE.TXT.
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.