Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

tests: switch to using SysProcAttr for non-root #1569

Merged
merged 3 commits into from
Oct 8, 2015
Merged

Conversation

jonboulle
Copy link
Contributor

Evidently the LockOSThread approach is a disaster in go. However, the
os/exec.Command struct does expose some ability to control the execution
parameters via the SysProcAttr - and in particular the Credentials
struct which (in principle) allows us to safely set the uid/gid for the
execed rkt process.

@@ -18,7 +18,11 @@ func Start(c *exec.Cmd) (pty *os.File, err error) {
c.Stdout = tty
c.Stdin = tty
c.Stderr = tty
c.SysProcAttr = &syscall.SysProcAttr{Setctty: true, Setsid: true}
if c.SysProcAttr == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yifan-gu
Copy link
Contributor

yifan-gu commented Oct 8, 2015

Waiting #1571

@jonboulle
Copy link
Contributor Author

I'm really missing why the permissions aren't getting propagated correctly.

@yifan-gu
Copy link
Contributor

yifan-gu commented Oct 8, 2015

I'm really missing why the permissions aren't getting propagated correctly.

Do you mean the db/sha512/ba/ now has 0777? I think we should pass PathPerm/FilePerm to the diskv.
Also let's set the defaultPathPerm to 0770, and here

BTW, we should not let the such consts scattered everywhere.

@jonboulle
Copy link
Contributor Author

Ready for review.

@yifan-gu yifan-gu changed the title [WIP] tests: switch to using SysProcAttr for non-root tests: switch to using SysProcAttr for non-root Oct 8, 2015
@yifan-gu
Copy link
Contributor

yifan-gu commented Oct 8, 2015

@jonboulle Thanks for the fix, LGTM.
btw can you uncomment these two lines to enable the test cases as the related prs should be merged now
https://github.com/coreos/rkt/pull/1569/files#diff-b8f7293584997a92e048ae9e5f8078a2L52
https://github.com/coreos/rkt/pull/1569/files#diff-b8f7293584997a92e048ae9e5f8078a2L68

@jonboulle
Copy link
Contributor Author

@yifan-gu go to sleep :)

- Use umask(0) in store write operations to ensure permissions are
  propagated appropriately
- Pass defaultPathPerm/defaultFilePerm through to disk
- Remove unused const in rkt package
Evidently the LockOSThread approach is a disaster in go. However, the
os/exec.Command struct does expose some ability to control the execution
parameters via the SysProcAttr - and in particular the Credentials
struct which (in principle) allows us to safely set the uid/gid for the
execed rkt process.

// TODO(jonboulle): non-root user breaks trying to read root-written
// config directories. Should be a better way to approach this. Should
// config directories be readable by the rkt group too?
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be readable by the rkt group. If we add the --auth option mentioned in #1568 we can leave /etc/rkt/auth.d as a place where shared credentials live and use --auth for per-user credentials.

We can leave this here in the meantime.

@iaguis
Copy link
Member

iaguis commented Oct 8, 2015

LGTM except the issue mentioned aboved. Will be fixed on a follow-up

iaguis added a commit that referenced this pull request Oct 8, 2015
tests: switch to using SysProcAttr for non-root
@iaguis iaguis merged commit f87cfdc into rkt:master Oct 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants