[LibOS] Add UID and GID to manifest #37
Conversation
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dzygann)
-- commits, line 4 at r1:
Please don't use "enables Gramine", it's a bit misleading. Maybe "allows Gramine to run" ?
-- commits, line 6 at r1:
"sets also" -> "also sets"
-- commits, line 7 at r1:
We generally use backticks (`) to indicate code/manifest/whatever variables, constants etc. So this would be
for the directive `loader.uid`/`loader.gid`.
Documentation/manifest-syntax.rst, line 188 at r1 (raw file):
(Default: 0) This specifies the user/group ID which will be used to run the executable in Gramine.
This sounds kind of misleading - one might think that these are host-level IDs. Maybe This specifies the initial, Gramine emulated user/group ID. ?
Documentation/manifest-syntax.rst, line 191 at r1 (raw file):
These specified values are also used for the effective user/group ID. By default the executable is running as root user (0). If the value is < 0 Gramine stops with an "Invalid Argument" error.
"Must be positive." would be enough (instead of the last sentence).
LibOS/shim/src/bookkeep/shim_thread.c, line 153 at r1 (raw file):
__atomic_store_n(&g_process.pgid, g_process.pid, __ATOMIC_RELEASE); /* Default user and group ids are `0` and already set. */
This comment can now be removed.
LibOS/shim/src/bookkeep/shim_thread.c, line 161 at r1 (raw file):
put_thread(cur_thread); return -EINVAL; }
We need to validate that:
uid_int64is non negativeuid_int64is not greater thanIDTYPE_MAX
LibOS/shim/src/bookkeep/shim_thread.c, line 169 at r1 (raw file):
put_thread(cur_thread); return -EINVAL; }
ditto (for gid_int64)
LibOS/shim/test/regression/test_libos.py, line 109 at r1 (raw file):
def test_105_uid_and_gid_from_file(self): stdout, _ = self.run_binary(['uid_gid.c'])
This should be the binary, not source code. How did it even work?
LibOS/shim/test/regression/uid_gid.c, line 4 at r1 (raw file):
#include <stdio.h> #include <sys/types.h> #include <stdlib.h>
Please sort includes.
LibOS/shim/test/regression/uid_gid.c, line 11 at r1 (raw file):
uid_t euid = geteuid(); if(uid != 80085 && euid != 80085) {
Space after if
LibOS/shim/test/regression/uid_gid.c, line 11 at r1 (raw file):
uid_t euid = geteuid(); if(uid != 80085 && euid != 80085) {
This should be || not &&.
LibOS/shim/test/regression/uid_gid.c, line 18 at r1 (raw file):
uid_t egid = getegid(); if(gid != 1337 && egid != 1337) {
ditto (space and ||)
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dzygann)
a discussion (no related file):
Does this influence file owner as reported by Graphene? I think it should, and it's worth mentioning in documentation (and testing in the uid_gid test).
LibOS/shim/src/bookkeep/shim_thread.c, line 171 at r1 (raw file):
} cur_thread -> uid = uid_int64;
No spaces around ->.
LibOS/shim/test/regression/manifest.template, line 6 at r1 (raw file):
loader.insecure__use_cmdline_argv = true # for uid_gid test
I think it's better to override uid/gid only for this one binary. Can you create a separate manifest?
LibOS/shim/test/regression/test_libos.py, line 109 at r1 (raw file):
def test_105_uid_and_gid_from_file(self): stdout, _ = self.run_binary(['uid_gid.c'])
This looks like it won't work: it should be uid_gid.
(You can run make regression to check if the Python code runs, or .../Scripts/run-pytest -k test_105 to run this one test through Python).
LibOS/shim/test/regression/uid_gid.c, line 11 at r1 (raw file):
uid_t euid = geteuid(); if(uid != 80085 && euid != 80085) {
1337 is ok, I guess, but I don't think we want values like 80085 in our code :)
Can you change to something more neutral? 1338 should be ok.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dzygann)
a discussion (no related file):
Previously, pwmarcz (Paweł Marczewski) wrote…
Does this influence file owner as reported by Graphene? I think it should, and it's worth mentioning in documentation (and testing in the
uid_gidtest).
But then again, it's more complicated - we should probably report user-created / mounted files as our uid/gid, and /sys/ as belonging to root, and for /proc/ it depends on the file... so maybe if it's not necessary for #2632, we shouldn't bother?
What do others think? @dimakuv @mkow @boryspoplawski
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @dzygann, and @mkow)
a discussion (no related file):
Previously, pwmarcz (Paweł Marczewski) wrote…
But then again, it's more complicated - we should probably report user-created / mounted files as our uid/gid, and /sys/ as belonging to root, and for /proc/ it depends on the file... so maybe if it's not necessary for #2632, we shouldn't bother?
What do others think? @dimakuv @mkow @boryspoplawski
Well, this kind of makes no sense in Graphene. We do not check any privileges, have 0 access or capabilities checks, so your app is allowed to do everything, regardless of uids and file ownership. That's why I don't like this approach of changing the UID, but afaik you cannot run postgres otherwise. I don't think there is much point in setting file ownership to process IDs at creation time, after all what application checks file ownership, instead of just trying to open the file?
There was a problem hiding this comment.
Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 28 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @boryspoplawski, @dzygann, @mkow, and @pwmarcz)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
Well, this kind of makes no sense in Graphene. We do not check any privileges, have 0 access or capabilities checks, so your app is allowed to do everything, regardless of uids and file ownership. That's why I don't like this approach of changing the UID, but afaik you cannot run postgres otherwise. I don't think there is much point in setting file ownership to process IDs at creation time, after all what application checks file ownership, instead of just trying to open the file?
I agree with Borys. I would prefer to keep permissions flat (all files can be accessed by the user, whatever its uid/gid is). And make all "change user/group" operations effectively nops. Let's not bother.
a discussion (no related file):
Any particular reason you set effective user ID/group ID as well? I guess it's fine, just want to understand the reasoning behind this.
-- commits, line 2 at r1:
I would prefer to be even more explicit:
[LibOS] Add `loader.uid` and `loader.gid` manifest options
-- commits, line 5 at r1:
This sentence is a bit too loose :)
I would reword like this: By default, Gramine uses the root user (uid = gid = 0), which may cause some apps to refuse to run.
-- commits, line 7 at r1:
The sentence is too verbose. Simply: We also set effective UID/GID to the same value as UID/GID respectively.
Whatever modification to your sentence you choose, please don't use the word directive -- we use manifest option instead.
-- commits, line 7 at r1:
Also, you forgot the dot at the end.
Documentation/manifest-syntax.rst, line 179 at r1 (raw file):
precedence. Add User ID and Group ID
Please remove Add
Documentation/manifest-syntax.rst, line 190 at r1 (raw file):
This specifies the user/group ID which will be used to run the executable in Gramine. These specified values are also used for the effective user/group ID. By default the executable is running as root user (0). If the value is < 0 Gramine stops with an
is running -> runs
(0) -> (uid = gid = 0)
Documentation/manifest-syntax.rst, line 191 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
"Must be positive." would be enough (instead of the last sentence).
+1 to Borys's suggestion, but better "Must be non-negative."
LibOS/shim/src/bookkeep/shim_thread.c, line 171 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
No spaces around
->.
Wait, how did this even compile?
UPDATE: Ok, apparently this is allowed in C. But anyway, please change.
LibOS/shim/test/regression/manifest.template, line 6 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
I think it's better to override uid/gid only for this one binary. Can you create a separate manifest?
+1 to Pawel's suggestion
LibOS/shim/test/regression/test_libos.py, line 108 at r1 (raw file):
os.remove('env_test_input') def test_105_uid_and_gid_from_file(self):
_from_file suffix is not needed here, just remove this suffix
LibOS/shim/test/regression/uid_gid.c, line 11 at r1 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
1337 is ok, I guess, but I don't think we want values like 80085 in our code :)
Can you change to something more neutral? 1338 should be ok.
+1, definitely no 80085. I prefer 42 and 24, but whatever magic numbers you choose, they should be not offensive :)
LibOS/shim/test/regression/uid_gid.c, line 12 at r1 (raw file):
if(uid != 80085 && euid != 80085) { errx(EXIT_FAILURE, "UID is not equal to the value in the the manifest");
UID is not equal to the value... -> UID/effective UID are not equal to the value...
LibOS/shim/test/regression/uid_gid.c, line 19 at r1 (raw file):
if(gid != 1337 && egid != 1337) { errx(EXIT_FAILURE, "GID is not equal to the value in the the manifest");
GID is not equal to the value... -> GID/effective GID are not equal to the value...
There was a problem hiding this comment.
Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 27 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @boryspoplawski, @dzygann, and @pwmarcz)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I agree with Borys. I would prefer to keep permissions flat (all files can be accessed by the user, whatever its uid/gid is). And make all "change user/group" operations effectively nops. Let's not bother.
OK, thanks. Resolving, no need to do anything here.
|
a discussion (no related file):
Yes, I have tested the change with the Postgres DB. It was complaining that I still trying to run it with the root user. After changing also the effective ID's the error disappeared. |
There was a problem hiding this comment.
Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @dzygann, and @pwmarcz)
a discussion (no related file):
Please do not amend/force push in the middle of the review. git has a nice --fixup option, please use it :) For the future, also do not rebase, it will be done before merging (squash all fixups + rebase).
Also when you fix some review commends, please respond on reviewable; it has a nice "Done." button you can press, if you just did what reviewer asked for (otherwise, please just respond).
-- commits, line 6 at r2:
is can be ?
LibOS/shim/src/bookkeep/shim_thread.c, line 170 at r2 (raw file):
if (uid_int64 < 0 || uid_int64 > IDTYPE_MAX || gid_int64 < 0 || gid_int64 > IDTYPE_MAX) { log_error("UID or GID is either < 0 or > %u", IDTYPE_MAX);
Nitpick: would be helpful to print which one does not pass the validation and print it's value.
LibOS/shim/test/regression/uid_gid.manifest.template, line 4 at r2 (raw file):
libos.entrypoint = "{{ entrypoint }}" loader.env.LD_LIBRARY_PATH = "/lib:{{ arch_libdir }}:/usr/{{ arch_libdir }}" loader.insecure__use_cmdline_argv = true
We do not use argv in this test so this is not needed. You will need to add argv0 override though.
There was a problem hiding this comment.
Reviewable status: 7 of 8 files reviewed, 19 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @dzygann, and @pwmarcz)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
Please do not amend/force push in the middle of the review.
githas a nice--fixupoption, please use it :) For the future, also do not rebase, it will be done before merging (squash all fixups + rebase).
Also when you fix some review commends, please respond on reviewable; it has a nice "Done." button you can press, if you just did what reviewer asked for (otherwise, please just respond).
Hi,
thanks for the hints. I didn't know the fixup feature in git and I'm not familiar with reviewable, but I will :)
Regarding the fixup approach, I can see that there is a hash before the fixup keyword. I can't see it on my local machine. Is it correct or did I screw up something?
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I would prefer to be even more explicit:
[LibOS] Add `loader.uid` and `loader.gid` manifest options
Done.
Previously, boryspoplawski (Borys Popławski) wrote…
Please don't use "enables Gramine", it's a bit misleading. Maybe "allows Gramine to run" ?
Changed to allows Gramine to run..
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This sentence is a bit too loose :)
I would reword like this:
By default, Gramine uses the root user (uid = gid = 0), which may cause some apps to refuse to run.
Done.
Previously, boryspoplawski (Borys Popławski) wrote…
"sets also" -> "also sets"
Done.
y use backticks (`) to ind
Ok
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The sentence is too verbose. Simply:
We also set effective UID/GID to the same value as UID/GID respectively.Whatever modification to your sentence you choose, please don't use the word
directive-- we usemanifest optioninstead.
Done.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Also, you forgot the dot at the end.
Done.
Previously, boryspoplawski (Borys Popławski) wrote…
is can be?
Done.
-- commits, line 11 at r3:
Is this right with the hash before the fixup keyword?
Documentation/manifest-syntax.rst, line 179 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove
Add
Done.
Documentation/manifest-syntax.rst, line 188 at r1 (raw file):
ounds kind of misleading - one might th
Done.
Documentation/manifest-syntax.rst, line 190 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
is running->runs
(0)->(uid = gid = 0)
Done.
Documentation/manifest-syntax.rst, line 191 at r1 (raw file):
Must be non-negative.
Changed to "Must be non-negative."
LibOS/shim/src/bookkeep/shim_thread.c, line 153 at r1 (raw file):
is comment can now
Done.
LibOS/shim/src/bookkeep/shim_thread.c, line 161 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
We need to validate that:
uid_int64is non negativeuid_int64is not greater thanIDTYPE_MAX
Done.
LibOS/shim/src/bookkeep/shim_thread.c, line 169 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto (for
gid_int64)
Done.
LibOS/shim/src/bookkeep/shim_thread.c, line 171 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Wait, how did this even compile?
UPDATE: Ok, apparently this is allowed in C. But anyway, please change.
Done.
LibOS/shim/test/regression/test_libos.py, line 108 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
_from_filesuffix is not needed here, just remove this suffix
Done.
LibOS/shim/test/regression/test_libos.py, line 109 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This should be the binary, not source code. How did it even work?
It didn't work... I overlooked this one.
LibOS/shim/test/regression/test_libos.py, line 109 at r1 (raw file):
n run make regression to c
After the change it works.
LibOS/shim/test/regression/uid_gid.c, line 4 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Please sort includes.
Done.
LibOS/shim/test/regression/uid_gid.c, line 11 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Space after
if
Done.
LibOS/shim/test/regression/uid_gid.c, line 11 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
This should be
||not&&.
Sure.
LibOS/shim/test/regression/uid_gid.c, line 11 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1, definitely no
80085. I prefer42and24, but whatever magic numbers you choose, they should be not offensive :)
ok :)
LibOS/shim/test/regression/uid_gid.c, line 12 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
UID is not equal to the value...->UID/effective UID are not equal to the value...
Done.
LibOS/shim/test/regression/uid_gid.c, line 18 at r1 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
ditto (space and
||)
Dito.
LibOS/shim/test/regression/uid_gid.c, line 19 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
GID is not equal to the value...->GID/effective GID are not equal to the value...
Done.
LibOS/shim/test/regression/manifest.template, line 6 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
+1 to Pawel's suggestion
Done.
There was a problem hiding this comment.
Reviewable status: 6 of 8 files reviewed, 19 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @pwmarcz)
LibOS/shim/src/bookkeep/shim_thread.c, line 170 at r2 (raw file):
uld be helpful to print which o
Is this okay?
LibOS/shim/test/regression/uid_gid.manifest.template, line 4 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
We do not use
argvin this test so this is not needed. You will need to addargv0override though.
Done.
There was a problem hiding this comment.
Reviewed 3 of 6 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dzygann, and @pwmarcz)
a discussion (no related file):
Previously, dzygann (Denis Zygann) wrote…
particular reason you set effective user ID/group ID as well?
Yes, I have tested the change with the Postgres DB. It was complaining that I still trying to run it with the root user. After changing also the effective ID's the error disappeared.
Read a bit more about effective UID/GID. Legit.
a discussion (no related file):
Just to expand on this misunderstanding with amend/force push.
We asked Denis to change the commit message significantly. Changing the commit message is not possible without amend and then force-push. Even though it is frowned upon to do git-amend / git-rebase / git force-push, there was no other way for Denis to satisfy our comments on the commit messages. And we didn't indicate that "please don't fix the commit messages now, we will fix them during the final git rebase". So Denis was right to just go and amend the commit message and force-push.
Regarding the fixup approach, I can see that there is a hash before the fixup keyword. I can't see it on my local machine. Is it correct or did I screw up something?
You did everything correctly. The hash before the fixup keyword is added by the Reviewable web page. It's not really in your git commit messages, it's just Reviewable being smart-ass.
Previously, dzygann (Denis Zygann) wrote…
Done.
Not done. Borys meant that you have a typo This is can be.... You should change it to proper English This can be...
But let's not do it now. As explained above, let's not do any git-amend / git-rebase now, but we'll postpone this to the final rebase.
Previously, dzygann (Denis Zygann) wrote…
Is this right with the hash before the fixup keyword?
Yes, this is normal. Reviewable adds it.
-- commits, line 16 at r4:
If you're curious: all commit messages in fixup commits are actually discarded. (That's the point of fixup commits -- they modify the code of previous commits, thus their commit messages are useless and will be discarded.) So the text you have here is redundant, but it doesn't hurt -- git will automatically remove this text during git-rebase.
Documentation/manifest-syntax.rst, line 189 at r4 (raw file):
This specifies the initial, Gramine emulated user/group ID and effective user/group ID. It must be non-negative. By default it runs as root user (uid = gid = 0).
By default it runs as root user -> It defaults to the root user (current version sounds weird; who runs? ID runs?)
LibOS/shim/src/bookkeep/shim_thread.c, line 169 at r1 (raw file):
Previously, dzygann (Denis Zygann) wrote…
Done.
ditto
LibOS/shim/src/bookkeep/shim_thread.c, line 170 at r2 (raw file):
Previously, dzygann (Denis Zygann) wrote…
uld be helpful to print which o
Is this okay?
What is %li? I didn't know prinf-formatting supports this :) We always use %ld, please change.
Also, users prefer to read in normal English, not math English :) So I suggest to rephrase:
'loader.uid' (%ld) is negative or greater than %u"
Also note that we use ' and not the backtick in Gramine messages.
LibOS/shim/src/bookkeep/shim_thread.c, line 169 at r4 (raw file):
} if (uid_int64 < 0 || uid_int64 > IDTYPE_MAX ) {
You have a redundant space before ). Please remove.
LibOS/shim/test/regression/uid_gid.manifest.template, line 4 at r2 (raw file):
Previously, dzygann (Denis Zygann) wrote…
Done.
Some things here are wrong. See https://github.com/gramineproject/graphene/blob/master/LibOS/shim/test/regression/attestation.manifest.template as an example.
- You have an explicit name of this test:
uid_gid. You should use this name forlibos.entrypointandloader.argv0_override - You need to add
sgx.trusted_files-- at leastgraphene.runtimedir()and your binaryuid_gid
Did you test this under SGX? You can do make SGX=1 sgx-tokens and then make SGX=1 regression to check if your new test works in SGX. I bet it currently doesn't pass.
LibOS/shim/test/regression/uid_gid.manifest.template, line 12 at r4 (raw file):
fs.mount.lib.path = "/lib" fs.mount.lib.uri = "file:{{ graphene.runtimedir() }}"
Please don't leave empty lines at the end of files.
There was a problem hiding this comment.
Reviewed 4 of 6 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dzygann)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @dzygann, and @pwmarcz)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
If you're curious: all commit messages in
fixupcommits are actually discarded. (That's the point of fixup commits -- they modify the code of previous commits, thus their commit messages are useless and will be discarded.) So the text you have here is redundant, but it doesn't hurt -- git will automatically remove this text during git-rebase.
But note that the commit one-liner must mach exactly (except for the fixup! part), because they are matched based on it when squashing later on.
LibOS/shim/src/bookkeep/shim_thread.c, line 170 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What is
%li? I didn't know prinf-formatting supports this :) We always use%ld, please change.Also, users prefer to read in normal English, not math English :) So I suggest to rephrase:
'loader.uid' (%ld) is negative or greater than %u"Also note that we use
'and not the backtick in Gramine messages.
It does support i, but we use d everywhere.
FWIW: I don't mind the current version (with > <).
There was a problem hiding this comment.
Reviewable status: 5 of 8 files reviewed, 9 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @dzygann, and @pwmarcz)
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Not done. Borys meant that you have a typo
This is can be.... You should change it to proper EnglishThis can be...But let's not do it now. As explained above, let's not do any git-amend / git-rebase now, but we'll postpone this to the final rebase.
It was done by the commit before ... Somehow it came back into the message ... Thanks for fixing it later!
ote that the commit one-liner mu
Thanks for the input!
Documentation/manifest-syntax.rst, line 189 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
By default it runs as root user->It defaults to the root user(current version sounds weird; who runs? ID runs?)
I hope this sounds more clearly
LibOS/shim/src/bookkeep/shim_thread.c, line 169 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done.
LibOS/shim/src/bookkeep/shim_thread.c, line 170 at r2 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
It does support
i, but we usedeverywhere.
FWIW: I don't mind the current version (with><).
Done.
LibOS/shim/src/bookkeep/shim_thread.c, line 169 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You have a redundant space before
). Please remove.
Done.
LibOS/shim/test/regression/uid_gid.manifest.template, line 4 at r2 (raw file):
u test this under SGX? Yo
Nope, I didn't test under SGX. I did "graphene-direct" test, only.
LibOS/shim/test/regression/uid_gid.manifest.template, line 12 at r4 (raw file):
don't leave empty lines
ok
There was a problem hiding this comment.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dzygann, and @pwmarcz)
Documentation/manifest-syntax.rst, line 189 at r4 (raw file):
Previously, dzygann (Denis Zygann) wrote…
I hope this sounds more clearly
to the root user -> as the root user (I think this is more correct English)
Also, please re-wrap your text to 80 chars per line. We use 100 chars per line in our C/Python/Makefile code, but 80 chars per line for Documentation. (The reason being: documentation is intended for "normal" reading, and people's eyes find it easier to read less chars per line. Science!)
LibOS/shim/test/regression/uid_gid.manifest.template, line 4 at r2 (raw file):
Previously, dzygann (Denis Zygann) wrote…
u test this under SGX? Yo
Nope, I didn't test under SGX. I did "graphene-direct" test, only.
Out of curiousity: you don't have access to an SGX-enabled machine?
LibOS/shim/test/regression/uid_gid.manifest.template, line 16 at r5 (raw file):
"file:{{ graphene.runtimedir() }}/", "file:uid_gid" ]
Here you have a "not-finished" line. Doesn't look good.
(I actually don't know why this happens for some people. I use the Vim editor, and I've never encountered this thingy myself. Is this something that other editors do, somehow remove the final \n in the file?)
There was a problem hiding this comment.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @dzygann)
LibOS/shim/test/regression/uid_gid.manifest.template, line 16 at r5 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Here you have a "not-finished" line. Doesn't look good.
(I actually don't know why this happens for some people. I use the Vim editor, and I've never encountered this thingy myself. Is this something that other editors do, somehow remove the final
\nin the file?)
I think it's often an editor option ("require final newline", "empty line at end of file" and such).
There was a problem hiding this comment.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @dzygann)
Previously, dzygann (Denis Zygann) wrote…
It was done by the commit before ... Somehow it came back into the message ... Thanks for fixing it later!
Still not done. Let's fix this at the final rebase.
|
Documentation/manifest-syntax.rst, line 189 at r4 (raw file): Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Done. Thanks for the additional description why to do that. |
There was a problem hiding this comment.
Reviewable status: 6 of 8 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @dzygann, and @pwmarcz)
LibOS/shim/test/regression/uid_gid.manifest.template, line 4 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Out of curiousity: you don't have access to an SGX-enabled machine?
I have a SGX-enabled machine :) It has a Intel(R) Core(TM) i5-8400T CPU. Unfortunately, it supports SGX 1, only. Do you have a recommendation, if I would like to buy a new one?
LibOS/shim/test/regression/uid_gid.manifest.template, line 16 at r5 (raw file):
Previously, pwmarcz (Paweł Marczewski) wrote…
I think it's often an editor option ("require final newline", "empty line at end of file" and such).
I found an option in the setting: ... ends with a line break
There was a problem hiding this comment.
Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @dzygann)
Documentation/manifest-syntax.rst, line 189 at r4 (raw file):
Previously, dzygann (Denis Zygann) wrote…
Done. Thanks for the additional description why to do that.
Not done.
LibOS/shim/test/regression/uid_gid.manifest.template, line 4 at r2 (raw file):
Previously, dzygann (Denis Zygann) wrote…
I have a SGX-enabled machine :) It has a Intel(R) Core(TM) i5-8400T CPU. Unfortunately, it supports SGX 1, only. Do you have a recommendation, if I would like to buy a new one?
Why is that one not enough?
|
Jenkins, test this please. |
There was a problem hiding this comment.
Reviewable status: 7 of 8 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @dzygann)
Documentation/manifest-syntax.rst, line 189 at r4 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Not done.
Right, thanks! Now, it should be done :)
LibOS/shim/test/regression/uid_gid.manifest.template, line 4 at r2 (raw file):
is that one not en
It works fine. I'm asking out of curiosity.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @dzygann)
|
On the previous run |
|
Jenkins, retest this please |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @dzygann)
a discussion (no related file):
@dzygann the newly added test fails (time-outs) on SGX-sanitizers pipeline, please debug. You can get the setup from .ci directory; generally speaking this is Gramine compiled in debug mode with SGX support, using clang and with some sanitizers enabled.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @dzygann)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
@dzygann the newly added test fails (time-outs) on SGX-sanitizers pipeline, please debug. You can get the setup from
.cidirectory; generally speaking this is Gramine compiled in debug mode with SGX support, using clang and with some sanitizers enabled.
Hi @boryspoplawski I'm not familiar with this approach. Can you explain it a little bit more in detail, which steps I have to do?
There was a problem hiding this comment.
Reviewed 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski and @dzygann)
LibOS/shim/test/regression/uid_gid.c, line 12 at r7 (raw file):
if (uid != 1338 || euid != 1338) { errx(EXIT_FAILURE, "UID/effective UID are not equal to the value in the the manifest");
You have the the, please remove one
LibOS/shim/test/regression/uid_gid.c, line 19 at r7 (raw file):
if (gid != 1337 || egid != 1337) { errx(EXIT_FAILURE, "GID/effective GID are not equal to the value in the the manifest");
ditto
LibOS/shim/test/regression/uid_gid.manifest.template, line 4 at r2 (raw file):
Previously, dzygann (Denis Zygann) wrote…
is that one not en
It works fine. I'm asking out of curiosity.
No need to buy a new machine. I was just surprused why you didn't run this test under graphene-sgx?
|
Jenkins, retest Jenkins-SGX-Sanitizers please (timed out on the newly added |
There was a problem hiding this comment.
This is weird. All of them are failing now. What happened?
Reviewable status: 7 of 8 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @dzygann)
LibOS/shim/test/regression/uid_gid.c, line 12 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You have
the the, please remove one
Done.
LibOS/shim/test/regression/uid_gid.c, line 19 at r7 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done.
There was a problem hiding this comment.
Right, unfortunately our Jenkins instance is not visible to outside, sorry about that.
It looks like compilation is failing because we recently renamed all instances of graphene to gramine (as part of project name change). And Jenkins is testing the result of merging your code.
I think you should rebase your branch on newest master; and do the renames in your files, of course. (There is also one more change: we now have sgx.debug = true in all our templates.)
Reviewed 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @dzygann)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
You have configuration file
.ci/linux-sgx-sanitizers.jenkinsfilewhich is run on the faulty pipeline. From there you can extract how exactly Gramine is built, but generally the differences (from normal build) are: enabled UBSAN and clang instead of gcc.
It looks like the error was a side-effect of not specifying sgx.nonpie_binary = true in the manifest, and not related to this change at all.
(And unlike GCC, Clang produces non-PIE binaries by default...)
There was a problem hiding this comment.
Our Jenkins is stupid. It automatically tries to merge your branch into master branch, and tests this:
Commit message: "Merge e476c8f110914f7e62767e1039d25f4003747b6e into 0e61d7ea023ded04a14291448898050c1e84f504"
There is no way to switch this stupid feature in Jenkins. So this bad merge resulted in this failure:
jinja2.exceptions.UndefinedError: 'graphene' is undefined
../../../../Scripts/manifest.mk:8: recipe for target 'uid_gid.manifest' failed
The only way to fix it is to rebase your PR on top of the current latest master. Otherwise Jenkins will try to merge every time, and your stuff will fail every time. So please rebase your PR (git rebase master and then you'll need to fix the problematic merges manually).
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, and @dzygann)
LibOS/shim/test/regression/uid_gid.manifest.template, line 1 at r8 (raw file):
loader.preload = "file:{{ graphene.libos }}"
This is why Jenkins is failing -- you still have the old graphene word here. Recently, we changed everything to "gramine" (see examples in the latest master branch).
There was a problem hiding this comment.
Okay, I did the rebase. Let's try it again :)
Reviewable status: 2 of 8 files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @dzygann, and @pwmarcz)
LibOS/shim/test/regression/uid_gid.manifest.template, line 1 at r8 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is why Jenkins is failing -- you still have the old
grapheneword here. Recently, we changed everything to "gramine" (see examples in the latest master branch).
Done.
|
Jenkins, retest this please |
There was a problem hiding this comment.
Reviewed 6 of 6 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @dzygann)
LibOS/shim/test/regression/uid_gid.c, line 12 at r7 (raw file):
Previously, dzygann (Denis Zygann) wrote…
Done.
Not done
LibOS/shim/test/regression/uid_gid.c, line 19 at r7 (raw file):
Previously, dzygann (Denis Zygann) wrote…
Done.
Not done.
There was a problem hiding this comment.
Reviewable status: 7 of 8 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @boryspoplawski, @dimakuv, @dzygann, and @pwmarcz)
LibOS/shim/test/regression/uid_gid.c, line 12 at r7 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Not done
Now, it should be done.
LibOS/shim/test/regression/uid_gid.c, line 19 at r7 (raw file):
Previously, boryspoplawski (Borys Popławski) wrote…
Not done.
Now, it should be done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @dzygann)
-- commits, line 65 at r10:
FYI: this message will be dropped anyway, no need to add it (it's how fixup commits work, their content is just amended, the message is discarded). Also there is a handy --fixup=commit_hash parameter to git commit which you can use (it will create a correct commit msg oneliner)
|
Jenkins, retest this please |
There was a problem hiding this comment.
Reviewed 5 of 6 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dzygann)
There was a problem hiding this comment.
Reviewed 1 of 6 files at r9.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dzygann)
a discussion (no related file):
The PR looks good to me now. Thanks for this effort! (Now let's wait for other reviewers, they may still have some comments.)
LibOS/shim/test/regression/uid_gid.manifest.template, line 18 at r10 (raw file):
sgx.trusted_files = [ "file:{{ gramine.runtimedir() }}/", "file:uid_gid"
We typically put a comma at the end of such lists in manifest files, so I would prefer:
"file:uid_gid",
This makes it easier to append more lines to trusted files, and is also valid in TOML. But not blocking, feel free to ignore.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dzygann)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The PR looks good to me now. Thanks for this effort! (Now let's wait for other reviewers, they may still have some comments.)
Except for commit message (which we will fix at rebase) looks good
There was a problem hiding this comment.
Reviewed 6 of 6 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, "fixup! " found in commit messages' one-liners (waiting on @dzygann)
There was a problem hiding this comment.
Reviewed 5 of 5 files at r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @boryspoplawski)
a discussion (no related file):
Previously, boryspoplawski (Borys Popławski) wrote…
Except for commit message (which we will fix at rebase) looks good
Done
Previously, boryspoplawski (Borys Popławski) wrote…
Still not done. Let's fix this at the final rebase.
Done. Also rephrased.
LibOS/shim/test/regression/uid_gid.manifest.template, line 18 at r10 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We typically put a comma at the end of such lists in manifest files, so I would prefer:
"file:uid_gid",This makes it easier to append more lines to trusted files, and is also valid in TOML. But not blocking, feel free to ignore.
Done
There was a problem hiding this comment.
Reviewed 5 of 5 files at r11, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
|
Jenkins, test this please |
|
The sanitizer runs again into an issue... Can anyone tell me what went wrong? |
|
Jenkins, retest Jenkins-Direct-Sanitizers please |
There was a problem hiding this comment.
@dzygann Jenkins failed to checkout the repo, probably due todays problem with Github.
Reviewable status:
By default, Gramine uses the root user (uid = gid = 0), which may cause some apps to refuse to run. This commit allows Gramine to run an app with another user than root. We also set effective UID/GID to the same value as UID/GID respectively. Signed-off-by: Denis Zygann <denis@zygann.de>
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
|
Jenkins, test this please |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r11, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dzygann)
a discussion (no related file):
Blocking for some time, need to figure out smth with @dzygann (privately)
|
a discussion (no related file): Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I was too late :) |
This implementation enables Gramine to run an executable with another user than root.
This becomes very handy in case of executables, which shouldn't run with the root user.
The implementation of this feature sets also the effective uid/gid to the value,
which is defined in the manifest for the directive loader.uid/gid
This topic has been discussed in the issue 2632
Signed-off-by: Denis Zygann denis@zygann.de
This change is