-
Notifications
You must be signed in to change notification settings - Fork 213
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
make cpanm secure by default #674
base: devel
Are you sure you want to change the base?
Conversation
I think this is good for 99% of the users out there with a working TLS via curl out of the box. My concern is that it would suddenly break the remaining 1% on their CI systems. I wonder if it makes sense to automatically fallback to HTTP for a while and then disable the fallback as a next step. |
Would not you face the same problem you are describing when disabling the fallback later? note: the |
We could also consider only supporting |
@miyagawa we did a new commit to this PR patching We also tweaked We believe this fully covers the edge cases you mentioned. Please let us know if you need any amends. (if this update is acceptable, would you like us to also send the PR to another branch, like |
If none of the available clients from HTTP::Tinyish support SSL then we should die with a better error message rather than trying to use 'undef' as a backend (which fix an error when calling $backend->new a few lines later). This is also adding an extra check inside the 'mirror' function. That function is used in multiple locations without checking directly the error status. The goal is to detect invalid certificate errors when SSL is supported by the backend.. Note that depending on the backend and probably client version the error message can differ. `HTTP::Tiny` Internal Exception raised with invalid certificates: SSL connection failed for cpan.metacpan.org: Invalid certificate authority locations error:0D07A086:asn1 ... `HTTP::Tinyish::LWP` Internal Exception raised with invalid certificates: 500 Can't connect to cpan.metacpan.org:443 () `HTTP::Tinyish::Curl` Internal Exception raised with invalid certificates: curl: (60) Peer certificate cannot be authenticated with known CA certificates More details here: http://curl.haxx.se/docs/sslcerts.html `HTTP::Tinyish::Wget` Internal Exception raised with invalid certificates: ... ERROR: cannot verify cpan.metacpan.org’s certificate, issued by ... This patch accounts for all the scenarios above. Signed-off-by: Breno G. de Oliveira <garu@cpan.org>
Sure, but that can be done much later. I would say that HTTPS by default but allowing automatic fallback is still better than HTTP by default. Nobody would disagree that HTTPs by default and disallowing fallback is the most secure, but I am not sure if we're ready to pull that trigger now. That can definitely change if e.g. perl ships with IO::Socket::SSL as a core module (iiuc there's a discussion about it in PSC). |
Automatic fallback from HTTPS to HTTP would imho be equivalent to not using HTTPS at all, as it could allow for a downgrade attack.
Wouldn't HTTPS support in those cases be provided by for example Allowing HTTP (or unverified HTTPS) with an explicit |
This is a backport of miyagawa#674 to the current release branch.
This is a backport of miyagawa#674 to the current release branch.
This is a backport of miyagawa#674 to the current release branch.
This is a backport of miyagawa#674 to the current release branch.
This is a backport of miyagawa#674 to the current release branch.
@miyagawa we have worked on an HTTP fallback, which is now #678, merging directly to master. It contains an If it pleases you, we can also do that same enforcement here. Thanks! |
i'm aware I was being unclear. I was not promoting automatic downgrade from HTTPS to HTTP. It was only about choosing the default whether it should be HTTPs or HTTP. Once HTTPS is chosen, of course it should not automatically fall back to HTTP. So the fallback to HTTP should only be allowed if:
The tricky bit here is that when the HTTP client gets a certificate error, there's no way to identify that if it's connected to an untrusted source, or if the user's TLS environment is not working (e.g. system certificates are too old). |
I think the behavior implemented here is providing what you describe:
but I would be encline to ignore that discussion for now, and focus on change from #678 which is the real fix for current customers. Note that with the current released version of But you are right when there is a certificate error, we do not know what's wrong. This is why we currently rely on the user and ask him to investigate the issue, and if needed use the |
Menlo-Legacy/lib/Menlo/CLI/Compat.pm
Outdated
@@ -2673,7 +2679,8 @@ sub mirror { | |||
die <<"DIE"; | |||
TLS issue found while fetching $uri:\n | |||
$reply->{content}\n | |||
Please verify your certificates or force an HTTP-only request/mirror at your own risk. | |||
Please verify your certificates or force an HTTP-only request/mirror | |||
using --insecure option at your own risk. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You say "at your own risk" a couple of times...
Using the whole tool is and has always been at my own risk/liability. "at your own risk" sounds like legal terms and seems to suggest there might be a time where someone else accepts the legal liability for the use of this tool.
If the aim is to say non-https is more risky than https it's worth saying "with the risks of non-secure http" or "but be aware of the risks of non-https"... Given how long cpan was non-http and non-https I'd say it might even be worth including a discussion of how risky it actually is to fetch tar balls without host verification or a secret communication channel.
There's a difference between the risks of usint the cpan cdn without https+verification and the risks of a 192 mirror without https+verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review. Could you please check if the new message properly addresses your concerns? Thank you!
Hi @miyagawa
I agree with the situations you're describing: hypothetical determinations like "this CA store is too old, ignore it" or similar will be fragile, and are likely to introduce vulnerabilities. A fatal error is an important feature when we cannot verify that the connection is to a trusted host. So, do we actually need to optimize for edge cases where users update their I'm hoping an explicit Thanks again for looking at this |
This is a backport of miyagawa#674 to the current release branch.
This is CVE-2024-45321 |
After applying this patch to actions-setup-perl, I received a report that |
The question is, where can one get a trustworthy CA chain from if your client doesn't have the ability to verify current day host keys? It seems like some choices for addressing old CA chains with a newer version of
Maybe it's worth mentioning in the "could not verify host" message that maybe you need a new version of |
OSes should provide a ca-certificates package and a secure way to update it. LWP and the next version of HTTP::Tiny use IO::Socket::SSL (and Net::SSLeay) to figure out the details of what CA store to use, defaulting to the OS one. I think its preferable to use the system CA store when possible as it's updated by the OS vendor, and to support custom PKI setups that would otherwise be ignored if Mozilla::CA is the default. Not familiar with Strawberry though.. |
I think that just means changing my suggested error message to include "or update your OS's ca-certs package", but at least it means there's a second, likely non-deadlocked, way out of the situation... which is nice. |
cpanm will fall back to (non-TLS) HTTP only requests when all default options are used AND TLS support is unavailable, but please note that, for security reasons, if TLS support is available but broken (e.g. invalid/expired certificates), requests will fail unless you explicitly use --insecure. Also note that if you use --insecure but provide custom HTTPS URLs, they will still fail if TLS support is not available. Similarly, if you provide custom HTTP-only URLs, they will go over plain HTTP and not require TLS support.
fa1a4dd
to
6121fa7
Compare
Hi Miyagawa! Thank you so much for all the work you put to cpanm, it's an amazing tool that I use pretty much daily.
Right now, cpanm downloads from HTTP, not HTTPS, even on systems with a fully working TLS.
This patch enforces HTTPS on everything, though still allowing one to force HTTP via
--from
or--mirror
.We have built and tested the fatpacked cpanm with these changes and they all work really well. Please let me know if you have any concerns or if there is anything I can do to get this merged and released.
Cheers!