Skip to content
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

Add specific errors for sealing failures #164

Merged
merged 3 commits into from
Oct 17, 2022
Merged

Conversation

nick-mobilecoin
Copy link
Collaborator

Add errors that provide more information on why a failure to seal data
occurs.

@nick-mobilecoin nick-mobilecoin self-assigned this Oct 11, 2022
@github-actions github-actions bot added the size/M PRs with less than 250 lines of changes label Oct 11, 2022
@nick-mobilecoin
Copy link
Collaborator Author

This initial change only updates the current error handling in sealing to use a seal specific error.
Subsequent commits will provide mirror parameter validation that sgx_seal_data_ex performs.

tservice/src/seal.rs Outdated Show resolved Hide resolved
tservice/src/seal.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@NotGyro NotGyro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me - I can see ways that it could be more specific, but having these broader categories of errors could be much easier to work with.

tservice/src/seal.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these errors are pointing in the wrong direction, unfortunately. The real issue that needs solving is "give the caller a more descriptive/fine-grained error code when the SDK would return something obtuse like INVALID_PARAMETER."

Like, INVALID_PARAMETER could mean "buffer not in enclave" or "buffer length too short" or "plaintext + mac length > u32::MAX" or any number of things, I'm saying you shouldn't have to read the SGX FFI PDF to figure that out, it should be immediately evident, even if that means duplicating the input validation that the SDK is doing in our rust code.

@github-actions github-actions bot added size/L PRs with more than 500 lines of changes and removed size/M PRs with less than 250 lines of changes labels Oct 13, 2022
Base automatically changed from feature/fix_urts_package to main October 13, 2022 17:22
@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Merging #164 (296ff6f) into main (428e87c) will decrease coverage by 0.51%.
The diff coverage is 56.66%.

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
- Coverage   84.31%   83.79%   -0.52%     
==========================================
  Files          44       44              
  Lines        2397     2444      +47     
==========================================
+ Hits         2021     2048      +27     
- Misses        376      396      +20     
Impacted Files Coverage Δ
tservice/src/seal.rs 52.54% <56.66%> (+1.77%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

tservice/src/seal.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some minor rewording of the Display format strings. For the logging case, it's generally prefered to keep them as a single sentence with no line breaks, due to how nested errors are displayed. e.g. an error hierarchy like:

FooError::Frobnicate(FrobnicateError::BlahBityBlah(BlahError::WastefulAlloc)

will end up getting printed as:

"Foo error while frobnicating: Frobincating requires blahbityblah: Blah is larger than necessary."

tservice/src/seal.rs Outdated Show resolved Hide resolved
nick-mobilecoin and others added 3 commits October 17, 2022 13:53
Add errors that provide more information on why a failure to seal data
occurs.
@nick-mobilecoin
Copy link
Collaborator Author

nick-mobilecoin commented Oct 17, 2022

Graphite Merge Job

Current status: ✅ Merged

This pull request was successfully merged as part of a stack.

This comment was auto-generated by Graphite.

Job Reference: MYnB2IroAk46QVSy8SQp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L PRs with more than 500 lines of changes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants