runtime: Stop using webpki to verify the IAS cert chain #4021
Conversation
This file contains 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
Yawning
force-pushed
the
yawning/feature/die-ring-die
branch
5 times, most recently
from
d118031 to
18bf728
Compare
Codecov Report
@@ Coverage Diff @@
## master #4021 +/- ##
==========================================
+ Coverage 66.78% 67.01% +0.22%
==========================================
Files 391 410 +19
Lines 38138 42183 +4045
==========================================
+ Hits 25472 28270 +2798
- Misses 9017 9928 +911
- Partials 3649 3985 +336
Continue to review full report at Codecov.
|
Yawning
force-pushed
the
yawning/feature/die-ring-die
branch
from
18bf728 to
0af5334
Compare
Yawning
requested review from
kostko,
peterjgilbert,
pro-wh and
ptrus
as code owners
kostko
reviewed
Jun 14, 2021
Comment on lines
+394
to
+399
| let anchor = match parse_x509_certificate(&cert_ders[1]) { | ||
| Ok((_, cert)) => cert, | ||
| Err(_) => return Err(AVRError::MalformedCertificateDER.into()), | ||
| }; |
There was a problem hiding this comment.
Suggested change
| let anchor = match parse_x509_certificate(&cert_ders[1]) { | |
| Ok((_, cert)) => cert, | |
| Err(_) => return Err(AVRError::MalformedCertificateDER.into()), | |
| }; | |
| let (_, anchor) = parse_x509_certificate(&cert_ders[1]) | |
| .map_err(|_| AVRError::MalformedCertificateDER.into())?; |
There was a problem hiding this comment.
error[E0282]: type annotations needed
--> runtime/src/common/sgx/avr.rs:397:10
|
397 | .map_err(|_| AVRError::MalformedCertificateDER.into())?;
| ^^^^^^^ ---------------------------------------- this method call resolves to `T`
| |
| cannot infer type for type parameter `F` declared on the associated function `map_err`
0af5334 to
8052db3
Compare
kostko
approved these changes
Jun 14, 2021
Yawning
force-pushed
the
yawning/feature/die-ring-die
branch
from
8052db3 to
e42dd0f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
It's German for "The Ring The".
This removes the webpki (and consequently) ring dependency by switching to manually verifying the IAS AVR certificate chain, and AVR signature. It makes a number of assumptions about how IAS does things (that are part of the API specification), so it is probably ok, though if Intel ever decides to change this, we will be rather sad.
The unfortunate thing about this is that I personally like ring/webpki as far as libraries go for the most part, but, having to carry around a fork just to get SGX support due to a trivial patch not being merged is not worth the hassle.
Fixes #2683