Add configuration for client TLS auth and rename variable for trusted… #1375

anuraaga · 2021-01-26T03:17:56Z

… cert of server.

Changes

Adds variables for configuring client TLS auth and rename / clarify the current configuration for a server's trusted certificate.

… cert of server.

iNikem · 2021-01-26T08:11:07Z

Changelog?

anuraaga · 2021-01-26T08:11:57Z

@iNikem Yeah generally add it after some approvals since until then it just causes daily merge conflicts :P

specification/protocol/exporter.md

arminru · 2021-01-26T08:56:41Z

specification/protocol/exporter.md

+/ Client certificate / key | The TLS certificate and private key to authenticate the client for mTLS. Should only be used for a secure connection.. | n/a | `OTLP_EXPORTER_OTLP_TLS_CERTIFICATE` `OTLP_EXPORTER_OTLP_SPAN_TLS_CERTIFICATE` `OTLP_EXPORTER_OTLP_METRIC_TLS_CERTIFICATE`, `OTLP_EXPORTER_OTLP_TLS_PRIVATE_KEY` `OTLP_EXPORTER_OTLP_SPAN_TLS_PRIVATE_KEY` `OTLP_EXPORTER_OTLP_METRIC_TLS_PRIVATE_KEY` |
+| Trusted certificate     | The trusted certificate to use when verifying a server's TLS credentials. Should only be used for a secure connection. | n/a               | `OTEL_EXPORTER_OTLP_TLS_TRUSTED_CERTIFICATE` `OTEL_EXPORTER_OTLP_SPAN_TLS_TRUSTED_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRIC_TLS_TRUSTED_CERTIFICATE` |


As per #1362 this should all be _TRACE_ instead of _SPAN_. Please don't forget updating the PR that gets merged last.

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

tigrannajaryan · 2021-01-26T15:18:47Z

Lets use TRACES instead of SPAN as it is proposed by #1362

SergeyKanzhelev · 2021-01-26T16:09:27Z

specification/protocol/exporter.md

@@ -10,7 +10,8 @@ The following configuration options MUST be available to configure the OTLP expo
 | -------------------- | ------------------------------------------------------------ | ----------------- | ------------------------------------------------------------ |
 | Endpoint             | Target to which the exporter is going to send spans or metrics. The endpoint MUST be a valid URL with scheme (http or https) and host, and MAY contain a port and path. A scheme of https indicates a secure connection. When using `OTEL_EXPORTER_ENDPOINT` with OTLP/HTTP, exporters SHOULD follow the collector convention of appending the version and signal to the path (e.g. `v1/traces` or `v1/metrics`). The per-signal endpoint configuration options take precedence and can be used to override this behavior. See the [OTLP Specification][otlphttp-req] for more details. | `https://localhost:4317` | `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_SPAN_ENDPOINT` `OTEL_EXPORTER_OTLP_METRIC_ENDPOINT` |
 | Protocol             | The protocol used to transmit the data. One of `grpc`,`http/json`,`http/protobuf`. | `grpc`               | `OTEL_EXPORTER_OTLP_PROTOCOL` `OTEL_EXPORTER_OTLP_SPAN_PROTOCOL` `OTEL_EXPORTER_OTLP_METRIC_PROTOCOL` |
-| Certificate File     | Path to certificate file for TLS credentials of gRPC client. Should only be used for a secure connection. | n/a               | `OTEL_EXPORTER_OTLP_CERTIFICATE` `OTEL_EXPORTER_OTLP_SPAN_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRIC_CERTIFICATE` |
+| Client certificate / key | The TLS certificate and private key to authenticate the client for mTLS. Should only be used for a secure connection. | n/a | `OTEL_EXPORTER_OTLP_TLS_CERTIFICATE` `OTEL_EXPORTER_OTLP_SPAN_TLS_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRIC_TLS_CERTIFICATE`, `OTEL_EXPORTER_OTLP_TLS_PRIVATE_KEY` `OTEL_EXPORTER_OTLP_SPAN_TLS_PRIVATE_KEY` `OTEL_EXPORTER_OTLP_METRIC_TLS_PRIVATE_KEY` |


Found a usage of this variable, example here: https://github.com/open-telemetry/opentelemetry-python/blob/db74594d93b7e42d26ed2061b7672d1ad823f30c/exporter/opentelemetry-exporter-otlp/tests/test_otlp_metric_exporter.py#L74 Can you please make sure to check that this is fine to change

That would be renamed to OTEL_EXPORTER_OTLP_METRIC_TLS_TRUSTED_CERTIFICATE I think. Is that what you'd like to confirm?

Could we still allow the old Environment variable but mark it deprecated with a migration?

Given how CLOSE we are to 1.0 I think it'd be good if we pretend we have users we don't want to break, to get in the habit of how these changes will have to be made going forward.

If languages want to support multiple variables I think they could, even without it being mentioned in the spec right?

My comment was about checking with python maintainers whether it is already heavily in use. As @jsuereth pointed we need to be careful with renaming. If not much adoption so far, PR good with me.

Well, there aren't any open source usage

https://github.com/search?q=OTEL_EXPORTER_OTLP_METRIC_CERTIFICATE&type=code

And if we were done making changes, we'd already be 1.0 - this doc doesn't have any label on it, not even feature-freeze like some of the other ones so it seems a bit unfair to require too much process. The variable seems to be a bug really since the description doesn't describe it, these are credentials of a server, not a client.

Languages can use a deprecation cycle to change the variable if they wish, but I don't think that's something to have here, or if we did, it would be a much more general document detailing a mandate for and how to deprecate features.

Not asking for much process. Maybe just a ack from @open-telemetry/python-approvers

Ok - @open-telemetry/python-approvers does this change look ok? Thanks!

…ication into client-tls

anuraaga · 2021-02-02T05:11:41Z

Any more updates needed on this?

anuraaga · 2021-02-04T00:18:39Z

@codeboten @ocelotl @lzchen Could you take a look at this PR? Thanks!

ocelotl · 2021-02-04T00:34:05Z

Sure, will do this tonight 👍

codeboten

This looks fine to me, not sure if there is a heavy use of this variable just yet.

ocelotl · 2021-02-04T05:10:45Z

specification/protocol/exporter.md

@@ -10,7 +10,8 @@ The following configuration options MUST be available to configure the OTLP expo
 | -------------------- | ------------------------------------------------------------ | ----------------- | ------------------------------------------------------------ |
 | Endpoint             | Target to which the exporter is going to send spans or metrics. The endpoint MUST be a valid URL with scheme (http or https) and host, and MAY contain a port and path. A scheme of https indicates a secure connection. When using `OTEL_EXPORTER_ENDPOINT` with OTLP/HTTP, exporters SHOULD follow the collector convention of appending the version and signal to the path (e.g. `v1/traces` or `v1/metrics`). The per-signal endpoint configuration options take precedence and can be used to override this behavior. See the [OTLP Specification][otlphttp-req] for more details. | `https://localhost:4317` | `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT` `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT` |
 | Protocol             | The protocol used to transmit the data. One of `grpc`,`http/json`,`http/protobuf`. | `grpc`               | `OTEL_EXPORTER_OTLP_PROTOCOL` `OTEL_EXPORTER_OTLP_TRACES_PROTOCOL` `OTEL_EXPORTER_OTLP_METRICS_PROTOCOL` |
-| Certificate File     | Path to certificate file for TLS credentials of gRPC client. Should only be used for a secure connection. | n/a               | `OTEL_EXPORTER_OTLP_CERTIFICATE` `OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRICS_CERTIFICATE` |
+| Client certificate / key | The TLS certificate and private key to authenticate the client for mTLS. Should only be used for a secure connection. | n/a | `OTEL_EXPORTER_OTLP_TLS_CERTIFICATE` `OTEL_EXPORTER_OTLP_TRACES_TLS_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRICS_TLS_CERTIFICATE`, `OTEL_EXPORTER_OTLP_TLS_PRIVATE_KEY` `OTEL_EXPORTER_OTLP_TRACES_TLS_PRIVATE_KEY` `OTEL_EXPORTER_OTLP_METRICS_TLS_PRIVATE_KEY` |


Just a question, mostly out of curiosity. I see a replacing of Path to certificate file... for The TLS certificate and private key.... The idea behind that is to use the contents of the certificate as the value of the environment variable?

Thanks for calling this out. This was tough for me since this table has both configuration options and environment variables. I don't think the SDK has to expose a programmatic interface that only accepts a path, and often it'd probably not a good idea since it makes it difficult to provide credentials in non-file ways such as embedding into the binary or using a KMS client. The environment variable makes sense to point to a path though. So I made the description generic to not specify the type to apply to both.

Perhaps we should move the environment variables out of here to the sdk-environment-variables doc since they seem somewhat mysterious to be defined here and would give us the flexibility when describing. That'd be for another PR though I think.

carlosalberto · 2021-02-04T16:47:46Z

@jsuereth Any opinion on this one? 😄

…ication into client-tls

The current version of the specification makes me think that Certificate File is intended to be used as part of the client authentication, which it is not. This change addresses part of the confusion discussed in open-telemetry#1363, the certificate file option and environment variable points to the certificate used to verify the server's certificate. See also open-telemetry#1375.

* clarify meaning of "Certificate File" The current version of the specification makes me think that Certificate File is intended to be used as part of the client authentication, which it is not. This change addresses part of the confusion discussed in #1363, the certificate file option and environment variable points to the certificate used to verify the server's certificate. See also #1375. * update changelog * Update CHANGELOG.md Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>

* clarify meaning of "Certificate File" The current version of the specification makes me think that Certificate File is intended to be used as part of the client authentication, which it is not. This change addresses part of the confusion discussed in open-telemetry/opentelemetry-specification#1363, the certificate file option and environment variable points to the certificate used to verify the server's certificate. See also open-telemetry/opentelemetry-specification#1375. * update changelog * Update CHANGELOG.md Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>

Add configuration for client TLS auth and rename variable for trusted…

a51be18

… cert of server.

anuraaga requested review from a team January 26, 2021 03:17

github-actions bot assigned SergeyKanzhelev Jan 26, 2021

iNikem approved these changes Jan 26, 2021

View reviewed changes

arminru reviewed Jan 26, 2021

View reviewed changes

Update specification/protocol/exporter.md

e3a228c

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

SergeyKanzhelev reviewed Jan 26, 2021

View reviewed changes

Anuraag Agrawal added 2 commits January 27, 2021 16:38

New names

e5c1f7f

Merge branch 'client-tls' of github.com:anuraaga/opentelemetry-specif…

438e536

…ication into client-tls

Base automatically changed from master to main January 27, 2021 21:16

Merge branch 'main' of github.com:open-telemetry/opentelemetry-specif…

24d9fa0

…ication into client-tls

codeboten approved these changes Feb 4, 2021

View reviewed changes

ocelotl reviewed Feb 4, 2021

View reviewed changes

ocelotl approved these changes Feb 4, 2021

View reviewed changes

lzchen approved these changes Feb 4, 2021

View reviewed changes

cijothomas mentioned this pull request Feb 5, 2021

OpenTelemetry.Exporter.OpenTelemetryProtocol support open-telemetry/opentelemetry-dotnet#1791

Closed

Anuraag Agrawal added 2 commits February 5, 2021 10:25

Merge branch 'main' of github.com:open-telemetry/opentelemetry-specif…

9caecbb

…ication into client-tls

Update changelog

e831e39

alanwest mentioned this pull request Feb 5, 2021

OtlpExporter: Integration test testing https support open-telemetry/opentelemetry-dotnet#1795

Closed

anuraaga closed this Feb 7, 2021

anuraaga mentioned this pull request Feb 21, 2021

Add additional env variables for Jaeger exporter #1453

Closed

codeboten mentioned this pull request Jul 7, 2021

clarify meaning of Certificate File #1803

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration for client TLS auth and rename variable for trusted… #1375

Add configuration for client TLS auth and rename variable for trusted… #1375

anuraaga commented Jan 26, 2021

iNikem commented Jan 26, 2021

anuraaga commented Jan 26, 2021

arminru Jan 26, 2021

tigrannajaryan commented Jan 26, 2021

SergeyKanzhelev Jan 26, 2021

anuraaga Jan 28, 2021

jsuereth Jan 29, 2021

anuraaga Jan 30, 2021

SergeyKanzhelev Feb 2, 2021

anuraaga Feb 2, 2021

SergeyKanzhelev Feb 2, 2021

anuraaga Feb 2, 2021

anuraaga commented Feb 2, 2021

anuraaga commented Feb 4, 2021

ocelotl commented Feb 4, 2021

codeboten left a comment

ocelotl Feb 4, 2021

anuraaga Feb 4, 2021

anuraaga Feb 4, 2021

carlosalberto commented Feb 4, 2021 •

edited

Loading

		/ Client certificate / key \| The TLS certificate and private key to authenticate the client for mTLS. Should only be used for a secure connection.. \| n/a \| `OTLP_EXPORTER_OTLP_TLS_CERTIFICATE` `OTLP_EXPORTER_OTLP_SPAN_TLS_CERTIFICATE` `OTLP_EXPORTER_OTLP_METRIC_TLS_CERTIFICATE`, `OTLP_EXPORTER_OTLP_TLS_PRIVATE_KEY` `OTLP_EXPORTER_OTLP_SPAN_TLS_PRIVATE_KEY` `OTLP_EXPORTER_OTLP_METRIC_TLS_PRIVATE_KEY` \|
		\| Trusted certificate \| The trusted certificate to use when verifying a server's TLS credentials. Should only be used for a secure connection. \| n/a \| `OTEL_EXPORTER_OTLP_TLS_TRUSTED_CERTIFICATE` `OTEL_EXPORTER_OTLP_SPAN_TLS_TRUSTED_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRIC_TLS_TRUSTED_CERTIFICATE` \|

Add configuration for client TLS auth and rename variable for trusted… #1375

Add configuration for client TLS auth and rename variable for trusted… #1375

Conversation

anuraaga commented Jan 26, 2021

Changes

iNikem commented Jan 26, 2021

anuraaga commented Jan 26, 2021

Choose a reason for hiding this comment

tigrannajaryan commented Jan 26, 2021

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

anuraaga commented Feb 2, 2021

anuraaga commented Feb 4, 2021

ocelotl commented Feb 4, 2021

codeboten 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

carlosalberto commented Feb 4, 2021 • edited Loading

carlosalberto commented Feb 4, 2021 •

edited

Loading