feat: httpproxy: add and use execute_simple() for more performance

Also handle errors better in hand_http_request()

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"

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;

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";
     }

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'}};
     }

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;