-
Notifications
You must be signed in to change notification settings - Fork 83
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
KBS: combine CoCo Token and Jwk Token verifier #524
base: main
Are you sure you want to change the base?
Conversation
34bc38c
to
47002cd
Compare
47002cd
to
3d973d9
Compare
Good good good. |
This PR also changes some logic of previous JWK of CoCoAS, so ptal @jialez0 |
3d973d9
to
8c27e4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Wdyt @mythi
My earlier comment in the original PR was that my preference is to not expose "ITA" as a token type to the user and that seems to be in this PR still. I was wondering whether we could just ask users to provide the list of trust anchors and try to parse the provided The only reason for "token type" to still exist seems to be the |
Yes. This PR supports the original ITA way, s.t. provode Another option is
Nice suggestion. The pro is flexibility while the con is extra complexity to remember the paths. I personally prefer the "token type" way as the user only needs to care about token type rather than claims inside that. I think you are caring about the name |
I was not very clear earlier. My comment was that can we have just one config entry. It would be easier for the user. It should be possible to handle
True, OTOH, we can always make known paths as the default and only provide the mechanism to add supplemental paths. |
Would it be OK to aim fixing #519 with PR too? |
Probably not. Currently it is hard for KBS to detect the JWT type (CoCoAS/ITA or else) runtimely. See #519 (comment) |
Do you mean that we have two config items
|
Could
Just one because |
Yes it can and it is implemented in this PR. What #519 aims is to support different public key claim path.
Ok. Could you provide an example for this in your mind? |
Right, sorry. I had missed that and got side tracked when you wrote that JWT detection cannot be done easily.
Instead of |
8f2d7af
to
2e9d713
Compare
Actually, the ITA token and CoCo Token are both JWTs. They both need a JWK to verify the JWT. The difference is the way to gather the JWK. This commit combined the two logic, and add two ways to get the JWK. 1. From the configured JwkSet when launching KBS 2. From the JWT's Header's jwk field. The two ways will check the jwk endorsement in different ways. The first way is to configure the trusted JwkSet from the config. The second way is to configure the trusted CA in config. Then get the public key cert chain from Jwk's x5c field. The both ways are also supported in this patch. Rust does not provide a mature crate to verify cert chain, thus openssl is used in this patch. We also abondon rustls and openssl feature of KBS because openssl is by default used. Then we use openssl by default to make the code base simpler. Signed-off-by: Xynnn007 <[email protected]>
Due to RFC 7515, JWK should be part of a JOSE Header rather than claim body. Signed-off-by: Xynnn007 <[email protected]>
Due to latest change, KBS will not maintain both rustls and openssl suites for HTTPS. Thus we need to delete all the options of HTTPS_CRYPTO config in documents and codes. Also, the latest change changes the config format of `attestation_token_config`, so we also change the type field to new name and rename `Jwk` to `ITA`. Signed-off-by: Xynnn007 <[email protected]>
2e9d713
to
2756ca3
Compare
@mythi Adds the change that you suggested. I think it is good to have some internal defined paths together with user defined extra paths. |
thanks! I personally feel it's better this way and I guess this now meets #519 also. My remaining concern is whether we need separate user facing config entries for |
Well as they are different things. One is jwkset and the other is root pem certs |
extra_teekey_paths: vec![], | ||
trusted_certs_paths: vec![], | ||
trusted_jwk_sets: vec![config.certs_file.clone()], | ||
insecure_key: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insecure_key: true, | |
insecure_key: false, |
If the take this path, we might need a third one for "OpenID Configuration endpoints" which is what the default ITA config prefers. But my point was that some generic config entry would probably keep things simple for the users. |
This patch is part of #514.
ITA token and CoCo Token are both JWTs. They both need a JWK to verify the JWT. The difference is the way to gather the JWK.
This commit combined the two logic, and add two ways to get the JWK.
The two ways will check the jwk endorsement in different ways. The first way is to configure the trusted JwkSet from the config. The second way is to configure the trusted CA in config. Then get the public key cert chain from Jwk's x5c field. The both ways are also supported in this patch.
The difference between CoCo Token and ITA Token are where Tee Public key is embedded inside the token, this could be configured in the launch toml of KBS, because it is hard to detect during runtime.
Rust does not provide a mature crate to verify cert chain, thus openssl is used in this patch. We also abondon rustls and openssl feature of KBS because openssl is by default used. Then we use openssl by default to make the code base simpler.
Close #486
cc @mythi