-
Notifications
You must be signed in to change notification settings - Fork 239
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
feat: Add support for mutual TLS. #1556
Conversation
@megabus-tobin Thank you for your contribution! Please consider separating the error handling changes in a separate PR. We will be able to review and release that change independent of verifying the correctness of the changes required for mTLS support. Happy Holidays! |
a848635
to
2ef502a
Compare
@arielvalentin I've split the small logging change off into #1557 Thanks! |
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.
Thank you, @megabus-tobin!
For anyone who's interested, the specs for these env vars can be found here: https://opentelemetry.io/docs/specs/otel/protocol/exporter/
Could you incorporate the new environment variables into the exporter tests? (here's the file for the OTLP library)
Anywhere the certificate_file
is mentioned may be a good place to add content for the client certificate and key.
2ef502a
to
2e40941
Compare
The I'm not sure what the best approach would be. Should I add a test certificate and key to the repository or refactor The latter would be very similar to what |
Sorry for my delated response. Great questions! I think adding the test certificate and key seems the least intrusive to what you've already built if that's not too much effort. If other approvers/maintainers disagree, please chime in! I like your idea about following the pattern in |
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
e84e75c
to
384a6a9
Compare
@kaylareopelle I have updated the tests to check that the client certificate and key are handled correctly. Some small notes:
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
http = Net::HTTP.new(uri.host, uri.port) | ||
http.use_ssl = uri.scheme == 'https' | ||
http.verify_mode = ssl_verify_mode | ||
http.ca_file = certificate_file unless certificate_file.nil? | ||
http.cert = OpenSSL::X509::Certificate.new(File.read(client_certificate_file)) unless client_certificate_file.nil? |
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.
Especially when running in containers, and using environment variables, it would be nice to be able to load the PEM encoded cert/key directly from environment variables, instead of having to find a way to write it to a file first.
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 agree but at least one file will still be needed for many/most users because it's assigned to Net::HTTP#ca_file
which can only be a path. Should I make the change to support either for the client cert and key despite that?
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 think that would make it inconsistent with other SDK implementations which all use file paths.
I think that if we have a request to do this we may want to steer folks in a direction to generate a file via a configMap or equivalent.
Would be nice to update the gRPC exporter as well. creds = if mtls_config_provided
GRPC::Core::ChannelCredentials.new(ca, key, cert)
else
:this_channel_is_insecure
end
@client = Opentelemetry::Proto::Collector::Trace::V1::TraceService::Stub.new(
"#{uri.host}:#{uri.port}",
creds
) |
@megabus-tobin, thank you for updating the tests! I think this is a solid approach. Are you planning to make the GRPC changes mentioned here as part of this PR? |
Add support for specifying client certificate and key for mutual TLS when exporting.
I have made these changes now. I've only performed minimal testing on this as it's not something I've needed to use previously and am not familiar with it. Unlike the HTTP exporter, there are almost no existing tests included in the library. |
| `headers:` | `OTEL_EXPORTER_OTLP_HEADERS` | | | ||
| `compression:` | `OTEL_EXPORTER_OTLP_COMPRESSION` | `"gzip"` | | ||
| `timeout:` | `OTEL_EXPORTER_OTLP_TIMEOUT` | `10` | | ||
| `ssl_verify_mode:` | `OTEL_RUBY_EXPORTER_OTLP_SSL_VERIFY_PEER` or | `OpenSSL::SSL:VERIFY_PEER` | |
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.
is there a constraint that we would like to put on disabling peer verification when using mTLS?
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've found it useful to still be able to disable host verification when testing as the certs don't always match the servers. As they're often just localhost or were determined by an overly dynamic DNS.
In our internal use case the certificate verification is sometimes only actually useful from the server's perspective where we have a publicly exposed collector behind a reverse proxy that verifies the client using a certificate we know it must already have. Admittedly in that case we do know hostname matches but in our earlier iterations of the setup it didn't and caused problem with some of the OTEL SDKs for other languages.
We chatted about this PR in the SIG last week. A question was raised about whether other languages also have this feature. The environment variables are referenced in the protocol exporter specification. They were added in this PR: open-telemetry/opentelemetry-specification#2370 The cc @mwear |
It's available in the Java SDK too but is configured differently (because Java). See https://opentelemetry.io/docs/languages/java/configuration/ and in particular the |
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.
Thanks for your work on this!
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. @robertlaurin would you mind taking a look at this when you get a chance?
Add support for specifying client certificate and key for mutual TLS when exporting.
Also log more information about why an SSL error occured as this can be very difficult to diagnose. This is logged in the same way as other probably fatal issues are, e.g. HTTP 404s.