feat: add signed RATSD token v2 responses#68
Conversation
cowbon
requested review from
deeglaze,
iolivergithub,
jraman567 and
thomas-fossati
as code owners
cowbon
force-pushed
the
ratsd-token-v2-pr
branch
from
bb391c6 to
4eddc28
Compare
thomas-fossati
left a comment
thomas-fossati
left a comment
There was a problem hiding this comment.
Thanks for the work! I have left a couple of comments inline.
I am slightly confused about how this relates to #65?
yogeshbdeshpande
left a comment
yogeshbdeshpande
left a comment
There was a problem hiding this comment.
Some fundamental changes required, before we can proceed with this...
| return nil, err | ||
| } | ||
|
|
||
| key, err := loadPrivateKey(keyPath) |
There was a problem hiding this comment.
I am not clear why you are assuming a private key to be loaded from a path. In fact, we should provide a method to share the Leaf Certificate and Certificate Chain to be added into the Header.
We need an API to provide a signer to the Evidence Collection
There was a problem hiding this comment.
Hi @yogeshbdeshpande, thanks for the review. I wonder when you said we need an API to provide the signer, does it mean to a POST method to upload the certificate chain to ratsd, or it's something else? Currently, this PR use a x509 certificate from the path to sign the CBOR
There was a problem hiding this comment.
I just revisited the ratsd-token branch you're working on, and it seems you prefer to use go-cose to do the signing. But it's still ratsd to provide the certificates and call go-cose to sign it. As mentioned earlier, should we provide a new API to set the certificates for signing? Also, the format of Evidence defined in your commit WIP: token code look different from docs/ratsd-token.cddl, while this PR is simply derived from what Thomas put in the CDDL.
I appreciate the token library you're working on, but this PR has a different purpose. Once it's ready, we can rework on this PR to have it based on token library you are going to add.
There was a problem hiding this comment.
Sure let us proceed now. I will review it one more time...
There was a problem hiding this comment.
Hi Yogesh, can you check if the most recent change is on the right track. If so, we might start from there and match against the requirements mentioned in #65
There was a problem hiding this comment.
@cowbon : This is a good start but we want a separate folder and a new package separate from existing tokens, which could the package for individual tokens such as tsm etc.
As a result, I have moved the code base to the new folder known as ratsd-token
Let us evolve their and it is fine, to use this branch.
There was a problem hiding this comment.
AFAIK the tokens.tsm is also used by Services to support SEV-SNP. With this PR, as both ratsd-token.Evidence and tokens.tsm are used publicly, I'm not sure if it's still worth to separate it out.
There was a problem hiding this comment.
@cowbon We need a new package for overall new token, which should be seperate from the other tokens fetched by RATSD. We can discuss this when you are back from holidays!
cowbon
force-pushed
the
ratsd-token-v2-pr
branch
2 times, most recently
from
b243fd3 to
bda8555
Compare
|
I have added more logic to how we should insert a Claim as UCCS inside RATSD/V2 token |
Signed-off-by: Ian Chin Wang <ian.chin.wang@oracle.com>
Signed-off-by: Ian Chin Wang <ian.chin.wang@oracle.com>
Add a reusable Evidence builder for RATSD tokens with go-cose based signing, verification, and unmarshaling. Refactor /ratsd/chares to use the new builder while preserving the existing legacy and v2 wire formats and their test coverage. Signed-off-by: Ian Chin Wang <ian.chin.wang@oracle.com>
Signed-off-by: Yogesh Deshpande <yogesh.deshpande@arm.com>
Signed-off-by: Yogesh Deshpande <yogesh.deshpande@arm.com>
3 participants
Summary
Testing