fix: scp/sftp: correctly bypass JIT MFA if asked to, when old helpers are used

Author: speed47

bin/shell/osh.pl

@@ -1671,17 +1671,20 @@ sub get_details_from_access_array {
     return R('OK', value => {sshKeysArgs => \@sshKeysArgs, mfaRequired => $mfaRequired});
 }
 
-sub do_jit_mfa {
+# can exit if prerequisites are not met (i.e. MFA required but not configured on account)
+# return a true OVH::Result if MFA is not needed/can be skipped
+# a false OVH::Result otherwise
+sub may_skip_mfa {
     my %params     = @_;
     my $mfaType    = $params{'mfaType'};       # password|totp|any|none
     my $actionType = $params{'actionType'};    # host|plugin
 
     if (!$mfaType || !$actionType) {
-        return R('ERR_MISSING_PARAMETER', msg => "Missing mandatory parameters to do_jit_mfa");
+        return R('ERR_MISSING_PARAMETER', msg => "Missing mandatory parameters to may_skip_mfa");
     }
 
     if (!grep { $mfaType eq $_ } qw{ totp password any none }) {
-        return R('ERR_INVALID_PARAMETER', msg => "Invalid parameter 'mfaType' for do_jit_mfa");
+        return R('ERR_INVALID_PARAMETER', msg => "Invalid parameter 'mfaType' for may_skip_mfa");
     }
 
     return R('OK_NO_MFA_REQUIRED') if $mfaType eq 'none';
@@ -1738,31 +1741,48 @@ sub do_jit_mfa {
 
     if ($skipMFA) {
         osh_print("... skipping as your account is exempt from MFA.");
+        return R('OK_ACCOUNT_HAS_MFA_BYPASS');
     }
     elsif ($realmMFA) {
         osh_print("... you already validated MFA on the bastion you're coming from.");
+        return R('OK_ACCOUNT_HAS_VALIDATED_MFA_REALM');
     }
     elsif ($ENV{'OSH_PROACTIVE_MFA'}) {
         osh_print("... you already validated MFA proactively.");
+        return R('OK_ACCOUNT_HAS_VALIDATED_MFA_PROACTIVELY');
     }
-    else {
-        $localfnret = OVH::Bastion::do_pamtester(self => $self, sysself => $sysself);
-        main_exit(OVH::Bastion::EXIT_MFA_FAILED, 'mfa_failed', $localfnret->msg) if !$localfnret;
-
-        # craft this so that the remote server, which can be a bastion in case we're chaining,
-        # can enforce its own policy. This should be serialized in LC_BASTION_DETAILS on egress side
-        my %mfaInfo = (
-            validated => \1,
-            reason    => "mfa_required_for_$actionType",
-            type      => {
-                password => $isMfaPasswordConfigured ? \1 : \0,
-                totp     => $isMfaTOTPConfigured     ? \1 : \0,
-            }
-        );
 
-        return R('OK_VALIDATED', value => {mfaInfo => \%mfaInfo});
+    # no skip, mfa is required
+    return R('KO_MFA_REQUIRED');
+}
+
+sub do_jit_mfa {
+    my %params     = @_;
+    my $mfaType    = $params{'mfaType'};       # password|totp|any|none
+    my $actionType = $params{'actionType'};    # host|plugin
+
+    my $localfnret = may_skip_mfa(mfaType => $mfaType, actionType => $actionType);
+    if ($localfnret->is_ok) {
+        # skip, localfnret includes the detailed reason
+        return $localfnret;
     }
-    return R('OK');
+
+    # otherwise, do mfa
+    $localfnret = OVH::Bastion::do_pamtester(self => $self, sysself => $sysself);
+    main_exit(OVH::Bastion::EXIT_MFA_FAILED, 'mfa_failed', $localfnret->msg) if !$localfnret;
+
+    # craft this so that the remote server, which can be a bastion in case we're chaining,
+    # can enforce its own policy. This should be serialized in LC_BASTION_DETAILS on egress side
+    my %mfaInfo = (
+        validated => \1,
+        reason    => "mfa_required_for_$actionType",
+        type      => {
+            password => $isMfaPasswordConfigured ? \1 : \0,
+            totp     => $isMfaTOTPConfigured     ? \1 : \0,
+        }
+    );
+
+    return R('OK_VALIDATED', value => {mfaInfo => \%mfaInfo});
 }
 
 sub do_plugin_jit_mfa {
@@ -1830,7 +1850,7 @@ sub do_plugin_jit_mfa {
     }
 
     # not required? we're done
-    if (!$mfaType) {
+    if (!$mfaType || may_skip_mfa(mfaType => $mfaType, actionType => 'plugin')) {
         if ($generateMfaToken) {
             # return a dummy token so that our caller is happy, then exit
             print("MFA_TOKEN=notrequired\n");
@@ -1888,8 +1908,6 @@ sub do_plugin_jit_mfa {
         return R('OK_JIT_MFA_VALIDATED');
     }
     elsif ($generateMfaToken) {
-        osh_print("MFA token generation requested, entering MFA phase...");
-
         # do MFA
         $localfnret = do_jit_mfa(
             actionType => 'plugin',

tests/functional/tests.d/340-selfaccesses.sh

@@ -196,208 +196,6 @@ testsuite_selfaccesses()
     nocontain "Permission denied"
     contain "$randomstr"
 
-    # scp & sftp
-
-    # these are the old pre-3.14.15 helper versions, we want to check for descendant compatibility
-    cat >/tmp/scphelper <<'EOF'
-#! /bin/sh
-while ! [ "$1" = "--" ] ; do
-    if [ "$1" = "-l" ] ; then
-        remoteuser="--user $2"
-        shift 2
-    elif [ "$1" = "-p" ] ; then
-        remoteport="--port $2"
-        shift 2
-    elif [ "$1" = "-s" ]; then
-        # caller is a newer scp that tries to use the sftp subsystem
-        # instead of plain old scp, warn because it won't work
-        echo "scpwrapper: WARNING: your scp version is recent, you need to add '-O' to your scp command-line, exiting." >&2
-        exit 1
-    else
-        sshcmdline="$sshcmdline $1"
-        shift
-    fi
-done
-host="$2"
-scpcmd=`echo "$3" | sed -e 's/#/##/g;s/ /#/g'`
-EOF
-    echo "exec ssh -p $remote_port $account0@$remote_ip -T \$sshcmdline -- \$remoteuser \$remoteport --host \$host --osh scp --scp-cmd \"\$scpcmd\"" >> /tmp/scphelper
-    chmod +x /tmp/scphelper
-
-    cat >/tmp/sftphelper <<'EOF'
-#! /usr/bin/env bash
-shopt -s nocasematch
-
-while ! [ "$1" = "--" ] ; do
-    # user
-    if [ "$1" = "-l" ] ; then
-        remoteuser="--user $2"
-        shift 2
-    elif [[ $1 =~ ^-oUser[=\ ]([^\ ]+)$ ]] ; then
-        remoteuser="--user ${BASH_REMATCH[1]}"
-        shift
-    elif [ "$1" = "-o" ] && [[ $2 =~ ^user=([0-9]+)$ ]] ; then
-        remoteuser="--user ${BASH_REMATCH[1]}"
-        shift 2
-
-    # port
-    elif [ "$1" = "-p" ] ; then
-        remoteport="--port $2"
-        shift 2
-    elif [[ $1 =~ ^-oPort[=\ ]([0-9]+)$ ]] ; then
-        remoteport="--port ${BASH_REMATCH[1]}"
-        shift
-    elif [ "$1" = "-o" ] && [[ $2 =~ ^port=([0-9]+)$ ]] ; then
-        remoteport="--port ${BASH_REMATCH[1]}"
-        shift 2
-
-    # other '-oFoo Bar'
-    elif [[ $1 =~ ^-o([^\ ]+)\ (.+)$ ]] ; then
-        sshcmdline="$sshcmdline -o${BASH_REMATCH[1]}=${BASH_REMATCH[2]}"
-        shift
-
-    # don't forward -s
-    elif [ "$1" = "-s" ]; then
-        shift
-
-    # other stuff passed directly to ssh
-    else
-        sshcmdline="$sshcmdline $1"
-        shift
-    fi
-done
-
-# after '--', remaining args are always host then 'sftp'
-host="$2"
-subsystem="$3"
-if [ "$subsystem" != sftp ]; then
-    echo "Unknown subsystem requested '$subsystem', expected 'sftp'" >&2
-    exit 1
-fi
-
-# if host is in the form remoteuser@remotehost, split it
-if [[ $host =~ @ ]]; then
-    remoteuser="--user ${host%@*}"
-    host=${host#*@}
-fi
-EOF
-    echo "exec ssh -p $remote_port $account0@$remote_ip -T \$sshcmdline -- \$remoteuser \$remoteport --host \$host --osh sftp" >> /tmp/sftphelper
-    chmod +x /tmp/sftphelper
-
-    ## get both helpers first
-    for proto in scp sftp; do
-        success $proto $a0 --osh $proto
-        if [ "$COUNTONLY" != 1 ]; then
-            get_json | $jq '.value.script' | base64 -d | gunzip -c > /tmp/${proto}wrapper
-            perl -i -pe 'print "BASTION_SCP_DEBUG=1\nBASTION_SFTP_DEBUG=1\n" if ++$line==2' "/tmp/${proto}wrapper"
-            chmod +x /tmp/${proto}wrapper
-        fi
-    done
-    unset proto
-
-    # scp
-
-    ## detect recent scp
-    local scp_options=""
-    if [ "$COUNTONLY" != 1 ]; then
-        if scp -O -S /bin/true a: b 2>/dev/null; then
-            echo "scp: will use new version params"
-            scp_options="-O"
-        else
-            echo "scp: will use old version params"
-        fi
-    fi
-
-    success forscp $a0 --osh selfAddPersonalAccess --host 127.0.0.2 --scpup --port 22
-
-    sleepafter 2
-    run scp_downloadfailnoright_old scp $scp_options -F $mytmpdir/ssh_config -S /tmp/scphelper -i $account0key1file $shellaccount@127.0.0.2:uptest /tmp/downloaded
-    retvalshouldbe 1
-    contain "Sorry, but even"
-
-    run scp_downloadfailnoright_new env BASTION_SCP_DEBUG=1 /tmp/scpwrapper -i $account0key1file $shellaccount@127.0.0.2:uptest /tmp/downloaded
-    retvalshouldbe 1
-    contain "Sorry, but even"
-
-    success forscp $a0 --osh selfAddPersonalAccess --host 127.0.0.2 --scpdown --port 22
-
-    sleepafter 2
-    run scp_downloadfailnofile_old scp $scp_options -F $mytmpdir/ssh_config -S /tmp/scphelper -i $account0key1file $shellaccount@127.0.0.2:uptest /tmp/downloaded
-    retvalshouldbe 1
-    contain "through the bastion from"
-    contain "Error launching transfer"
-    contain "No such file or directory"
-    nocontain "Permission denied"
-
-    run scp_downloadfailnofile_new /tmp/scpwrapper -i $account0key1file $shellaccount@127.0.0.2:uptest /tmp/downloaded
-    retvalshouldbe 1
-    contain "through the bastion from"
-    contain "Error launching transfer"
-    contain "No such file or directory"
-    nocontain "Permission denied"
-
-    run scp_invalidhostname_old scp $scp_options -F $mytmpdir/ssh_config -S /tmp/scphelper -i $account0key1file $shellaccount@_invalid._invalid:uptest /tmp/downloaded
-    retvalshouldbe 1
-    contain REGEX "Sorry, couldn't resolve the host you specified|I was unable to resolve host"
-
-    run scp_invalidhostname_new /tmp/scpwrapper -i $account0key1file $shellaccount@_invalid._invalid:uptest /tmp/downloaded
-    retvalshouldbe 1
-    contain REGEX "Sorry, couldn't resolve the host you specified|I was unable to resolve host"
-
-    success scp_upload_old scp $scp_options -F $mytmpdir/ssh_config -S /tmp/scphelper -i $account0key1file /etc/passwd $shellaccount@127.0.0.2:uptest
-    contain "through the bastion to"
-    contain "Done,"
-
-    success scp_upload_new /tmp/scpwrapper -i $account0key1file /etc/passwd $shellaccount@127.0.0.2:uptest
-    contain "through the bastion to"
-    contain "Done,"
-
-    success scp_download_old scp $scp_options -F $mytmpdir/ssh_config -S /tmp/scphelper -i $account0key1file $shellaccount@127.0.0.2:uptest /tmp/downloaded
-    contain "through the bastion from"
-    contain "Done,"
-
-    success scp_download_new /tmp/scpwrapper -i $account0key1file $shellaccount@127.0.0.2:uptest /tmp/downloaded
-    contain "through the bastion from"
-    contain "Done,"
-
-    success forscpremove1 $a0 --osh selfDelPersonalAccess --host 127.0.0.2 --scpup   --port 22
-    success forscpremove2 $a0 --osh selfDelPersonalAccess --host 127.0.0.2 --scpdown --port 22
-
-    # sftp
-
-    run sftp_no_access_old sftp -F $mytmpdir/ssh_config -S /tmp/sftphelper -i $account0key1file $shellaccount@127.0.0.2
-    retvalshouldbe 255
-    contain "Sorry, but even"
-
-    run sftp_no_access_new /tmp/sftpwrapper -i $account0key1file $shellaccount@127.0.0.2
-    retvalshouldbe 255
-    contain "Sorry, but even"
-
-    success forsftp $a0 --osh selfAddPersonalAccess --host 127.0.0.2 --sftp --port 22
-
-    if [ "$COUNTONLY" != 1 ]; then
-        cat >"/tmp/sftpcommands" <<'EOF'
-ls
-exit
-EOF
-    fi
-
-    success sftp_access_old sftp -F $mytmpdir/ssh_config -b /tmp/sftpcommands -S /tmp/sftphelper -i $account0key1file $shellaccount@127.0.0.2
-    contain 'sftp> ls'
-    contain 'uptest'
-    contain 'sftp> exit'
-    contain '>>> Done,'
-
-    success sftp_access_new /tmp/sftpwrapper -b /tmp/sftpcommands -i $account0key1file $shellaccount@127.0.0.2
-    contain 'sftp> ls'
-    contain 'uptest'
-    contain 'sftp> exit'
-    contain '>>> Done,'
-
-    success forsftpremove $a0 --osh selfDelPersonalAccess --host 127.0.0.2 --sftp --port 22
-
-    # /scp & sftp
-
     # (forced commands)
 
     # ESCAPE HELL