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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: devel
Choose a base branch
from
Open

make cpanm secure by default #674

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6a3853c
s/http/https/g
garu Apr 26, 2024
3efe8e2
Raise warnings when fail to use SSL
atoomic Apr 27, 2024
6684b93
Add --insecure to switch to http
atoomic Apr 28, 2024
b686ffc
Merge branch 'miyagawa:devel' into secure-by-default
garu May 17, 2024
6121fa7
fallback to HTTP-only when TLS is unavailable
garu May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion App-cpanminus/script/cpanm.PL
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,9 +555,16 @@ Defaults to true (man pages generated) unless C<-L|--local-lib-contained>
option is supplied in which case it's set to false. You can disable
it with C<--no-man-pages>.

=item --insecure

By default, cpanm only uses HTTPS. If your environment lacks TLS
support, you can consider at your own risk to use HTTP instead by
using the C<--insecure> flag.
Be aware that when using that argument all requests will use HTTP.

=item --lwp

Uses L<LWP> module to download stuff over HTTP. Defaults to true, and
Uses L<LWP> module to download stuff over HTTPS. Defaults to true, and
you can say C<--no-lwp> to disable using LWP, when you want to upgrade
LWP from CPAN on some broken perl systems.

Expand Down
30 changes: 21 additions & 9 deletions Menlo-Legacy/lib/Menlo/CLI/Compat.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ sub new {
try_lwp => 1,
try_wget => 1,
try_curl => 1,
use_http => 0,
uninstall_shadows => ($] < 5.012),
skip_installed => 1,
skip_satisfied => 0,
Expand Down Expand Up @@ -199,6 +200,7 @@ sub parse_options {
'lwp!' => \$self->{try_lwp},
'wget!' => \$self->{try_wget},
'curl!' => \$self->{try_curl},
'insecure!' => \$self->{use_http},
'auto-cleanup=s' => \$self->{auto_cleanup},
'man-pages!' => \$self->{pod2man},
'scandeps' => \$self->{scandeps},
Expand Down Expand Up @@ -453,7 +455,7 @@ sub search_common {
$self->chat("Found $found->{module} $found->{module_version} which doesn't satisfy $want_version.\n");
}
}

return;
}

Expand Down Expand Up @@ -977,7 +979,7 @@ sub append_args {
my($self, $cmd, $phase) = @_;

return $cmd if ref $cmd ne 'ARRAY';

if (my $args = $self->{build_args}{$phase}) {
$cmd = join ' ', Menlo::Util::shell_quote(@$cmd), $args;
}
Expand Down Expand Up @@ -2657,6 +2659,10 @@ sub mirror {
return $self->file_mirror($uri, $local);
}

# HTTPTinyish does not provide an option to disable
# certificates check, let's switch to http on demand.
$uri =~ s/^https:/http:/ if $self->{use_http};

my $reply = $self->{http}->mirror($uri, $local);

if ( $uri =~ /^https:/ && ref $reply
Expand All @@ -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.
Copy link

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.

Copy link
Contributor Author

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!

DIE
}
}
Expand Down Expand Up @@ -2722,14 +2729,16 @@ sub configure_http {

require HTTP::Tinyish;

my $use_http = $self->{use_http};

my @try = qw(HTTPTiny);
unshift @try, 'Wget' if $self->{try_wget};
unshift @try, 'Curl' if $self->{try_curl};
unshift @try, 'LWP' if $self->{try_lwp};

my @protocol = ('https');
my @protocol = ( $use_http ? 'http' : 'https' );
push @protocol, 'http'
if grep /^http:/, @{$self->{mirrors}};
if !$use_http && grep /^http:/, @{$self->{mirrors}};

my $backend;
for my $try (map "HTTP::Tinyish::$_", @try) {
Expand All @@ -2745,11 +2754,14 @@ sub configure_http {
}
}

if ( !$backend ) {
$self->diag_fail( join( ', ', @protocol )." not supported by available HTTP Clients." );
}
# In case we use https protocol by default
# and then later we try to perform non https requests
# we still want these requests to succeed
# Note: this is disabling the client cache optimization above
# and will fail later for SSL requests as no clients support TLS
$backend ||= 'HTTP::Tinyish';

$backend->new(agent => "Menlo/$Menlo::VERSION", verify_SSL => 1);
$backend->new(agent => "Menlo/$Menlo::VERSION", $use_http ? () : ( verify_SSL => 1 ) );
}

sub init_tools {
Expand Down