search

confidential-containers / guest-components

Confidential Containers Guest Tools and Components
Apache License 2.0
83 stars 95 forks source link

AA/kbs_protocol: Update to 0.2.0 to fix JWE decryption logic due to RFC7516 #820

Open Xynnn007 opened 5 days ago

Xynnn007 commented 5 days ago

Per RFC7516, the AEAD's auth tag should be included inside the JWE body. We fix this to align with trustee side

https://github.com/confidential-containers/trustee/pull/597

Let's wait after https://github.com/confidential-containers/trustee/pull/597 gets merged.

Xynnn007 commented 4 days ago

@mkulke Updated the protocol version. ~Also update the test image of kbs using code of https://github.com/confidential-containers/trustee/pull/597.~

~If ok, we can merge this first and I will update the kbs side code.~

Xynnn007 commented 4 days ago

cc @deeglaze

mythi commented 4 days ago

@mkulke Updated the protocol version. Also update the test image of kbs using code of confidential-containers/trustee#597.

We don't need to change the test image if the protocol version changes so the rcar_client.rs changes can be dropped (the test is skipped until the server image comes with the matching protocol version)

Xynnn007 commented 4 days ago

We don't need to change the test image if the protocol version changes so the rcar_client.rs changes can be dropped (the test is skipped until the server image comes with the matching protocol version)

This will make the CI red. The client side code now will use the latest kbs image of CoCo Community, which is 0.1.1 rather than 0.2.0.

If you do not want to change the test image. It would make sense once I get approvals of this PR w/ the commit that changes test image built from the KBS side code. Then I revert the test image part of the commit. The CI will be red, but we all know what happened and get the PR merged.

mythi commented 4 days ago

This will make the CI red.

c89b96b3 has regression. There is no ProtocolVersion error anymore.

Xynnn007 commented 4 days ago

Once https://github.com/confidential-containers/trustee/pull/600 gets merged, we can restart the failed CI here and it will be green.

Xynnn007 commented 4 days ago

Ok. All tests passed. I think it can be merged now

deeglaze commented 4 days ago

This still doesn't protect the protected headers. See https://datatracker.ietf.org/doc/html/rfc7516#appendix-A.4.5

And example https://datatracker.ietf.org/doc/html/rfc7516#section-3.3 particularly this item:

Let the Additional Authenticated Data encryption parameter be ASCII(BASE64URL(UTF8(JWE Protected Header))).

Xynnn007 commented 4 days ago

@deeglaze Thanks for this! Let me do a deeper fix tomorrow.

Xynnn007 commented 3 days ago

Seems that the current Response (JWE Json) does not have aad field. Let's go back to https://github.com/virtee/kbs-types/pull/44 upstream first.