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

runtime: Add support for PCS attestation #4806

Merged
merged 17 commits into from
Aug 19, 2022
Merged

Conversation

kostko
Copy link
Member

@kostko kostko commented Jun 16, 2022

No description provided.

@kostko kostko added the c:runtime Category: runtime label Jun 16, 2022
@kostko kostko force-pushed the kostko/feature/pcs-attestation branch from 9e948b6 to 9cc9377 Compare July 5, 2022 16:09
@kostko kostko force-pushed the kostko/feature/pcs-attestation branch 6 times, most recently from 25f87b6 to 4e1a8b5 Compare July 17, 2022 19:24
@kostko kostko force-pushed the kostko/feature/pcs-attestation branch from 4e1a8b5 to 52f91b8 Compare July 28, 2022 12:44
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #4806 (1bed3f1) into master (bc9532b) will decrease coverage by 0.69%.
The diff coverage is 61.58%.

@@            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     
Impacted Files Coverage Δ
go/ias/init.go 100.00% <ø> (ø)
go/runtime/host/protocol/types.go 54.54% <ø> (ø)
go/runtime/host/sgx/ecdsa.go 7.69% <3.92%> (-4.31%) ⬇️
go/common/sgx/ias/avr.go 41.11% <36.84%> (-1.23%) ⬇️
go/common/sgx/pcs/tcb.go 50.41% <41.50%> (-4.18%) ⬇️
go/common/node/node.go 68.26% <43.75%> (-0.20%) ⬇️
go/keymanager/api/api.go 80.64% <50.00%> (ø)
go/registry/api/api.go 57.33% <50.00%> (-0.39%) ⬇️
go/common/sgx/quote/quote.go 55.55% <55.55%> (ø)
...o/consensus/tendermint/apps/scheduler/scheduler.go 71.42% <57.14%> (-0.19%) ⬇️
... and 81 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kostko kostko force-pushed the kostko/feature/pcs-attestation branch 4 times, most recently from 9ae6d2d to a334fe0 Compare August 2, 2022 09:21
@kostko kostko marked this pull request as ready for review August 2, 2022 18:28
@kostko kostko force-pushed the kostko/feature/pcs-attestation branch 3 times, most recently from c9f8531 to d7e4fe5 Compare August 4, 2022 17:17
go/common/sgx/pcs/quote.go Show resolved Hide resolved
go/common/sgx/pcs/tcb.go Show resolved Hide resolved
go/common/sgx/pcs/tcb.go Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/pcs-attestation branch 5 times, most recently from 8add9a9 to d0cff55 Compare August 10, 2022 08:51
@pro-wh
Copy link
Contributor

pro-wh commented Aug 10, 2022

image
alright let's get reviewing

Copy link
Contributor

@pro-wh pro-wh left a 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))
Copy link
Contributor

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.
Copy link
Contributor

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

go/common/node/sgx.go Outdated Show resolved Hide resolved
go/common/node/sgx.go Show resolved Hide resolved
return nil
case 1:
// New version, call the default unmarshaler.
type scv1 SGXConstraints
Copy link
Contributor

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

go/common/sgx/pcs/quote.go Show resolved Hide resolved
}

tcbInfo, err := bnd.TCBInfo.open(ts, pk)
qeInfo, err := bnd.QEIdentity.open(ts, policy, pk)
Copy link
Contributor

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"`
Copy link
Contributor

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)
Copy link
Contributor

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?

Copy link
Member Author

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).

go/common/sgx/pcs/tcb.go Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/pcs-attestation branch from 82a2feb to 4714f82 Compare August 12, 2022 10:14
@pro-wh
Copy link
Contributor

pro-wh commented Aug 12, 2022

for the pcs stuff itself, do we have any design overview?

@pro-wh
Copy link
Contributor

pro-wh commented Aug 15, 2022

thanks, I'll read through

Copy link
Contributor

@pro-wh pro-wh left a 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?

Copy link
Contributor

@pro-wh pro-wh left a 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

@kostko
Copy link
Member Author

kostko commented Aug 16, 2022

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?)

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.

what do we use for the custom quoting enclave? an open source one from Intel?

That's right. It is verified against the QE identity obtained from PCS.

@kostko kostko force-pushed the kostko/feature/pcs-attestation branch from 4714f82 to e0ff39d Compare August 16, 2022 09:17
@pro-wh
Copy link
Contributor

pro-wh commented Aug 16, 2022

the PCS artifacts can be cached for a while

nice!

QE identity obtained from PCS

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).
@kostko kostko force-pushed the kostko/feature/pcs-attestation branch from e0ff39d to 1bed3f1 Compare August 18, 2022 17:16
@kostko kostko merged commit 0d478be into master Aug 19, 2022
@kostko kostko deleted the kostko/feature/pcs-attestation branch August 19, 2022 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:runtime Category: runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants