feat: Add support for mutual TLS.

[megabus-tobin]

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.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: megabus-tobin / name: Tobin Richard (384a6a9)

[arielvalentin]

@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!

[megabus-tobin]

@arielvalentin I've split the small logging change off into #1557

Thanks!

[kaylareopelle]

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.

[megabus-tobin]

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.

The Net:HTTP class makes this a bit harder than it should be. Unlike ca_cert, the cert and key members aren't strings, they're OpenSSL::X509::Certificate and OpenSSL::PKey::RSA objects.

I'm not sure what the best approach would be. Should I add a test certificate and key to the repository or refactor OpenTelemetry::Exporter::OTLP::Exporter.initialize to optionally take the object directly and fallback to trying to load them from the environment?

The latter would be very similar to what OpenTelemetry::Exporter::OTLP::Exporter.prepare_endpoint already does. It would also have the small advantage of allowing for more exotic ways of obtaining the certificate and key but the utility of that is questionable since you need a file for the CA cert anyway.

[kaylareopelle]

The Net:HTTP class makes this a bit harder than it should be. Unlike ca_cert, the cert and key members aren't strings, they're OpenSSL::X509::Certificate and OpenSSL::PKey::RSA objects.

I'm not sure what the best approach would be. Should I add a test certificate and key to the repository or refactor OpenTelemetry::Exporter::OTLP::Exporter.initialize to optionally take the object directly and fallback to trying to load them from the environment?

The latter would be very similar to what OpenTelemetry::Exporter::OTLP::Exporter.prepare_endpoint already does. It would also have the small advantage of allowing for more exotic ways of obtaining the certificate and key but the utility of that is questionable since you need a file for the CA cert anyway.

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 OpenTelemetry::Exporter::OTLP::Exporter.prepare_endpoint, but like you mentioned, I'm not sure about the utility of that approach.

[github-actions]

👋 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 keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

[megabus-tobin]

@kaylareopelle I have updated the tests to check that the client certificate and key are handled correctly.

Some small notes:

1. To test that explicit TLS settings are preferred to environment variables I needed a second cert/key that would be loaded in the unlikely event the test was going to fail.
2. Initially I generated the cert/key on the fly but since a path is needed this would have made it necessary to ensure a writeable filesystem when running tests. Instead I have put the test certs/keys into the same directory as the tests and referenced that via __FILE__.
3. The certs/keys I generated are obviously meaningless. E.g. CN = a.example.tld, O = Internet Widgits Pty Ltd. By the time this gets reviewed they will probably also be expired, which the tests don't care about.
4. Rather than reading the certs/keys at the point of comparison, I've done it once near the top of the file. Apart from avoiding a little repetition this makes it much easier to spot if the files got removed, etc. rather than an individual test appearing to fail.

[github-actions]

👋 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 keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

[szechyjs]

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.

[megabus-tobin]

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?

[szechyjs]

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
)

https://github.com/open-telemetry/opentelemetry-ruby/blob/main/exporter/otlp-grpc/lib/opentelemetry/exporter/otlp/grpc/trace_exporter.rb#L34

[kaylareopelle]

@kaylareopelle I have updated the tests to check that the client certificate and key are handled correctly.

@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?