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

cargo: expose reqwest TLS feature flags #96

Merged
merged 2 commits into from
Feb 8, 2019

Conversation

lucab
Copy link
Member

@lucab lucab commented Feb 8, 2019

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

Closes #86 (superseded).

This exposes two underlying TLS feature flags from reqwest, allowing
consumers to pick openssl (default) or rustls implementation.
@lucab lucab requested a review from steveej February 8, 2019 10:08
@lucab
Copy link
Member Author

lucab commented Feb 8, 2019

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
Copy link
Contributor

steveej commented Feb 8, 2019

Please explain the consequences for consumers after this change.

@lucab
Copy link
Member Author

lucab commented Feb 8, 2019

@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
Copy link
Contributor

steveej commented Feb 8, 2019

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
Copy link
Member Author

lucab commented Feb 8, 2019

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. openshift/cincinnati#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
Copy link
Contributor

steveej commented Feb 8, 2019

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.

Copy link
Contributor

@steveej steveej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proceeding with my comment there's just one change to make: please add your explanation to the toplevel README.md

@lucab
Copy link
Member Author

lucab commented Feb 8, 2019

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

@lucab lucab mentioned this pull request Feb 8, 2019
@steveej steveej merged commit 1f4359d into camallo:master Feb 8, 2019
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.

2 participants