Skip to content

Per role quote policies#6471

Open
martintomazic wants to merge 3 commits intomasterfrom
martin/feature/per-role-quote-policies-v2
Open

Per role quote policies#6471
martintomazic wants to merge 3 commits intomasterfrom
martin/feature/per-role-quote-policies-v2

Conversation

@martintomazic
Copy link
Contributor

@martintomazic martintomazic commented Mar 4, 2026

Replaces #6410 following simplifications agreed in the #6387.

Considerations:

  • Can be simplified further: Per role quote policies #6471 (comment)
  • We can make the runtime sign the role information as part of the attestation (similar to the node ID), so if the host lied about its role during RHP it can be later verified.
    • New RHP method, technically should not be breaking, add role as part of the hash structure. Breaking?
  • Architectural smell exposed: Support for compute runtime quote policy #6387 (comment).
  • e2e test or at least sanity that everything is wired correctly and access gated.

@netlify
Copy link

netlify bot commented Mar 4, 2026

Deploy Preview for oasisprotocol-oasis-core canceled.

Name Link
🔨 Latest commit efbc5bc
🔍 Latest deploy log https://app.netlify.com/projects/oasisprotocol-oasis-core/deploys/69a97a06514b0c00080e3c3f

Comment on lines +39 to +44
// PerRolePolicy defines additional role specific quote policies, that overwrite
// Policy when node with these roles does an attestation.
//
// A valid entry is for either [RoleComputeWorker] or [RoleObserver]. Single entry
// should not encode multiple roles.
PerRolePolicy map[RolesMask]quote.Policy `json:"per_role_policy,omitempty"`
Copy link
Contributor Author

@martintomazic martintomazic Mar 4, 2026

Choose a reason for hiding this comment

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

Given that:

  • We only allow it for observer and compute roles
  • Keymanager runtime kind does not allow it
  • We don't see discriminating observer/compute access, as they both have access to same secrets,

How about simplifying things further and instead have ComputePolicy *quote.Policy that only applies for the compute runtimes for the observer/compute roles.

This should simplify assumptions, tests, comments, validations and avoid the need for new "at most one runtime sgx role" invariant.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense to me, no need to have a general map that introduces unnecessary complexity.


// Verify verifies the node's TEE capabilities, at the provided timestamp and height.
func (c *CapabilityTEE) Verify(teeCfg *TEEFeatures, ts time.Time, height uint64, constraints []byte, nodeID signature.PublicKey, isFeatureVersion242 bool) error {
func (c *CapabilityTEE) Verify(teeCfg *TEEFeatures, ts time.Time, height uint64, constraints []byte, nodeID signature.PublicKey, nodeRoles RolesMask, isFeatureVersion242 bool) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NIT: All this functions should accept validation options structs?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, this is why we introduced the TEEFeatures struct a while back (which is now somewhat obsolete due to the use of feature versions). There are already too many parameters everywhere.


// ValidateBasic performs basic descriptor validity checks.
func (n *Node) ValidateBasic(strictVersion bool) error {
func (n *Node) ValidateBasic(strictVersion bool, isFeatureVersion242 bool) error {
Copy link
Contributor Author

@martintomazic martintomazic Mar 4, 2026

Choose a reason for hiding this comment

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

Nit: There is not such thing as ValidateBasic. Only Validate with clear invariants and tests for the invariants the Validate will check. Moreover, the validation params should probably be passes as part of the validation options struct. Possibly this check could also be part of the VerifyRegisterNodeArgs directly which avoids some additional changes, but feels off there.

@martintomazic martintomazic force-pushed the martin/feature/per-role-quote-policies-v2 branch from 64ef9a0 to a790a46 Compare March 4, 2026 11:35
Comment on lines +600 to +602
if r.Kind == KindKeyManager && cs.PerRolePolicy != nil {
return fmt.Errorf("%w: invalid SGX TEE constraints: keymanager runtime with per-role policies", ErrInvalidArgument)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or make it part of the ValidateBasic, again this should be Validate with validate option struct so that it is clear what we are validating.

@martintomazic martintomazic linked an issue Mar 5, 2026 that may be closed by this pull request
Comment on lines +107 to +110
// PolicyFor returns a matching per-role policy when present, or otherwise falls back to the default policy.
//
// This function expects role mask that has at most one runtime SGX role.
func (s *SGXConstraints) PolicyFor(roles RolesMask) *quote.Policy {
Copy link
Contributor Author

@martintomazic martintomazic Mar 5, 2026

Choose a reason for hiding this comment

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

It would be so much nicer if we would have RuntimeSGXRole type so that comments, invariants and corner cases are instead captured by the type system. Even per role policies could use this type as a key. Not sure there is practical / idiomatic way to achieve this in go.

@martintomazic martintomazic force-pushed the martin/feature/per-role-quote-policies-v2 branch from a790a46 to efbc5bc Compare March 5, 2026 12:41
@martintomazic martintomazic marked this pull request as ready for review March 5, 2026 13:11
@martintomazic
Copy link
Contributor Author

Ready for the first round of reviews.

Please focus on the considerations outlined in the context and existing threads.

Given motivation and intended usage of this feature, I am in favor of replacing per-role policies with a single compute runtime policy that applies to both observer and compute roles.

@@ -0,0 +1,4 @@
go/registry/api: Allow at most one runtime SGX role
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
go/registry/api: Allow at most one runtime SGX role
go/registry/api: Allow at most one runtime TEE role


// AtMostOneRuntimeSGXRole returns true when RoleMask has at most one SGX runtime role.
func (m RolesMask) AtMostOneRuntimeSGXRole() bool {
sgxRoles := m & (RoleComputeWorker | RoleObserver | RoleKeyManager)
Copy link
Member

Choose a reason for hiding this comment

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

The set of roles should be a top-level constant. Also note that these are not SGX-specific so you should use "TEE" instead of "SGX".


// Verify verifies the node's TEE capabilities, at the provided timestamp and height.
func (c *CapabilityTEE) Verify(teeCfg *TEEFeatures, ts time.Time, height uint64, constraints []byte, nodeID signature.PublicKey, isFeatureVersion242 bool) error {
func (c *CapabilityTEE) Verify(teeCfg *TEEFeatures, ts time.Time, height uint64, constraints []byte, nodeID signature.PublicKey, nodeRoles RolesMask, isFeatureVersion242 bool) error {
Copy link
Member

Choose a reason for hiding this comment

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

Agreed, this is why we introduced the TEEFeatures struct a while back (which is now somewhat obsolete due to the use of feature versions). There are already too many parameters everywhere.

Comment on lines +39 to +44
// PerRolePolicy defines additional role specific quote policies, that overwrite
// Policy when node with these roles does an attestation.
//
// A valid entry is for either [RoleComputeWorker] or [RoleObserver]. Single entry
// should not encode multiple roles.
PerRolePolicy map[RolesMask]quote.Policy `json:"per_role_policy,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

That makes sense to me, no need to have a general map that introduces unnecessary complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for compute runtime quote policy

2 participants