Skip to content

Add sysctl allowList#693

Merged
squeed merged 1 commit intocontainernetworking:masterfrom
mmirecki:POC_sysctl_whitelist
Mar 2, 2022
Merged

Add sysctl allowList#693
squeed merged 1 commit intocontainernetworking:masterfrom
mmirecki:POC_sysctl_whitelist

Conversation

@mmirecki
Copy link
Contributor

@mmirecki mmirecki commented Jan 20, 2022

The tuning-cni allows to set any sysctl on the created container. The admin might however want to restrict the user to a predefined whitelist of sysctls.
This PR is a POC with a proposal on how this could be implemented.

Tuning-cni would try to read a whitelist file from /tuningwhitelist/sysctlwhitelist (NOTE: temporary location only), and check the incoming sysctls agains it. Cni would return error if an unathorized sysctl is detected.
The tuning-cni would behaves as before is the file is missing.

The POC is meant to only show the concept, the actual implementation will probably differ.

@s1061123
Copy link
Contributor

s1061123 commented Jan 20, 2022

Hi @mmirecki It seems to be good feature for tuning/sysctl, especially Kubernetes with multus.

  • Regarding whitelist/blacklist config file, current PR assumes that the same path of tuning however, I suppose user want to put it in different path from executable binary.
  • The path of whitelist/blacklist could be configurable, I suppose. Of course, we could have default path for that, e.g. /etc/cni/tuning/whitelist.conf or some.
  • How about to use regexp for the sysctl list, instead of simple list of sysctl?

@mmirecki mmirecki force-pushed the POC_sysctl_whitelist branch from a1a1cf9 to 87b11d0 Compare January 21, 2022 13:27
@mmirecki
Copy link
Contributor Author

mmirecki commented Jan 21, 2022 •
edited
Loading

Hi @mmirecki It seems to be good feature for tuning/sysctl, especially Kubernetes with multus.

  • Regarding whitelist/blacklist config file, current PR assumes that the same path of tuning however, I suppose user want to put it in different path from executable binary.

/etc/cni/tuning/whitelist.conf is not the default

  • The path of whitelist/blacklist could be configurable, I suppose. Of course, we could have default path for that, e.g. /etc/cni/tuning/whitelist.conf or some.

I've set this as the default one. I'll add an override when suggestions appear.

  • How about to use regexp for the sysctl list, instead of simple list of sysctl?

Done

@mmirecki mmirecki force-pushed the POC_sysctl_whitelist branch 2 times, most recently from f28219d to 330db60 Compare January 24, 2022 13:31
@mmirecki mmirecki force-pushed the POC_sysctl_whitelist branch from 330db60 to 274984f Compare February 1, 2022 15:24
@mmirecki mmirecki changed the title DRAFT: Add sysctl whitelist Add sysctl whitelist Feb 1, 2022
@mmirecki mmirecki marked this pull request as ready for review February 1, 2022 15:24
s1061123
s1061123 reviewed Feb 4, 2022
View reviewed changes
Copy link
Contributor

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

As far as I looked, the code seems good to me. I recommend unit-test for the code, in https://github.com/containernetworking/plugins/blob/master/plugins/meta/tuning/tuning_test.go

@mmirecki mmirecki force-pushed the POC_sysctl_whitelist branch 3 times, most recently from 7ef6b59 to 05cfe66 Compare February 9, 2022 11:55
@mmirecki
Copy link
Contributor Author

mmirecki commented Feb 9, 2022

As far as I looked, the code seems good to me. I recommend unit-test for the code, in https://github.com/containernetworking/plugins/blob/master/plugins/meta/tuning/tuning_test.go

@s1061123 , added tests, replied to other comments
Thanks

@cgoncalves
Copy link

cgoncalves commented Feb 9, 2022

Instead of whitelist, consider allow list, permit list or other variations.

@dcbw
Copy link
Member

dcbw commented Feb 9, 2022

@mmirecki I agree with @cgoncalves ; would you mind whitelist -> allowlist?

@mmirecki mmirecki force-pushed the POC_sysctl_whitelist branch from 05cfe66 to a45a457 Compare February 9, 2022 19:47
@mmirecki
Copy link
Contributor Author

mmirecki commented Feb 9, 2022

@mmirecki I agree with @cgoncalves ; would you mind whitelist -> allowlist?

-> allowlist

s1061123
s1061123 reviewed Feb 24, 2022
View reviewed changes
@mmirecki mmirecki force-pushed the POC_sysctl_whitelist branch from a45a457 to 8820efb Compare February 24, 2022 14:40
@mmirecki mmirecki changed the title Add sysctl whitelist Add sysctl allowList Feb 24, 2022
Add sysctl allowlist
96c3af8 
Signed-off-by: mmirecki <[email protected]>
@mmirecki mmirecki force-pushed the POC_sysctl_whitelist branch from 8820efb to 96c3af8 Compare February 24, 2022 14:41
s1061123
s1061123 approved these changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thank you for the changes!

@dcbw
Copy link
Member

dcbw commented Mar 2, 2022

/lgtm

@squeed
Copy link
Member

squeed commented Mar 2, 2022

Looks good to me. When you have a moment, can you file a PR in https://github.com/containernetworking/cni.dev/ documenting this? Thanks

@squeed squeed merged commit 3512b10 into containernetworking:master Mar 2, 2022
@mmirecki
Copy link
Contributor Author

mmirecki commented Mar 3, 2022

Looks good to me. When you have a moment, can you file a PR in https://github.com/containernetworking/cni.dev/ documenting this? Thanks

@squeed Here's the doc pr: containernetworking/cni.dev#99

@mmirecki mmirecki mentioned this pull request Mar 23, 2022
Closed
This was referenced Jan 28, 2023
Merged
Merged
@Luap99 Luap99 mentioned this pull request Aug 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@s1061123 s1061123 s1061123 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mmirecki @s1061123 @cgoncalves @dcbw @squeed