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: Add note on unsupported self-signed certificates #1382

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

exdx
Copy link
Contributor

@exdx exdx commented Aug 1, 2023

This clarifies the rustls position on self-signed certificates.
Users writing tests using rustls should be aware that a
self-signed cert won't work as expected.

Signed-off-by: Dan Sover dan.sover@avalabs.org

@djc
Copy link
Member

djc commented Aug 1, 2023

@ctz now that we control webpki, do you want to maintain that self-signed certificates are a non-feature (that is, are intentionally not supported)? Or should we add an opt-in dangerous_config() feature to support them somehow?

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #1382 (34f9ad1) into main (cc201f5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1382   +/-   ##
=======================================
  Coverage   96.34%   96.34%           
=======================================
  Files          64       64           
  Lines       14808    14808           
=======================================
  Hits        14267    14267           
  Misses        541      541           

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

@exdx
Copy link
Contributor Author

exdx commented Aug 1, 2023

Yes, I noticed there was a test

#[cfg(feature = "dangerous_configuration")]
that supports self-signed certs but only in a dangerous configuration. I can update the PR to reflect that.

@exdx
Copy link
Contributor Author

exdx commented Aug 1, 2023

My personal opinion is that this is helpful -- developers looking to use rustls might see this and know which configuration to use for self-signed certs, in tests especially.

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 for the PR! I have a request for a change w.r.t the wording being added.

README.md Outdated Show resolved Hide resolved
@exdx
Copy link
Contributor Author

exdx commented Aug 3, 2023

Thanks for the PR! I have a request for a change w.r.t the wording being added.

Sure, that makes sense to me. I'll change it. I'm on vacation for a week or so but will update afterwards. Thanks.

@exdx exdx requested a review from cpu August 15, 2023 11:51
@exdx
Copy link
Contributor Author

exdx commented Aug 15, 2023

@djc @cpu I have went ahead and updated the README. The PR is ready to go now 👍

@cpu
Copy link
Member

cpu commented Aug 15, 2023

@exdx Thanks, the update looks good. Could you squash the branch history down into just one commit updating README.md?

@ctz
Copy link
Member

ctz commented Aug 15, 2023

now that we control webpki, do you want to maintain that self-signed certificates are a non-feature

I think such a thing would have to be a separate, explicit API at the webpki-level. Which would call for a separate, explicit API at the rustls level to call it.

When people say "self-signed certificates", they are generally saying "turn off the security, I don't care" rather than asking caring much about the exact shape of the certificate chain -- we have an extension point and example code for doing that already.

I'm not sure just changing webpki to deal with self signed certs would suffice either -- the certificates tend to come from openssl one-liners -- far away from what is acceptable under the BRs. It seems like it wouldn't be useful to say "we support self-signed certs, but you have a to have basicConstraints, and subjectAlternativeNames extensions" etc.

I think that it's generally harder to generate a reasonable self-signed certificate and pin it than just using something like mkcert?

@cpu
Copy link
Member

cpu commented Aug 15, 2023

@exdx Thanks, the update looks good.

I spoke too soon, as pointed out by CI failure I think the updated text needs to be in lib.rs and then updated in README.md with admin/pull-readme.

@exdx
Copy link
Contributor Author

exdx commented Aug 15, 2023

@exdx Thanks, the update looks good.

I spoke too soon, as pointed out by CI failure I think the updated text needs to be in lib.rs and then updated in README.md with admin/pull-readme.

Sounds good, I'll take a look

This clarifies the rustls position on self-signed certificates.
Users writing tests using rustls should be aware that a
self-signed cert won't work as expected.

Signed-off-by: Dan Sover <dan.sover@avalabs.org>
@exdx
Copy link
Contributor Author

exdx commented Aug 15, 2023

@cpu Updated!

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.

Looks good. Thanks for landing the revisions :-)

@cpu cpu enabled auto-merge August 15, 2023 18:00
@cpu cpu added this pull request to the merge queue Aug 15, 2023
Merged via the queue into rustls:main with commit 9330cd7 Aug 15, 2023
21 checks passed
@exdx exdx deleted the docs/self-signed-cert branch August 15, 2023 18:12
@cpu cpu mentioned this pull request Sep 15, 2023
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