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.

Sign up for GitHub

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

Jump to bottom

JWE encryption is missing base64url-encoded protected header as AEAD #583

Open
deeglaze opened this issue Nov 18, 2024 · 3 comments · May be fixed by #597
Open

JWE encryption is missing base64url-encoded protected header as AEAD #583

deeglaze opened this issue Nov 18, 2024 · 3 comments · May be fixed by #597
Labels
bug Something isn't working

Comments

@deeglaze
Copy link

Describe the bug

The JWE protected_header is not protected AEAD as expected by the RFC for JWE.

How to reproduce

n/a

CoCo version information

all

What TEE are you seeing the problem on

Snp

Failing command and relevant log output

No response
@deeglaze deeglaze added the bug Something isn't working label Nov 18, 2024
@mkulke
Copy link
Contributor

mkulke commented Nov 22, 2024

I think that's true. tag should not be set to an empty string for a protected_header, I think?

trustee/kbs/src/jwe.rs

Lines 51 to 64 in 423e208

let protected_header = json!(
{
"alg": RSA_ALGORITHM.to_string(),
"enc": AES_GCM_256_ALGORITHM.to_string(),
});
Ok(Response {
protected: serde_json::to_string(&protected_header)
.context("serde protected_header failed")?,
encrypted_key: URL_SAFE_NO_PAD.encode(wrapped_sym_key),
iv: URL_SAFE_NO_PAD.encode(iv),
ciphertext: URL_SAFE_NO_PAD.encode(encrypted_payload_data),
tag: "".to_string(),
})

looking at the python jwcrypt impl: there is a deprecation notice about using RSA1_5 as alg, but we can override that and then there is a tag in the resulting jwe (I would have expected it to fail)

>>> from jwcrypto import jwk, jwe
>>> key = jwk.JWK.generate(kty='RSA', size=4096)
>>> jwe_token = jwe.JWE(plaintext="hi", protected={ "alg": "RSA1_5", "enc": "A256GCM" }, algs=["RSA1_5", "A256GCM"])
>>> jwe_token.add_recipient(key)
>>> jwe_token
JWE.from_json_token("{"ciphertext":"XBE","encrypted_key":"TeC60wunpwFy1EdOXjOUCNsKHE2lHaZy09eVU7kBE0vPoHxPISUgEqwNK3iqiwes4uDK8EJEGC1ozOehfhxQ5eA2TZpy_cJLVQUkkkk5vbhV6IVxtTFL7ZWTfpipDnxlAIrOrtF7vWnvu0XACEtRW0eIU16ZE_Jkh9ebyL4VWgmDp-FVRCmQnHzoAPOWAUXlQf7vQyJdHzGN2MuoYxprSGlwwSkonBfmKlKLaxtiJN9T_pqZ4d6JlA8wJrGOvyVkdsXs6g0wkLoDhJE2eba5CtG-hJkoal46vzVOgIKZTHEoQ7LGIeDKKSXE6ZQhHvxADMq_uJaWP9TNKXfVRPlpByX_tAlbYP_JzyT8AO-FABVtdLtpfFegr4g5kRUkS1UlR1WeIuGsPDhvJM0G4V_ZgN5Um44W0at4cRuArnx_YWaRSW8p_Ze9rxSxTdY5GIBTWrjsAi8vctJXflfVYuguDWJ7IV2_mG-yQph4QufTo59RlnAOYNqaVVhQ9w8cxseH0-ML2x_TmNQVxFzQAnJiEJkTET7uyCCqDjXOvxBRC1l-tI1z2C-i5VbnFbxUxCtC1Xn3CwsUk3NaYS7NKe4wSx9V9s7saOX8IMYv-XSS2vip2N4KXO5qdUASa_uF32rObz6zYfbkks_6R6sZSwjhAMtxaeZw7JCv-4sdgt1IzYI","iv":"LAsxB7-32DAZ3vlM","protected":"eyJhbGciOiJSU0ExXzUiLCJlbmMiOiJBMjU2R0NNIn0","tag":"RF44xv0cvzOjZZv3ojouXA"}")

@Xynnn007
Copy link
Member

Yes. Currently we are using the function encrypt which will automatically append the auth tag to the returned ciphertext. To follow JWE RFC, we'd use encrypt_in_place_detached to explicitly mark the tag into responsed JWE.

Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Nov 25, 2024
@Xynnn007
KBS: fix JWE format
9f268f6 
Fixes confidential-containers#583.

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 `ciphertest`. Although this is also secure, it's
not standard.

We fix this by expcilitly extract the tag and include it into the jwe
body. Also, we use an AAD `CoCo` to do AEAD. This should be align with
the guest-components side.

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007 Xynnn007 linked a pull request Nov 25, 2024 that will close this issue
Open
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Nov 26, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
57d5ea2 
Fixes confidential-containers#583.

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 `ciphertest`. Although this is also secure, it's
not standard.

We fix this by expcilitly extract the tag and include it into the jwe
body. Also, we use an AAD `CoCo` to do AEAD. This should be align with
the guest-components side.

This 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]>
@Xynnn007
Copy link
Member

Xynnn007 commented Nov 27, 2024 •
edited
Loading

Ok. To follow rfc7516, we have the following roadmap

Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 2, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
4e6ee5c 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 2, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
7fb7897 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 2, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
56ae015 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 2, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
99d53ee 
Fixes confidential-containers#583.

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 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]>
@Xynnn007 Xynnn007 moved this to We have a plan in Trustee Roadmap Dec 2, 2024
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 10, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
2be8904 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 10, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
7b9e7c7 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 10, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
547ed02 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 12, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
08e47d3 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 12, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
1d260e2 
Fixes confidential-containers#583.

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 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]>
mkulke pushed a commit to mkulke/trustee that referenced this issue Dec 12, 2024
@Xynnn007 @mkulke
KBS: Update KBS protocol to 0.2.0 to fix JWE
413078f 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 17, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
2dca25b 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 18, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
24aa82d 
Fixes confidential-containers#583.

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 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]>
Xynnn007 added a commit to Xynnn007/kbs that referenced this issue Dec 19, 2024
@Xynnn007
KBS: Update KBS protocol to 0.2.0 to fix JWE
e49016a 
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]>
@fitzthum fitzthum moved this from We have a plan to We have code in Trustee Roadmap Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
Trustee Roadmap
Status: We have code
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

KBS: Update KBS protocol to 0.2.0 to fix JWE format due to RFC7516 Xynnn007/kbs
3 participants
@mkulke @deeglaze @Xynnn007