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.

Sign up for GitHub

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

Jump to bottom

Use extension trait to simplify config #150

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Use extension trait to simplify config #150

merged 1 commit into from
Oct 28, 2024

Conversation

djc
Copy link
Member

@djc djc commented Oct 25, 2024

No description provided.

@djc djc requested review from cpu, ctz and complexspaces October 25, 2024 09:18
@djc djc mentioned this pull request Oct 25, 2024
Merged
ctz
ctz approved these changes Oct 25, 2024
View reviewed changes
Copy link
Member

@ctz ctz left a comment

Choose a reason for hiding this comment

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

Seems like a big improvement!

@djc
Copy link
Member Author

djc commented Oct 25, 2024

Another idea: we could potentially add another trait implemented directly for ClientConfig, such that you could do ClientConfig::with_platform_verifier() and ClientConfig::with_platform_verifier_and_provider(provider).

Worth doing over the current approach of rustls_platform_verifier::tls_config()?

@djc djc force-pushed the extension-trait branch from f920d46 to 6e6d73a Compare October 25, 2024 09:43
cpu
cpu approved these changes Oct 25, 2024
View reviewed changes
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Nice!

@cpu
Copy link
Member

cpu commented Oct 25, 2024

Worth doing over the current approach of rustls_platform_verifier::tls_config()?

Your proposal seems a bit tidier to me. I'd be 👍

@complexspaces
Copy link
Collaborator

complexspaces commented Oct 25, 2024 •
edited
Loading

If we don't remove tls_config, maybe we should reimplement it in terms of the new extension trait internally for better coverage? Otherwise I think this looks fine.

@complexspaces complexspaces mentioned this pull request Oct 26, 2024
Merged
4 tasks
@djc djc mentioned this pull request Oct 27, 2024
Merged
@djc djc force-pushed the extension-trait branch from 6e6d73a to dfd00a8 Compare October 27, 2024 20:15
@djc
Copy link
Member Author

djc commented Oct 27, 2024

Implemented a "maximalist" approach with two new traits, that deprecates the old tls_config*() API.

@djc djc force-pushed the extension-trait branch 3 times, most recently from ae9b31d to c8bfc6f Compare October 27, 2024 20:24
cpu
cpu approved these changes Oct 28, 2024
View reviewed changes
@djc djc force-pushed the extension-trait branch from c8bfc6f to de7ce46 Compare October 28, 2024 13:27
@djc
Copy link
Member Author

djc commented Oct 28, 2024

Updated the README and also used the new ConfigBuilder::crypto_provider() API to make sure we use the matching crypto provider.

@djc djc force-pushed the extension-trait branch from de7ce46 to dde835b Compare October 28, 2024 13:33
@djc djc merged commit 08d5fd9 into main Oct 28, 2024
22 checks passed
@cpu cpu deleted the extension-trait branch October 30, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpu cpu cpu approved these changes

@ctz ctz ctz approved these changes

@complexspaces complexspaces complexspaces approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@djc @cpu @complexspaces @ctz