feat(trace-otlp-grpc): configure security with env vars #2827

svetlanabrennan · 2022-03-10T16:18:33Z

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Partially Fixes #2706 (issue)

Currently the transport security for the gRPC exporter can be set either programmatically or the default insecure transport is used. This PR adds insecure environment variables and certificate file environment variables that related to security configuration.

Short description of the changes

Added "insecure" and "certificate file" configuration env vars.
Added configureSecurity function that sets up the transport security configuration.
Updated default url from localhost:4317 to http://localhost:4317 as discussed during the SIG.
Removed grpc(s) scheme and tests related to checking `grpc(s) scheme.

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Unit Tests

Checklist:

Followed the style guidelines of this project
Unit tests have been added
Documentation has been updated

TODO:

Clarify how security should be configured in certain cases (scheme in endpoint, default security)
Clarify using OTEL_EXPORTER_OTLP_SPAN_INSECURE as part of configureSecurity function.

Signed-off-by: Svetlana Brennan <[email protected]>

codecov · 2022-03-10T16:24:04Z

Codecov Report

Merging #2827 (11df1a1) into main (22085fc) will decrease coverage by 0.00%.
The diff coverage is 92.72%.

@@            Coverage Diff             @@
##             main    #2827      +/-   ##
==========================================
- Coverage   92.52%   92.51%   -0.01%     
==========================================
  Files         183      183              
  Lines        5980     6029      +49     
  Branches     1268     1283      +15     
==========================================
+ Hits         5533     5578      +45     
- Misses        447      451       +4

Impacted Files	Coverage Δ
...ckages/opentelemetry-core/src/utils/environment.ts	`96.00% <ø> (ø)`
...ental/packages/otlp-grpc-exporter-base/src/util.ts	`78.26% <92.45%> (+11.07%)`	⬆️
.../exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts	`100.00% <100.00%> (ø)`
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts	`100.00% <100.00%> (ø)`

svetlanabrennan · 2022-03-10T17:03:39Z

This PR doesn't break anything when it comes to setting up transport security.

I would like some feedback on the following items:

Spec says that the default "insecure" should be "false" which means a secure transport channel. The current default implementation (before this pr) is "insecure". Should we leave this as it or change it to match the spec that it should be secure as the default (which means it'll be a breaking change)?
Spec says that a scheme of https indicates a secure connection and takes precedence over the "insecure" config settings. The current implementation (before this pr) doesn't take the scheme in the endpoint into consideration at all. Should we add logic that takes scheme in endpoint into consideration in order to determine security. Here are some current use cases with the scheme endpoint of the current implementation (before this pr and continued with this pr):

A. using all defaults - no endpoint specified, no channel credentials specified => returns insecure channel
Note: spec says we should create secure channel but current implementation creates insecure

B. endpoint = localhost:4317, no channel credentials specified => returns insecure channel
Note: follows spec (no scheme means insecure channel)

C. endpoint = http://localhost:4317, no channel credentials specified => returns insecure channel
Note: follows spec (http scheme means insecure channel)

D. endpoint = https://localhost:4317, no channel credentials specified => returns insecure channel
Note: spec says this should be a secure channel but current implementation creates insecure

E. endpoint = grpc://localhost:4317, no channel credentials specified => returns insecure channel
Note: follows spec (http scheme means insecure channel)

F. endpoint = grpcs://localhost:4317, no channel credentials specified => returns insecure channel
Note: spec says this should be a secure channel but current implementation creates insecure

G. endpoint = https://localhost:4317, channel credentials (either secure or insecure) specified => return 
same `channelCredentials`
Note: there's no details from the spec on what to do in this case but I think that if a user specifically 
created channel credentials programmatically (either insecure or secure) that we should use those 
exact credentials no matter what scheme the endpoint contains. Also this is what is happening with 
the current implementation (before this pr and continued with this pr) - we use the security credentials 
set by the user.

Spec mentions that OTEL_EXPORTER_OTLP_SPAN_INSECURE is obsolete but they are still supported because of a stable release of the spec. Should we read this OTEL_EXPORTER_OTLP_SPAN_INSECURE env var in my configureSecurity function?

dyladan · 2022-03-17T09:45:39Z

This PR doesn't break anything when it comes to setting up transport security.

I would like some feedback on the following items:

Spec says that the default "insecure" should be "false" which means a secure transport channel. The current default implementation (before this pr) is "insecure". Should we leave this as it or change it to match the spec that it should be secure as the default (which means it'll be a breaking change)?

Spec says that a scheme of https indicates a secure connection and takes precedence over the "insecure" config settings. The current implementation (before this pr) doesn't take the scheme in the endpoint into consideration at all. Should we add logic that takes scheme in endpoint into consideration in order to determine security. Here are some current use cases with the scheme endpoint of the current implementation (before this pr and continued with this pr):
A. using all defaults - no endpoint specified, no channel credentials specified => returns insecure channel
Note: spec says we should create secure channel but current implementation creates insecure

B. endpoint = localhost:4317, no channel credentials specified => returns insecure channel
Note: follows spec (no scheme means insecure channel)

C. endpoint = http://localhost:4317, no channel credentials specified => returns insecure channel
Note: follows spec (http scheme means insecure channel)

D. endpoint = https://localhost:4317, no channel credentials specified => returns insecure channel
Note: spec says this should be a secure channel but current implementation creates insecure

E. endpoint = grpc://localhost:4317, no channel credentials specified => returns insecure channel
Note: follows spec (http scheme means insecure channel)

F. endpoint = grpcs://localhost:4317, no channel credentials specified => returns insecure channel
Note: spec says this should be a secure channel but current implementation creates insecure

G. endpoint = https://localhost:4317, channel credentials (either secure or insecure) specified => return 
same `channelCredentials`
Note: there's no details from the spec on what to do in this case but I think that if a user specifically 
created channel credentials programmatically (either insecure or secure) that we should use those 
exact credentials no matter what scheme the endpoint contains. Also this is what is happening with 
the current implementation (before this pr and continued with this pr) - we use the security credentials 
set by the user. 
Spec mentions that OTEL_EXPORTER_OTLP_SPAN_INSECURE is obsolete but they are still supported because of a stable release of the spec. Should we read this OTEL_EXPORTER_OTLP_SPAN_INSECURE env var in my configureSecurity function?

Discussed in SIG yesterday and spent a little more time reading the spec today. The spec states "The endpoint SHOULD accept any form allowed by the underlying gRPC client implementation" and also only specifies https scheme as overriding the secure config. Given that, I think we should respect the insecure config in all cases (including secure by default) except when the scheme is https where the spec specifically says we should be secure. The only thing I'm not sure about is default behavior if nothing is configured because a secure connection to localhost is likely to fail certificate validation. I have opened open-telemetry/opentelemetry-specification#2420 for this question.

dyladan · 2022-03-28T15:54:26Z

@svetlanabrennan what's the current status here?

svetlanabrennan · 2022-03-28T19:23:48Z

@svetlanabrennan what's the current status here?

I just need confirmations on these use cases before I push up my changes to this pr:

if user wants to use all defaults, return secure channel
anytime user provides their own credentials, use those same credentials (no matter the scheme or env insecure settings)
if user sets https scheme return secure channel (ignoring insecure env settings)
if user sets http scheme and no other items (like providing credentials or env insecure settings) return insecure channel
if user sets no scheme and no other items (like providing credentials or env insecure settings) return secure channel
if user sets http scheme and env insecure = false, return secure channel
if user sets http scheme and env insecure = true, return insecure channel
if user sets no scheme and env insecure = false, return secure channel
if user sets no scheme and env insecure = true, return insecure channel

Do you (and anyone else) agree with this configuration?

Signed-off-by: Svetlana Brennan <[email protected]>

dyladan · 2022-04-06T15:11:57Z

if user wants to use all defaults, return secure channel

According to @tigrannajaryan open-telemetry/opentelemetry-specification#2420 it should be insecure to localhost. Tigran agrees spec should be clarified.

anytime user provides their own credentials, use those same credentials (no matter the scheme or env insecure settings)

This seems reasonable

if user sets https scheme return secure channel (ignoring insecure env settings)

Yes

if user sets http scheme and no other items (like providing credentials or env insecure settings) return insecure channel

This is not as clear. I'll ask Tigran. The spec says insecure is false by default, but http implies insecure.

if user sets no scheme and no other items (like providing credentials or env insecure settings) return secure channel

Yes

if user sets http scheme and env insecure = false, return secure channel

Yes. I think this will also be the answer for "user sets http scheme and no other items" but we'll wait for Tigran.

if user sets http scheme and env insecure = true, return insecure channel

Yes

if user sets no scheme and env insecure = false, return secure channel

Yes

if user sets no scheme and env insecure = true, return insecure channel

Yes

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan · 2022-04-06T23:01:21Z

Thank you for that feedback @dyladan. I'll wait for clarification on use case 4 and 6.

Note: this pr is also held up by the two lerna configurations issue.

dyladan · 2022-04-07T12:59:46Z

if user wants to use all defaults, return secure channel

According to @tigrannajaryan open-telemetry/opentelemetry-specification#2420 it should be insecure to localhost. Tigran agrees spec should be clarified.

Tigran has clarified that the intention of the spec was for the default to be an insecure localhost connection.

if user sets http scheme and no other items (like providing credentials or env insecure settings) return insecure channel

This is not as clear. I'll ask Tigran. The spec says insecure is false by default, but http implies insecure.

Tigran's comment here clarifies that endpoint with http scheme overrides insecure setting. Insecure setting is only used if no scheme is provided.

if user sets http scheme and env insecure = false, return secure channel

Yes. I think this will also be the answer for "user sets http scheme and no other items" but we'll wait for Tigran.

The same clarification above also applies to this. The insecure config is only used if a scheme is not provided.

…use cases Signed-off-by: Svetlana Brennan <[email protected]>

…telemetry-js into add-insecure-config

svetlanabrennan · 2022-04-07T15:50:09Z

The same clarification above also applies to this. The insecure config is only used if a scheme is not provided.

Perfect. Thanks for the clarification.

Signed-off-by: Svetlana Brennan <[email protected]>

dyladan · 2022-05-09T20:55:38Z

Sorry for the delay on review here. i've got this on my list for the morning

dyladan

Thanks for doing the significant research and spec legwork on this. Sorry it took so long to get reviewed.

Signed-off-by: Svetlana Brennan <[email protected]>

legendecas

This is great work, Thank you! LGTM % nits.

experimental/CHANGELOG.md

legendecas · 2022-05-16T15:14:20Z

experimental/packages/exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts

@@ -55,7 +54,7 @@ export class OTLPTraceExporter
        ? validateAndNormalizeUrl(getEnv().OTEL_EXPORTER_OTLP_TRACES_ENDPOINT)
        : getEnv().OTEL_EXPORTER_OTLP_ENDPOINT.length > 0
          ? validateAndNormalizeUrl(getEnv().OTEL_EXPORTER_OTLP_ENDPOINT)
-          : DEFAULT_COLLECTOR_URL;
+          : validateAndNormalizeUrl(DEFAULT_COLLECTOR_URL);


Do we need to validate a hardcoded default URL?

Yes because the DEFAULT_COLLECTOR_URL didn't have a scheme so we didn't need to pass it to validateAndNormalizeUrl like we do when user adds a url to config.url or when using env vars.

Now that I updated DEFAULT_COLLECTOR_URL to have an http scheme (like we discussed in the sig so it matches the spec) so we need to pass it to validateAndNormalizeUrl to remove the scheme.

legendecas · 2022-05-16T15:14:43Z

experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/src/OTLPMetricExporter.ts

@@ -59,7 +57,7 @@ class OTLPMetricExporterProxy extends OTLPGRPCExporterNodeBase<ResourceMetrics,
        ? validateAndNormalizeUrl(getEnv().OTEL_EXPORTER_OTLP_METRICS_ENDPOINT)
        : getEnv().OTEL_EXPORTER_OTLP_ENDPOINT.length > 0
          ? validateAndNormalizeUrl(getEnv().OTEL_EXPORTER_OTLP_ENDPOINT)
-          : DEFAULT_COLLECTOR_URL;
+          : validateAndNormalizeUrl(DEFAULT_COLLECTOR_URL);


Same answer as this one

experimental/packages/otlp-grpc-exporter-base/src/util.ts

Signed-off-by: Svetlana Brennan <[email protected]>

Co-authored-by: Chengzhong Wu <[email protected]>

Fixes #2420 ## Changes - Clarify the insecure option for gRPC exporters Related issues - open-telemetry/opentelemetry-js#2827

svetlanabrennan added 7 commits March 1, 2022 15:52

feat(trace-otlp-grpc): add insecure configs

a614e9e

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): add unit tests

5de22f2

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): wip add certificate and tests

abb85b7

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): fix security config and unit tests

25ff0a7

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): add env and certificate tests

ff0c262

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): wip certificate tests

10ac242

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): fix lint error

b8f6b79

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan requested a review from a team March 10, 2022 16:18

Merge branch 'main' into add-insecure-config

ec98d2e

Merge branch 'main' into add-insecure-config

5d1cc75

feat(trace-otlp-grpc): merge main to add-insecure-config

703ff36

Signed-off-by: Svetlana Brennan <[email protected]>

svetlanabrennan mentioned this pull request Apr 5, 2022

REQUEST: New membership for svetlanabrennan open-telemetry/community#1019

Closed

6 tasks

svetlanabrennan and others added 3 commits April 5, 2022 13:26

Merge branch 'main' into add-insecure-config

3c124fe

feat(trace-otlp-grpc): wip add additional security setting checks

fd39a9a

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): update default url to http scheme

cee07d3

Signed-off-by: Svetlana Brennan <[email protected]>

dyladan mentioned this pull request Apr 6, 2022

Clarify TLS/Secure default behavior open-telemetry/opentelemetry-specification#2420

Closed

svetlanabrennan added 2 commits April 6, 2022 17:37

feat(trace-otlp-grpc): wip add tests

1eba925

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): wip refactor function around insecure setting

66c9471

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'main' into add-insecure-config

dcba873

svetlanabrennan added 2 commits April 7, 2022 10:48

feat(trace-otlp-grpc): wip update returned security setting for some …

87ca75c

…use cases Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'add-insecure-config' of github.com:svetlanabrennan/open…

1fb546e

…telemetry-js into add-insecure-config

svetlanabrennan and others added 12 commits April 21, 2022 12:03

feat(trace-otlp-grpc): fix changelog and get security from env func

bb8dfaa

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'main' into add-insecure-config

ba6b11f

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): wip troubleshoot

16c62b7

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): fix readme

899d478

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'main' into add-insecure-config

6126f5d

Merge branch 'main' into add-insecure-config

941937d

feat(trace-otlp-grpc): refactor code and fix lint

5b3b351

Signed-off-by: Svetlana Brennan <[email protected]>

feat(trace-otlp-grpc): remove grpc scheme and grpc scheme tests

748db96

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'main' into add-insecure-config

79e0ae4

Merge branch 'main' into add-insecure-config

30bdd55

Merge branch 'main' into add-insecure-config

b99dfa2

Merge branch 'main' into add-insecure-config

4f275e7

svetlanabrennan and others added 2 commits May 10, 2022 16:39

Merge branch 'main' into add-insecure-config

0828912

Merge branch 'main' into add-insecure-config

2182c5e

dyladan approved these changes May 11, 2022

View reviewed changes

svetlanabrennan and others added 4 commits May 11, 2022 11:27

Merge branch 'main' into add-insecure-config

ab12999

Merge branch 'main' into add-insecure-config

9289b9e

feat(trace-otlp-grpc): fix credentials for failing test afer main merge

2156472

Signed-off-by: Svetlana Brennan <[email protected]>

Merge branch 'main' into add-insecure-config

39e3126

legendecas reviewed May 16, 2022

View reviewed changes

svetlanabrennan and others added 3 commits May 16, 2022 15:29

feat(trace-otlp-grpc): move changelog to unreleased section

4ae7510

Signed-off-by: Svetlana Brennan <[email protected]>

Use exact match for protocol check to avoid leaking cases like httpx

531f19a

Co-authored-by: Chengzhong Wu <[email protected]>

Merge branch 'main' into add-insecure-config

11df1a1

legendecas approved these changes May 17, 2022

View reviewed changes

legendecas merged commit a9c59da into open-telemetry:main May 17, 2022

pichlermarc mentioned this pull request Jun 3, 2022

fix(grpc-exporter): use non-normalized URL to determine channel security #3019

Merged

9 tasks

ChengJinbao added a commit to ChengJinbao/opentelemetry-specification that referenced this pull request Nov 16, 2022

Clarify gRPC insecure option (#2476)

d9506eb

Fixes #2420 ## Changes - Clarify the insecure option for gRPC exporters Related issues - open-telemetry/opentelemetry-js#2827

tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-proto that referenced this pull request Apr 20, 2023

Clarify gRPC insecure option (#2476)

4385da5

Fixes #2420 ## Changes - Clarify the insecure option for gRPC exporters Related issues - open-telemetry/opentelemetry-js#2827

joaopgrassi pushed a commit to dynatrace-oss-contrib/semantic-conventions that referenced this pull request Mar 21, 2024

Clarify gRPC insecure option (#2476)

92fb1f5

Fixes #2420 ## Changes - Clarify the insecure option for gRPC exporters Related issues - open-telemetry/opentelemetry-js#2827

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(trace-otlp-grpc): configure security with env vars #2827

feat(trace-otlp-grpc): configure security with env vars #2827

svetlanabrennan commented Mar 10, 2022 •

edited

Loading

codecov bot commented Mar 10, 2022 •

edited

Loading

svetlanabrennan commented Mar 10, 2022 •

edited

Loading

dyladan commented Mar 17, 2022

dyladan commented Mar 28, 2022

svetlanabrennan commented Mar 28, 2022

dyladan commented Apr 6, 2022

svetlanabrennan commented Apr 6, 2022

dyladan commented Apr 7, 2022

svetlanabrennan commented Apr 7, 2022

dyladan commented May 9, 2022

dyladan left a comment

legendecas left a comment

legendecas May 16, 2022

svetlanabrennan May 16, 2022

legendecas May 16, 2022

svetlanabrennan May 16, 2022

feat(trace-otlp-grpc): configure security with env vars #2827

feat(trace-otlp-grpc): configure security with env vars #2827

Conversation

svetlanabrennan commented Mar 10, 2022 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

Checklist:

TODO:

codecov bot commented Mar 10, 2022 • edited Loading

Codecov Report

svetlanabrennan commented Mar 10, 2022 • edited Loading

dyladan commented Mar 17, 2022

dyladan commented Mar 28, 2022

svetlanabrennan commented Mar 28, 2022

dyladan commented Apr 6, 2022

svetlanabrennan commented Apr 6, 2022

dyladan commented Apr 7, 2022

svetlanabrennan commented Apr 7, 2022

dyladan commented May 9, 2022

dyladan left a comment

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

legendecas May 16, 2022

Choose a reason for hiding this comment

svetlanabrennan May 16, 2022

Choose a reason for hiding this comment

legendecas May 16, 2022

Choose a reason for hiding this comment

svetlanabrennan May 16, 2022

Choose a reason for hiding this comment

svetlanabrennan commented Mar 10, 2022 •

edited

Loading

codecov bot commented Mar 10, 2022 •

edited

Loading

svetlanabrennan commented Mar 10, 2022 •

edited

Loading