m4sterchain commented on code in PR #221:
URL:
https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/221#discussion_r2278205987
##########
examples/tls_client-rs/ta/src/main.rs:
##########
@@ -64,34 +63,35 @@ fn invoke_command(cmd_id: u32, _params: &mut Parameters) ->
Result<()> {
}
// This code is based on the Rustls example:
-//
https://github.com/rustls/rustls/blob/v/0.21.0/examples/src/bin/simpleclient.rs
+//
https://github.com/rustls/rustls/blob/v/0.23.12/examples/src/bin/simpleclient.rs
// with modifications by Teaclave to demonstrate Rustls usage in the TA.
// Licensed under the Apache License, Version 2.0.
fn tls_client() {
- let mut root_store = RootCertStore::empty();
-
root_store.add_server_trust_anchors(webpki_roots::TLS_SERVER_ROOTS.0.iter().map(|ta|
{
- OwnedTrustAnchor::from_subject_spki_name_constraints(
- ta.subject,
- ta.spki,
- ta.name_constraints,
- )
- }));
- trace_println!("[+] root_store added");
+ // Create our custom providers
+ let crypto_provider = Arc::new(rustls_provider::optee_crypto_provider());
+ let time_provider = Arc::new(rustls_provider::optee_time_provider());
+
+ let root_store = RootCertStore {
+ roots: webpki_roots::TLS_SERVER_ROOTS.into(),
+ };
- let config = rustls::ClientConfig::builder()
- .with_safe_defaults()
+ let mut config =
rustls::ClientConfig::builder_with_details(crypto_provider, time_provider)
+ .with_safe_default_protocol_versions()
+ .expect("inconsistent cipher-suite/versions selected")
.with_root_certificates(root_store)
.with_no_client_auth();
- trace_println!("[+] config created");
- let server_name = "google.com".try_into().unwrap();
+ // Allow using SSLKEYLOGFILE.
+ config.key_log = Arc::new(rustls::KeyLogFile::new());
Review Comment:
Any reason to allow KeyLogFile in TA?
##########
build_optee_libraries.sh:
##########
@@ -41,7 +41,7 @@ export CROSS_COMPILE64="aarch64-linux-gnu-"
# build optee_os and optee_client for qemu_v8
git clone https://github.com/OP-TEE/optee_os.git -b $OPTEE_VERSION
$OPTEE_DIR/optee_os
-(cd $OPTEE_DIR/optee_os && make PLATFORM=vexpress-qemu_armv8a)
+(cd $OPTEE_DIR/optee_os && make PLATFORM=vexpress-qemu_armv8a
CFG_TA_FLOAT_SUPPORT=n)
Review Comment:
I am not sure the reason for this change.
But we have another `build_optee_libraries.sh` in `scripts/setup` for
update, if this change is necessary.
##########
examples/tls_server-rs/ta/src/main.rs:
##########
@@ -131,36 +144,44 @@ pub fn do_tls_write(session_id: u32, buf: &mut [u8]) ->
usize {
rc
}
-fn make_config() -> Arc<rustls::ServerConfig> {
- let certs = load_certs();
- let privkey = load_private_key();
- let config = rustls::ServerConfig::builder()
- .with_safe_defaults()
+fn make_config() -> anyhow::Result<Arc<rustls::ServerConfig>> {
+ trace_println!("[+] Creating crypto provider");
+ let crypto_provider = Arc::new(rustls_provider::optee_crypto_provider());
+
+ trace_println!("[+] Creating time provider");
+ let time_provider = Arc::new(rustls_provider::optee_time_provider());
+
+ let certs = load_certs().context("Failed to load certificates")?;
+ trace_println!("[+] Loaded {} certificates", certs.len());
+
+ let private_key = load_private_key().context("Failed to load private
key")?;
+ trace_println!(
Review Comment:
Private data should never be printed inside Trusted Applications (TAs),
especially in demonstration examples.
##########
examples/tls_server-rs/ta/test-ca/ecdsa/ca.cert:
##########
@@ -1,12 +1,12 @@
-----BEGIN CERTIFICATE-----
-MIIByjCCAVCgAwIBAgIUSA11/39PY7uM9Nc2ITnV1eHzaKYwCgYIKoZIzj0EAwIw
-HDEaMBgGA1UEAwwRcG9ueXRvd24gRUNEU0EgQ0EwHhcNMTkwNjA5MTcxNTEyWhcN
-MjkwNjA2MTcxNTEyWjAcMRowGAYDVQQDDBFwb255dG93biBFQ0RTQSBDQTB2MBAG
-ByqGSM49AgEGBSuBBAAiA2IABLsXWEKs2xXCgW1OcC63pCPjQo0q3VnPc1J24n6m
-Xwxpg398nzR4n3iHcYA0pKgEneBstSOsXOhbNZ09DAvEr3iSc8ByWWntEbWVjY3g
-9Kt6Q6Y1sXGkaUIiP9be5lIQRaNTMFEwHQYDVR0OBBYEFKD72TTU/GXhb3/D1/Z7
-hD/ZG6lKMB8GA1UdIwQYMBaAFKD72TTU/GXhb3/D1/Z7hD/ZG6lKMA8GA1UdEwEB
-/wQFMAMBAf8wCgYIKoZIzj0EAwIDaAAwZQIxAL9FtbNV7i9trxukhakfTvbXCHgE
-2pIOT5r/Vc5kSrPU4vJu2MOJz6X/JCX15IbZlQIwJxYfsD8QTQf8J9bP9Pq4SY71
-obja/vQ6UBixlRB5vDSG0UuukL4kzlyUKpHkwUcj
+MIIBvDCCAUKgAwIBAgIUcfobdnUAGg7+ZKWcoJDhubf76iEwCgYIKoZIzj0EAwIw
+FTETMBEGA1UEAwwKdGVzdHNlcnZlcjAeFw0yNTA4MTMwMzQ5MTFaFw0zNTA4MTEw
+MzQ5MTFaMBUxEzARBgNVBAMMCnRlc3RzZXJ2ZXIwdjAQBgcqhkjOPQIBBgUrgQQA
+IgNiAARuhizLUdjzt2IufRL1ELnTris+0kHEkc1WYS/vSaj+8dMRZ9/MW8ScsMjn
+zs0K6XMme4o3TuPgFm3MdiWlIwHBN4CjkPN3u0U3N3Lw1DHvl/oUMfQ/mgBt9/Cz
+osOnSx2jUzBRMB0GA1UdDgQWBBRIkFT2dS2wya07AoKzpoL0E6SO/zAfBgNVHSME
+GDAWgBRIkFT2dS2wya07AoKzpoL0E6SO/zAPBgNVHRMBAf8EBTADAQH/MAoGCCqG
+SM49BAMCA2gAMGUCMQCD4bitceb4NJ9Snf3X3Watg3YlWv5mmeT3K6khxEiypQOd
+/XJC+UvUisbD9srbWLUCMBydSCIzPsS4CGIWQ+lZY1+tqWQ6FVW9iTQ7pYgZSibD
Review Comment:
Is there any specific reason for changing the existing test certs/keys?
It’s good to add a script that explains how to generate these test-CA
artifacts. However, for the existing artifacts, we should keep them unchanged
and reuse them as a demonstration that the updated TLS example remains
compatible.
##########
examples/tls_server-rs/ta/src/main.rs:
##########
@@ -92,12 +94,23 @@ fn invoke_command(cmd_id: u32, params: &mut Parameters) ->
Result<()> {
}
pub fn new_tls_session(session_id: u32) {
Review Comment:
1. This function should return a Result, as there are multiple failures
inside its impl.
2. The unwrap should always be eliminated, in our codebase.
I suggest to run `cargo clippy` to help check and follow best Rust coding
practices.
##########
examples/tls_client-rs/ta/Cargo.toml:
##########
@@ -17,38 +17,30 @@
[package]
name = "ta"
-version = "0.4.0"
+version = "0.5.0"
authors = ["Teaclave Contributors <[email protected]>"]
license = "Apache-2.0"
repository = "https://github.com/apache/incubator-teaclave-trustzone-sdk.git";
description = "An example of Rust OP-TEE TrustZone SDK."
edition = "2018"
[dependencies]
-libc = { path = "../../../rust/libc" }
proto = { path = "../proto" }
-optee-utee-sys = { path = "../../../optee-utee/optee-utee-sys" }
-optee-utee = { path = "../../../optee-utee" }
+optee-utee-sys = "0.4.0"
+optee-utee = "0.4.0"
-# use new ported version
-rustls = { git = "https://github.com/DemesneGH/rustls-optee.git";, branch =
"0.21.0-optee", features = ["dangerous_configuration"]}
-ring = "=0.16.20"
-webpki-roots = "0.21"
-webpki = "=0.21.0"
-sct = "=0.7.0"
+rustls_provider = { path = "../../../crates/rustls_provider" }
+rustls = { version = "0.23.12", default-features = false, features = ["std"] }
+webpki-roots = "1"
[build-dependencies]
proto = { path = "../proto" }
-optee-utee-build = { path = "../../../optee-utee-build" }
+optee-utee-build = "0.4.0"
[profile.release]
panic = "abort"
lto = false
opt-level = 3
[patch.crates-io]
-ring = { git = "https://github.com/DemesneGH/ring-optee.git";, branch =
"0.16.20-optee" }
-
-# Patch optee-utee for rustls
-[patch."https://github.com/apache/incubator-teaclave-trustzone-sdk.git";]
-optee-utee = { path = "../../../optee-utee" }
+getrandom = { git =
"https://github.com/DemesneGH/incubator-teaclave-crates.git"; }
Review Comment:
Can we depend on unpatched getrandom and leveraging its `custom` feature?
https://docs.rs/getrandom/0.2.16/getrandom/index.html#macros
If it works, we don't need to maintain the ported crates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
