Skip to content

Conversation

@crosbymichael
Copy link
Member

Signed-off-by: Michael Crosby [email protected]

}
fifoName := filepath.Join(containerRoot, execFifoFilename)
oldMask := syscall.Umask(0000)
if err := syscall.Mkfifo(fifoName, 0666); err != nil {
Copy link
Member

@cyphar cyphar Jun 14, 2016

Choose a reason for hiding this comment

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

Doesn't this mean that any user can start a container created by root? This doesn't make sense to me IMO -- what's the usecase? With the rootless container setup, my hope was that each user's containers could only be controlled by them (with root being a special case) -- this means that any user will be able to start any other users' containers. Why is this necessary? That's why I liked the signal setup, because it retained that access control.

Sure, at the moment the only access control is "starting a container if it's been set up already". But I can imagine cases where people might not want an unprivileged user to start a container before they've run all of their hooks.

My plan with the rootless container setup is for everything to be per-user, rather than a global free-for-all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change this to 0622 so that only root can read and unblock the container's process

Copy link
Member

@cyphar cyphar Jun 14, 2016

Choose a reason for hiding this comment

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

Since we chowning the FIFO, surely we can just make it 0600? Why do other users need to be able to write to the FIFO?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the uid at that point basically 0,0 ?

Meaning that, in a userns context, the user at the time of writing to the fifo would be matched against "other".

Copy link
Member

Choose a reason for hiding this comment

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

No, because we chown the FIFO when creating the container (it's the next line after this). If we didn't chown it, then we wouldn't have any guarantee that the UID would be mapped (and thus nobody in the container could read from the socket).

Signed-off-by: Michael Crosby <[email protected]>
@crosbymichael
Copy link
Member Author

@LK4D4 @mrunalp can you please look at this PR. It fixes permissions errors with the fifo under userns and non root users in regular containers on some machines with umask issues

@mrunalp
Copy link
Contributor

mrunalp commented Jun 15, 2016

@crosbymichael Yeah, will review it shortly.

@mrunalp
Copy link
Contributor

mrunalp commented Jun 15, 2016

LGTM

Approved with PullApprove

1 similar comment
@LK4D4
Copy link
Contributor

LK4D4 commented Jun 15, 2016

LGTM

Approved with PullApprove

@LK4D4 LK4D4 merged commit cc29e3d into opencontainers:master Jun 15, 2016
@crosbymichael crosbymichael deleted the fifo-userns branch June 15, 2016 20:01
@crosbymichael crosbymichael restored the fifo-userns branch June 15, 2016 20:28
@cyphar
Copy link
Member

cyphar commented Jun 15, 2016

@crosbymichael I still had some open concerns about the permissions:

Since we chowning the FIFO, surely we can just make it 0600? Why do other users need to be able to write to the FIFO?

No [ we don't need to allow other users to write ], because we chown the FIFO when creating the container (it's the next line after this). If we didn't chown it, then we wouldn't have any guarantee that the UID would be mapped (and thus nobody in the container could read from the socket).

@crosbymichael crosbymichael deleted the fifo-userns branch July 25, 2016 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants