Nas backup: Allow credentials in backup repository mount options for cifs mount

api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java

@@ -49,15 +49,21 @@ public enum ApiArgValidator {
     RFCComplianceDomainName,
 
     /**
-     * Validates command option strings to avoid unsafe/code-like content.
+     * Validates mount command option strings to avoid unsafe/code-like content.
      */
-    SafeCommandOptions((param, annotation) -> {
+    SafeMountCommandOptions((param, annotation) -> {
         if (BaseCmd.CommandType.STRING.equals(annotation.type())) {
-            validateSafeCommandOptions(param, annotation.name());
+            validateSafeMountCommandOptions(param, annotation.name());
         }
     });
 
-    private static final Pattern SAFE_COMMAND_OPTIONS_PATTERN = Pattern.compile("^[A-Za-z0-9,._=:/+\\-\\s]*$");
+    /**
+     * A mount option list is a comma separated list of "key" or "key=value" entries. Keys stay
+     * restrictive. Values additionally allow the punctuation that commonly appears in credentials,
+     * for instance a CIFS username of the form user@domain or a password containing !#%^~.
+     */
+    private static final Pattern SAFE_MOUNT_COMMAND_OPTION_PATTERN =
+            Pattern.compile("[A-Za-z0-9_.\\-]+(=[A-Za-z0-9_.\\-+:/@!#%^~=]*)?");
 
     private static final String[] UNSAFE_TOKENS = {
             "$(", "`", "&&", "||", ";", "|", ">", "<"
@@ -79,14 +85,22 @@ public void validate(final Object paramObj, final Parameter annotation) {
         }
     }
 
-    private static void validateSafeCommandOptions(final Object param, final String argName) {
+    private static void validateSafeMountCommandOptions(final Object param, final String argName) {
+        if (param == null) {
+            return;
+        }
         final String value = String.valueOf(param);
-        if (StringUtils.isBlank(value)) {
+        // An empty value clears the mount options and is allowed. A value that only looks empty is
+        // not: whitespace is rejected everywhere else in the list, and the backup script would pass
+        // it on to mount as an option of its own.
+        if (value.isEmpty()) {
             return;
         }
 
-        if (!SAFE_COMMAND_OPTIONS_PATTERN.matcher(value).matches()) {
-            throwInvalidParameterValueException(argName, "contains unsupported or unsafe characters");
+        for (final String option : value.split(",", -1)) {
+            if (!SAFE_MOUNT_COMMAND_OPTION_PATTERN.matcher(option).matches()) {
+                throwInvalidParameterValueException(argName, "contains unsupported or unsafe characters");
+            }
         }
 
         final String normalized = value.toLowerCase(Locale.ROOT);

api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/AddBackupRepositoryCmd.java

@@ -58,7 +58,7 @@ public class AddBackupRepositoryCmd extends BaseCmd {
     private String provider;
 
     @Parameter(name = ApiConstants.MOUNT_OPTIONS, type = CommandType.STRING, description = "shared storage mount options",
-            validations = {ApiArgValidator.SafeCommandOptions})
+            validations = {ApiArgValidator.SafeMountCommandOptions})
     private String mountOptions;
 
     @Parameter(name = ApiConstants.ZONE_ID,

scripts/vm/hypervisor/kvm/nasbackup.sh

@@ -154,8 +154,10 @@ mount_operation() {
   if [ ${NAS_TYPE} == "cifs" ]; then
     MOUNT_OPTS="${MOUNT_OPTS},nobrl"
   fi
-  mount -t ${NAS_TYPE} ${NAS_ADDRESS} ${mount_point} $([[ ! -z "${MOUNT_OPTS}" ]] && echo -o ${MOUNT_OPTS}) | tee -a "$logFile"
-  if [ $? -eq 0 ]; then
+  local mount_args=(-t "${NAS_TYPE}" "${NAS_ADDRESS}" "${mount_point}")
+  [[ -n "${MOUNT_OPTS}" ]] && mount_args+=(-o "${MOUNT_OPTS}")
+  mount "${mount_args[@]}" 2>>"$logFile" | tee -a "$logFile"
+  if [ "${PIPESTATUS[0]}" -eq 0 ]; then
       log -ne "Successfully mounted ${NAS_TYPE} store"
   else
       echo "Failed to mount ${NAS_TYPE} store"

server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java

@@ -174,7 +174,7 @@ private void validateField(final Object paramObj, final Parameter annotation) th
                             break;
                     }
                     break;
-                case SafeCommandOptions:
+                case SafeMountCommandOptions:
                     validator.validate(paramObj, annotation);
                     break;
                 default:

server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java

@@ -95,7 +95,7 @@ public static class TestCmd extends BaseCmd {
         @Parameter(name = "vmHostNameParam", type = CommandType.STRING, validations = {ApiArgValidator.RFCComplianceDomainName})
         String vmHostNameParam;
 
-        @Parameter(name = "mountOptions", type = CommandType.STRING, validations = {ApiArgValidator.SafeCommandOptions})
+        @Parameter(name = "mountOptions", type = CommandType.STRING, validations = {ApiArgValidator.SafeMountCommandOptions})
         String mountOptions;
 
         @Override
@@ -151,6 +151,126 @@ public void processMountOptionsParameter_Valid() {
         Assert.assertEquals("vers=4.1,soft,timeo=600,retrans=2", cmd.mountOptions);
     }
 
+    @Test
+    public void processMountOptionsParameter_AcceptsCifsCredentials() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        // CIFS credentials routinely contain punctuation that is harmless in a mount option list.
+        final String options = "[email protected],password=P@ssw0rd!#%^~,vers=3.0";
+        params.put("mountOptions", options);
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+        Assert.assertEquals(options, cmd.mountOptions);
+    }
+
+    @Test
+    public void processMountOptionsParameter_AcceptsBase64LikePassword() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        final String options = "username=backup,password=YWJjZGVmZ2g=";
+        params.put("mountOptions", options);
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+        Assert.assertEquals(options, cmd.mountOptions);
+    }
+
+    @Test
+    public void processMountOptionsParameter_AcceptsCephFsOptions() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        // A CephFS repository is mounted with the cephx user and a bare option such as defaults.
+        final String options = "name=user,secret=xyz,defaults";
+        params.put("mountOptions", options);
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+        Assert.assertEquals(options, cmd.mountOptions);
+    }
+
+    @Test
+    public void processMountOptionsParameter_AcceptsCephFsBase64Secret() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        // A cephx key is base64, so it can contain + / and trailing =.
+        final String options = "name=cloudstack,secret=AQBvE2VmS0J8FxAA9F1c2Wq+8kZ3Xn5Yz7Lw==,defaults";
+        params.put("mountOptions", options);
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+        Assert.assertEquals(options, cmd.mountOptions);
+    }
+
+    @Test
+    public void processMountOptionsParameter_AcceptsCephFsSecretFile() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        final String options = "name=user,secretfile=/etc/ceph/secret.key,_netdev";
+        params.put("mountOptions", options);
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+        Assert.assertEquals(options, cmd.mountOptions);
+    }
+
+    @Test(expected = ServerApiException.class)
+    public void processMountOptionsParameter_RejectCephFsSecretWithCommandSubstitution() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        params.put("mountOptions", "name=user,secret=$(cat /etc/ceph/keyring)");
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+    }
+
+    @Test
+    public void processMountOptionsParameter_AcceptsEmptyValueToClearTheOptions() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        params.put("mountOptions", "");
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+        Assert.assertEquals("", cmd.mountOptions);
+    }
+
+    @Test(expected = ServerApiException.class)
+    public void processMountOptionsParameter_RejectWhitespaceOnly() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        // Only looks empty: the backup script would hand this to mount as an option.
+        params.put("mountOptions", "   ");
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+    }
+
+    @Test(expected = ServerApiException.class)
+    public void processMountOptionsParameter_RejectWhitespace() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        // Whitespace would turn into additional arguments to mount.
+        params.put("mountOptions", "vers=4.1,soft -o remount,rw");
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+    }
+
+    @Test(expected = ServerApiException.class)
+    public void processMountOptionsParameter_RejectCommandSubstitution() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        params.put("mountOptions", "vers=4.1,password=$(id)");
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+    }
+
+    @Test(expected = ServerApiException.class)
+    public void processMountOptionsParameter_RejectBackticks() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        params.put("mountOptions", "vers=4.1,password=`id`");
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+    }
+
+    @Test(expected = ServerApiException.class)
+    public void processMountOptionsParameter_RejectGlob() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        params.put("mountOptions", "vers=4.1,credentials=/etc/*");
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+    }
+
+    @Test(expected = ServerApiException.class)
+    public void processMountOptionsParameter_RejectOptionWithoutKey() {
+        final HashMap<String, String> params = new HashMap<String, String>();
+        params.put("mountOptions", "vers=4.1,=value");
+        final TestCmd cmd = new TestCmd();
+        paramProcessWorkerSpy.processParameters(cmd, params);
+    }
+
     @Test(expected = ServerApiException.class)
     public void processMountOptionsParameter_RejectCodeLikeContent() {
         final HashMap<String, String> params = new HashMap<String, String>();