Skip to content
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

Add Semantic conventions for TLS/SSL encrypted communication #1854

Closed
wants to merge 7 commits into from

Conversation

svrnm
Copy link
Member

@svrnm svrnm commented Aug 6, 2021

Fixes #1652

Changes

This is a follow up to #1652 providing a suggestion to include semantic conventions for TLS/SSL encrypted communication. It would add a lot of value to have those standardised, so there is a common way to find sources of TLS/SSL related issues.

@svrnm svrnm requested review from a team August 6, 2021 13:57
## Certificate attributes

These attributes may be used for any operation for details on the certificates.
Fingerprints and serial numbers MUST be provided in *hexadecimal colon notation*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify more. IPv6 also could be said to use hex colon notation, but uses 4 chars per block and lowercase.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Giving it another try:

Fingerprints and serial numbers MUST be provided in tuples of hexadecimal numbers separated by colon (:) with letters A-F in uppercase, e.g. 04:C8:04:4B:BB:F2:4E:2B:7A:37:25:91:64:00:54:95:91:2C.
This is a widely-used notation by CLI tools like openssl or browsers to display those certificate details.'

@Oberon00
Copy link
Member

This looks fine to me now, but since TLS is an intricate topic, I'd love to get some reviewers that have some domain knowledge.

@svrnm
Copy link
Member Author

svrnm commented Aug 10, 2021

This looks fine to me now, but since TLS is an intricate topic, I'd love to get some reviewers that have some domain knowledge.

Agreed, the intend of this PR is kicking off the discussion, but more domain expertise than mine is required for sure

@christian-schwarzbauer
Copy link

one more generic question, because I'm curious:
what would these "TLS/SSL client and server Spans" be exactly?
the point in time of establishing/terminating a TLS connection?

@Oberon00
Copy link
Member

Good point this should be clarified in this PR. IMHO this is a "general" semantic convention, i.e. there are no TLS/SSL spans, but you can e.g. enrich an HTTP span with TLS/SSL information. So I assume the duration of the span in practice would not include the SSL handshake. This is has to be this way also because of a design problem: If you started the span already at the SSL handshake, you would not be able to provide the parent span, which is required already at span start. The only pracical way to work around this would be by providing a custom start timestamp.

@svrnm
Copy link
Member Author

svrnm commented Aug 11, 2021

This issue/PR is coming from adding TLS/SSL instrumentation to the net instrumentation for Node.JS:

https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-net/src/instrumentation.ts#L116

It's possible to have a TLS span that's the parent of the encapsulated net (=http,...) span, although this might not be the best option: in the same sense one could have a TCP span, a DNS span, etc. for all the pieces until the application layer connection is established.

@Oberon00's suggestion to enrich a network span with those attributes (and a span event) might be a more viable solution. That's actually how browser resource timing APIs are doing it these days: https://www.w3.org/TR/resource-timing-2/#sec-performanceresourcetiming

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 19, 2021
@svrnm
Copy link
Member Author

svrnm commented Aug 19, 2021

I am currently not able to follow up. What’s the intended workflow to keep this open for a while? I still want to proceed if there is no strong reason not have this in the semantic conventions.

@Oberon00
Copy link
Member

Oberon00 commented Aug 19, 2021

I think just commenting is enough. I think converting to a draft PR should keep the stale bot from activating, but that may discourage feedback.

@github-actions github-actions bot removed the Stale label Aug 20, 2021
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 28, 2021
@github-actions
Copy link

github-actions bot commented Sep 4, 2021

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add semantic convention for describing SSL/TLS connections
4 participants