stage1,tests: add support for non-numerical uid/gid by monstermunchkin · Pull Request #2159 · rkt/rkt

monstermunchkin · 2016-02-08T22:31:44Z

Support for non-numerical uid/gid added, as specified by appc.

Resolves #1919

alban · 2016-02-09T11:57:43Z

+	pexit_if((f = fopen(path, "r")) == NULL, "Unable to open \"%s\"", path);
+
+	while (getline(&line, &llen, f) != -1) {
+		s = strdup(line);


shouldn't s be free()'d when done?

alban · 2016-02-09T12:17:53Z

Thanks for this work!

All the parsing is done in C via appexec.c and not in Go via stage1/init/common/pod.go:appToSystemd. I wonder why this choice. It might be easier to do in Go because:

There is already related code for that in group.go (but it might change in rkt should not use /etc/group directly #2105 so not sure if it is a good idea to use that)
When the file is missing in the ACI, the error is produced late, after the pod has been started rather than before. It means it cannot use the structured error formatting (Make errors structured #1937 /cc @blixtra ) and the errors are less user-friendly. For example, instead of Cannot stat "/opt/stage2/rkt-inspect/rootfs/etc/nofile", I would prefer to see something like "The ACI image xyz uses group /foo/xyz but file /foo/xyz does not exist in the ACI" or similar.

What do you think?

monstermunchkin · 2016-02-09T14:20:05Z

I just thought I'd validate shortly before entering the chroot. Other than that, there was no particular reason I chose C over Go.

Thanks for pointing out group.go, I didn't know about that code. If the ability to parse /etc/group-like files doesn't get removed completely, I could use it. It might also be useful having similar code which parses /etc/passwd-like files. Perhaps in pkg/passwd/passwd.go?

Structured and user-friendly errors are a good point. I'll be moving the validation to Go then.

alban · 2016-02-09T14:33:31Z

@monstermunchkin not sure what form it should take. I added a note in #2105.

monstermunchkin · 2016-02-11T20:03:27Z

So, I have moved the validation to stage1/init/common/pod.go:appToSystemd. The group package can now also read from a specified file, and the passwd package has been added which does the same, only for passwd-like files.

Please let me know what you think about the code.

alban · 2016-02-16T13:00:15Z

+	if strings.HasPrefix(app.User, "/") {
+		var stat syscall.Stat_t
+		if err = syscall.Lstat(strings.Join([]string{common.AppRootfsPath(p.Root, appName),
+			app.User}, "/"), &stat); err != nil {


Why do you append a "/" at the end of the filename for lstat?

ditto for the groups below.

This should be filepath.Join(common.AppRootfsPath(p.Root, appName), app.User).

Oh right, I read filepath.Join instead of strings.Join. I agree that using filepath.Join is better in that case.

monstermunchkin · 2016-02-18T17:52:46Z

Thanks for the feedback, I did some changes.

When using path-like uid/gid, UnshiftRange is called, which is now part of uid/uid.go. The function prototype is basically the same as ShiftRange. It could however be changed to accept a single argument, and return (uint32, error) instead of (uint32, uint32, error).

The function prototypes of parseGroupLine and parsePasswdLine have been changed, and return an error if necessary. The related tests have been updated as well.

alban · 2016-02-19T14:51:04Z

+	}
+
+	for i, tt := range tests {
+		g, err := parsePasswdLine(tt.line)


p, err := ...

alban · 2016-02-19T15:07:39Z

One small change, then the code looks good to me. Can you add an entry in CHANGELOG.md?

@krnowak can you review the changes in tests/functional.mk?

krnowak · 2016-02-19T15:24:29Z

Lemme see.

krnowak · 2016-02-19T15:49:55Z

-	// This is a partial implementation for app.User and app.Group:
-	// For now, only numeric ids (and the string "root") are supported.
-	var uid, gid int
+	var _uid, gid int


Why prepending an underline to the uid variable?

Otherwise it conflicts with the uid package since uid.NewBlankUidRange() is called shortly after. Are there any preferred naming rules in situations like these?

Oh, I missed that. Ok, let's keep it like that then. I'm not aware of any rules for that.

krnowak · 2016-02-19T16:16:57Z

+		}
+		uidReal, _, err := uidRange.UnshiftRange(stat.Uid, 0)
+		if err != nil {
+			return errwrap.Wrap(fmt.Errorf("unable to determine real uid"), err)


errors.New.

krnowak · 2016-02-19T16:18:09Z

Some nitpicks, otherwise LFAD.

The passwd package looks up given user name.

Allow looking up gids from a specified file. This makes gid validation easier.

Instead of relying on the caller to provide a Group struct which parseGroupLine then fills with data, a Group struct is created within the function and then returned. If the line is not parseable, an error is returned instead.

Add support for non-numerical uid/gid, as specified by appc. Uid/gid can be numerical, textual, or path-like. Resolves #1919

alban · 2016-02-19T19:15:29Z

Thank you! LGTM.

stage1,tests: add support for non-numerical uid/gid

alban added this to the v1.2.0 milestone Feb 9, 2016

alban added area/usability component/stage0 kind/enhancement priority/P2 labels Feb 9, 2016

alban reviewed Feb 9, 2016
View reviewed changes

alban added the needs/some-changes label Feb 9, 2016

alban mentioned this pull request Feb 9, 2016

rkt should not use /etc/group directly #2105

Open

alban added needs/review and removed needs/some-changes labels Feb 16, 2016

alban reviewed Feb 16, 2016
View reviewed changes

alban added needs/some-changes and removed needs/review labels Feb 16, 2016

alban mentioned this pull request Feb 16, 2016

rkt: run as unprivileged user #1585

Open

alban reviewed Feb 19, 2016
View reviewed changes

krnowak reviewed Feb 19, 2016
View reviewed changes

monstermunchkin added 6 commits February 19, 2016 18:07

pkg: new passwd package

965446a

The passwd package looks up given user name.

pkg/group: allow lookup from specified file

2edfc49

Allow looking up gids from a specified file. This makes gid validation easier.

pkg/group: refactor parseGroupLine function

9bf0d1f

Instead of relying on the caller to provide a Group struct which parseGroupLine then fills with data, a Group struct is created within the function and then returned. If the line is not parseable, an error is returned instead.

pkg/uid: add function to unshift uid/gid

fa8d2f3

stage1: support non-numerical uid/gid

4213490

Add support for non-numerical uid/gid, as specified by appc. Uid/gid can be numerical, textual, or path-like. Resolves #1919

CHANGELOG: add entry for non-numerical UID/GID

d9bd82e

alban added reviewed/lgtm and removed needs/some-changes labels Feb 19, 2016

alban added a commit that referenced this pull request Feb 19, 2016

Merge pull request #2159 from monstermunchkin/issue-1919

849adaf

stage1,tests: add support for non-numerical uid/gid

alban merged commit 849adaf into rkt:master Feb 19, 2016

alban mentioned this pull request Feb 25, 2016

release v1.1.0 #2227

Merged

iaguis mentioned this pull request Apr 12, 2016

stage1: the string "root" doesn't work for user/group if the image doesn't have /etc/{passwd,group} #2411

Closed

Conversation

monstermunchkin commented Feb 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alban commented Feb 9, 2016

Uh oh!

monstermunchkin commented Feb 9, 2016

Uh oh!

alban commented Feb 9, 2016

Uh oh!

monstermunchkin commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monstermunchkin commented Feb 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alban commented Feb 19, 2016

Uh oh!

krnowak commented Feb 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krnowak commented Feb 19, 2016

Uh oh!

alban commented Feb 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants