Allow credentials in backup repository mount options for cifs mount - #14009
Open
abh1sar wants to merge 1 commit into
Open
Allow credentials in backup repository mount options for cifs mount#14009abh1sar wants to merge 1 commit into
abh1sar wants to merge 1 commit into
Conversation
The SafeCommandOptions whitelist added for command injection hardening only
accepted [A-Za-z0-9,._=:/+-] and whitespace. Mount options are how credentials
reach a CIFS backup repository, and any realistic value is rejected: an Active
Directory username such as user@domain, or a password containing @ ! # % ^ ~.
Adding or updating such a repository fails with "contains unsupported or unsafe
characters".
The list is now parsed for what it is, a comma separated list of "key" or
"key=value" entries, with the punctuation that appears in credentials allowed
in values only. It is not a loosening across the board. Keys keep the old
restrictive character set, and everything the shell treats specially or expands
is still rejected in both: whitespace, quotes, $ ` ; | & < > ( ) { } [ ] \ and
the glob characters * and ?. Whitespace was previously accepted and is not any
more, since that is what would let a value turn into extra mount arguments.
nasbackup.sh interpolated the options into the mount command unquoted, so a
value containing whitespace or a glob was split or expanded by the shell before
mount saw it. The command is now built as an array and the options passed as a
single quoted argument, so the option list cannot influence anything but the -o
argument regardless of what validation allows through.
abh1sar
force-pushed
the
nas-cifs-mount
branch
from
August 29, 2026 16:14
0de29a0 to
17b20fd
Compare
Contributor
Author
|
@blueorangutan package |
abh1sar
requested review from
shwstppr and
weizhouapache
and
a lite review from Copilot
August 29, 2026 16:15
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates CloudStack’s handling of backup repository mount options so CIFS credentials (and similar secrets like cephx keys) can be provided safely, without reintroducing command injection risk.
Changes:
- Replaces the overly restrictive
SafeCommandOptionsvalidation with a mount-options–aware validator (SafeMountCommandOptions) that allows common credential punctuation in values while keeping option keys restrictive. - Updates KVM
nasbackup.shto construct themountcommand as an argument array and pass-ooptions as a single quoted argument. - Extends
ParamProcessWorkerTestcoverage for accepted/rejected mount option patterns (credentials punctuation, base64-like values, and unsafe tokens).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
api/src/main/java/org/apache/cloudstack/api/ApiArgValidator.java |
Introduces SafeMountCommandOptions with key/value-aware parsing and tighter validation rules. |
api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/AddBackupRepositoryCmd.java |
Switches mountOptions validation to SafeMountCommandOptions. |
api/src/main/java/org/apache/cloudstack/api/command/user/backup/repository/UpdateBackupRepositoryCmd.java |
Switches mountOptions validation to SafeMountCommandOptions. |
server/src/main/java/com/cloud/api/dispatch/ParamProcessWorker.java |
Routes the new validator in parameter validation dispatch. |
server/src/test/java/com/cloud/api/dispatch/ParamProcessWorkerTest.java |
Adds tests for credential-friendly mount options and rejection of unsafe patterns. |
scripts/vm/hypervisor/kvm/nasbackup.sh |
Builds mount invocation via an array and passes options as a single argument to avoid shell splitting/expansion. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
92
to
95
| final String value = String.valueOf(param); | ||
| if (StringUtils.isBlank(value)) { | ||
| return; | ||
| } |
Comment on lines
+292
to
+294
| 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" |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14009 +/- ##
=========================================
Coverage 17.79% 17.79%
- Complexity 15995 15997 +2
=========================================
Files 5928 5928
Lines 534306 534310 +4
Branches 65383 65384 +1
=========================================
+ Hits 95069 95087 +18
+ Misses 428467 428452 -15
- Partials 10770 10771 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The SafeCommandOptions whitelist added for command injection hardening in 56ad044 only accepts [A-Za-z0-9,._=:/+-] and whitespace.
Mount options are how credentials reach a CIFS backup repository, and any realistic value is rejected: an Active Directory username such as user@domain, or a password containing @ ! # % ^ ~. Adding or updating such a repository fails with "contains unsupported or unsafe characters".
The list is now parsed for what it is, a comma separated list of "key" or "key=value" entries, with the punctuation that appears in credentials allowed in values only. It is not a loosening across the board. Keys keep the old restrictive character set, and everything the shell treats specially or expands is still rejected in both: whitespace, quotes, $ ` ; | & < > ( ) { } [ ] \ and the glob characters * and ?. Whitespace was previously accepted and is not any more, since that is what would let a value turn into extra mount arguments.
nasbackup.sh interpolated the options into the mount command unquoted, so a value containing whitespace or a glob was split or expanded by the shell before mount saw it. The command is now built as an array and the options passed as a single quoted argument, so the option list cannot influence anything but the -o argument regardless of what validation allows through.
Not that this doesn't affect existing repositories, only the new ones.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?