-
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
Merged
jul-sh
merged 10 commits into
project-oak:main
from
jul-sh:refactor-remote-attestation-handshake-to-make-unary-requests-easier
Mar 22, 2022
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
0789621
Do not clone additional info into each ServerHandshaker.
4963406
Pass a reference to handshaker steps, only cloning in the steps that …
f6e6797
obey cargo clippy: use slices over Vecs
b761612
Revert previous approach of passing reference per method call
c490e31
Use reference counting for cheaper clones
238a1a9
Merge branch 'main' into refactor-remote-attestation-handshake-to-mak…
3bb64bd
Update unary attestation
f626cf6
Update server_verifier
2e06b25
explicitly get reference for clarity
5523c4c
Store additional_info vec in SessionTracker to remove excess cloning
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ use anyhow::Context; | |
use futures::{Stream, StreamExt}; | ||
use oak_remote_attestation::handshaker::{AttestationBehavior, Encryptor, ServerHandshaker}; | ||
use oak_utils::LogError; | ||
use std::pin::Pin; | ||
use std::{pin::Pin, sync::Arc}; | ||
use tonic::{Request, Response, Status, Streaming}; | ||
|
||
/// Handler for subsequent encrypted requests from the stream after the handshake is completed. | ||
|
@@ -93,7 +93,7 @@ pub struct AttestationServer<F, L: LogError> { | |
/// Processes data from client requests and creates responses. | ||
request_handler: F, | ||
/// Configuration information to provide to the client for the attestation step. | ||
additional_info: Vec<u8>, | ||
additional_info: Arc<Vec<u8>>, | ||
/// Error logging function that is required for logging attestation protocol errors. | ||
/// Errors are only logged on server side and are not sent to clients. | ||
error_logger: L, | ||
|
@@ -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 commentThe 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? |
||
error_logger, | ||
}) | ||
} | ||
|
@@ -136,8 +136,8 @@ where | |
) -> Result<Response<Self::StreamStream>, Status> { | ||
let tee_certificate = self.tee_certificate.clone(); | ||
let request_handler = self.request_handler.clone(); | ||
let additional_info = self.additional_info.clone(); | ||
let error_logger = self.error_logger.clone(); | ||
let additional_info = self.additional_info.clone(); | ||
|
||
let response_stream = async_stream::try_stream! { | ||
let mut request_stream = request_stream.into_inner(); | ||
|
@@ -148,7 +148,7 @@ where | |
error_logger.log_error(&format!("Couldn't create self attestation behavior: {:?}", error)); | ||
Status::internal("") | ||
})?, | ||
additional_info, | ||
additional_info | ||
); | ||
while !handshaker.is_completed() { | ||
let incoming_message = request_stream.next() | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,7 @@ use crate::{ | |
MAXIMUM_MESSAGE_SIZE, REPLAY_PROTECTION_ARRAY_LENGTH, SERVER_IDENTITY_HEADER, | ||
}, | ||
}; | ||
use alloc::{vec, vec::Vec}; | ||
use alloc::{sync::Arc, vec, vec::Vec}; | ||
use anyhow::{anyhow, Context}; | ||
use assert_matches::assert_matches; | ||
use quickcheck::{quickcheck, TestResult}; | ||
|
@@ -96,7 +96,7 @@ fn test_serialize_server_identity() { | |
transcript_signature: Vec<u8>, | ||
signing_public_key: Vec<u8>, | ||
attestation_info: Vec<u8>, | ||
additional_info: Vec<u8>, | ||
additional_info: Arc<Vec<u8>>, | ||
) -> TestResult { | ||
if ephemeral_public_key.len() > KEY_AGREEMENT_ALGORITHM_KEY_LENGTH | ||
|| random.len() > REPLAY_PROTECTION_ARRAY_LENGTH | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. What does this test do? |
||
); | ||
} | ||
|
||
#[test] | ||
|
@@ -202,7 +204,7 @@ fn test_deserialize_message() { | |
default_array(), | ||
default_array(), | ||
vec![], | ||
vec![], | ||
Arc::new(vec![]), | ||
); | ||
let deserialized_server_identity = deserialize_message(&server_identity.serialize().unwrap()); | ||
assert_matches!(deserialized_server_identity, Ok(_)); | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.