kbs-client: Add native-tls feature

[iroykaufman]

This PR introduces an option to use native-tls with kbs-client.
This can be beneficial for projects that want to use kbs-client as a dependency for API calls to KBS, but still want to use native-tls.

[Xynnn007]

hey @iroykaufman Do you have any scenarios where only openssl can work, while default tls cannot?

Historically, we bring in rust-tls because at that time openssl cannot be statically compiled. But now it supports, thus we can consider to fully turn to openssl if possible.

[iroykaufman]

hey @iroykaufman Do you have any scenarios where only openssl can work, while default tls cannot?

Historically, we bring in rust-tls because at that time openssl cannot be statically compiled. But now it supports, thus we can consider to fully turn to openssl if possible.

OpenSSL supports FIPS and PQC, so it can be beneficial to use OpenSSL instead of rust-tls because of those standards.

[Xynnn007]

OpenSSL supports FIPS and PQC, so it can be beneficial to use OpenSSL instead of rust-tls because of those standards.

Sounds good. In future we might consider fully embrace openssl.

[mythi]

this looks similar to what I recently evaluated with the question: can I build everything with the same crypto implementation/configuration. this work makes sense but it looks the PR topic is a bit off (more than just kbs-client)?

[iroykaufman]

this looks similar to what I recently evaluated with the question: can I build everything with the same crypto implementation/configuration. this work makes sense but it looks the PR topic is a bit off (more than just kbs-client)?

My understanding is that we cannot build everything using native-tls/openssl because tonic lacks support for it. I chose this PR topic because it adds the native-tls feature for the kbs-client.

[Jakob-Naucke]

this work makes sense but it looks the PR topic is a bit off (more than just kbs-client)?

It's because reqwest is set to use default-tls (becomes rustls) at workspace level and features are only additive to that (i.e. you can't use a crate from workspace but then not use a feature defined at that level AFAICT, and I didn't want to start tracking versions below workspace level).

kbs/Cargo.toml should also probably set a TLS implementation for reqwest, we'll do that before moving out of draft.

But I don't think there's a non-hacky way to set a TLS implementation at workspace level.

[mythi]

this looks similar to what I recently evaluated with the question: can I build everything with the same crypto implementation/configuration. this work makes sense but it looks the PR topic is a bit off (more than just kbs-client)?

My understanding is that we cannot build everything using native-tls/openssl because tonic lacks support for it. I chose this PR topic because it adds the native-tls feature for the kbs-client.

Oh sorry, I only took a quick look and it looked like there's changes beyond what kbs-client needs. These are all for just kbs-client but the overall goal is to have native-tls used as much as possible, right?

[Jakob-Naucke]

Oh sorry, I only took a quick look and it looked like there's changes beyond what kbs-client needs. These are all for just kbs-client but the overall goal is to have native-tls used as much as possible, right?

Our immediate focus for now was kbs-client but if desired it shouldn't be much work to extend to other crates. What we didn't know before raising was

In future we might consider fully embrace openssl.

, so I don't know if going all-native-tls is too drastic for now/breaks other things.

BTW the CI failures seem unrelated but I haven't been contributing for a while so I could be wrong

[Xynnn007]

@Jakob-Naucke

Oh sorry, I only took a quick look and it looked like there's changes beyond what kbs-client needs. These are all for just kbs-client but the overall goal is to have native-tls used as much as possible, right?

Our immediate focus for now was kbs-client but if desired it shouldn't be much work to extend to other crates. What we didn't know before raising was

In future we might consider fully embrace openssl.

, so I don't know if going all-native-tls is too drastic for now/breaks other things.

Let's start with the kbs-client crate. I think it's acceptable. Going fully to native-tls might be our target but NOT this PR's responsibility. We will take a try in later PRs.

I think @mythi cares about, is that it looks like the PR have touched some codes that might not be related to the PR's target. I did not take much look on the PR as it's still draft. And I'd like to offer help when it gets ready.

BTW the CI failures seem unrelated but I haven't been contributing for a while so I could be wrong

I have retriggered the CI. The CI is not always to be happy, and we're working on those issues.

[Xynnn007]

one nit. Also a rebase is needed

[Xynnn007]

can we add another feature named default-tls = ["reqwest/default-tls"] and default= ["default-tls"] ? I think a crate-level feature controls tls suites makes sense, wdyt?

[iroykaufman]

I think it's a good idea. It also allows us to use --no-default-features with default-tls.

[Xynnn007]

LGTM. Thanks! @iroykaufman @Jakob-Naucke

[fitzthum]

LGTM

can have a bigger discussion about our crypto backends but this PR seems fine