-
Notifications
You must be signed in to change notification settings - Fork 148
go: Per-role quote policies #6410
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for oasisprotocol-oasis-core canceled.
|
| rak signature.PublicKey, | ||
| rek *x25519.PublicKey, | ||
| nodeID signature.PublicKey, | ||
| n *Node, |
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.
NIT: Followed the pattern from the admission policy. Alternative is to pass nodeID and roles (bitmask) by value.
Passing whole struct by reference feels dangerous as accidental mutations in the helper functions (possibly in the future) may affect the logic upstream.
fddedc5 to
9e35bd3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6410 +/- ##
===========================================
+ Coverage 0 64.51% +64.51%
===========================================
Files 0 698 +698
Lines 0 68130 +68130
===========================================
+ Hits 0 43955 +43955
- Misses 0 19130 +19130
- Partials 0 5045 +5045 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
13d201f to
5aec2d9
Compare
runtime/src/identity.rs
Outdated
| // TODO Add per role quote policy. | ||
| // But if the runtime cannot trust the host how can it obtain the "role" that is being used for? | ||
| // Query consensus registry using `node_id`, to obtain the current node struct and thus it's roles? | ||
| // But the host could set any node_id... |
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.
?
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.
cc @peternose | @kostko.
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.
It needs to query verified consensus state via the consensus verifier.
The host cannot spoof the node ID as it is included in the TEE capability which includes a node ID which is checked during on-chain registration.
Binding generated during attestation:
oasis-core/runtime/src/attestation.rs
Lines 143 to 150 in f73f73c
| // Sign the report data, latest verified consensus height, REK and host node ID. | |
| let consensus_state = self.consensus_verifier.latest_state().await?; | |
| let height = consensus_state.height(); | |
| let rek = self.identity.public_rek(); | |
| let h = SGXAttestation::hash(&verified_quote.report_data, &node_id, height, &rek); | |
| let signature = self.identity.sign(ATTESTATION_SIGNATURE_CONTEXT, &h)?; | |
| Ok(Body::RuntimeCapabilityTEERakQuoteResponse { height, signature }) |
And verified by consensus during each registration refresh:
oasis-core/go/common/node/sgx.go
Line 250 in f73f73c
| return sa.verifyAttestationSignature(sc, rak, rek, verifiedQuote.ReportData, nodeID, height) |
We could also change the way the host returns its identity by having the runtime produce a nonce which the host must sign using a special context and return the signature together with the public key. This would prove the host has access to the private key.
5aec2d9 to
db8d2a6
Compare
Observe that already prior to this commit consensus and runtime quote verification with regards to runtime constaints were not symetric. Consensus part in addition to the runtime constrains may also apply default constaints from the consensus parameters. The runtime part on the other hand only applies runtime constraints. Currently, this is not problematic as as default policy as per the consensus parameters is nil, but this may change in the future. Nit: I would change to sc.ApplyDefaultConstraints(cfg.SGX), as normaly the mutated part should be pointer receiver.
Alternative would be to pass the expected roles during initialization and bypass using host protocol.
This verification was redundant, possibly causing confusing logs.
db8d2a6 to
750e25a
Compare
Wip.
Closes #6387, follows #6331
Status:
This is ready for preliminary high level review, mainly I would like us to focus on the TODO, that I also explained under Challenges in the design doc.
Once we align on the requirements, and finalize some implementation/design decision I will write the final code and thorough test suite, given this is security critical PR. Please ignore style and code quality until then.