Skip to content

Allow credentials in backup repository mount options for cifs mount #14009

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

Open
abh1sar wants to merge 1 commit into
base: 4.22
Choose a base branch
from
Open

Allow credentials in backup repository mount options for cifs mount #14009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 18 additions & 7 deletions api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
"$(", "`", "&&", "||", ";", "|", ">", "<"
Expand All @@ -79,14 +85,19 @@ 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)) {
return;
}
Comment on lines 92 to 95

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);
Expand Down
2 changes: 1 addition & 1 deletion ...java/org/apache/cloudstack/api/command/user/backup/repository/AddBackupRepositoryCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,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,
Expand Down
2 changes: 1 addition & 1 deletion ...a/org/apache/cloudstack/api/command/user/backup/repository/UpdateBackupRepositoryCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public class UpdateBackupRepositoryCmd extends BaseCmd {
private String address;

@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.CROSS_ZONE_INSTANCE_CREATION, type = CommandType.BOOLEAN, description = "backups in this repository can be used to create Instances on all Zones")
Expand Down
4 changes: 3 additions & 1 deletion scripts/vm/hypervisor/kvm/nasbackup.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,9 @@ 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}) 2>&1 | tee -a "$logFile"
mount_args=(-t "${NAS_TYPE}" "${NAS_ADDRESS}" "${mount_point}")
[[ -n "${MOUNT_OPTS}" ]] && mount_args+=(-o "${MOUNT_OPTS}")
mount "${mount_args[@]}" 2>&1 | tee -a "$logFile"
Comment on lines +292 to +294
if [ $? -eq 0 ]; then
log -ne "Successfully mounted ${NAS_TYPE} store"
else
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
104 changes: 103 additions & 1 deletion server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -151,6 +151,108 @@ 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 = "username=backup@corp.example.com,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(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>();
Expand Down
Loading