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

Support X.509 Certificate in Attestation Token. #265

Merged
merged 8 commits into from
Jan 16, 2024

Conversation

jialez0
Copy link
Member

@jialez0 jialez0 commented Dec 21, 2023

@jialez0 jialez0 marked this pull request as draft December 21, 2023 04:08
@jialez0 jialez0 marked this pull request as ready for review December 21, 2023 04:08
@jialez0 jialez0 marked this pull request as draft December 21, 2023 05:01
@jialez0 jialez0 force-pushed the token-cert branch 2 times, most recently from 1defd83 to 42d7a4f Compare December 21, 2023 06:28
@jialez0 jialez0 marked this pull request as ready for review December 21, 2023 06:33
@jialez0 jialez0 force-pushed the token-cert branch 2 times, most recently from 4e2313a to ca01ece Compare December 21, 2023 07:12
@jialez0 jialez0 force-pushed the token-cert branch 2 times, most recently from 814bcb7 to 9f2996f Compare December 22, 2023 09:16
@jialez0 jialez0 requested a review from sameo as a code owner December 22, 2023 09:16
@jialez0 jialez0 force-pushed the token-cert branch 5 times, most recently from 56ea272 to d265339 Compare December 26, 2023 07:14
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

Nice job. I think this is a good feature. I made a few notes.

A couple of other things:

Might be good to update the quickstart guide or some of the dockerfiles to make this change easier on users, especially since I think things might break if a user does not supply some trusted certs to the KBS. It would be good to clarify the default behavior.

I guess this does not support updating the certs at runtime. That is probably ok.

kbs/src/api/src/token/coco.rs Outdated Show resolved Hide resolved
kbs/src/api/src/token/coco.rs Outdated Show resolved Hide resolved
kbs/src/api/src/token/coco.rs Outdated Show resolved Hide resolved
kbs/src/api/src/token/coco.rs Outdated Show resolved Hide resolved
kbs/src/api/src/token/mod.rs Outdated Show resolved Hide resolved
kbs/src/api/src/token/mod.rs Outdated Show resolved Hide resolved
kbs/src/api/src/token/coco.rs Outdated Show resolved Hide resolved
kbs/docs/config.md Outdated Show resolved Hide resolved
kbs/src/api/src/token/coco.rs Outdated Show resolved Hide resolved
@jialez0 jialez0 force-pushed the token-cert branch 3 times, most recently from 57330b1 to 5fc203c Compare January 2, 2024 08:42
@jialez0
Copy link
Member Author

jialez0 commented Jan 2, 2024

@fitzthum Thanks for reviewing, now this PR is updated.

@jialez0 jialez0 force-pushed the token-cert branch 4 times, most recently from 3d76c0a to e8c8b0c Compare January 3, 2024 02:12
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

One more comment. Also, are we doing anything to verify requests made from the KBS to the AS? In particular when the AS sends a set policy request to the AS, the AS should probably check if the request is genuine. Do we support this?

kbs/quickstart.md Show resolved Hide resolved
@jialez0
Copy link
Member Author

jialez0 commented Jan 11, 2024

One more comment. Also, are we doing anything to verify requests made from the KBS to the AS? In particular when the AS sends a set policy request to the AS, the AS should probably check if the request is genuine. Do we support this?

@fitzthum When we use KBS with Builtin-AS or gRPC-AS, we have assumed that AS actually runs in the same trusted security domain as KBS and is controlled by the same entity (such as tenants). So under this premise, the interfaces provided by AS are all available for direct invocation without verification now.

We are currently building a scenario which using KBS with RESTful-AS (although we do not fully support it yet). In this scenario, AS and KBS may run on different nodes and be controlled by different entities, which AS may require some additional validation of request from KBS. This is the enhancement point we need to consider next.

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

I think this is fine to merge, but lack of validation of requests (particularly SetPolicy) to the AS seems like potentially a big problem.

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Thanks. Only some nits.

@jialez0 jialez0 force-pushed the token-cert branch 2 times, most recently from ff28b30 to 8990880 Compare January 16, 2024 08:27
@jialez0 jialez0 merged commit 5a4163c into confidential-containers:main Jan 16, 2024
15 checks passed
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