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

KBS: Update KBS protocol to 0.2.0 to fix JWE format due to RFC7516 #597

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Xynnn007
Copy link
Member

@Xynnn007 Xynnn007 commented Nov 25, 2024

Fixes #583.

This patch does a bunch of fixes per RFC 7516.

  1. AEAD Auth Tag is now expcilitly included inside the tag part.
  2. Add new JWE algorithms RSA OAEP and ECDH-ES+A256KW
  3. Mark RSA PKCS#1 v1.5 padding scheme as deprecated as it is not recommended.
  4. Add unit tests based on josekit to check the compatibility of JWE used by Response.
  5. Update rust toolchain to 1.83.0

depends on
virtee/kbs-types#53

@Xynnn007 Xynnn007 requested a review from a team as a code owner November 25, 2024 03:13
Xynnn007 added a commit to Xynnn007/guest-components that referenced this pull request Nov 25, 2024
Per RFC7516, the AEAD's auth tag should be included inside the JWE body.
We fix this to align with trustee side

confidential-containers/trustee#597

Signed-off-by: Xynnn007 <[email protected]>
Xynnn007 added a commit to Xynnn007/guest-components that referenced this pull request Nov 25, 2024
Per RFC7516, the AEAD's auth tag should be included inside the JWE body.
We fix this to align with trustee side

confidential-containers/trustee#597

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007
Copy link
Member Author

Xynnn007 commented Nov 25, 2024

Ok the test error happened because the guest-components side code is old. I just put another commit that references my repo to include the commit that includes the AEAD update.

I think we can merge GC side code firstly. Then go back to this PR. Then another PR on gc side to revert the test image change.

Cargo.toml Outdated Show resolved Hide resolved
Xynnn007 added a commit to Xynnn007/guest-components that referenced this pull request Nov 26, 2024
Per RFC7516, the AEAD's auth tag should be included inside the JWE body.
We fix this to align with trustee side

confidential-containers/trustee#597

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007
Copy link
Member Author

Xynnn007 commented Nov 26, 2024

I removed the kbs_protocol updating commit. The CI must fail. We can merge confidential-containers/guest-components#820 (comment) first and I will add the kbs_protocol updating commit back to this PR to make the CI happy.

@Xynnn007
Copy link
Member Author

cc @deeglaze

Xynnn007 added a commit to Xynnn007/guest-components that referenced this pull request Nov 26, 2024
Per RFC7516, the AEAD's auth tag should be included inside the JWE body.
We fix this to align with trustee side

confidential-containers/trustee#597

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007 Xynnn007 marked this pull request as draft November 27, 2024 03:42
@Xynnn007
Copy link
Member Author

Xynnn007 commented Dec 2, 2024

Test locally and passed. Let's wait for virtee/kbs-types#45 to be merged and update the kbs-types crate rev.

@Xynnn007 Xynnn007 force-pushed the fix-jwe branch 2 times, most recently from 7fb7897 to 56ae015 Compare December 2, 2024 04:02
@Xynnn007 Xynnn007 marked this pull request as ready for review December 2, 2024 04:03
@@ -227,6 +229,12 @@ Encryption algorithm used to encrypt the output of the KBS service API.
The output of the KBS service API. It must be encrypted with the KBS-generated
ephemeral key.

- `aad` (Required if AEAD is used)
Copy link
Member Author

@Xynnn007 Xynnn007 Dec 2, 2024

Choose a reason for hiding this comment

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

Hi @tylerfanelli I added the aad and tag parts to this document. They all follow RFC7516. Thanks for the help from upstream kbs-types.

@Xynnn007 Xynnn007 changed the title KBS: fix JWE format KBS: Upadte KBS protocol to 0.2.0 to fix JWE format Dec 2, 2024
@Xynnn007 Xynnn007 changed the title KBS: Upadte KBS protocol to 0.2.0 to fix JWE format KBS: Upadte KBS protocol to 0.2.0 to fix JWE format due to RFC7516 Dec 2, 2024
@tylerfanelli
Copy link
Contributor

Test locally and passed. Let's wait for virtee/kbs-types#45 to be merged and update the kbs-types crate rev.

Released here.

@Xynnn007 Xynnn007 force-pushed the fix-jwe branch 3 times, most recently from 7b9e7c7 to 547ed02 Compare December 10, 2024 08:19
@Xynnn007 Xynnn007 requested review from mythi and mkulke December 11, 2024 02:21
@deeglaze
Copy link

also, we get a build warning now, similar to jwcrypto, RSA1_5 seems to be deprecated in josekit:

This is my plan. To use ecdsa keys in this PR, too

hmm, I wouldn't switch to EC for TEE keys without more consideration.

All the technologies depend on ECC already, and it’s much faster to generate keys at boot for ECC. The coconut-svsm project will be switching to ecc from rsa.

@mkulke
Copy link
Contributor

mkulke commented Dec 12, 2024

All the technologies depend on ECC already, and it’s much faster to generate keys at boot for ECC. The coconut-svsm project will be switching to ecc from rsa.

I guess that's fair. I remember asking about this at one point and there was a limitation for some TEE that would make switching the TEE keypair to EC hard. I don't remember details though and the concern might not be valid anymore. We can just go ahead, and we'll notice problems on a CI somewhere.

@deeglaze
Copy link

also, we get a build warning now, similar to jwcrypto, RSA1_5 seems to be deprecated in josekit:

warning: use of deprecated variant josekit::jwe::RSA1_5: This algorithm is no longer
recommended.

is this something that should concern us, can we change our TEE key algo? it might be a good point in time since we're about to have a breaking protocol change anyway

Please. I’d advocate for a couple changes actually. KBS ought to distinguish between keys and secrets to allow for different handling. For example, we would prefer to store a key encryption key (aes-256-kw or kwp) wrapped by an HSM key and use attestation as part of authorization to rewrap the key to a key derived from a shared secret between the TEE and HSM. This is the COSE algorithm ECDH-ES-HKDF-256. The salt for the hkdf can be a parameter. It does mean sending more data to kbs, but it allows kbs to be less stateful, and it allows keys to be hsm protected in an attestable way, provided you have key creation attestations for the wrapping key. The key material is only visible to the HSM and the TEE. You’ll want to ensure through attestation that the TEE key forwarded from the verifier to the HSM is only the one from the TEE to make sure the verifier isn’t sneakily rewrapping the key to its own key too, but we’ve known that verifiers should be attested.

Initial provisioning of keys from first boot becomes a requirement.

When you have keys protected in their own way, you can use the appropriate algorithms for key wrapping in transit. For secrets like rollback counters, you can use the current JWE, since there’s generally no service-specific non-extraction requirement for secrets the way there are with keys.

@mkulke
Copy link
Contributor

mkulke commented Dec 13, 2024

Please. I’d advocate for a couple changes actually. KBS ought to distinguish between keys and secrets to allow for different handling. For example, we would prefer to store a key encryption key (aes-256-kw or kwp) wrapped by an HSM key and use attestation as part of authorization to rewrap the key to a key derived from a shared secret between the TEE and HSM. This is the COSE algorithm ECDH-ES-HKDF-256. The salt for the hkdf can be a parameter. It does mean sending more data to kbs, but it allows kbs to be less stateful, and it allows keys to be hsm protected in an attestable way, provided you have key creation attestations for the wrapping key. The key material is only visible to the HSM and the TEE. You’ll want to ensure through attestation that the TEE key forwarded from the verifier to the HSM is only the one from the TEE to make sure the verifier isn’t sneakily rewrapping the key to its own key too, but we’ve known that verifiers should be attested.

Initial provisioning of keys from first boot becomes a requirement.

When you have keys protected in their own way, you can use the appropriate algorithms for key wrapping in transit. For secrets like rollback counters, you can use the current JWE, since there’s generally no service-specific non-extraction requirement for secrets the way there are with keys.

thx, it might be good to copy your thoughts to a discrete issue on this repo (verbatim is fine), since we probably won't fully address it in this PR (i hope 😅) and it's easier to track this way. I understand there are venues to decouple kbs from the handling of key material, so I feel there might be a conceptual overlap with sealed secrets/KMS/HSM backends and it's probably good to understand which role those facilities would play in the suggested scheme. There's also a dedicated Trustee Sync, which would be a great place to present those ideas.

@Xynnn007
Copy link
Member Author

Xynnn007 commented Dec 16, 2024

@deeglaze Thanks for the suggestion! If interested, please feel free to join our bi-weekly meeting mentioned by Magnus to share your great ideas.

I replaced RSA PKCS1v15 with ECDH-ES+A256KW (Concat KDF is used as KDF) on curve P256.

I still worry about the perfect forward secrecy that seems not to be fully conquered by JWE. Thus I am considering to promote it in future with existing TLS suites to improve this.

Newly edited: ECDH-ES-HKDF-256 looks similar with ECDH-ES+A256KW. They both use an ephemeral Elliptic Curve key pair from KBS side and the Elliptic Curve public key stated by TEE public key to negotiate a shared secret to derive a shared encryption key. The flaw is once the TEE private key is exposed to an attacker, the historical data flow can be decrypted by recalculating the shared secret. To cover this, we might need some mechanism to negotiate a shared secret based on both sides' ephemeral Elliptic Curve key pair, where the TEE public key is only used to sign the ephemeral Elliptic Curve public key part from TEE side. In this case, even if the TEE public key is exposed to an attacker, the attacker still cannot recover the historical ephemeral Elliptic Curve key pair thus cannot re-construct the shared encryption key. Thus the historical data flow is still confidential.

@Xynnn007 Xynnn007 force-pushed the fix-jwe branch 2 times, most recently from 0dc32de to e70e893 Compare December 16, 2024 07:44
@Xynnn007 Xynnn007 force-pushed the fix-jwe branch 11 times, most recently from bb4e4f1 to 6db712e Compare December 18, 2024 01:34
@Xynnn007 Xynnn007 marked this pull request as ready for review December 18, 2024 01:36
@Xynnn007 Xynnn007 marked this pull request as draft December 18, 2024 01:36
@Xynnn007 Xynnn007 marked this pull request as ready for review December 18, 2024 02:03
@Xynnn007
Copy link
Member Author

I did some change upon the PR description. Compability tests are passed with josekit crate. Now we can wait for upstream PR to get merged and then I rebase the dep.

@Xynnn007 Xynnn007 requested a review from mkulke December 18, 2024 02:28
Fixes confidential-containers#583.

This commit does two things:
1. Fix JWE format to align with RFC 7516.
2. Use more strong algorithm for JWE.

Due to RFC 7516, the JWE AEAD Auth Tag should be expcilitly be included
inside the `tag` part. Before this commit, the tag is actually included
as the suffix of the `ciphertext`.

We fix this by expcilitly extract the tag and include it into the jwe
body.

Also, we fix the AAD calculation logic, s.t. derived from
ProtectedHeader which is also specifiled by RFC7516. This should be
align with the guest-components side.

This commit supports the following JWE algorithms for KBS response:

1. RSA PKCS v1.5 Padding. This algorithm is not recommended but for
compability it is still reserved.
2. RSA OAEP.
3. ECDH-ES-A256KW with curve P256. This is recommended as EC algorithms
are more fast and safe.

which algorithm is used is decided by the TEE public key sent by the
client.

Some more unit tests to test the compability is added to make sure the
algorithm is implemented as standard.

Both change will make the kbs_client not able to connect to the KBS.
Thus we update the KBS protocol version from 0.1.1 to 0.2.0.

Signed-off-by: Xynnn007 <[email protected]>
Some dependencies like [email protected] requires a rust toolchain version
over 1.81.0. Thus we update this to 1.83.0 to track the latest rust
toolchain.

This commit also fixes lint error introduced by new version of rust
toolchain.

Signed-off-by: Xynnn007 <[email protected]>
KBS Build with default feature and KBS build with built-in CoCo AS
feature are the same, thus we delete the duplicated one and add more
information.

Signed-off-by: Xynnn007 <[email protected]>
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.

LGTM. Thanks for taking care of this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: We have code
Development

Successfully merging this pull request may close these issues.

JWE encryption is missing base64url-encoded protected header as AEAD
6 participants