chansen / p5-http-tiny

Tiny HTTP Client
https://metacpan.org/dist/HTTP-Tiny
53 stars 52 forks source link

verify_SSL being true by default (redux) #134

Closed jmdh closed 3 years ago

jmdh commented 4 years ago

[I originally wrote this as an addendum to #68 , and throughout this message I have referred to 'you' meaning the maintainer of HTTP::Tiny. I now notice that "@ghost" is not an actual account, but a placeholder for a deleted account, so I'm missing context about whether that person speaks for the current maintainers. Since that previous issue is closed, it seems appropriate to send this as a new issue]

In Debian, we considered a request from a user to change this default - you can see the discussion at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=954089 which follows broadly the same lines as in #68 , with the additional context that we're 5 years on and acquiring SSL certificates from broadly accepted CAs is easier/free.

In summary, we believe the right thing to do for our users is to verify SSL certificates by default when they supply https URLs, and that it is better to make this policy decision in HTTP::Tiny rather than in (at least) 30 separate packages.

I acknowledge that you feel that this is unreasonably placing trust in a flawed CA system, which I can certainly sympathize with; however this is question about reasonable expectations that processing https URLs will apply certificate verification. It strictly improves the default security model of the system, even if it does not address all its weaknesses. Collectively, this is the direction that software in Debian has been headed, but at the browser level and the library level.

You can see the patch we plan to apply at https://salsa.debian.org/perl-team/interpreter/perl/-/commit/1490431e40e22052f75a0b3449f1f53cbd27ba92

Thanks for your work in maintaining HTTP::Tiny, and happy to hear if you have any further thoughts about this topic.

jmdh commented 4 years ago

There's a couple of additional factors which I should have included here:

  1. Debian is better-placed than a vanilla CPAN distribution to ensure that a useful set of CA certificates is available before making this change: we will make this package depend on the ca-certificates package in Debian. This might be a consideration before adopting such a change upstream.
  2. We're in a good point in the release cycle to be able to consider a change of this nature with sufficient time to prove (or otherwise) that breakage is kept to an acceptable level before it becomes part of a stable release.
dweekly commented 4 years ago

Hi, @jmdh - this discussion is timely as I'm starting to explore ways to better Secure Perl and was looking also at re-starting this conversation about defaults. I think it's great the Debian is taking a very pro-security stance but worry a little bit that different modules invoking HTTP::Tiny will have a different result when run on a Debian system versus elsewhere. This could easily cause a Perl author on Debian to be confident that they don't need to call any SSL verification settings in their code because HTTP::Tiny dutifully verifies server identity, only to find that their code doesn't provide those protections elsewhere. So it would be ideal if the change were made holistically (i.e. here) in my opinion.

xdg commented 4 years ago

Hi, @jmdh. I'm the "ghost" account, from when I switched "dagolden" from an ID to an organization.

While I was the last one to release this module to CPAN, I'm not actively maintaining it anymore. I can make a good case for both sides of the argument to make or not make this change. I think it comes down to "backwards compatibility" vs "security affordance" and a question of that sort is inherently subjective. Given that the module is in the Perl core -- indeed, the whole reason for existence is to allow the Perl core to bootstrap CPAN over HTTP -- I think this decision has to be taken by them.

I believe you're participating in (or at least aware of) the Perl governance discussions. Once that's resolved and there is a new steering council, I think this is the sort of decision that needs to be put to them. I'm happy to help make and release a change if they decide to make one.

Personally, my thought is that it would be more Perlish to add a new module, e.g. HTTPS::Tiny, that subclasses HTTP::Tiny and has the desired behavior. Yes, people have to switch to use it, but we've seen such campaigns before around other problematic modules. And those who refuse to change are unaffected.

With respect to the Debian patch, that's obviously an expedient way to do it and I have no opinion about what Debian should do. However, the patch linked does not seem to be in the spirit of Paragraph 3 of the Artistic license, which requires "prominent" notice of changes. The source code is not commented at all about the change. The documentation changes I would describe as "subtle" rather than "prominent". I would expect something more along the lines of "B\<NOTE>: Debian has modified the original source of HTTP::Tiny to enable verify_SSL by default. The rationale for this change is..." and that prominent notice should be both in the documentation of the verify_SSL attribute as well as in the SSL Support section.

Also, I see that the patch doesn't disable support for Mozilla::CA. That module can go a year or more without updates. Debian, in proposing this patch, is asserting that the CA list on any Debian system is authoritative, trustworthy and up-to-date. Given that, perhaps the patch should be more invasive to favor the Debian-distributed CAs. That wouldn't go upstream, so Debian would have to maintain a patch regardless.

xdg commented 3 years ago

On reflection, we shouldn't make this change for backwards compatibility.

stigtsp commented 2 years ago

Just to mention: NixOS unstable now includes the patch from Debian to enable verify_SSL by default.

gregoa commented 1 year ago

Looks like this change has been made with 77f557ef84698efeb6eed04e4a9704eaf85b741d, which AFAIK is shipped since 0.083 (and perl core since 5.38-RC1).

Thanks for making this change!

stigtsp commented 1 year ago

Looks like this change has been made with 77f557e, which AFAIK is shipped since 0.083 (and perl core since 5.38-RC1).

Note that there is a bug in 77f557e where the environment variable name for reverting the previous behavior doesn't match with the documentation. There is a PR pending that fixes that:

gregoa commented 1 year ago

Thanks for the heads-up!

I guess that should go into https://github.com/Perl/perl5/ for 5.38-RC2 as well? /cc @rjbs

Cheers,\ gregor

rjbs commented 1 year ago

It has been merged.