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

added dangerous feature for accepting self signed certs #48

Closed
wants to merge 1 commit into from
Closed

added dangerous feature for accepting self signed certs #48

wants to merge 1 commit into from

Conversation

rustysec
Copy link

@rustysec rustysec commented Feb 8, 2017

The const is already there and needs to be manually set for testing. Using a feature, we can avoid editing source and this could be passed along by projects using rustls as a dependency. Sometimes it is necessary to accept a self signed certificate (internal networks, ephemeral certificate creation, etc).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.889% when pulling b5df179 on rustysec:dangerous into c54e19b on ctz:master.

@mcpherrinm
Copy link

I'd be pretty nervous about making this a compile-time option: It wouldn't be visible from code inspection alone whether this is set - you need to know the options you've built with, or testing. I can see the value in having this option, but I'm wary of this PR.

In fact, I think your message is a great example why: People turn off certificate validation at the first sign of trouble. If you want to accept a self-signed certificate in an internal network, you probably know what the certificate should be, so add it to a RootCertStore and connect with verification on.

@rustysec
Copy link
Author

While I agree with you in principal, it's not always that simple. If you are bringing a device online for a short period of time and need to enable TLS between 100k endpoints (using rustls), adding the cert to the local store is not always an option. There are risks here, for sure, but it's already an option in the code. And if there are crates using rustls as a dependency (hyper-rustls) it's currently a little cumbersome setup with the feature enabled.

While this probably isn't a feature that will be widely used, it is something I see value in for various workflows. There are workarounds, but this is the most ergonomic in some scenarios. This PR won't make or break anything for me.

@ctz
Copy link
Member

ctz commented Feb 16, 2017

I appreciate the contribution here, but I'm not really comfortable having something like this in the public API. A fork of rustls with DANGEROUS_DISABLE_VERIFY set to true might be a better option, or perhaps some extra API for TOFU-like TLS usages.

@ctz ctz closed this Feb 16, 2017
@rustysec
Copy link
Author

Upon further consideration, I concur. Thanks for the discussion, love the rustls.

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