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

spec: clarify allowed characters in ContainerID #679

Closed
dcbw opened this issue Jun 6, 2019 · 7 comments · Fixed by #698
Closed

spec: clarify allowed characters in ContainerID #679

dcbw opened this issue Jun 6, 2019 · 7 comments · Fixed by #698
Assignees

Comments

@dcbw
Copy link
Member

dcbw commented Jun 6, 2019

The spec doesn't say anything about the characters allowed in a ContainerID. We probably shouldn't allow filesystem path characters like "/" or ":", or a ContainerID of just "." or ".." since the ID is used by some plugins in cache file names.

@jellonek
Copy link
Member

jellonek commented Jun 7, 2019

Shouldn't that then be sanitized on plugins level, before they would use that on filesystem? That reason for trimming available character set in this case seems to be invalid to me.

Still, if you would proceed with that, I would suggest to use https://golang.org/pkg/os/#PathSeparator instead of hardcoding "/" there.

@dcbw
Copy link
Member Author

dcbw commented Jun 12, 2019

@bboreham suggests that we should limit the length of ContainerID too.

@dcbw
Copy link
Member Author

dcbw commented Jun 12, 2019

We have decided to add filesystem-safe ContainerID escaping functions to containernetworking/cni/pkg/utils/ (in a new file there) because any sane plugin should do that regardless of restrictions on ContainerID content, plus it doesn't seem like a huge win to get all pedantic on ContainerID content.

@jellonek
Copy link
Member

Having a common/reusable way to sanitize the thing is a very good idea.

@dcbw
Copy link
Member Author

dcbw commented Jun 19, 2019

And this time around, we've decided to restrict allowed characters in ContainerID and enforce that restriction in libcni and skel.

@mccv1r0
Copy link
Member

mccv1r0 commented Jun 19, 2019

IIRC also for networkName

@dcbw
Copy link
Member Author

dcbw commented Jun 20, 2019

IIRC also for networkName

@mccv1r0 agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants