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 an initial security document #2461

Merged
merged 3 commits into from
Feb 13, 2021

Conversation

Capture relevant security information and considerations.

Attempts to address
open-telemetry/opentelemetry-collector-contrib#2232.
@flands flands requested a review from a team February 11, 2021 03:39
@flands
Copy link
Contributor Author

flands commented Feb 11, 2021

@alolita @mx-psi make an attempt at this -- feedback welcome

@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #2461 (f801fbc) into main (9de6dd7) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2461      +/-   ##
==========================================
+ Coverage   91.76%   91.79%   +0.02%     
==========================================
  Files         265      265              
  Lines       15111    15111              
==========================================
+ Hits        13867    13871       +4     
+ Misses        866      863       -3     
+ Partials      378      377       -1     
Impacted Files Coverage Δ
exporter/otlpexporter/otlp.go 74.35% <0.00%> (+2.56%) ⬆️
exporter/exporterhelper/metricshelper.go 100.00% <0.00%> (+5.40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9de6dd7...f801fbc. Read the comment docs.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Great initial version! Just need to clarify a couple of points, but looks good!

docs/security.md Outdated Show resolved Hide resolved
docs/security.md Outdated Show resolved Hide resolved
docs/security.md Outdated Show resolved Hide resolved
docs/security.md Show resolved Hide resolved
docs/security.md Outdated Show resolved Hide resolved
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

This looks good! My main concern is that this document might work very well for Collector developers but it might be a bit hard to read as docs for end-users: the doc sometimes reads like a spec, describing the desired behavior of the Collector but not how does one ensure that that behavior is met.

I left a couple of comments for some links that could be included to help end-users reading this meet that behavior, but I am undecided on whether they fit on this doc or not.

docs/security.md Outdated Show resolved Hide resolved
docs/security.md Outdated Show resolved Hide resolved
- Add TL;DR
- Add both end-user and component developer security information
- Link to examples or more information
- Link open issues
@flands
Copy link
Contributor Author

flands commented Feb 11, 2021

@jpkrohling @mx-psi @ericmustin thanks for the feedback! I attempted to incorporate all of it - please take another look and let me know what you think.

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

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

lgtm for the areas i had feedback on, looks like there's a test flaking but i don't think that's specific to this pr

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of minor comments.

docs/security.md Show resolved Hide resolved
docs/security.md Show resolved Hide resolved
docs/security.md Outdated Show resolved Hide resolved
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Looks good after adressing @jpkrohling comments. Thanks for doing this!

flands added a commit to flands/opentelemetry-collector that referenced this pull request Feb 13, 2021
Move contributing doc within Getting Involved link.

Depends on open-telemetry#2461
@bogdandrutu bogdandrutu merged commit 05d7f38 into open-telemetry:main Feb 13, 2021
@flands flands deleted the flands/security branch February 13, 2021 21:43
flands added a commit to flands/opentelemetry-collector that referenced this pull request Feb 13, 2021
Move contributing doc within Getting Involved link.

Depends on open-telemetry#2461
bogdandrutu pushed a commit that referenced this pull request Feb 14, 2021
Move contributing doc within Getting Involved link.

Depends on #2461
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
The .submit-request job has been renamed to
.submit-signing-request. The job fails when referring
to the old name, so this updates the GitLab CI yaml
file to use the new name.
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants