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

validate: Check that the given namespace path is a symlink #1221

Merged
merged 1 commit into from
Dec 11, 2016

Conversation

sameo
Copy link
Contributor

@sameo sameo commented Dec 10, 2016

When checking if the provided networking namespace is the host
one or not, we should first check if it's a symbolic link or not
as in some cases we can use persistent networking namespace under
e.g. /var/run/netns/.

Signed-off-by: Samuel Ortiz sameo@linux.intel.com

@mrunalp
Copy link
Contributor

mrunalp commented Dec 10, 2016

LGTM

Approved with PullApprove

return false, nil
}

return true, nil
Copy link
Member

Choose a reason for hiding this comment

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

This should be written as all one-line (no if statement):

return fi.Mode()&os.ModeSymlink == os.ModeSymlink, nil

// it is not the host namespace.
return nil
}

// readlink on the path provided in the struct
destOfContainer, err := os.Readlink(path)
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused what benefit this new code has -- this will always error out if path is not a symlink.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cyphar If path is not symlink, i.e. a persistent namespace, it will not error out.
It will only error out if path is a symlink and point to the same namespace as /proc/self/ns/net, which is considered to be the host namespace.

In other words, this routine will error out if path is not pointing at a persistent namespace and if it's a symbolic link pointing at the host namespace.

Copy link
Member

@cyphar cyphar Dec 10, 2016

Choose a reason for hiding this comment

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

My point is that readlink will always error out if you try to read a non-symlink:

% touch /tmp/uts-ns
% sudo unshare --uts=/tmp/uts-ns hostname FOO
% sudo readlink /tmp/uts-ns
% echo $?
1

EDIT: Oops, I though you were returning an error when it wasn't a symlink. My bad.

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, I just re-read your code and I understand what you mean! Personally I'd prefer if you did something like check that the *LinkError from Readlink is EINVAL. But this works.

When checking if the provided networking namespace is the host
one or not, we should first check if it's a symbolic link or not
as in some cases we can use persistent networking namespace under
e.g. /var/run/netns/.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
@cyphar
Copy link
Member

cyphar commented Dec 10, 2016

LGTM. Sorry about that confusion. 😸

Approved with PullApprove

@mrunalp
Copy link
Contributor

mrunalp commented Dec 11, 2016

LGTM

Approved with PullApprove

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.

3 participants