Nas backup: Allow credentials in backup repository mount options for cifs mount by abh1sar · Pull Request #14009 · apache/cloudstack

abh1sar · 2026-08-29T16:13:42Z

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

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
Build/CI
Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

abh1sar · 2026-08-29T16:15:05Z

@blueorangutan package

Copilot

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 SafeCommandOptions validation with a mount-options–aware validator (SafeMountCommandOptions) that allows common credential punctuation in values while keeping option keys restrictive.
Updates KVM nasbackup.sh to construct the mount command as an argument array and pass -o options as a single quoted argument.
Extends ParamProcessWorkerTest coverage 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.

codecov · 2026-08-29T16:23:13Z

Codecov Report

❌ Patch coverage is 58.33333% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.36%. Comparing base (9c8bdf3) to head (8767a8d).
⚠️ Report is 6 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...ava/org/apache/cloudstack/api/ApiArgValidator.java	63.63%	3 Missing and 1 partial ⚠️
...ava/com/cloud/api/dispatch/ParamProcessWorker.java	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.20   #14009   +/-   ##
=========================================
  Coverage     16.35%   16.36%           
- Complexity    13587    13592    +5     
=========================================
  Files          5669     5669           
  Lines        501474   501478    +4     
  Branches      60913    60914    +1     
=========================================
+ Hits          82030    82066   +36     
+ Misses       410251   410219   -32     
  Partials       9193     9193

Flag	Coverage Δ
uitests	`4.14% <ø> (ø)`
unittests	`17.22% <58.33%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DaanHoogland · 2026-09-01T08:15:30Z

@blueorangutan package

blueorangutan · 2026-09-01T08:16:07Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-09-01T09:17:09Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19037

weizhouapache

@abh1sar
code lgtm

will it cause potential security issue ? I have not checked

abh1sar · 2026-09-08T13:03:35Z

@abh1sar code lgtm

will it cause potential security issue ? I have not checked

@weizhouapache No, it shouldn't. everything the shell treats specially or expands is still rejected.

@shwstppr can you please also take a look

shwstppr · 2026-09-09T10:42:12Z

@blueorangutan package

blueorangutan · 2026-09-09T10:44:09Z

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

shwstppr

Please check if 4.20 also needs changes.
Changes look good

shwstppr · 2026-09-09T11:17:48Z

+     * Validates mount command option strings to avoid unsafe/code-like content.
     */
-    SafeCommandOptions((param, annotation) -> {
+    SafeMountCommandOptions((param, annotation) -> {


@abh1sar should these SafeMountCommandOptions refactors also be made in 4.20 branch?

Right, rebased to 4.20

blueorangutan · 2026-09-09T11:53:05Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19174

abh1sar · 2026-09-09T13:37:09Z

@blueorangutan package

blueorangutan · 2026-09-09T13:38:04Z

@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-09-09T13:51:43Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19179

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.

Validation returned early for a blank value, so a list of nothing but whitespace was accepted although whitespace is rejected everywhere else in the list. The two sides then disagreed about it: the restore wrapper treats it as no options at all, while the backup script sees a non empty string and hands it to mount as an option of its own. An empty value still clears the options, anything else is now validated. The array holding the mount command is also declared local, as arrays assigned in a bash function are otherwise global. mount_point and dest stay as they are, the callers of mount_operation read them.

abh1sar · 2026-09-09T13:58:01Z

@blueorangutan package

blueorangutan · 2026-09-09T14:00:05Z

@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-09-09T15:03:33Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19180

weizhouapache · 2026-09-09T15:11:00Z

@blueorangutan test

blueorangutan · 2026-09-09T15:12:05Z

@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2026-09-09T16:38:12Z

[SF] Trillian Build Failed (tid-16958)

abh1sar · 2026-09-10T05:50:44Z

@blueorangutan test

blueorangutan · 2026-09-10T05:52:03Z

@abh1sar a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2026-09-10T21:38:23Z

[SF] Trillian test result (tid-16967)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 52942 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14009-t16967-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

boring-cyborg Bot added component:api component:kvm labels Aug 29, 2026

abh1sar force-pushed the nas-cifs-mount branch from 0de29a0 to 17b20fd Compare August 29, 2026 16:14

abh1sar requested review from shwstppr and weizhouapache and a lite review from Copilot August 29, 2026 16:15

Copilot started reviewing on behalf of abh1sar August 29, 2026 16:16 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

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

Comment thread scripts/vm/hypervisor/kvm/nasbackup.sh Outdated

DaanHoogland added this to CloudStack Testing Aug 31, 2026

github-project-automation Bot moved this to Backlog in CloudStack Testing Aug 31, 2026

DaanHoogland moved this from Backlog to Ready in CloudStack Testing Sep 1, 2026

abh1sar changed the title ~~Allow credentials in backup repository mount options for cifs mount~~ Nas backup: Allow credentials in backup repository mount options for cifs mount Sep 3, 2026

weizhouapache reviewed Sep 7, 2026

View reviewed changes

stag7824 mentioned this pull request Sep 9, 2026

kvm: trim whitespace around backup repository mount options on restore #14096

Open

2 tasks

shwstppr reviewed Sep 9, 2026

View reviewed changes

abh1sar force-pushed the nas-cifs-mount branch from 5af611f to 051d5ff Compare September 9, 2026 13:32

boring-cyborg Bot added component:networking component:UI labels Sep 9, 2026

abh1sar changed the base branch from 4.22 to 4.20 September 9, 2026 13:33

abh1sar added 2 commits September 9, 2026 19:21

abh1sar force-pushed the nas-cifs-mount branch from 051d5ff to 8767a8d Compare September 9, 2026 13:57

Conversation

abh1sar commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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?

Uh oh!

abh1sar commented Aug 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Sep 1, 2026

Uh oh!

blueorangutan commented Sep 1, 2026

Uh oh!

blueorangutan commented Sep 1, 2026

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

abh1sar commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shwstppr commented Sep 9, 2026

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

abh1sar Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

abh1sar commented Sep 9, 2026

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

abh1sar commented Sep 9, 2026

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

weizhouapache commented Sep 9, 2026

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

abh1sar commented Sep 10, 2026

Uh oh!

blueorangutan commented Sep 10, 2026

Uh oh!

blueorangutan commented Sep 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

abh1sar commented Aug 29, 2026 •

edited

Loading

codecov Bot commented Aug 29, 2026 •

edited

Loading

abh1sar commented Sep 8, 2026 •

edited

Loading