-
Notifications
You must be signed in to change notification settings - Fork 115
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
runtime: Add support for PCS attestation #4806
Conversation
9e948b6
to
9cc9377
Compare
25f87b6
to
4e1a8b5
Compare
4e1a8b5
to
52f91b8
Compare
Codecov Report
@@ Coverage Diff @@
## master #4806 +/- ##
==========================================
- Coverage 67.12% 66.42% -0.70%
==========================================
Files 456 460 +4
Lines 50539 50791 +252
==========================================
- Hits 33923 33739 -184
- Misses 12460 12856 +396
- Partials 4156 4196 +40
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
9ae6d2d
to
a334fe0
Compare
c9f8531
to
d7e4fe5
Compare
8add9a9
to
d0cff55
Compare
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.
good stuff so far leading up to the actual PCS related work
@@ -35,6 +35,8 @@ var ( | |||
methodStateToGenesis = serviceName.NewMethod("StateToGenesis", int64(0)) | |||
// methodGetEvents is the GetEvents method. | |||
methodGetEvents = serviceName.NewMethod("GetEvents", int64(0)) | |||
// methodConsensusParameters is the ConsensusParameters method. | |||
methodConsensusParameters = serviceName.NewMethod("ConsensusParameters", int64(0)) |
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.
oo this looks suitable for us to allow on our public grpc node
// Determine Entity structure version. | ||
v, err := cbor.GetVersion(data) | ||
if err != nil { | ||
v = 0 // Previous SGXConstraints structures were not versioned. |
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.
interesting, maybe we should have built something like this into cbor.GetVersion
although I suppose now it's too late, if some uses already defined a version 0
return nil | ||
case 1: | ||
// New version, call the default unmarshaler. | ||
type scv1 SGXConstraints |
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.
seems more common to define the versioned struct first and define an unversioned type based on that
} | ||
|
||
tcbInfo, err := bnd.TCBInfo.open(ts, pk) | ||
qeInfo, err := bnd.QEIdentity.open(ts, policy, pk) |
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.
ℹ️ swapped 🤷 and added policy
argument
// QuotePolicy is the quote validity policy. | ||
type QuotePolicy struct { | ||
// TCBValidityPeriod is the validity (in days) of the TCB collateral. | ||
TCBValidityPeriod uint16 `json:"tcb_validity_period"` |
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.
ℹ️ previously we treated it as valid until the "next update" time
if nextUpdate, err = time.Parse(TimestampFormat, ti.NextUpdate); err != nil { | ||
return fmt.Errorf("pcs/tcb: invalid issue date: %w", err) | ||
if _, err = time.Parse(TimestampFormat, ti.NextUpdate); err != nil { | ||
return fmt.Errorf("pcs/tcb: invalid next update date: %w", err) |
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.
so what was this then, if not something we should use to judge the validity period? should we even try to parse it, if we're not using it?
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.
We could skip parsing, I left it there to check whether the structure is well-formed (I don't expect it to ever be malformed as it is signed by Intel).
82a2feb
to
4714f82
Compare
for the pcs stuff itself, do we have any design overview? |
thanks, I'll read through |
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.
summary of the materials on PCS:
- PCS stands for "provisioning certification service," a component in Intel's DCAP line of work
- in the DCAP way of doing attestation, it's intended that the node obtain Intel's signature (regarding CPU authenticity) on its own, as opposed to the remote challenger asking IAS about a quote
- the node gets that first signature from PCS
- then, that signature from Intel certifies an Intel architectural enclave called the "provisioning certification enclave" and its "provisioning certification key" (PCK) which then signs for a custom quoting enclave and its "attestation key," which then the node then uses to produce quotes for specific application enclaves
- thus the proof in the DCAP way is an entire chain of signatures: Intel's root key in PCS -> PCK -> attestation key -> the enclave
- the overall workflow is very similar to how we would previously have nodes talk to IAS themselves (resulting in a proof we called an AVR), although this condenses multiple IAS interactions with a single PCS interaction when we want to generate proofs for multiple enclaves
and as for DCAP's stated goals:
- "eliminates runtime dependancies [sic] on external services" -> still have to contact PCS, so our use still depends on external services (unless we say node operators should do that on their own and it's not part of oasis-node? would that even make a practical difference?)
- "enables trust decisions to be made in-house" -> IAS previously gave various TCB info and we already had to make decisions in-house. this just seems more complicated, with more layers of non-application enclaves needing all sorts of comparisons
the actual implementations for generating and verifying these proofs were built in earlier PRs
extra questions:
- what do we use for the custom quoting enclave? an open source one from Intel?
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.
entering this as approved in that I don't oppose this change, although I haven't looked at the implementation.
other reviewers, please continue to review.
authors, let me know if there's anything specific in the code that you want feedback on
It is slightly better in the sense that the PCS artifacts can be cached for a while while IAS responses cannot be (as everything is coupled together in AVRs). So a PCS outage would not immediately start causing availability problems (while IAS would) for nodes that have the artifacts cached. Note that currently we do not implement caching of TCB infos, but this can be done transparently in the future.
That's right. It is verified against the QE identity obtained from PCS. |
4714f82
to
e0ff39d
Compare
nice!
oh I see there's this endpoint https://api.portal.trustedservices.intel.com/documentation#pcs-qe-identity-v3 under the heading "... for ECDSA Attestation." there were aterials talking about how a custom quoting system could be built using ECDSA, so that must be an instantiation of the idea provided by Intel |
This removes the old ias.debug.allow_debug_enclaves option which was used only in tests. The new option covers both IAS and PCS quotes.
This avoids an additional query for latest height given that the host is updating the node on every consensus layer block anyway.
Aborting in case the verifier fails makes it more explicit for the host that the runtime will be unusable (otherwise all requests would return an internal verifier error anyway).
e0ff39d
to
1bed3f1
Compare
No description provided.