Enclave host compatibility check #2532
Conversation
|
enclave_host_compat@24870 aka 20210506.7 vs main ewma over 20 builds from 24526 to 24849 Click to see table
|
src/enclave/main.cpp
Outdated
| @@ -59,6 +61,13 @@ extern "C" | |||
| } | |||
| #endif | |||
|
|
|||
| // Host and enclave versions must match. Otherwise the node may crash much | |||
| // later (e.g. unhandled ring buffer message on either end) | |||
| if (std::string(host_version) != ccf::ccf_version) | |||
There was a problem hiding this comment.
I might be missing something here but the comment in the PR says you compare commits but this is comparing versions. Not that I think there is any issue with comparing either one.
There was a problem hiding this comment.
Good point. The ccf::ccf_version is retrieved from common/version.h which is generated by cmake based on the long version string which is derived by git describe --tags, and changes for every commit (e.g. "ccf-1.0.0-rc1-40-ge1d549c04" for my current local checkout). So yeah, it's referred to as version because it will be a nice and neat version for major releases (e.g. ccf-1.0.0) but is more specific for everything in between.
src/common/enclave_interface_types.h
Outdated
| NodeAlreadyCreated = 2, | ||
| VersionMismatch = 3, | ||
| ConsensusNotAllowed = 4, | ||
| TooManyThreads = 5, |
There was a problem hiding this comment.
MoreThreadsThanTCSCount would be a little more precise here
There was a problem hiding this comment.
This seems to be hardcoded to 24 in our code base, independently of the oe_sign.conf NumTCS. Am I missing something?
| #include "ds/json.h" | ||
| #include "ds/logger.h" | ||
| #include "ds/spin_lock.h" | ||
| #include "ds/stacktrace_utils.h" | ||
| #include "enclave.h" | ||
| #include "enclave_time.h" | ||
| #include "oe_shim.h" | ||
| #include "version.h" |
There was a problem hiding this comment.
Just a note, but don't think its actually a problem at the moment.
Most of our directory-less includes are in the same directory as the current file. This is an exception, since its in build/generated. This is not installed, so is not visible to an app. Its unclear that this is different and may confuse us when it breaks in future - perhaps we should put these all under build/_gen/generated, so we can call them generated/version.h here (but keeping an extra layer so we're adding build/_gen to the include dirs, not build/).
This won't break at the moment because apps don't actually build this file (its built into libccf, and they link to that). But now that it looks like we have a ccf_version that can be used within the enclave, its probably only a matter of time before we try to use that in an app, and find that the include is surprisingly unavailable.
I'm not sure what we'd do to fix this. Maybe I'm saying version.h should be put under include/ccf as a public header, even though its generated?
There was a problem hiding this comment.
This a very good point, I think distributing version.h as a public include is an excellent idea.
|
I think a check on the host would be more natural than a check in the enclave. If we ever want to use mixed versions, I think this will always be a newer host trying to talk to an older enclave (ie - the host is cheaper to rebuild and redeploy than the enclave, so if we can patch just the host that's preferable). If the enclave is checking versions, this new host needs to impersonate an old host, and potentially different old host-versions depending on which enclave its talking to. If the host did the check, this could be more naturally expressed in the host code ("I'm 2.4.5, but I know I can still talk to an enclave on 2.4.3 or 2.4.4"). RE: Dealing with out-strings - I think we just create a big-enough buffer (say 128 chars), and give the enclave an out-ptr and size as we do for certs. |
…clave_host_compat
Good point. This is what I initially intended but the idea somehow got lost along the way. This is now done. |
Resolves #1307
We now check that the enclave and the host have been built from the exact same commit.
The check is run by the enclave so that we don't have to deal with the awkward ECALL handling of
outstrings (see https://github.com/openenclave/openenclave/blob/cd72fd7069488ba6f453c8f5f47bd9fd9a6e6c0d/docs/GettingStartedDocs/Edger8rGettingStarted.md#you-are-on-your-own),but at the cost of not being able to report a useful error message when the version check fails (enclave log message is not output by the host as too early in the enclave creation process). I also tried to delay the point at which the version check is done, so that we can output a useful error message, but it seems too late and the node has already done a fair bit of work by then.Edit: the enclave now returns a specific error code on the first ECALL, that the host can print.As always, feedback welcome!