Skip to content

Conversation

@vardhaman22
Copy link
Contributor

No description provided.

also set privileged false for the pod
@vardhaman22 vardhaman22 requested review from a team as code owners April 2, 2025 04:03
Comment on lines +263 to +264
if path == "/" {
return fmt.Errorf("root path '/' is not allowed")
Copy link
Member

Choose a reason for hiding this comment

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

Not quite sure we support running it on Windows nodes, either way this looks a bit more future proof:

Suggested change
if path == "/" {
return fmt.Errorf("root path '/' is not allowed")
if path == "/" || path == "\" {
return fmt.Errorf("root path is not allowed")

hostPathSet := make(map[string]bool)

for _, path := range hostPaths {
if !filepath.IsAbs(path) {
Copy link
Member

Choose a reason for hiding this comment

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

This only checks whether the path is rooted, and therefore would return true for /abc/.../bin. We must call filepath.Clean(path) to ensure that becomes /bin before we do all the checks we are doing here.

Suggested change
if !filepath.IsAbs(path) {
path = filepath.Clean(path)
if !filepath.IsAbs(path) {

}

for _, protected := range protectedDirs {
if path == protected || strings.HasPrefix(path, protected+"/") {
Copy link
Member

Choose a reason for hiding this comment

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

This will probably not work very well if executed in Windows. Perhaps string(filepath.Separator) could be used instead. Either way, it would be good to have unit tests showing all the inputs and outputs we have considered.

Suggested change
if path == protected || strings.HasPrefix(path, protected+"/") {
if path == protected || strings.HasPrefix(path, protected+"/") {

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.

2 participants