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

Added two security enhancements to AA #273

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

jialez0
Copy link
Member

@jialez0 jialez0 commented Jul 11, 2023

  1. Update TEE key when generate evidence
    We should not use the same pair of RSA keys for a long time, as this will bring forward security issues.
    This change alleviates this issue by updating the TEE key every time the attestation is redone.

  2. Allow setting custom KBS certificates to enable TLS
    Add parameters for setting customized KBS Root certificate when creating HTTP client, so that TLS communication can be enabled in various deployment scenarios.

@mythi
Copy link
Contributor

mythi commented Jul 21, 2023

@jialez0 thanks a lot for adding e26aa2c, I will give it a try as soon as possible

We should not use the same pair of RSA keys for a long time,
as this will bring forward security issues.
This commit alleviates this issue by updating the TEE key
every time the attestation is redone.

Signed-off-by: Jiale Zhang <zhangjiale@linux.alibaba.com>
@jialez0 jialez0 force-pushed the https branch 3 times, most recently from 7bdcf20 to a663da6 Compare July 28, 2023 03:32
@jialez0
Copy link
Member Author

jialez0 commented Jul 28, 2023

@mythi Now this PR is rebased, you can try to test it. We are trying to get this PR merged :-)

attestation-agent/kbs_protocol/src/lib.rs Outdated Show resolved Hide resolved
attestation-agent/kbs_protocol/src/lib.rs Outdated Show resolved Hide resolved
attestation-agent/kbs_protocol/src/lib.rs Outdated Show resolved Hide resolved
attestation-agent/kbs_protocol/src/lib.rs Show resolved Hide resolved
attestation-agent/kbs_protocol/src/lib.rs Outdated Show resolved Hide resolved
Add parameters for setting customized KBS Root certificate when creating HTTP client,
so that TLS communication can be enabled in various deployment scenarios.

Signed-off-by: Jiale Zhang <zhangjiale@linux.alibaba.com>
@jialez0
Copy link
Member Author

jialez0 commented Jul 28, 2023

@Xynnn007 Updated :-)

@Xynnn007 Xynnn007 merged commit c6306eb into confidential-containers:main Jul 28, 2023
11 checks passed
@mythi
Copy link
Contributor

mythi commented Jul 28, 2023

@mythi Now this PR is rebased, you can try to test it. We are trying to get this PR merged :-)

@jialez0 how did you test this with the kbs client tool? My app gets an error

called `Result::unwrap()` on an `Err` value: error sending request for url (https://127.0.0.1:8080/kbs/v0/auth): error trying to connect: error:0A000086:SSL routines:tls_post_process_server_certificate:certificate verify failed:../ssl/statem/statem_clnt.c:1883: (self-signed certificate)

besides that error, another weird part is it uses openssl even if I'm asking kbs_protocol to use rust-crypto

@Xynnn007
Copy link
Member

besides that error, another weird part is it uses openssl even if I'm asking kbs_protocol to use rust-crypto

I think the dependency issue will be fixed by this commit 24e2b57#diff-8a5ee983a9fbee31b805469c7a7f54371823f89a8f9c7946bda449462bf6d188R41-R42 I have test locally.

@mythi
Copy link
Contributor

mythi commented Jul 31, 2023

besides that error, another weird part is it uses openssl even if I'm asking kbs_protocol to use rust-crypto

I think the dependency issue will be fixed by this commit 24e2b57#diff-8a5ee983a9fbee31b805469c7a7f54371823f89a8f9c7946bda449462bf6d188R41-R42 I have test locally.

My binary gets host OpenSSL dependency regardless of this change.

@mythi
Copy link
Contributor

mythi commented Jul 31, 2023

besides that error, another weird part is it uses openssl even if I'm asking kbs_protocol to use rust-crypto

I think the dependency issue will be fixed by this commit 24e2b57#diff-8a5ee983a9fbee31b805469c7a7f54371823f89a8f9c7946bda449462bf6d188R41-R42 I have test locally.

My binary gets host OpenSSL dependency regardless of this change.

But that's not important right now. The real issue is: seanmonstar/reqwest#1260

@mythi
Copy link
Contributor

mythi commented Jul 31, 2023

besides that error, another weird part is it uses openssl even if I'm asking kbs_protocol to use rust-crypto

I think the dependency issue will be fixed by this commit 24e2b57#diff-8a5ee983a9fbee31b805469c7a7f54371823f89a8f9c7946bda449462bf6d188R41-R42 I have test locally.

My binary gets host OpenSSL dependency regardless of this change.

But that's not important right now. The real issue is: seanmonstar/reqwest#1260

So I got the functionality verified after I changed:

diff --git a/attestation-agent/kbs_protocol/src/lib.rs b/attestation-agent/kbs_protocol/src/lib.rs
index 1d57e6d..0e25666 100644
--- a/attestation-agent/kbs_protocol/src/lib.rs
+++ b/attestation-agent/kbs_protocol/src/lib.rs
@@ -286,6 +286,7 @@ fn build_http_client(kbs_root_certs_pem: Vec<String>) -> Result<reqwest::Client>
     }
 
     client_builder
+        .use_rustls_tls()
         .build()
         .map_err(|e| anyhow!("Build KBS http client failed: {:?}", e))
 }

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

Successfully merging this pull request may close these issues.

3 participants