-
Notifications
You must be signed in to change notification settings - Fork 203
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
[Pal/Linux-SGX] Implement gramine-sgx-get-token in Rust #639
base: master
Are you sure you want to change the base?
[Pal/Linux-SGX] Implement gramine-sgx-get-token in Rust #639
Conversation
Signed-off-by: Jakub Kądziołka <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have anything against cargo
1, but either way we'll have to make sure this compiles without network access. This means that while packaging I'll do something like this: https://salsa.debian.org/rust-team/debcargo-conf#id22 to .cargo/config.toml
and we'll have to make sure this still works, by either making sure dependencies are available in debian, or vendoring those dependencies. I still don't know what we'll have to do for rpm, where we don't have rust libraries.
Reviewed 17 of 18 files at r1.
Reviewable status: 17 of 18 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NieDzejkob)
a discussion (no related file):
I see there are doc comments, which is good. Will (in the future) there be a possibility to publish rustdoc somewhere along our main documentation at rtfd? (I'm not saying it has to be done now, only if there's a way to do it later).
.cargo/config.toml
line 6 at r1 (raw file):
# to have manual Cargo invokations reuse it. [build] target-dir = "build/target"
I don't think this is good idea. If you need manual cargo invocations, those shouldn't go into any builddir. If you'd like to rebuild something in builddir, you can always do ninja -C <builddir> <target-name>
, or even skip target name which should theoretically rebuild only targets affected (assuming .d
dependencies don't lie).
common/rust/Cargo.toml
line 4 at r1 (raw file):
name = "gramine_common" version = "0.0.0" edition = "2021"
Edition 2021 means rustc 1.56, right? Or is there some additional requirement that in practice means some newer rustc version is required?
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 13 at r1 (raw file):
[dependencies] gramine_common = { path = "../../../../../../common/rust" } aesm-client = { version = "0.5.3", features = ["sgxs"], optional = true }
Not in debian, probably needs vendoring or a mechanism to provide sources without downloading from network while packaging.
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 14 at r1 (raw file):
gramine_common = { path = "../../../../../../common/rust" } aesm-client = { version = "0.5.3", features = ["sgxs"], optional = true } argh = "0.1.7"
Not in debian, but debian has clap-2.33.3 in bullseye and clap-3.1.6 in bookworm. Can we change to clap?
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 15 at r1 (raw file):
aesm-client = { version = "0.5.3", features = ["sgxs"], optional = true } argh = "0.1.7" failure = "0.1.8"
bullseye and bookwork have 0.1.7, can we downgrade?
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 16 at r1 (raw file):
argh = "0.1.7" failure = "0.1.8" hex = "0.4.3"
bullseye has 0.4.0, bookworm has 0.4.3 so it's OK I think.
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 17 at r1 (raw file):
failure = "0.1.8" hex = "0.4.3" sgx-isa = "0.3.3"
Not in debian, probably needs vendoring.
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 18 at r1 (raw file):
hex = "0.4.3" sgx-isa = "0.3.3" sgxs = "0.7.2"
Not in debian, https://docs.crs/sgxs look poor, probably needs vendoring and documenting.
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 19 at r1 (raw file):
sgx-isa = "0.3.3" sgxs = "0.7.2" sha2 = "0.10.2"
Debian has 0.9.2, can we downgrade?
Scripts/meson.build
line 14 at r1 (raw file):
run_cargo_cmd = [ run_cargo_prog, '@INPUT@', '@OUTPUT@', meson.build_root() / 'target',
this should be @PRIVATE_DIR
(>path to a directory where the custom target must store all its intermediate files)
https://mesonbuild.com/Reference-manual_functions.html#custom_target
Scripts/run-cargo.sh
line 20 at r1 (raw file):
PROFILE="dev" else PROFILE="$BUILD_TYPE"
get_option('buildtype')
can be any of: plain
, debug
, debugoptimized
, release
, minsize
, custom
. Are all of those valid here? (I saw debugoptimized
in config.toml
, but not others.
https://mesonbuild.com/Builtin-options.html#core-options
Footnotes
-
apart from general predilection for shoveling more dependencies into projects ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 17 of 18 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NieDzejkob and @woju)
a discussion (no related file):
Previously, woju (Wojtek Porczyk) wrote…
I see there are doc comments, which is good. Will (in the future) there be a possibility to publish rustdoc somewhere along our main documentation at rtfd? (I'm not saying it has to be done now, only if there's a way to do it later).
We can build cargo doc
in CI and publish on a domain we control.
Alternatively, though I don't think this is a great idea, we could publish our crates to crates.io and have docs.rs build them. But then we need to concern ourselves with versioning those crates and stuff.
.cargo/config.toml
line 6 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
I don't think this is good idea. If you need manual cargo invocations, those shouldn't go into any builddir. If you'd like to rebuild something in builddir, you can always do
ninja -C <builddir> <target-name>
, or even skip target name which should theoretically rebuild only targets affected (assuming.d
dependencies don't lie).
The target
directory is pretty well-managed by cargo and having two separate ones will only make rebuilds slower.
Running cargo through ninja is not a complete solution — you could want to run cargo check
on a particular crate you're working on, for example.
common/rust/Cargo.toml
line 4 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Edition 2021 means rustc 1.56, right? Or is there some additional requirement that in practice means some newer rustc version is required?
Higher rustc version requirements may appear when code uses a particular feature that was stabilized after 1.56 — you don't need any explicit opt-in for that. If we want to support a particular rustc version, we should probably test building with it in CI.
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 14 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Not in debian, but debian has clap-2.33.3 in bullseye and clap-3.1.6 in bookworm. Can we change to clap?
I partly chose argh
over clap
because it's smaller and should have less effect on compile time. I don't think taking into account which crates are available in Debian is a good strategy long-term, especially considering that we'll always need to handle some crates that aren't. Proper tooling shouldn't make the actual count of crates outside of Debian a problem.
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 15 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
bullseye and bookwork have 0.1.7, can we downgrade?
ditto
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 18 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Not in debian, https://docs.crs/sgxs look poor, probably needs vendoring and documenting.
Not opposed to either forking or submitting PRs upstream for this. Upstream seems to be pretty active in this regard.
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 19 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Debian has 0.9.2, can we downgrade?
ditto
Scripts/meson.build
line 14 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
this should be
@PRIVATE_DIR
(>path to a directory where the custom target must store all its intermediate files)
https://mesonbuild.com/Reference-manual_functions.html#custom_target
We want to share this between all crates we build, though.
Scripts/run-cargo.sh
line 20 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
get_option('buildtype')
can be any of:plain
,debug
,debugoptimized
,release
,minsize
,custom
. Are all of those valid here? (I sawdebugoptimized
inconfig.toml
, but not others.
I went by our own documentation, which only mentions debug
, debugoptimized
and release
. I guess I can create profiles for the rest too, but I'm not even sure what custom
and plain
should do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 17 of 18 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @NieDzejkob and @woju)
a discussion (no related file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
We can build
cargo doc
in CI and publish on a domain we control.Alternatively, though I don't think this is a great idea, we could publish our crates to crates.io and have docs.rs build them. But then we need to concern ourselves with versioning those crates and stuff.
I guess we can think about either hosting a subproject, or doing build customisation: https://docs.readthedocs.io/en/stable/build-customization.html.
I'm resolving this.
a discussion (no related file):
Given the thread about cargo and dependencies, I started to doubt if we really want cargo. I did some helloworld-level attempt at linking rust with C in pure meson and it worked, so I'd like to discuss if instead of calling cargo, could we just write things in rust like we write in C. I understand this is suboptimal from Rust's POV, but we're doing it already in C, so it's not a regression, and rewriting in plain Rust is already a progress because of memory safety features.
We'll skip the rust "ecosystem", but this is not unheard of. Linux kernel will most likely do the same:
- thread 1: https://lore.kernel.org/rust-for-linux/CAFRnB2W+8XqOj0aL5DP2cMNLO2uR0DpPk-qB-oC7r=zT5N5gGA@mail.gmail.com/
- thread 2: https://lore.kernel.org/rust-for-linux/CAKfU0DLS5icaFn0Mve6+y9Tn1vL+eLKqfquvXbX4oCpYH+VapQ@mail.gmail.com/
- LWN article: https://lwn.net/Articles/889924/ with some great quotes like "For these developers, the idea of working in an environment where complex libraries are not obtainable with a few keystrokes starts to have a distinct lack of appeal after a while." or "The first temptation will be to either run and hide or to respond in a way that may not be compliant with anybody's code of conduct. "
I guess this needs to be discussed on one of the Monday's meetings.
.cargo/config.toml
line 6 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
The
target
directory is pretty well-managed by cargo and having two separate ones will only make rebuilds slower.Running cargo through ninja is not a complete solution — you could want to run
cargo check
on a particular crate you're working on, for example.
If you want other things with cargo, I guess you can add run_target()
(https://mesonbuild.com/Reference-manual_functions.html#run_target), or pass --target-dir
manually. I don't want to suggest people messing with builddir.
common/rust/Cargo.toml
line 4 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
Higher rustc version requirements may appear when code uses a particular feature that was stabilized after 1.56 — you don't need any explicit opt-in for that. If we want to support a particular rustc version, we should probably test building with it in CI.
OK, so we need some policy as to which version of rustc we require and reject all the dependencies that would make us bump our rustc requirement, and move it consciously, because in the end this affects on which distro versions we can package. Can you make a suggestion, preferably as an edit somewhere in Documentation/
?
In the meantime, I'll try to do something with CI (#512)
Pal/src/host/Linux-SGX/tools/get-token/Cargo.toml
line 14 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
I partly chose
argh
overclap
because it's smaller and should have less effect on compile time. I don't think taking into account which crates are available in Debian is a good strategy long-term, especially considering that we'll always need to handle some crates that aren't. Proper tooling shouldn't make the actual count of crates outside of Debian a problem.
The problem is, I don't want to be responsible for every dependency. If we use dependencies from Debian and they're compromised, then we can blame someone else (like tell them the usual thing in this case, "change your distribution"). If we vendor things, we become responsible for them (updating them and/or fixing security problems). That's why we very much care about what dependencies are in debian/ubuntu. It's not only about rust, although this PR brought the bigger amount of dependencies than we ever had, so it will be a far bigger problem here.
The best thing to do, like always, would be to have as few dependencies as possible (even 0 and drop cargo altogether), but given the state of rust ecosystem, I wonder if we wouldn't be able to do anything.
Scripts/meson.build
line 14 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
We want to share this between all crates we build, though.
Geee, IDK. This is something meson doesn't like, but I guess as long as we won't create target
directory or target, it'll work. OK, let's leave it as-is, but please add a comment about it so we won't make it "proper meson" in the future.
Scripts/run-cargo.sh
line 20 at r1 (raw file):
Previously, NieDzejkob (Jakub Kądziołka) wrote…
I went by our own documentation, which only mentions
debug
,debugoptimized
andrelease
. I guess I can create profiles for the rest too, but I'm not even sure whatcustom
andplain
should do.
plain
is -Ddebug=false -Doptimization=0
and custom
is if you put some values to -Ddebug
and -Doptimization
and they don't fit match any predefined -Dbuildtype
values (see the table in the link).
If you don't want to support custom
, then we need to explicitly error out somewhere (here or in meson.build
).
Signed-off-by: Jakub Kądziołka <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 14 of 19 files reviewed, 11 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 @NieDzejkob and @woju)
.cargo/config.toml
line 6 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
If you want other things with cargo, I guess you can add
run_target()
(https://mesonbuild.com/Reference-manual_functions.html#run_target), or pass--target-dir
manually. I don't want to suggest people messing with builddir.
Okay, I can see this being problematic — if we do it like this, running cargo build
manually with a different set of --features
than meson used will cause a misconfigured binary to be used. For cargo check
and similar, the build cache is largely separate anyway, so I'll just add the target
that can result from running cargo
manually to .gitignore
.
This did suggest a test case, though — will cargo get ran again when the set of features changes due to e.g. meson configure
. I tested this with a dirty hack: I changed the conditional to check direct
instead of the SGX driver, as I can have a machine where both configurations work without much effort. Then changing this via meson configure
did trigger a rebuild.
Scripts/meson.build
line 14 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
Geee, IDK. This is something meson doesn't like, but I guess as long as we won't create
target
directory or target, it'll work. OK, let's leave it as-is, but please add a comment about it so we won't make it "proper meson" in the future.
Done.
Scripts/run-cargo.sh
line 20 at r1 (raw file):
Previously, woju (Wojtek Porczyk) wrote…
plain
is-Ddebug=false -Doptimization=0
andcustom
is if you put some values to-Ddebug
and-Doptimization
and they don't fit match any predefined-Dbuildtype
values (see the table in the link).If you don't want to support
custom
, then we need to explicitly error out somewhere (here or inmeson.build
).
New revision now looks at optimization
and debug
directly, therefore supporting all buildtypes.
Description of the changes
This PR migrates our implementation of
gramine-sgx-get-token
to Rust. This is because:gramine-sgx
generate the token when the enclave is first launched, which requires an implementation we can link to Gramine itself. Python is not suitable for that.While Meson has some built-in support for building Rust code, it does not support making use of any crates, instead insisting on building each dependency with Meson. This is so impractical that I use
custom_target
instead.One question that might come up is the usage of dependencies from crates.io.
serde
). Indeed, external crates are considered easily accessible and are often cited as reasons to avoid additions tostd
(e.g.rand
).Cargo.lock
contains the source tarball hashes of all dependencies used. This is already no worse than some other dependencies Gramine downloads duringmeson setup
.cargo update
.cargo crev
is one possible solution I'm aware of, which involves a web of trust with users signing "I have reviewed the code and haven't found anything suspicious" or "I am the author of this crate".aesm_client
,sgxs
, andsgx_isa
). Like everything else though, they are licensed appropriately and work for our usecase. In the worst case, we can always fork these.How to test this PR?
I have done the following:
oot
setups (where AESM is required) and non-oot
setups (where a dummy token is used).This change is