-
Notifications
You must be signed in to change notification settings - Fork 120
Stop cloning additional_info into each ServerHandshaker #2574
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
Stop cloning additional_info into each ServerHandshaker #2574
Conversation
f51e850
to
05db503
Compare
This is important, as with unary request the runtime will keep a cache of ServerHandshaker instances
05db503
to
0789621
Compare
pub fn next_step( | ||
&mut self, | ||
message: &[u8], | ||
additional_info: Vec<u8>, |
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 may be missing something, but what determines what to pass here, and at exactly which step? AFAICT the additional info is only used at one particular step, so we still need to pass something else for all the other steps? Looking at the tests below it seems that we do keep passing the same value, but that is only actually used in one.
Also the idea of this method is that we just pipe in messages from the underlying transport, and it should move along the various steps in the state machine (and eventually produce the encryptor), so adding this argument here seems to complicate the API.
I'll let @conradgrobler and @ipetr0v weigh in too.
Another thing to consider is that, in practice, in most protocols that have some space for additional info, only a fixed amount of additional data can be passed in, usually 256 bits, usually the result of a hashing function (rather than the actual data). So maybe another approach worth considering in order to reduce the memory usage would be to make this a fixed size array, but keep it in the constructor. Would that work?
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.
Yes, only used some for some steps, passing it always is still not optimal.
what determines what to pass here
additional_info
maps to this config. Afaik the config stays unchanged after the runtime is initiated. Hence it'd always be a clone of the same vec.
Which leads me to the suggestion I raised in the chat, and which be the optimal approach for memory: We create a single instance of the additional_info vec in the loader, and only pass down references (with a static lifetime) [Ref http://chat/room/AAAAp6czer4/-EYB2OqgJQs].
Is that safe? Or do we expect the config (which is what additional_info contains atm) to ever be changed/dropped after the runtime is started?
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 think it's safe to assume it does not change, but I'm not sure that implies it can be 'static
-- maybe it can be coerced to a static value at runtime by "leaking" it? but I'm not sure that's idiomatic.
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.
Also see my other comment below, maybe the fix you are looking for is just a one line change there?
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.
How about using an Arc<Vec<u8>>
rather than a static reference? Cloning it should be cheap, and it would remove the need to pass the additional info into every step.
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.
If passing it to the method is not idiomatic, we can make it a shared reference (std::sync::Arc) indeed. That would probably be easier than declaring the lifetimes.
The only blocker is that as of #2556 the oak_remote_attestation
crate is no_std compatible. This means we cannot use std::sync::Arc
without breaking that.
@conradgrobler Do we plan to support shared references in no_std in the 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.
You can use alloc::sync::Arc
. We will always have an allocator, even in no_std environements.
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.
Thanks for the pointer, will use that. Also realized that for no_std we'll probs be moving the gRPC communication logic out of the trusted runtime anyways.
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.
Will we? I thought all this stuff will remain in the trusted binary, since it drives the remote attestation protocol.
Anyways, I still don't think we should change this method to accept the additional info param. Especially in the non-std, blocking model, this method should look like a call
or invoke
function from bytes to bytes (with some extra wrappers), so that we can compose it more nicely with the rest of the building blocks of the runtime. And at some point it should itself also accept another callback that it invokes with the decrypted data from remote attestation, which itself would follow the same paradigm.
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.
Will we? I thought all this stuff will remain in the trusted binary, since it drives the remote attestation protocol.
The gRPC protocol implementation only, remote attestation logic will ofc still be performed within the trusted binary. Most of the changes in this PR do concern the "remote attestation", but tonic's (multi-threaded) gRPC server implementation is what drives the original need for cloning here. The async trait service implementation is why the compiler cannot infer lifetimes for the remote attestation code used within.
Anyways, I still don't think we should change this method to accept the additional info param
yes, we're on the same page. As covered in the prev discussion, we'll count references for now. :)
@@ -114,7 +114,7 @@ where | |||
Ok(Self { | |||
tee_certificate, | |||
request_handler, | |||
additional_info, | |||
additional_info: Arc::new(additional_info), |
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.
Do we prefer a let declaration over instantiating the Arc in the struct instantiation?
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.
This looks fine to me.
@@ -114,7 +114,7 @@ where | |||
Ok(Self { | |||
tee_certificate, | |||
request_handler, | |||
additional_info, | |||
additional_info: Arc::new(additional_info), |
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.
Do we prefer a let declaration over instantiating the Arc in the struct instantiation?
@tiziano88 @conradgrobler would you mind taking another look? Updated this to use reference counting. Thanks! |
@tiziano88 @conradgrobler would you mind taking another look? Updated this to use reference counting. |
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.
LG, fewer clones means less risk that state may get out of sync, but I still think that we may refactor this entire code to be non-async because of no_std requirement, and in that case we should be able to use plain &
refs with explicit lifetimes. Anyways we can look into it later.
@jul-sh just to confirm, is that (i.e. non-async) the plan for unary attestation? Once we separate it from the gRPC logic of course (which can stay async). Actually I think I'm still a bit unclear exactly which crates will go into the trusted vs untrusted binary eventually, could you remind me?
@@ -114,7 +114,7 @@ where | |||
Ok(Self { | |||
tee_certificate, | |||
request_handler, | |||
additional_info, | |||
additional_info: Arc::new(additional_info), |
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.
This looks fine to me.
@@ -30,7 +30,7 @@ pub fn into_server_identity_verifier( | |||
config_verifier: ConfigurationVerifier, | |||
) -> ServerIdentityVerifier { | |||
let server_verifier = move |server_identity: ServerIdentity| -> anyhow::Result<()> { | |||
let config = ConfigurationInfo::decode(server_identity.additional_info.as_ref())?; | |||
let config = ConfigurationInfo::decode(&*server_identity.additional_info.as_slice())?; |
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 think there should be some way of doing this without &*
, maybe a combination of as_bytes
, as_slice
, as_ref
, but I'm never sure in which order.
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.
yeah, I wasn't sure what the rusty convention is here. The syntax I first used was even more unwieldy: &**server_identity.additional_info
😅
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.
Changed to server_identity.additional_info.as_ref().as_slice()
which seems nicer
Yup, that'd pretty much what I tried to express in #2574 (comment), sorry if it wasn't clear. Does mean that these code changes will be obsolete soon, but I think it was still a valueable excerise.
Yup! We'd have to, since the current plan for no_std is to not support threads.
Depends on how we make no_std work, but on a high level as follows: trusted binary: current runtime minus |
Reproducibility Index:
Reproducibility Index diff: diff --git a/reproducibility_index b/reproducibility_index
index 8deef26..c3f2f8d 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,2 +1,2 @@
-70f60debbe969656ee5486c1bfecad28c2eedcf45a0546b03c8db73a641b261a ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
-2619b250a929ecc3fda9d4c1f761b7a151e5bc892426d0ff70024512a1c2e86b ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe
+7725608949b8b30dc1b57bf822d97862ba05a82c8078bbf7f278d364e2e48218 ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
+a65809ded8fda966b79e8d5e8ef23b817716405a08f6ed6d20e9766c30a393c7 ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe
|
@@ -131,7 +131,9 @@ fn test_serialize_server_identity() { | |||
assert!(result.is_ok()); | |||
TestResult::from_bool(result.unwrap()) | |||
} | |||
quickcheck(property as fn(Vec<u8>, Vec<u8>, Vec<u8>, Vec<u8>, Vec<u8>, Vec<u8>) -> TestResult); | |||
quickcheck( | |||
property as fn(Vec<u8>, Vec<u8>, Vec<u8>, Vec<u8>, Vec<u8>, Arc<Vec<u8>>) -> TestResult, |
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.
What does this test do?
We presently clone
additional_info
for each client that remotely attests the server. This doesn't seem quite ideal, as this value should be consistent everywhere. Cloning it might lead to unexpected state propagation.