Feat Attestation Token distribution. #74
Conversation
|
cc @surajssd |
jialez0
force-pushed
the
token
branch
2 times, most recently
from
da18b61 to
81ff52d
Compare
|
@sameo Thanks for reviewing! Now the code is update and you can take a look again:-) |
There was a problem hiding this comment.
@jialez0 The PR looks good now, thanks a lot.
I had some further comments on the documentation part and the missing OpenAPI changes. I sent a PR instead for you at jialez0#1
Please review it and feel free to squash it into your PR. Once that's done we could go ahead and merge, unless @Xynnn007 or @surajssd have further comments.
|
Haven't had a chance to really review, but I'm glad we're adding this feature. Will be very useful for more complex workloads. Including the public key of the TEE in the token will prevent replay, right? |
Correct. And yes, this is going to be useful for many workloads, including e.g. the ML ones that were presented last Thursday. |
There was a problem hiding this comment.
I like the general direction this is headed, I suggest some changes for better compatibility with other attestation services and i think we need to think about the big picture a bit.
To implement the passport model we'll need to have:
- an actual "attestation-service" that acts as a RATS verifier. This could be implemented as a version of KBS which only exposes
/attestand/token-certificate-chainendpoints, but giving it a proper name would be helpful for communication. - a KBS that does not perform attestation itself and instead calls
token.verify()acting as the RATS relying party.
In that case the relying-party-KBS could be stateless (no session/cookie), and the attestation token would be passed together with the /resource/... calls. The TEE would communicate separately with an attestation service and (1+) KBS.
Does this direction make sense?
src/api_server/src/session.rs
Outdated
| let protected_header = json!( | ||
| { | ||
| "alg": RSA_ALGORITHM.to_string(), | ||
| "enc": AES_GCM_256_ALGORITHM.to_string(), | ||
| }); | ||
|
|
||
| Ok(Response { | ||
| protected: serde_json::to_string(&protected_header)?, |
There was a problem hiding this comment.
This header is not protected, while it should be passed to encrypt as AAD.
There was a problem hiding this comment.
I don't have much experience in this area, will the header here be unsafe? Specifically, how should it be encrypted?
Additionally, this is not the scope of this PR, we can create a new PR to fix it after this PR is merged.
@sameo What do you think?
There was a problem hiding this comment.
The JWE spec defines that the "protected" field should be integrity protected. This is "additional authenticated data" in AES-GCM, the resulting tag goes in the "tag" field. It does not need to be encrypted.
Otherwise we the field should be called "unprotected".
I'm OK if we address this later. This will be a breaking change.
jialez0
force-pushed
the
token
branch
2 times, most recently
from
9dea9ce to
9239ad3
Compare
|
@jepio Thanks for reviewing, now this PR is update and resolved your comments. Please review it again, thanks. |
I agree with you, we do need a clear RATS verifier to issue attestation token and a clear KBS as RATS rely party that does not perform attestation itself. This means we may need to reorganize and split our existing code at the repository level, and carefully rename them. I think this is a necessary task, and we need to start considering it now. @sameo |
Fixed: confidential-containers#20 This commit first categorizes the KBS HTTP API into three categories and reorganizes the code accordingly: 1. attest: RESTful APIs that related to attestation 2. resource: RESTful APIs that to get secret resources, need attestation verification 3. config: RESTful APIs that configure KBS and AS, require user authentication 4. public: RESTful APIs that is public This commit then implement the Token distribution and verification function module. Signed-off-by: Jiale Zhang <[email protected]>
Co-authored-by: Samuel Ortiz <[email protected]> Co-authored-by: Jiale Zhang <[email protected]> Signed-off-by: Jiale Zhang <[email protected]>
Fixed: #20