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.

Sign up for GitHub

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

Jump to bottom

Add safeio Package and Replace all io.ReadAll Invocations #22602

Merged
merged 4 commits into from Jan 12, 2023
Merged

Add safeio Package and Replace all io.ReadAll Invocations #22602

merged 4 commits into from Jan 12, 2023

Conversation

nathanjsweet
Copy link
Member

@nathanjsweet nathanjsweet commented Dec 7, 2022 •
edited

  • Add safeio Package to Replace ReadAll
  • Update customvet
  • Replace all io.ReadAll Invocations
  • Update CODEOWNERS file
internal-feature: We removed all instances of io.ReadAll to reduce the attack surface of possible DoS attacks.

@nathanjsweet nathanjsweet requested review from a team as code owners December 7, 2022 18:17
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 7, 2022
@nathanjsweet nathanjsweet added the release-note/misc This PR makes changes that have no direct user impact. label Dec 7, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 7, 2022
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/add-safeio-package branch 4 times, most recently from a965bc1 to 299c51a Compare December 7, 2022 21:58
@nathanjsweet nathanjsweet requested review from a team as code owners December 7, 2022 21:58
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/add-safeio-package branch from 299c51a to 6fdbed6 Compare December 7, 2022 22:10
bimmlerd
bimmlerd requested changes Dec 8, 2022
View reviewed changes
pkg/safeio/safeio.go Show resolved Hide resolved
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/add-safeio-package branch 2 times, most recently from ec75d7d to 97b96c9 Compare December 8, 2022 21:45
@christarazi christarazi mentioned this pull request Dec 9, 2022
Closed
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/add-safeio-package branch 2 times, most recently from 8f9962c to cdb30c0 Compare December 14, 2022 00:55
@christarazi christarazi added kind/enhancement This would improve or streamline existing functionality. area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels Dec 14, 2022
bimmlerd
bimmlerd approved these changes Dec 14, 2022
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

LGTM, nice work! 🚀

I don't have much context on the limits we set, but they seem reasonable :)

cilium/cmd/policy.go Outdated Show resolved Hide resolved
cilium/cmd/policy.go Outdated Show resolved Hide resolved
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/add-safeio-package branch from cdb30c0 to 74e8e29 Compare December 14, 2022 20:18
tommyp1ckles
tommyp1ckles approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@tommyp1ckles tommyp1ckles left a comment

Choose a reason for hiding this comment

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

Azure changes LGTM

aditighag
aditighag reviewed Dec 16, 2022
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

LGTM! The customvet linter should highlight new invocations of ReadAll, right? But I don't see it being called from a GH action that's run as part of the CI suite.

@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/add-safeio-package branch from 74e8e29 to c73312a Compare January 10, 2023 17:37
@nathanjsweet
Copy link
Member Author

@aditighag It's in the precheck

@nathanjsweet
Copy link
Member Author

nathanjsweet commented Jan 10, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sAgentPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: cannot install connectivity-check

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

@nathanjsweet
Add safeio Package to Replace ReadAll
a4144c9 
io.ReadAll is a dangerous method in a production
system as it can easily create a DoS vulnerability.
The safeio package creates a ReadAllLimit method
that attempts to read the entireity of an io.Reader
up to a limit.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
@nathanjsweet
Update customvet
73bcd27 
customvet now supports linting away
io.ReadAll invocations.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
@nathanjsweet
Replace all io.ReadAll Invocations
1be8791 
io.ReadAll is a potentially dangerous method invocation
in a production system. All instances of this method
are replaced with safeio.ReadAllLimit.

Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
@nathanjsweet
Update CODEOWNERS file for pkg/safeio
d9bb96b 
Signed-off-by: Nate Sweet <nathanjsweet@pm.me>
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/add-safeio-package branch from c73312a to d9bb96b Compare January 10, 2023 20:22
@nathanjsweet
Copy link
Member Author

/test

@nathanjsweet nathanjsweet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 11, 2023
@nathanjsweet
Copy link
Member Author

nathanjsweet commented Jan 11, 2023 •
edited

All checks have passed. If another rebase has to happen because of another go.mod conflict, I will not rerun the entire test suite. This PR is ready to merge.

@ti-mo ti-mo merged commit 88d2e2b into master Jan 12, 2023
@ti-mo ti-mo deleted the pr/nathanjsweet/add-safeio-package branch January 12, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@christarazi christarazi christarazi approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@aditighag aditighag aditighag approved these changes

@rolinh rolinh Awaiting requested review from rolinh rolinh was automatically assigned from cilium/vendor

Assignees
No one assigned
Labels
area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@nathanjsweet @pchaigno @christarazi @tommyp1ckles @bimmlerd @aditighag @ti-mo