Update rp_filter only when /proc is write accessible #901
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mogren
reviewed
Apr 6, 2020
pkg/networkutils/network.go
Outdated
| if err != nil { | ||
| return errors.Wrapf(err, "failed to configure %s RPF check", primaryIntf) | ||
| if n.procSys.IsPathWriteAccessible(primaryIntfRPFilter) { | ||
| // Setting RPF will be removed from aws-node when we bump our major version. |
There was a problem hiding this comment.
Bump "major" or "minor" version? Do you mean v1.7.0?
There was a problem hiding this comment.
I meant when we do 1.7.0
anguslees
reviewed
Apr 7, 2020
| @@ -42,3 +45,8 @@ func (p *procSys) Get(key string) (string, error) { | |||
| func (p *procSys) Set(key, value string) error { | |||
| return ioutil.WriteFile(p.path(key), []byte(value), 0644) | |||
| } | |||
|
|
|||
| // IsPathWriteAccessible verifies if aws-node pod can write to the specified path | |||
| func (p *procSys) IsPathWriteAccessible(key string) bool { | |||
There was a problem hiding this comment.
This encourages TOCTOU (time of check is not time of use) race attacks btw. A more robust pattern is to just attempt the real Set and see if the error result is "permission denied" (os/error.IsPermission(err) in golang).
There was a problem hiding this comment.
oops looks like I missed this :/ Here TOCTOU is not possible because when aws-node starts up it will either have /proc accessible or not depending on privileged flag. It won't change during the execution. Please correct me if otherwise.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: #796
Description of changes:
This change is required to remove privileged permission for aws-node pods.
Once this is merged, in the next release we can remove privileged permission for aws-node and perform the operation through init container as described in the Issue link above.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.