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

Align trust vector with draft-ietf-rats-ar4si #37

Merged
merged 1 commit into from
Sep 17, 2022
Merged

Align trust vector with draft-ietf-rats-ar4si #37

merged 1 commit into from
Sep 17, 2022

Conversation

setrofim
Copy link
Collaborator

Bring the TrustVector within Attestation result into closer alignment
with https://datatracker.ietf.org/doc/draft-ietf-rats-ar4si/

  • Align the claims inside the TrustVector with those specified by the
    ar4si spec.
  • Change their values to be integers with values in the int8 range
    (note: due to protobuf limitations, actual representation is int32).
  • Add a TrustTier type that corresponds to the "Trustworthiness Tier"
    concept from ar4si. ARStatus has methods to covert its value into a
    tier.
  • The overall Test status is now a TrustTier. This is now set
    automatically by the core verifier from the TrustVector, rather than
    relying on Scheme plugins to update it (note: plugins can still
    override it, if necessary).
  • Add "veraison-verifier-added-claims" extension to the attestation
    result and allow it to be populated by policy.

@@ -64,7 +64,10 @@ func (s Scheme) GetFormat() proto.AttestationFormat {
return proto.AttestationFormat_PSA_IOT
}

func (s Scheme) SynthKeysFromSwComponent(tenantID string, swComp *proto.Endorsement) ([]string, error) {
func (s Scheme) SynthKeysFromSwComponent(
Copy link
Collaborator

Choose a reason for hiding this comment

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

There seems to be no change except the newlines introduced. Any specific reason for this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To keep the lines short, to prevent wrapping and make them easier to parse.

Copy link
Collaborator

@yogeshbdeshpande yogeshbdeshpande Sep 16, 2022

Choose a reason for hiding this comment

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

ok, understood, was just wondering should we consistently do it at least in the file we are currently touching??
SynthKeysFromTrustAnchors is nearly 100+ column width but not modified so was in a little fix hence the question?

@@ -136,7 +139,10 @@ func (s Scheme) GetTrustAnchorID(token *proto.AttestationToken) (string, error)
), nil
}

func (s Scheme) ExtractVerifiedClaims(token *proto.AttestationToken, trustAnchor string) (*scheme.ExtractedClaims, error) {
func (s Scheme) ExtractVerifiedClaims(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question as line 67

proto/result.go Outdated
// Claim values as defined by
// https://datatracker.ietf.org/doc/draft-ietf-rats-ar4si/
const (
// common
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does //common here means?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The group of consts bellow are common to all claims. (The subsequent groups are titled with the claim they're for).

Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

🏆

proto/result.go Outdated Show resolved Hide resolved

// Claim values as defined by
// https://datatracker.ietf.org/doc/draft-ietf-rats-ar4si/
const (
Copy link
Contributor

Choose a reason for hiding this comment

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

this is awesome, and it's key information for plugin writers. I am wondering how to make it as visible as possible?

@yogeshbdeshpande
Copy link
Collaborator

Just a side note:

  1. Perhaps we should create a github issue to move Attestation Results to a new repo, as discussed in our meeting.

  2. This change adds a change in apiclient and evcli tools as well, so we should track that effort as well!

@thomas-fossati
Copy link
Contributor

  1. This change adds a change in apiclient and evcli tools as well, so we should track that effort as well!

which change are you thinking of?

@yogeshbdeshpande
Copy link
Collaborator

yogeshbdeshpande commented Sep 16, 2022

  1. This change adds a change in apiclient and evcli tools as well, so we should track that effort as well!

which change are you thinking of?

  1. This change adds a change in apiclient and evcli tools as well, so we should track that effort as well!

which change are you thinking of?

I was mentioning about reading the Attestation Results and echoing what has been received in return response from Veraison. At the moment this bit of code is not present.

@thomas-fossati
Copy link
Contributor

At the moment this bit of code is not present.

If that's the case there should be no change to track, right? What am I missing?

@yogeshbdeshpande
Copy link
Collaborator

At the moment this bit of code is not present.

If that's the case there should be no change to track, right? What am I missing?

Well, we have veraison/evcli#12 on evcli -> I think we do not need anything here. So we are ok.
We do not have a similar issue on apiclient ? Need tracking there as well...

@yogeshbdeshpande
Copy link
Collaborator

At the moment this bit of code is not present.

If that's the case there should be no change to track, right? What am I missing?

Well, we have veraison/evcli#12 on evcli -> I think we do not need anything here. So we are ok. We do not have a similar issue on apiclient ? Need tracking there as well...

Discussed the confusion: veraison/evcli#13 will address the core problem.

For apiclient, let the composer of the library display this information. Suitable api functions will be provided for users to include so that they have the required information at their disposal.

Bring the TrustVector within Attestation result into closer alignment
with https://datatracker.ietf.org/doc/draft-ietf-rats-ar4si/

- Align the claims inside the TrustVector with those specified by the
  ar4si spec.
- Change their values to be integers with values in the int8 range
  (note: due to protobuf limitations, actual representation is int32).
- Add a TrustTier type that corresponds to the "Trustworthiness Tier"
  concept from ar4si. ARStatus has methods to covert its value into a
  tier.
- The overall Test status is now a TrustTier. This is now set
  automatically by the core verifier from the TrustVector, rather than
  relying on Scheme plugins to update it (note: plugins can still
  override it, if necessary).
- Add "veraison-verifier-added-claims" extension to the attestation
  result and allow it to be populated by policy.

Signed-off-by: Sergei Trofimov <[email protected]>
Co-authored-by: Thomas Fossati <[email protected]>
@setrofim setrofim merged commit 90955ff into main Sep 17, 2022
@setrofim setrofim deleted the result branch September 17, 2022 11:24
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.

3 participants