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

Improved TLS configuration #570

Conversation

SvenKeimpema
Copy link
Contributor

This update addresses an issue in previous versions of zitadel-rust where the crate was unable to connect to the server. The cause of this issue was that the previous versions of this crate was unable to find certificates which consistently results in the error message "could not set up TLS connection".

The reason of this error message is that there isn't always a certificate within the image thus resulting in this error.

To fix this issue I've added an option for the user that lets him choose between using tls-roots or using tls-webpki-roots

However, it's important to note that this change introduces a breaking change to the crate's configuration options.
Users are now required to choose between using tls-roots or tls-webpki-roots for TLS certificate configurations.

Whilst this change may be inconvenient, it is pretty usual within other crates choose between tls-webpki-roots and tls-roots.

Here's an example of how to use the new configuration:

zitadel = {features=["tls-roots"]}

or

zitadel = {features=["tls-webpki-roots"]}

@buehler
Copy link
Collaborator

buehler commented Sep 5, 2024

Hi @SvenKeimpema

Thank you for contributing!

This does not have to be a breaking change. You could add the tls-webpki roots as optional feature and not enable it by default. And in the default features, you enable what already has been in place. As such, users who have the need can disable default features and use the specialized case, right?

Otherwise I'm fine with the change :)

@SvenKeimpema
Copy link
Contributor Author

Hello @buehler

Thanks for the feedback! In the new commit should be an updated version of the cargo.toml where I put tls-roots as a default option in the features fields. This should fix the issue where it now won't be a breaking change.

PS:
For anyone reading this now if you wan't to use tls-webpki-roots instead of tis-roots u should add default-features = false into your cargo.toml file and enable the feature tls-webpki-roots

buehler
buehler previously approved these changes Sep 5, 2024
@buehler buehler enabled auto-merge (squash) September 5, 2024 14:18
@buehler
Copy link
Collaborator

buehler commented Sep 5, 2024

Hey @SvenKeimpema

I tried to merge, but I think clippy has something to rant about. Would you mind to fx the error and update the pr?

Thank you for the effort

auto-merge was automatically disabled September 5, 2024 14:41

Head branch was pushed to by a user without write access

@SvenKeimpema
Copy link
Contributor Author

@buehler haha tried to access the feature I removed, ran clippy and seems to be working now.

@buehler buehler enabled auto-merge (squash) September 5, 2024 17:48
@buehler buehler merged commit 4ebde7b into smartive:main Sep 5, 2024
2 checks passed
@dmeijboom dmeijboom deleted the feature/brain-312-add-support-for-tls-webki-roots-in-zitadel-rust-api branch September 5, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants