From d6291f3ad418fa4cf9903d636c2b1399a6c2f936 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?St=C3=A9phane=20Lesimple?= Date: Wed, 2 Jun 2021 09:35:30 +0000 Subject: [PATCH] feat: httpproxy: add and use execute_simple() for more performance Also handle errors better in hand_http_request() --- bin/admin/packages-check.sh | 4 +- bin/proxy/osh-http-proxy-worker | 38 +++++++++++++---- lib/perl/OVH/Bastion.pm | 8 +++- lib/perl/OVH/Bastion/ProxyHTTP.pm | 71 +++++++++++++++++++++---------- lib/perl/OVH/Bastion/execute.inc | 71 +++++++++++++++++++++++++++++++ 5 files changed, 157 insertions(+), 35 deletions(-) diff --git a/bin/admin/packages-check.sh b/bin/admin/packages-check.sh index 63dd6e4..cbda723 100755 --- a/bin/admin/packages-check.sh +++ b/bin/admin/packages-check.sh @@ -76,7 +76,7 @@ elif echo "$DISTRO_LIKE" | grep -q -w rhel; then expect openssh-server nc bash perl-CGI perl(Test::More) passwd \ cracklib-dicts perl-Time-Piece perl-Time-HiRes diffutils \ perl-Sys-Syslog pamtester google-authenticator qrencode-libs \ - util-linux-user" + util-linux-user perl-LWP-Protocol-https" if [ "$DISTRO_VERSION_MAJOR" = 7 ]; then wanted_list="$wanted_list fortune-mod coreutils" fi @@ -116,7 +116,7 @@ elif echo "$DISTRO_LIKE" | grep -q -w suse; then perl-libwww-perl perl-Digest perl-IO-Socket-SSL \ perl-Net-Server cryptsetup mosh expect openssh \ coreutils netcat-openbsd bash perl-CGI iputils \ - perl-Time-HiRes perl-Unix-Syslog hostname" + perl-Time-HiRes perl-Unix-Syslog hostname perl-LWP-Protocol-https" wanted_list="$wanted_list google-authenticator-libpam" # perl-GnuPG [ "$opt_syslogng" = 1 ] && wanted_list="$wanted_list syslog-ng" diff --git a/bin/proxy/osh-http-proxy-worker b/bin/proxy/osh-http-proxy-worker index d84078a..29e00d8 100755 --- a/bin/proxy/osh-http-proxy-worker +++ b/bin/proxy/osh-http-proxy-worker @@ -56,7 +56,8 @@ sub log_and_exit { OVH::Bastion::log_access_insert(%merged); push @headers, ["X-Bastion-Local-Status" => $code]; - HEXIT('OK', value => {code => $code, msg => $msg, body => $body . "\n", headers => \@headers, allowed => $merged{'allowed'}}); + OVH::Bastion::json_output(R('OK', value => {code => $code, msg => $msg, body => $body . "\n", headers => \@headers, allowed => $merged{'allowed'}}), no_delimiters => 1); + exit 0; } my $pass = delete $ENV{'PROXY_ACCOUNT_PASSWORD'}; @@ -93,7 +94,8 @@ push @headers, ["X-Bastion-Request-Length" => "" . length($content)]; # if we're being called by the monitoring, just exit happily if ($monitoring) { - HEXIT('OK', value => {code => 200, msg => 'OK', body => $OVH::Bastion::VERSION, allowed => 1}); + OVH::Bastion::json_output(R('OK', value => {code => 200, msg => 'OK', body => $OVH::Bastion::VERSION, allowed => 1}), no_delimiters => 1); + exit 0; } $fnret = OVH::Bastion::is_bastion_account_valid_and_existing(account => $account); # time: 20ms @@ -275,11 +277,15 @@ my $req = HTTP::Request->new($method => "https://" . $remotemachine . ':' . $rem $req->content($content); # passthru some tolerated headers from client->bastion to bastion->device req -foreach my $key (qw{ accept content-type content-length content-encoding }) { - my @values = grep { /^\Q$key\E:/i } @client_headers; - next if not @values; - $values[0] =~ s/^\Q$key\E:\s*//i; - $req->header($key, $values[0]); +my %passthrough_headers; +foreach my $pattern (qw{ accept content-type content-length content-encoding x-test-[a-z-]+ }) { + foreach my $header (grep { /^$pattern:/i } @client_headers) { + my ($key, $value) = $header =~ /^([^:]+):\s*(.+)$/; + $passthrough_headers{lc $key} = $value; + } +} +foreach my $key (keys %passthrough_headers) { + $req->header($key, $passthrough_headers{$key}); } $req->header('Accept-Encoding' => scalar HTTP::Message::decodable()); @@ -433,5 +439,19 @@ if ($res) { } OVH::Bastion::log_access_insert(%log_params); -HEXIT('OK', value => {code => $res->code + 0, msg => $res->message, body => $res->decoded_content, headers => \@headers, allowed => 1}) if $res; -HEXIT('OK', value => {code => 504, msg => "Device Timeout", body => "Device Timeout\n", headers => \@headers, allowed => 1}); +my %ret = ( + headers => \@headers, + allowed => 1, +); +if ($res) { + $ret{'code'} = $res->code + 0; + $ret{'msg'} = $res->message; + $ret{'body'} = $res->decoded_content; +} +else { + $ret{'code'} = 504; + $ret{'msg'} = "Device Timeout"; + $ret{'body'} = "Device Timeout\n"; +} +OVH::Bastion::json_output(R('OK', value => \%ret), no_delimiters => 1); +exit 0; diff --git a/lib/perl/OVH/Bastion.pm b/lib/perl/OVH/Bastion.pm index 45574b2..eccc7ed 100644 --- a/lib/perl/OVH/Bastion.pm +++ b/lib/perl/OVH/Bastion.pm @@ -130,7 +130,7 @@ my %_autoload_files = ( qw{ is_user_in_group is_group_existing is_valid_uid get_next_available_uid is_bastion_account_valid_and_existing is_account_valid is_account_existing access_modify is_valid_group is_valid_group_and_existing add_user_to_group get_group_list get_account_list get_realm_list is_admin is_super_owner is_auditor is_group_aclkeeper is_group_gatekeeper is_group_owner is_group_guest is_group_member get_remote_accounts_from_realm is_valid_ttl build_re_from_wildcards } ], configuration => [qw{ load_configuration_file main_configuration_directory load_configuration config account_config plugin_config group_config json_load }], - execute => [qw{ sysret2human execute result_from_helper helper_decapsulate helper }], + execute => [qw{ sysret2human execute execute_simple result_from_helper helper_decapsulate helper }], interactive => [qw{ interactive }], jail => [qw{ jailify }], log => [qw{ syslog syslog_close syslogFormatted warn_syslog log_access_insert log_access_update log_access_get }], @@ -338,6 +338,7 @@ sub json_output { ## no critic (ArgUnpacking) my $R = shift; my %params = @_; my $force_default = $params{'force_default'}; + my $no_delimiters = $params{'no_delimiters'}; my $command = $params{'command'} || $ENV{'PLUGIN_NAME'}; my $JsonObject = JSON->new->utf8; @@ -350,7 +351,10 @@ sub json_output { ## no critic (ArgUnpacking) # rename forbidden strings $encoded_json =~ s/JSON_(START|OUTPUT|END)/JSON__$1/g; - if ($ENV{'PLUGIN_JSON'} eq 'GREP' and not $force_default) { + if ($no_delimiters) { + print $encoded_json; + } + elsif ($ENV{'PLUGIN_JSON'} eq 'GREP' and not $force_default) { $encoded_json =~ tr/\r\n/ /; print "\nJSON_OUTPUT=$encoded_json\n"; } diff --git a/lib/perl/OVH/Bastion/ProxyHTTP.pm b/lib/perl/OVH/Bastion/ProxyHTTP.pm index e61c5e6..f19f1f9 100644 --- a/lib/perl/OVH/Bastion/ProxyHTTP.pm +++ b/lib/perl/OVH/Bastion/ProxyHTTP.pm @@ -9,6 +9,7 @@ use OVH::Result; use OVH::Bastion; use CGI; +use JSON; use Fcntl qw(:flock); use Time::HiRes (); use MIME::Base64; @@ -184,6 +185,50 @@ sub run { return $self->SUPER::run($self, %params); } +# used twice in process_http_request(): get the worker cmd to execute, launch it +# and decapsulate the result with the proper error checks +## no critic (Subroutines::RequireArgUnpacking) +sub _exec_worker_and_get_result { + my $self = shift; + my @cmd = @_; + my $fnret; + + $fnret = OVH::Bastion::execute_simple(cmd => \@cmd); + $fnret or return $self->log_and_exit(500, "Internal Error (couldn't exec worker)", "Couldn't exec worker (" . $fnret->msg . ")", {comment => "worker_exec_failed"}); + + if ($fnret->value->{'sysret'} != 0) { + return $self->log_and_exit( + 500, + "Internal Error (worker returned a non-zero exit value)", + "Worker returned a non-zero exit value (" . $fnret->value->{'sysret'} . ")", + {comment => "worker_non_zero_exit"} + ); + } + + if (!$fnret->value->{'output'}) { + return $self->log_and_exit(500, "Internal Error (worker returned no data)", "Worker returned no data", {comment => "worker_no_data"}); + } + + my $json_decoded; + eval { $json_decoded = decode_json($fnret->value->{'output'}); }; + if ($@) { + return $self->log_and_exit( + 500, + "Internal Error (worker returned invalid JSON)", + "Worker returned " + . (length($fnret->value->{'output'})) + . " bytes of data but JSON decoding failed ($@). The first 500 bytes follow:\n" + . substr($fnret->value->{'output'}, 0, 500), + {comment => "worker_invalid_json"} + ); + } + + $fnret = OVH::Bastion::helper_decapsulate($json_decoded); + $fnret or return $self->log_and_exit(500, "Internal Error (worker returned an error)", "Worker returned an error (" . $fnret->msg . ")", {comment => "worker_error"}); + + return $fnret; +} + # overrides parent func sub process_http_request { my ($self, $client) = @_; @@ -229,17 +274,7 @@ sub process_http_request { my @cmd = ("sudo", "-n", "-u", "proxyhttp", "--", "/usr/bin/env", "perl", "-T", "/opt/bastion/bin/proxy/osh-http-proxy-worker"); push @cmd, "--monitoring", "--uniqid", $ENV{'UNIQID'}; - $fnret = OVH::Bastion::execute(cmd => \@cmd, noisy_stdout => 0, noisy_stderr => 1, is_helper => 1); - $fnret - or return $self->log_and_exit(500, "Internal Error (couldn't call helper)", "Couldn't call helper (" . $fnret->msg . ")", {comment => "exec1_failed"}); - - $fnret = OVH::Bastion::result_from_helper($fnret->value->{'stdout'}); - $fnret - or return $self->log_and_exit(500, "Internal Error (helper execution failed)", "Helper execution failed (" . $fnret->msg . ")", {comment => "exec2_failed"}); - - $fnret = OVH::Bastion::helper_decapsulate($fnret->value); - $fnret - or return $self->log_and_exit(500, "Internal Error (helper returned an error)", "Helper returned an error (" . $fnret->msg . ")", {comment => "exec3_failed"}); + $fnret = $self->_exec_worker_and_get_result(@cmd); my $workerversion = $fnret->value->{'body'}; @@ -419,20 +454,12 @@ sub process_http_request { $ENV{'PROXY_ACCOUNT_PASSWORD'} = $pass; undef $pass; $self->{'_log'}{'request_body_length'} = length($content); - $fnret = OVH::Bastion::execute(cmd => \@cmd, noisy_stdout => 0, noisy_stderr => 1, is_helper => 1); - $fnret - or return $self->log_and_exit(500, "Internal Error (couldn't call helper)", "Couldn't call helper (" . $fnret->msg . ")", {comment => "exec1_failed"}); + + $fnret = $self->_exec_worker_and_get_result(@cmd); + delete $ENV{'PROXY_ACCOUNT_PASSWORD'}; delete $ENV{'PROXY_POST_DATA'}; - $fnret = OVH::Bastion::result_from_helper($fnret->value->{'stdout'}); - $fnret - or return $self->log_and_exit(500, "Internal Error (helper execution failed)", "Helper execution failed (" . $fnret->msg . ")", {comment => "exec2_failed"}); - - $fnret = OVH::Bastion::helper_decapsulate($fnret->value); - $fnret - or return $self->log_and_exit(500, "Internal Error (helper returned an error)", "Helper returned an error (" . $fnret->msg . ")", {comment => "exec3_failed"}); - if (ref $fnret->value->{'headers'} eq 'ARRAY') { push @{$self->{'_supplementary_headers'}}, @{$fnret->value->{'headers'}}; } diff --git a/lib/perl/OVH/Bastion/execute.inc b/lib/perl/OVH/Bastion/execute.inc index d7af8bd..0d6e2e8 100644 --- a/lib/perl/OVH/Bastion/execute.inc +++ b/lib/perl/OVH/Bastion/execute.inc @@ -325,6 +325,77 @@ sub execute { ); } +# This is a simplified version of execute(), only supporting to launch a command, +# closing STDIN immediately, and merging STDERR/STDOUT into a global output that can +# then be returned to the caller. It removes a lot of complicated locking problems +# execute() has to work with at the expense of efficiency. +# Most notably, execute() reads STDOUT and STDERR one byte at a time in some cases, +# while execute_simple() uses a buffer of 16K instead, which is several orders of +# magnitude faster for commands outputting large amounts of data (several megabytes) for example. +sub execute_simple { + my %params = @_; + my $cmd = $params{'cmd'}; # command to execute, must be an array ref (with possible parameters) + my $must_succeed = $params{'must_succeed'}; # if the executed command returns a non-zero exit value, turn OK_NON_ZERO_EXIT to ERR_NON_ZERO_EXIT + + my $fnret; + + require Scalar::Util; + foreach (@$cmd) { + if (Scalar::Util::tainted($_) && /(.+)/) { + + # to be able to warn under -T; untaint it. we're going to crash right after anyway. + require Carp; + warn(Carp::longmess("would exec <" . join('^', @$cmd) . "> but param '$1' is tainted!")); + } + } + + my $child_in; + my $child_out = gensym; + osh_debug("about to run_cmd_simple ['" . join("','", @$cmd) . "']"); + my $pid; + eval { $pid = open3($child_in, $child_out, undef, @$cmd); }; + if ($@) { + chomp $@; + return R('ERR_EXEC_FAILED', msg => "Couldn't exec requested command ($@)"); + } + close($child_in); + osh_debug("waiting for child PID $pid to complete..."); + + my $output; + while (1) { + my $buffer; + my $nbread = read $child_out, $buffer, 16384; + if (not defined $nbread) { + + # oww, abort reading + warn("execute_simple(): error while reading from command ($!), aborting"); + last; + } + last if ($nbread == 0); # EOF + $output .= $buffer; + } + close($child_out); + + osh_debug("all fds are EOF, waiting for pid $pid indefinitely"); + waitpid($pid, 0); + my $child_exit_status = $?; + + $fnret = sysret2human($child_exit_status); + osh_debug("cmd returned with " . $fnret->msg); + return R( + $fnret->value->{'status'} == 0 ? 'OK' : ($must_succeed ? 'ERR_NON_ZERO_EXIT' : 'OK_NON_ZERO_EXIT'), + value => { + sysret => $child_exit_status >> 8, + sysret_raw => $child_exit_status, + output => $output, + status => $fnret->value->{'status'}, + coredump => $fnret->value->{'coredump'}, + signal => $fnret->value->{'signal'}, + }, + msg => "Command exited with " . sysret2human($child_exit_status)->msg, + ); +} + sub result_from_helper { my $input = shift;