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

docs: clarify self-signed certificate limitation. #1480

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

bdaehlie
Copy link
Contributor

That is a limitation of webpki and should be noted there.

See this PR for webpki.

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!

@djc
Copy link
Member

djc commented Sep 18, 2023

I don't think this is a good change.

For most users, webpki is an implementation detail of the rustls default verifier. Even if/when we make the webpki verifier more explicit in the API (for example, by making callers pass a WebPkiVerifiers::from(roots) to the config builder API), for a large majority of users rustls is intimately connected with webpki -- that's probably even more true of the relatively unsophisticated users who don't have a good idea about the trade-offs of self-signed certificates.

Instead, I think we should be upfront about this limitation and its nuances. For example, that it can be made to work by implementing a custom verifier, or how easy we can make it to support private CA setups.

@ctz
Copy link
Member

ctz commented Sep 18, 2023

How about this?

  • Using CA certificates directly to authenticate a server/client. CA certificates should
    only issue other certificates, and cannot be used alone for authentication. All authentication can be turned off -- see our example code.)

Note I'm carefully avoiding saying "self-signed", which I think can be used to mean three different things:

  1. a standalone CA certificate that has CN=localhost (or something) and therefore authenticates itself. This is specific case webpki doesn't support.
  2. a locally-administered PKI (ie, "i signed this myself, it is self-signed")
  3. turn-off-all-authentication-I-don't-care (ie, "don't bother me with this certificate bidness")

@djc
Copy link
Member

djc commented Sep 18, 2023

That's better, although I would say we actually need to have the word "self-signed" in there to make unsophisticated users understand? Could just be a parenthetical "(these are often said to be self-signed certificates)".

@cpu
Copy link
Member

cpu commented Sep 28, 2023

@bdaehlie Do you want to iterate on this with the suggested text from the feedback above, or would it be helpful if I adopted this and got it across the line?

@bdaehlie
Copy link
Contributor Author

bdaehlie commented Oct 1, 2023

If you're able to take this over that would be great, thanks.

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #1480 (9a23112) into main (992e236) will not change coverage.
Report is 4 commits behind head on main.
The diff coverage is n/a.

❗ Current head 9a23112 differs from pull request most recent head 71488bf. Consider uploading reports for the commit 71488bf to get more accurate results

@@           Coverage Diff           @@
##             main    #1480   +/-   ##
=======================================
  Coverage   96.46%   96.46%           
=======================================
  Files          72       72           
  Lines       15105    15105           
=======================================
  Hits        14571    14571           
  Misses        534      534           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cpu
Copy link
Member

cpu commented Oct 2, 2023

@ctz @djc I updated this branch with the suggestions from above. PTAL

@cpu cpu changed the title Remove self-signed certificates from limitations list. docs: clarify self-signed certificate limitation. Oct 2, 2023
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.

(fwiw)

README.md Outdated Show resolved Hide resolved
The self-signed certificate limitation imposed by the default webpki
certificate verifier is somewhat nuanced. This commit updates the README
to reflect some of this nuance.
@cpu cpu added this pull request to the merge queue Oct 4, 2023
Merged via the queue into rustls:main with commit 188e304 Oct 4, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants