apache / incubator-teaclave-sgx-sdk

Apache Teaclave (incubating) SGX SDK helps developers to write Intel SGX applications in the Rust programming language, and also known as Rust SGX SDK.
https://teaclave.apache.org
Apache License 2.0
1.17k stars 262 forks source link

rustls & webpki dependencies are messed up in mesalock #206

Open brenzi opened 4 years ago

brenzi commented 4 years ago

There are inconsistencies between referencing crates with branch=mesalock_sgx and tag=sgx_v1.1.0

i.e. this here causes trouble: https://github.com/mesalock-linux/rustls/blob/d9a91f5037f5c85f761279d97b838256285a1314/Cargo.toml#L19

rustls tag sgx_v1.1.0 depends on webpki branch=mesaloch_sgx but I'd expect it to depend on tag sgx_v1.1.0

In our Cargo.toml we now have a mess between branch and tag dependencies leading to this (after a cargo update):

error: failed to select a version for the requirement `webpki-roots = "^0.17.0"`
  candidate versions found which didn't match: 0.19.0
  location searched: Git repository https://github.com/mesalock-linux/webpki-roots?branch=mesalock_sgx
required by package `substratee_worker_enclave v4.0.0 (/home/abrenzikofer/substraTEE-worker/enclave)`
piotr-roslaniec commented 4 years ago

I encountered a similar issue:

# Cargo.toml
webpki = { git = "https://github.com/mesalock-linux/webpki", tag = "sgx_1.1.0" }
webpki-roots = { git = "https://github.com/mesalock-linux/webpki-roots", tag = "sgx_1.1.0" }

Causes errors:

error[E0308]: mismatched types
   --> src/ra.rs:187:68
    |
187 |     let mut client = rustls::ClientSession::new(&Arc::new(config), dns_name);
    |                                                                    ^^^^^^^^ expected struct `webpki::name::DNSNameRef`, found a different struct `webpki::name::DNSNameRef`
    |
note: Perhaps two different versions of crate `webpki` are being used?
   --> src/ra.rs:187:68
    |
187 |     let mut client = rustls::ClientSession::new(&Arc::new(config), dns_name);
    |                                                                    ^^^^^^^^

cargo tree confirms that two different webpki version are used:

$ cargo tree -p webpki
error: There are multiple `webpki` packages in your project, and the specification `webpki` is ambiguous.
Please re-run this command with `-p <spec>` where `<spec>` is one of the following:
  https://github.com/mesalock-linux/webpki#0.21.2
  https://github.com/mesalock-linux/webpki#0.21.2

In mesalock-linux/webpki-roots:sgx_1.1.0 we can see:

webpki = { git = "https://github.com/mesalock-linux/webpki", branch = "mesalock_sgx" }

I suggest replacing branch with the tag:

webpki = { git = "https://github.com/mesalock-linux/webpki", tag = "sgx_1.1.0" }

Pinning by branch = "mesalock_sgx" solves it for me:

# Cargo.toml
webpki = { git = "https://github.com/mesalock-linux/webpki", branch = "mesalock_sgx"" }
webpki-roots = { git = "https://github.com/mesalock-linux/webpki-roots", branch = "mesalock_sgx" }
dingelish commented 4 years ago

maybe make everything depend on tag is a better choice?

it'll take a while to update all dependencies under mesalock-linux, and then every dependency would be forced to add tag = in its Cargo.toml

brenzi commented 4 years ago

I endorse your suggestion @dingelish. tag everywhere. It's not perfect because you still don't know the upstream version. The question is if you want to include that version in the tag too (We might have discussed this before): v1.2.3_sgx1.1.0

dingelish commented 4 years ago

@brenzi Plan to merge from branch v1.1.1-testing to master this weekend, followed by tagging every dependencies. It'll be a breaking change.

We wonder if using + is better, which means v1.2.3+sgx1.1.1. please advise. thanks!

brenzi commented 4 years ago

The + makes sense IMO

dingelish commented 4 years ago

The + makes sense IMO

how do we handle rustcrypto styled repo? such as https://github.com/mesalock-linux/rustcrypto-utils-sgx

they contain multiple crate in one single repo. it's really hard to tag it ... any idea?

brenzi commented 4 years ago

tricky one. Basically I think it is good practise to have one repo per version number and vice versa. This way this issue couldn't happen. But as you're not in charge of the upstream repo we're out of luck. You could of course split the fork into single repos, but that makes rebasing a pain.

Maybe going down to the toml? https://github.com/mesalock-linux/rustcrypto-utils-sgx/blob/2a0f1222fcb0b26405f3d731a091121a9db293f4/blobby/Cargo.toml#L3

version = "0.1.2-sgx-1-1-1"

(I hope this syntax is undestood by cargo?)

And then the tag for the repo would have to be

sgx1.1.1

but in the depending crates we could refer to "version".

dingelish commented 4 years ago

thanks! I just tried with toml version. result shows that we can sgx info in Cargo.toml like

[package]
name = "xxx"
version = "0.1.0+sgx1.1.1"
authors = ["Yu Ding <dingelish@gmail.com>"]
edition = "2018"

For dependents, it's the same as before:

[dependencies]
xxx = { version = "0.1.0" }

cargo would warn about unused metadata (warning: version requirement 0.1.0+sgx for dependency xxx includes semver metadata which will be ignored, removing the metadata is recommended to avoid confusion) on the following:

xxx = { version = "0.1.0+sgx1.1.1" }
dingelish commented 4 years ago

and i have a tool to inspect dependencies: cargo-deplist https://github.com/dingelish/cargo-deplist

it outputs a topological sort of dependency graph along with source metadata. it seems we can find duplication or errors with the help of it.

future improvements: high-lighting the duplicated/suspicious one according to some rules