fussybeaver / bollard

Docker daemon API in Rust
Apache License 2.0
911 stars 134 forks source link

Fix rustls features #464

Closed stormshield-gt closed 1 month ago

stormshield-gt commented 2 months ago

I've gone too swiftly while developing https://github.com/fussybeaver/bollard/pull/441 and I didn't test all the cases. This PR amend that, and add extra checks to the

fussybeaver commented 1 month ago

Hey, thanks for this and sorry for the delay in review...

I have a couple of questions..

None of the checks fail on master currently:

Is there a compile that failed on master that passes on your PR ? The PR otherwise looks fine.

Finally, if this really is an issue, then it looks like we may need to backport this as a separate patch release.

stormshield-gt commented 1 month ago

Thanks for the review!

None of the checks fail on master currently

Good catch, I've added a test that currently failed the master https://app.circleci.com/pipelines/github/fussybeaver/bollard/1395/workflows/d2053e50-d1ae-4019-b5c5-63d73350cf24/jobs/14227 and then pushed the fix.

To do so, I removed the webpki feature. Indeed, as stipulated in the webpki_roots readme warning, webpki_roots shouldn't be used in production except in some rare cases, as it requires recompiling each time the roots evolve. I still use it for the test, as does rustls organization repositories.

Finally, if this really is an issue, then it looks like we may need to backport this as a separate patch release.

If you agree with the webpki feature removal, then it would necessitate a minor release.

fussybeaver commented 1 month ago

So.. what I understand now, the problem currently in master is that we're still using the ring crypto implementation, even if the user specifies the aws-lc-rs crypto implementation and forgets to use CryptoProvider::install_default() ?

stormshield-gt commented 1 month ago

The user can only use the ring provider feature, and the aws-lc-rs and ssl-providerless aren't usable because the necessary functions are gated under the feature ssl instead of ssl-providerless. So if someone wants to use the aws-lc-rs feature, it will have this compiler error.

stormshield-gt commented 1 month ago

The current failed on CI is also on master with the command cargo check --no-default-features --all-targets --features ssl. As this wasn't checked in CI before, https://github.com/fussybeaver/bollard/pull/465 seems to have introduced the failure. Maybe a separate PR should be open to fix this ?

fussybeaver commented 1 month ago

Thanks for sticking with this! If you can document the entry functions to connecting with ssl that it can panic, then I'm happy to merge this PR.

stormshield-gt commented 1 month ago

I've added a panic section to the documentation of the concerned functions

fussybeaver commented 1 month ago

Thanks for putting the work into this