camallo / dkregistry-rs

A pure-Rust asynchronous library for Docker Registry API v2
Apache License 2.0
62 stars 39 forks source link

cargo: expose reqwest TLS feature flags #96

Closed lucab closed 5 years ago

lucab commented 5 years ago

This exposes two underlying TLS feature flags from reqwest, allowing consumers to pick openssl (default) or rustls implementation.

Closes https://github.com/camallo/dkregistry-rs/pull/86 (superseded).

lucab commented 5 years ago

I verified that re-exposing the feature flag works with:

$ cargo build --no-default-features --features reqwest-rustls  --example tags
$ ldd target/debug/examples/tags
        linux-vdso.so.1 (0x00007ffce4b8a000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f5618d53000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f5618d49000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f5618d28000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f5618d0e000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f5618b4d000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f56198af000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f56189ca000)

/cc @steveeJ

steveej commented 5 years ago

Please explain the consequences for consumers after this change.

lucab commented 5 years ago

@steveeJ do you want me to add a note to readme / docstring like https://docs.rs/reqwest/0.9.9/reqwest/#optional-features? Or are you just wondering about what's the default behavior?

steveej commented 5 years ago

do you want me to add a note to readme / docstring like https://docs.rs/reqwest/0.9.9/reqwest/#optional-features? Or are you just wondering about what's the default behavior?

The latter, I'm clueless ;-)

lucab commented 5 years ago

Oh, sorry, let me go in more details here then.

reqwest recently gained feature flags to let consumers switch the underlying TLS implementation: https://docs.rs/reqwest/0.9.6/reqwest/#optional-features Without further configuration, the default set is default-tls (i.e. openssl). rustls implementation can be used instead by disabling the default one and activating rustls-tls.

This PR just re-exposes those options to consumers of dkregistry. This adds reqwest-default-tls in our default set, which in turns enables default-tls in reqwest (note the linkage against openssl libcrypto and libssl):

$ cargo build --example tags
$ ldd target/debug/examples/tags
        linux-vdso.so.1 (0x00007fff8a9e4000)
        libssl.so.1.0.2 => /usr/lib/x86_64-linux-gnu/libssl.so.1.0.2 (0x00007fbb20b66000)
        libcrypto.so.1.0.2 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.0.2 (0x00007fbb20901000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007fbb208fc000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007fbb208f2000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fbb208d1000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fbb208b7000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fbb206f4000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fbb214c0000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fbb20571000)

However, consumers can switch to rustls-only via the reqwest-rustls feature, in a way similar to reqwest flow (this embeds rustls in the binary and removes any openssl linkage):

$ cargo build --no-default-features --features reqwest-rustls  --example tags
$ ldd target/debug/examples/tags
        linux-vdso.so.1 (0x00007ffce4b8a000)
        libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f5618d53000)
        librt.so.1 => /lib/x86_64-linux-gnu/librt.so.1 (0x00007f5618d49000)
        libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f5618d28000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f5618d0e000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f5618b4d000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f56198af000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f56189ca000)

If a consumer doesn't care about any of this, not specifying any feature flag will result in reqwest default/historical behavior of just using openssl. https://github.com/openshift/cincinnati/pull/57 should fit in this case and not be concerned with rustls at all anymore.

I hope it is a bit clearer now, sorry for being terse initially. That said, I think we can still bikeshed on features name (should I stick to the same ones reqwest uses?) and see where to document them (is this comment enough or should I add a persistent note somewhere else?)

steveej commented 5 years ago

Thanks for elaborating, very helpful and I understand the point now; we enable consumers of dkregistry-rs to configure the transitive dependency reqwest.

That said, I think we can still bikeshed on features name (should I stick to the same ones reqwest uses?) and see where to document them (is this comment enough or should I add a persistent note somewhere else?)

I'm fine with the naming and the comment is enough for me to understand it, so it's likely enough for others if we add it to the docs.

lucab commented 5 years ago

Ack, I pushed an additional commit on top to document these new feature flags.