Cleanup namespace validation #3723
Conversation
| return fmt.Errorf("namespace name cannot contain any special characters (%q): %s", pathSeparators, nsName) | ||
| } | ||
|
|
||
| specialAliases := []string{".", "..", "~"} |
There was a problem hiding this comment.
This check should be retained
There was a problem hiding this comment.
. and .. are retained (
nerdctl/pkg/store/filestore_unix.go
Line 29 in a3decf8
Tilde is no longer, as it is not a filesystem restriction / alias. However, as mentioned, containerd will enforce ^[A-Za-z0-9]+(?:[._-](?:[A-Za-z0-9]+))*$ on namespace creation, so, tilde is still out.
| @@ -239,6 +240,9 @@ Config file ($NERDCTL_TOML): %s | |||
| return fmt.Errorf("invalid cgroup-manager %q (supported values: \"systemd\", \"cgroupfs\", \"none\")", cgroupManager) | |||
| } | |||
| } | |||
| if err = store.ValidatePathComponent(globalOptions.Namespace); err != nil { | |||
There was a problem hiding this comment.
nit:
can we add a comment stating something like namespace should be a single valid path component.
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#path-segment-names
There was a problem hiding this comment.
Sure. Let me do that real quick.
apostasie
force-pushed
the
namespace-validate
branch
from
a3decf8 to
690ae13
Compare
Signed-off-by: apostasie <[email protected]>
apostasie
force-pushed
the
namespace-validate
branch
from
690ae13 to
b8f4d9c
Compare
|
Thanks @djdongjin Latest push addresses your comment, and has been rebased to account for newly added unit tests. |
|
|
Cleanup namespace validation
Cleanup namespace validation
Cleanup namespace validation
Cleanup namespace validation
Cleanup namespace validation
Currently, namespace name is only validated in certain circumstances (eg: if we are going to access a container statedir). This may lead to inconsistencies, where we would allow forbidden namespaces in certain scenarios but properly restrict them in other scenarios.
Also, the current set of rules for validation are inadequate, and fail to cover a range of conditions that will fail on the filesystem.
So, suggesting:
This is introducing changes. Specifically, it will restrict previously allowed namespaces:
con[0-9], etc)And on the other hand, relax restrictions on certain patterns that were previously forbidden for no apparent reason (tilde, etc).
Most importantly though, note that containerd will anyhow identifier-validate the namespace name whenever it touches it... with the much more restrictive
^[A-Za-z0-9]+(?:[._-](?:[A-Za-z0-9]+))*$- so, this is merely about cleaning things up on our side.Let me know what you think.