-
Notifications
You must be signed in to change notification settings - Fork 226
Deplatforming #7049
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
Deplatforming #7049
Conversation
…EST_PLATFORM for some test control
It seems to me that in 99% of cases, this can be automatically decided without security implications, because either the join policy, or the consortium-issued transition service to open decide whether the attestation is acceptable. Having a way to force virtual is likely useful, either through an env var or through the configuration, but is not security critical for the reason above. Whether that's good usability is arguable.
Because we (still) use ctest as a launcher, I agree that this is good, but that's something we could decide to revisit, as the decrease in compile-time parameters also diminishes the value of that layer. |
100% |
I'm coming round to the idea of automatic platform detection, let's discuss tomorrow. Just listing a few discussion points:
|
I assume you mean a virtual attestation? I would say yes, not because I can think of a reason why it might be useful, but because it seems really easy to do, and could come in handy when isolating failures.
So long as the tests/test infra enforce a specific platform is being tested, I don't think so? If we are worried that the test auto-detection can fail silently too, we could push the number of tests run to bencher, and set up a threshold alert?
The attestation gets stored in Tx0. I think we want to tighten the initial consortium check to only open if we're happy with the platform, and we should be good from there. |
I'm currently convinced that auto-detection is fine, and we don't need any explicit/tested overrides. In my head: This means a CCF node will look for SNP IOCTL devices, and produce an SNP attestation if it finds them (at this point requiring some additional SNP endorsement server config), else it'll fall back and produce a virtual attestation. The Python infra will do the same check independently, to decide whether it is expecting SNP attestations (with associated tests), and to confirm the attestation format before submitting service open. CMake needs no knowledge of what platforms the tests are running on/targeting, so this can go entirely (I think - maybe something about test labels that need to contain platform info, but I think that can be inserted later). Then we convince ourselves that some of the tests are actually running on SNP, by writing a standalone "confirm_this_is_snp" script/util, and calling that early in the appropriate CI jobs. Possible future things we can support in future:
|
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.
Pull Request Overview
This PR removes the build‐time split between SNP and Virtual targets in both CMake and code, consolidating enclave handling into a single build with runtime platform detection.
- Eliminates
COMPILE_TARGET
andPLATFORM_*
defines, simplifying CMake scripts and sample configs - Introduces
infra.platform_detection
for test suites and replaces compile‐time checks withis_snp()
/is_virtual()
- Updates C++ host/enclave layers to use
ccf::pal::platform
and refactors quote generation
Reviewed Changes
Copilot reviewed 70 out of 71 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tla/make_traces.sh | Removed -DCOMPILE_TARGET=virtual from trace‐generation script |
tests/infra/platform_detection.py | Added runtime detection (is_snp , is_virtual , SNP_SUPPORT ) |
tests/infra/network.py | Unified key setup to always symlink |
tests/connections.py | Replaced conditional timeout with fixed value |
tests/infra/utils.py | Removed enclave_type parameter and adjusted signature |
Comments suppressed due to low confidence (2)
tests/infra/network.py:542
- The original code used
cp
for SNP platforms andln -s
for Virtual; unconditionally usingln -s
may break the intended behavior on SNP. Consider applyinginfra.platform_detection.is_snp()
here to choosecp
when true, preserving the prior logic.
cmd = ["ln", "-s"]
tests/connections.py:193
- [nitpick] Tests previously used a longer timeout when running on SNP (
timeout=3
); fixing it at 1s may introduce flakiness on SNP platforms. Consider restoring a conditional timeout based oninfra.platform_detection.is_snp()
.
timeout=1,
The build-time distinction between SNP and Virtual is unhelpful, results in a huge amount of duplication in our CMake. This was necessary on SGX, where the targets had significantly different build options, but is no longer doing anything.
This PR removes the
COMPILE_TARGET
option from CMake. This has a bunch of knock-on effects, that I think fall into a few distinct camps:ccf_kv.host
andccf_kv.snp
, we just haveccf_kv
. I've manually tried to verify in each instance that the calls to each target were always identical, but I may have missed some.PLATFORM_
preprocessor define. Most places were#if PLATFORM_VIRTUAL || PLATFORM_SNP
, so the#if/#endif
pair can be trivially removed. Deciding what kind of quote we produce is now a run-time decision (each binary is capable of trying to produce either format), based on the existingenclave.platform
config argument. I think that ought to be promoted to a CLI argument (security critical), but that's deferred.cchost
and then a.so
for each "enclave" application. But those applications are nowlibjs_generic.so
, rather thanlibjs_generic.virtual.so
/libjs_generic.snp.so
.COMPILE_TARGET
, but it's now calledCCF_TEST_PLATFORM
, and hopefully used in few enough places to clarify intent. Perhaps the default value should be derived from capability detection? TBD.I've only run a handful of builds and tests locally, this is likely missing some obvious breakages, so opening as a Draft initially and awaiting CI coverage.
Also, I think this is a big enough breaking change that it shouldn't affect
6.0
, so I propose arelease/6.x
branch.Follow-ups:
lts_
story, but I think right now we can be pretty aggressive? But let's do it separately to derisk.UPDATE
As often happens, this PR lived for a while and changed significantly in that period. The description above is now slightly inaccurate, but kept for posterity. The final merge leans hard on platform detection to avoid any required top-level configuration. Overrides via env var are still possible, but the default behaviour (independently per-node and in Python infra) is to test once at startup for SNP device support, else fall-back to virtual.