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

Xynnn007 · 2024-11-25T03:17:45Z

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

~~Let's wait after confidential-containers/trustee#597 gets merged.~~

This patch does a bunch of fixes per RFC7516.

AEAD Auth Tag is now expcilitly included inside the tag part.
Add TEE key pair support for RSA OAEP and ECDH-ES+A256KW(by default)
Mark RSA PKCS#1 v1.5 padding scheme as deprecated as it is not recommended.

mkulke

I guess we should also bump KBS_PROTOCOL_VERSION. Is there a rationale how we increase the versions. would it be 0.2.0 or 0.1.1. if it's a breaking change, not merely additive, I would assume we have to bump it to 0.2.0.

Xynnn007 · 2024-11-26T06:49:09Z

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

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

Xynnn007 · 2024-11-26T06:59:51Z

cc @deeglaze

mythi · 2024-11-26T07:01:10Z

@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 · 2024-11-26T07:05:49Z

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 · 2024-11-26T07:41:23Z

This will make the CI red.

c89b96b3 has regression. There is no ProtocolVersion error anymore.

Xynnn007 · 2024-11-26T09:57:10Z

Once confidential-containers/trustee#600 gets merged, we can restart the failed CI here and it will be green.

Xynnn007 · 2024-11-26T15:56:29Z

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

deeglaze · 2024-11-26T16:00:22Z

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 · 2024-11-26T16:43:39Z

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

Xynnn007 · 2024-11-27T03:41:49Z

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

Xynnn007 · 2024-12-02T03:51:09Z

Let's wait for virtee/kbs-types#45 to be merged, too

Xynnn007 · 2024-12-02T06:07:42Z

The lint error is not related to this PR. Fixed in #828

Xynnn007 · 2024-12-18T02:29:40Z

Let's wait for the upstream fix to be merged and then I will take a rebase. Other parts are ready for review.

Xynnn007 · 2025-02-06T01:52:49Z

Changes happen since last approvals. PTAL @huoqifeng @mkulke

cc @fitzthum @mythi

huoqifeng

LGTM.

fitzthum

Ok, I finally reviewed this PR. I have maybe 2 comments that are important (possibly there is a little bug) and a bunch of less important suggestions.

attestation-agent/deps/crypto/src/asymmetric.rs

attestation-agent/deps/crypto/src/symmetric.rs

fitzthum · 2025-02-18T19:05:51Z

attestation-agent/deps/crypto/src/rust/aes256gcm.rs

+    let nonce = Nonce::from_slice(iv);
+    let mut plaintext = encrypted_data.to_vec();
+    cipher
+        .decrypt_in_place_detached(nonce, aad, &mut plaintext, tag.into())


Why are we using the detached version of this function?

The non-detached version implies the tag lies as the suffix of the cipher text, while jwe detaches tag as a separate claim in jwt.

attestation-agent/deps/crypto/src/rust/aes256gcm.rs

attestation-agent/deps/crypto/src/native/ec.rs

attestation-agent/deps/crypto/src/rust/ec.rs

attestation-agent/kbs_protocol/src/keypair.rs

fitzthum · 2025-02-18T19:55:25Z

attestation-agent/kbs_protocol/src/keypair.rs

+                bail!("Unmatched curve: {}", crv);
+            }
+
+            let cek = key.unwrap_key(cipher_text, x, y, KeyWrapAlgorithm::EcdhEsA256Kw)?;


We are using x and y from the header here. Should we be using them from the public key that we store? The RSA methods don't take the public key from the header but for EC we do. What is the thinking behind this?

Due to https://datatracker.ietf.org/doc/html/rfc7518#section-4.6, The x and y here are of ephemeral key. They are used to make a Diffie Hellman protocol with x and y of the key we store to finally derive the decryption key to decrypt the ciphertext. The logic of obtaining decryption key with elliptic curve is different from that of RSA directly decrypting with private key.

This patch adds more crypto suites to the crypto crate 1. Add AES-256-GCM AEAD API. 2. Mark RSA PKCS#1 v1.5 Padding encryption scheme as "deprecated". 3. Add EC suites for key wrapping. Now supports P256 curve and ECDH-ES-A256KW algorithm. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

This patch updates KBS protocol to v0.2.0. The change mainly includes 1. Replace RSA-PKCS1v15 to ECDH-ES-A256KW. The former algorithm is not declared as deprecated in https://www.ietf.org/archive/id/draft-madden-jose-deprecate-none-rsa15-00.html#section-1.2 Also, some fixups to make the KBS protocol's Response fully compatible with JWE standard are made, including explicitly parse `tag` in the flattened JSON serialization. Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Xynnn007 · 2025-02-19T10:10:40Z

Fixed the comments and take a rebase.

fitzthum

LGTM. Thanks for taking this on.

Last call for review before this is merged.

fitzthum · 2025-02-20T21:26:24Z

Ok, let's try to get the Trustee side in soonish.

Xynnn007 requested a review from a team as a code owner November 25, 2024 03:17

Xynnn007 force-pushed the fix-aead branch from eca6d3d to 2bfe355 Compare November 25, 2024 03:20

mkulke reviewed Nov 25, 2024

View reviewed changes

Xynnn007 force-pushed the fix-aead branch from 2bfe355 to 82dbd62 Compare November 26, 2024 06:46

Xynnn007 mentioned this pull request Nov 26, 2024

KBS: Update KBS protocol to 0.2.0 to fix JWE format due to RFC7516 confidential-containers/trustee#597

Open

Xynnn007 changed the title ~~AA/kbs_protocol: fix JWE decryption logic due to RFC7516~~ AA/kbs_protocol: Update to 0.2.0 to fix JWE decryption logic due to RFC7516 Nov 26, 2024

Xynnn007 requested review from mkulke and mythi November 26, 2024 06:59

Xynnn007 force-pushed the fix-aead branch from 82dbd62 to 6656961 Compare November 26, 2024 09:49

Xynnn007 mentioned this pull request Nov 27, 2024

JWE encryption is missing base64url-encoded protected header as AEAD confidential-containers/trustee#583

Open

Xynnn007 marked this pull request as draft November 27, 2024 03:41

Xynnn007 force-pushed the fix-aead branch 2 times, most recently from c496ed0 to 1729cf0 Compare December 2, 2024 03:50

Xynnn007 force-pushed the fix-aead branch from 1729cf0 to be43e49 Compare December 2, 2024 04:00

Xynnn007 marked this pull request as ready for review December 2, 2024 04:03

Xynnn007 force-pushed the fix-aead branch from be43e49 to dfbdcc0 Compare December 2, 2024 05:40

Xynnn007 force-pushed the fix-aead branch 2 times, most recently from 80308ec to a739c05 Compare December 10, 2024 08:33

Xynnn007 force-pushed the fix-aead branch from 6fecbd0 to 352e0c7 Compare December 16, 2024 10:09

Xynnn007 mentioned this pull request Dec 16, 2024

Response: fix some bugs of Response struct virtee/kbs-types#53

Merged

Xynnn007 force-pushed the fix-aead branch 8 times, most recently from 182f0c0 to 7d280e2 Compare December 17, 2024 09:20

Xynnn007 marked this pull request as ready for review December 18, 2024 01:32

Xynnn007 requested review from huoqifeng and mkulke December 18, 2024 02:30

Xynnn007 force-pushed the fix-aead branch from 7d280e2 to a118c98 Compare January 7, 2025 03:51

Xynnn007 force-pushed the fix-aead branch 3 times, most recently from b0fee4f to 169af3d Compare February 6, 2025 01:38

huoqifeng approved these changes Feb 18, 2025

View reviewed changes

fitzthum reviewed Feb 18, 2025

View reviewed changes

Xynnn007 added 3 commits February 19, 2025 18:09

chore: update thiserror to 2.0.11

e04fe92

Signed-off-by: Xynnn007 <xynnn@linux.alibaba.com>

Xynnn007 force-pushed the fix-aead branch from 169af3d to e04fe92 Compare February 19, 2025 10:09

fitzthum approved these changes Feb 19, 2025

View reviewed changes

fitzthum merged commit f2e9125 into confidential-containers:main Feb 20, 2025
29 checks passed

Xynnn007 deleted the fix-aead branch February 21, 2025 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Xynnn007 commented Nov 25, 2024 •

edited

Loading

mkulke left a comment

Xynnn007 commented Nov 26, 2024 •

edited

Loading

Xynnn007 commented Nov 26, 2024

mythi commented Nov 26, 2024

Xynnn007 commented Nov 26, 2024 •

edited

Loading

mythi commented Nov 26, 2024 •

edited

Loading

Xynnn007 commented Nov 26, 2024

Xynnn007 commented Nov 26, 2024

deeglaze commented Nov 26, 2024

Xynnn007 commented Nov 26, 2024

Xynnn007 commented Nov 27, 2024

Xynnn007 commented Dec 2, 2024

Xynnn007 commented Dec 2, 2024

Xynnn007 commented Dec 18, 2024

Xynnn007 commented Feb 6, 2025

huoqifeng left a comment

fitzthum left a comment

fitzthum Feb 18, 2025

Xynnn007 Feb 19, 2025

fitzthum Feb 18, 2025

Xynnn007 Feb 19, 2025

Xynnn007 commented Feb 19, 2025

fitzthum left a comment

fitzthum commented Feb 20, 2025

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

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

Conversation

Xynnn007 commented Nov 25, 2024 • edited Loading

mkulke left a comment

Choose a reason for hiding this comment

Xynnn007 commented Nov 26, 2024 • edited Loading

Xynnn007 commented Nov 26, 2024

mythi commented Nov 26, 2024

Xynnn007 commented Nov 26, 2024 • edited Loading

mythi commented Nov 26, 2024 • edited Loading

Xynnn007 commented Nov 26, 2024

Xynnn007 commented Nov 26, 2024

deeglaze commented Nov 26, 2024

Xynnn007 commented Nov 26, 2024

Xynnn007 commented Nov 27, 2024

Xynnn007 commented Dec 2, 2024

Xynnn007 commented Dec 2, 2024

Xynnn007 commented Dec 18, 2024

Xynnn007 commented Feb 6, 2025

huoqifeng left a comment

Choose a reason for hiding this comment

fitzthum left a comment

Choose a reason for hiding this comment

fitzthum Feb 18, 2025

Choose a reason for hiding this comment

Xynnn007 Feb 19, 2025

Choose a reason for hiding this comment

fitzthum Feb 18, 2025

Choose a reason for hiding this comment

Xynnn007 Feb 19, 2025

Choose a reason for hiding this comment

Xynnn007 commented Feb 19, 2025

fitzthum left a comment

Choose a reason for hiding this comment

fitzthum commented Feb 20, 2025

Xynnn007 commented Nov 25, 2024 •

edited

Loading

Xynnn007 commented Nov 26, 2024 •

edited

Loading

Xynnn007 commented Nov 26, 2024 •

edited

Loading

mythi commented Nov 26, 2024 •

edited

Loading