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

Add deployment considerations section to README #142

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

complexspaces
Copy link
Collaborator

This PR adds documentation and more explicit recommendations to the README of the crate to support its preferred use in the Rust app and library ecosystem. This has been a WIP stash for a pretty long time (since this old Discord conversation) and I've really been wanting to finish it out for several reasons.

These changes also include noting tradeoffs that rustls users might need to consider when doing something different to show supporting evidence.

@complexspaces complexspaces added the documentation Improvements or additions to documentation label Sep 18, 2024
@complexspaces complexspaces requested review from djc, cpu and ctz September 18, 2024 05:49
@complexspaces complexspaces self-assigned this Sep 18, 2024
@complexspaces complexspaces force-pushed the deployment-documentation branch from 2628a71 to 0155c04 Compare September 18, 2024 05:56
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

This is great!

I think it would be good to explain the Linux/BSD situation in a little more detail. In particular, while it inherits trust store updates from the OS, currently these are only configured on program startup, so in its current state on Linux/BSD the platform verifier ends up being the same as just using rustls-native-certs directly AFAICT.

Also it might be good to talk about scenarios where the use of webpki-roots could make sense, for example in applications that are (a) deployed frequently in (b) minimal containers.

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.

Thanks for writing this down. Once we land this, I think we can delete https://github.com/rustls/rustls-native-certs/?tab=readme-ov-file#should-i-use-this-or-webpki-roots and refer to this as a single source.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
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.

This is great, thank you! I had a handful of comments but don't consider any of them blocking. Feel free to disregard if you think I'm barking up the wrong tree on any of my points.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@complexspaces
Copy link
Collaborator Author

I addressed most of the first set of feedback. I apologize for taking longer then expected, time slipped away 😅.

The things still pending are:

  • Documentation on BSD/Linux, requested by @djc
  • Correct vs full wording for distrust decisions
  • Remaining section clarifications from @cpu

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for the updates!

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.

Thanks! Updates look good. There are still a couple open discussion threads but whether or not they're resolved I think this is a great improvement. Happy for it to land as-is if you disagree with my remaining items.

@complexspaces
Copy link
Collaborator Author

I believe I've now addressed all the remaining feedback from today. Please take a final look when available.

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.

Thanks again for taking all of our feedback into consideration.

@complexspaces
Copy link
Collaborator Author

Of course, and I appreciate the thorough review to make sure the first version of this is high-quality.

@complexspaces complexspaces merged commit 387ca04 into main Oct 2, 2024
20 checks passed
@complexspaces complexspaces deleted the deployment-documentation branch October 2, 2024 16:42
@complexspaces
Copy link
Collaborator Author

Once we land this, I think we can delete https://github.com/rustls/rustls-native-certs/?tab=readme-ov-file#should-i-use-this-or-webpki-roots and refer to this as a single source.

I've started that work in this PR.

@djc
Copy link
Member

djc commented Oct 2, 2024

I think it's also time for a release in this crate, right?

stormshield-gt pushed a commit to stormshield-gt/rustls-platform-verifier that referenced this pull request Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants