You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "crepererum (via GitHub)" <gi...@apache.org> on 2023/11/02 14:54:47 UTC
[PR] refactor: feature-switch for `object_store` CA certs [arrow-rs]
crepererum opened a new pull request, #5030:
URL: https://github.com/apache/arrow-rs/pull/5030
# Which issue does this PR close?
Closes #4870.
# Rationale for this change
See issue.
# What changes are included in this PR?
Feature switches, do NOT make any choice by default.
# Are there any user-facing changes?
**Breaking:** Users now must choose their CA source.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: feature-switch for `object_store` CA certs [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5030:
URL: https://github.com/apache/arrow-rs/pull/5030#issuecomment-1791541036
I also seem to remember @roeap mentioning using openssl as an option with delta-rs, this may or may not be relevant here?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: feature-switch for `object_store` CA certs [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5030:
URL: https://github.com/apache/arrow-rs/pull/5030#issuecomment-1792366641
Having spoken with Marco, we've decided on an alternative path forward for this, so closing for now to save other reviewer's time
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: feature-switch for `object_store` CA certs [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #5030:
URL: https://github.com/apache/arrow-rs/pull/5030#issuecomment-1792203233
> So to check my understanding is correct, currently we enable `rustls-tls` which is equivalent to `rustls-tls-webpki-roots` which uses `webpki-roots`.
correct.
> If you then add `rustls-tls-native-roots` it will **also** include any system CA present, see [here](https://github.com/seanmonstar/reqwest/blob/50dbaf391087cfa951accc765126b4f5d017d8a3/src/async_impl/client.rs#L481).
correct.
> This PR is therefore only adding the ability to use feature flags to only use system roots. This feels like a less common requirement, but is definitely something we should permit
not correct: this PR removes `rustls-tls` and replaces it by `rustls-tls-manual-roots` which is "enable rustls but do NOT include any roots at all, neither webpki nor system". It then offers you two feature switches which allows you to include one or both of them (in the latter case they are merged).
> I wonder if as part of https://github.com/apache/arrow-rs/issues/5034 we could expose methods like [use_preconfigured_tls](https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.use_preconfigured_tls). This would give downstreams full control of how they want to configure TLS should they so wish. Would this be sufficient?
You would still need to expose a "use system CAs" feature because w/o that system CAs aren't available at all. The reason is that using system CAs needs additional dependencies, see <https://crates.io/crates/rustls-native-certs/>. We could also bake-in both (webpki and system) and expose a switch, but that makes `object_store` quite heavy (and also doesn't provide users any way to escape the license issue). We could also clone all features (`cloud`, `aws`, ...) and introduce new variants (e.g. `cloud-no-ca`, `aws-no-ca`, ...) but I'm not sure if that's any better.
> I don't disagree, this was an oversight on my part I enabled rustls-tls not realising it would override the system CA store, but I would really rather avoid a breaking change if we can avoid it.
With my prev. paragraph, I think we basically have the following options:
- do nothing
- include everything (webpki and system) all the time and expose runtime config options, that makes this crate here quite heavy (due to the larger amount of indirect deps)
- have more feature variants, comes w/ maintenance burden
- breaking change
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: feature-switch for `object_store` CA certs [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on code in PR #5030:
URL: https://github.com/apache/arrow-rs/pull/5030#discussion_r1380402269
##########
object_store/src/lib.rs:
##########
@@ -443,6 +453,13 @@
))]
compile_error!("Features 'gcp', 'aws', 'azure', 'http' are not supported on wasm.");
+#[cfg(all(
+ feature = "cloud",
+ not(feature = "tls-native-roots"),
+ not(feature = "tls-webpki-roots"),
+))]
+compile_error!("Feature 'cloud' needs at a CA root feature, use either 'tls-native-roots' or 'tls-webpki-roots'.");
Review Comment:
Technically you could silently ignore that and do not use any CA source (apart from the manual one). However this leads to runtime errors ("invalid cert") and is likely surprising to many users (esp. if they don't test any real cloud store in CI), so I've opted for forcing people to make a choice during compilation time.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: feature-switch for `object_store` CA certs [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5030:
URL: https://github.com/apache/arrow-rs/pull/5030#issuecomment-1792181851
So to check my understanding is correct, currently we enable `rustls-tls` which is equivalent to `rustls-tls-webpki-roots` which uses `webpki-roots`.
If you then add `rustls-tls-native-roots` it will **also** include any system CA present, see [here](https://github.com/seanmonstar/reqwest/blob/50dbaf391087cfa951accc765126b4f5d017d8a3/src/async_impl/client.rs#L481).
This PR is therefore only adding the ability to use feature flags to **only** use system roots. This feels like a less common requirement, but is definitely something we should permit.
I wonder if as part of #5034 we could expose methods like [use_preconfigured_tls](https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.use_preconfigured_tls). This would give downstreams full control of how they want to configure TLS should they so wish. Would this be sufficient?
> If you wanna have a default, IMHO this should be "system" rather than "webpki"
I don't disagree, this was an oversight on my part I enabled `rustls-tls` not realising it would override the system CA store, but I would really rather avoid a breaking change if we can avoid it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: feature-switch for `object_store` CA certs [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold closed pull request #5030: refactor: feature-switch for `object_store` CA certs
URL: https://github.com/apache/arrow-rs/pull/5030
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: feature-switch for `object_store` CA certs [arrow-rs]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #5030:
URL: https://github.com/apache/arrow-rs/pull/5030#issuecomment-1792112417
If you wanna have a default, IMHO this should be "system" rather than "webpki" for the following reasons:
- CAs should be a system admin config, not an application choice
- related to prev. point: bundles CAs don't play nice with all sorts things people want (or have to) do, like TLS interception (for debugging or compliance reasons), hardening (e.g. by restricting the set of CAs)
- also related to first point: having one set of CAs makes packaging way easier (ref: [Arch Linux change](https://archlinux.org/todo/use-system-ca-store/))
- license issues (webpki-roots is MPL-2.0, which strictly speaking is a copyleft license and way weaker than what is standard in the Rust ecosystem)
- you easily have multiple bundles in a single application (anecdotal evidence: InfluxDB IOx had THREE different bundles at some point)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: feature-switch for `object_store` CA certs [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5030:
URL: https://github.com/apache/arrow-rs/pull/5030#issuecomment-1791538152
I'm a little torn on this, I feel quite strongly that things should work out of the box without users having to understand the nuances of bundled vs system provided CA bundles... Is there any way we could use whatever the current default is, but provide a runtime mechanism to override this on ClientOptions or similar?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org