SSLCertService: Fix account id requirement by using caller account id as fallback - #13818
SSLCertService: Fix account id requirement by using caller account id as fallback#13818resmo wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Aligns CertService SSL certificate listing behavior with other CloudStack APIs by falling back to the caller’s account when no explicit accountId (and no other filter like project/LB/cert) is provided, removing an unnecessary hard requirement that caused client friction (e.g., automation modules).
Changes:
- Update
listSslCertsto use the caller account ID as the default whenaccountIdis not provided. - Add a unit test ensuring the no-filter case queries certificates for the caller’s account.
- Minor cleanup: parameterized logging and correct string comparison for key algorithm checks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java | Implements caller-account fallback for listing certs; minor logging/string-compare adjustments; updates PEM reader close handling. |
| server/src/test/java/org/apache/cloudstack/network/ssl/CertServiceTest.java | Adds a regression test validating caller-account fallback behavior for listSslCerts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:375
- The comment and logic here are misleading: this block is not about "encryption for DSA"; it conditionally performs an RSA signature round-trip to validate that the keypair matches, and it skips validation for any non-RSA algorithm (not just DSA). Consider updating the comment and using a null-safe string comparison for clarity.
// No encryption for DSA
if (!pubKey.getAlgorithm().equals("RSA")) {
return;
}
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #13818 +/- ##
=============================================
- Coverage 19.64% 3.41% -16.24%
=============================================
Files 6368 487 -5881
Lines 574889 41867 -533022
Branches 70353 7912 -62441
=============================================
- Hits 112962 1429 -111533
+ Misses 449656 40238 -409418
+ Partials 12271 200 -12071
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:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:206
- The owner-selection condition can silently ignore a provided
accountNamewhendomainIdis missing because it usesStringUtils.isNotEmpty(...)as a gate. This bypasses_accountMgr.finalizeOwner(...)validation (which would throw whenaccountName != null && domainId == null), and the current&&/||expression is also hard to read due to operator precedence. Consider keying onaccountName != null(not non-empty) and grouping theprojectId/accountNamecases explicitly so invalid parameter combinations are rejected instead of being ignored.
Account owner = null;
if (StringUtils.isNotEmpty(listSslCertCmd.getAccountName()) && listSslCertCmd.getDomainId() != null || listSslCertCmd.getProjectId() != null) {
owner = _accountMgr.finalizeOwner(caller, listSslCertCmd.getAccountName(), listSslCertCmd.getDomainId(), listSslCertCmd.getProjectId());
} else {
owner = caller;
api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/ListSslCertsCmd.java:28
import org.apache.cloudstack.api.response.*;introduces a wildcard import, which is inconsistent with the surrounding API command classes in this package that use explicit response imports (e.g. CreateLoadBalancerRuleCmd.java:30-34, DeleteSslCertCmd.java:27-28). Using explicit imports avoids accidental unused dependencies and keeps diffs more readable.
import org.apache.cloudstack.api.response.*;
streamline ssl cert list api, deprecate accountid
1d05a20 to
8a3871f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:203
- The owner resolution condition mixes && and || without parentheses and only calls finalizeOwner when (accountName && domainId) or projectId is set. This means invalid combinations like specifying accountName without domainId are silently ignored (finalizeOwner would normally throw), and it also allows ambiguous requests when accountId is supplied together with account/domainId or projectId. Consider computing a single
hasOwnerParamsflag, callingfinalizeOwnerwhenever any owner-related parameter is provided (so validation/permission checks run), and rejecting combinations that include both deprecatedaccountIdand the new owner parameters.
Account owner = null;
if (StringUtils.isNotEmpty(listSslCertCmd.getAccountName()) && listSslCertCmd.getDomainId() != null || listSslCertCmd.getProjectId() != null) {
owner = _accountMgr.finalizeOwner(caller, listSslCertCmd.getAccountName(), listSslCertCmd.getDomainId(), listSslCertCmd.getProjectId());
add a note about mutually exclusive with account
e.g. domainId with account
not a common verify in cloudstack but in terms of unexpected results, it should be verified.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (5)
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:236
- When
accountIdis provided,_accountMgr.getAccount(accountId)may return null;owner.getId()will then throw a NullPointerException. Also, listing by another accountId performs the DB query before any access check, which can leak whether that account has certificates (empty list returns normally, non-empty list can throw oncheckAccess). Validate the account exists and check caller access to the account before querying.
Account owner = null;
if (StringUtils.isNotEmpty(accountName)) {
owner = _accountMgr.finalizeOwner(caller, accountName, domainId, projectId);
} else if (accountId != null) {
owner = _accountMgr.getAccount(accountId);
} else {
owner = caller;
}
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:227
- This change now rejects requests that specify more than one of
certificateid,lbid,projectid, oraccount/accountid. Previously, extra parameters were tolerated (with a defined precedence via theif (certId) ... else if (lbRuleId) ...chain). This is potentially a breaking API behavior change for clients that passed multiple filters.
// Validate that only one of certid, lbid, projectid, or accountid/account can be specified
ArrayList<Object> params = new ArrayList<>();
params.add(certId);
params.add(accountId != null ? accountId : accountName);
params.add(lbRuleId);
params.add(projectId);
int nonNullIds = 0;
for (Object param : params) {
if (param != null) {
nonNullIds++;
}
}
if (nonNullIds > 1) {
throw new InvalidParameterValueException("Only one of certid, lbid, projectid, or accountid/account can be specified");
}
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:232
StringUtils.isNotEmpty(accountName)treats whitespace-only values as provided, which will callfinalizeOwnerwith an invalid account name (e.g.," "). Since the intent is to treat "no account filter" as fallback to caller, useisNotBlankhere (consistent with validating blank/empty account names as absent).
if (StringUtils.isNotEmpty(accountName)) {
owner = _accountMgr.finalizeOwner(caller, accountName, domainId, projectId);
} else if (accountId != null) {
api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/ListSslCertsCmd.java:91
- Extra whitespace in the method signature (
public String ...) is inconsistent with the surrounding code style and may fail strict formatting/checkstyle rules.
public String getAccountName() {
return accountName;
}
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:212
- The account filter validation treats an empty
accountstring as "provided" (accountName != null/param != null), which can incorrectly trigger the mutual-exclusion error and the "only one of" check. Also,domainidcan currently be passed withoutaccountand will be silently ignored (falls back to caller). Treat blank account names as absent and rejectdomainidwithoutaccount.
This issue also appears in the following locations of the same file:
- line 212
- line 229
- line 230
if (accountId != null && (accountName != null || domainId != null)) {
throw new InvalidParameterValueException("The accountid and account/domainid are mutually exclusive");
}
// Validate that only one of certid, lbid, projectid, or accountid/account can be specified
96e51f2 to
db72cc6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:402
pubKey.getAlgorithm().equals("RSA")can throw ifgetAlgorithm()ever returns null; using a constant-first equals avoids a potential NPE and is consistent with the surrounding defensive style.
if (!pubKey.getAlgorithm().equals("RSA")) {
api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/ListSslCertsCmd.java:91
- Minor formatting: double space in method signature (
public String) is inconsistent with the surrounding accessors.
public String getAccountName() {
return accountName;
}
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:216
domainidis accepted even whenaccountis not provided, but it is then ignored by the validation/counting logic (it isn't included innonNullIdsandfinalizeOwneris skipped). This can lead to surprising behavior (e.g.,domainid+certidpasses validation anddomainidhas no effect). Consider rejectingdomainidwithoutaccount, and count account selection only whenaccountis non-blank so empty strings don't block other filters.
This issue also appears on line 402 of the same file.
// Validate that only one of certid, lbid, projectid, or accountid/account can be specified
ArrayList<Object> params = new ArrayList<>();
params.add(certId);
params.add(accountId != null ? accountId : accountName);
params.add(lbRuleId);
api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/ListSslCertsCmd.java:55
- The updated
accountIdparameter description mentions only mutual exclusivity withaccount, but the implementation also treatsdomainidas part of that mutually-exclusive pair. Updating the description helps API users understand the constraint.
This issue also appears on line 89 of the same file.
@Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, required = false, description = "Account ID and " + ApiConstants.ACCOUNT + " are mutually exclusive.")
private Long accountId;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:234
domainIdis accepted by the API but is ignored whenaccountis not provided (becausefinalizeOwneris only called whenaccountNameis non-empty). This can silently drop user input and produce confusing results. Validate thatdomainIdis only allowed together with a non-blankaccount, and treat blankaccountas absent.
Account owner = null;
if (StringUtils.isNotEmpty(accountName)) {
owner = _accountMgr.finalizeOwner(caller, accountName, domainId, projectId);
} else {
owner = caller;
}
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:216
- The mutually-exclusive-parameter counting treats
accountNameas “present” even when it is blank (because it’s added directly toparams). This can cause confusing validation failures for inputs likeaccount=" "and can also make it harder to reason about the “only one of …” check. Consider countingaccountNameonly when it is non-blank.
ArrayList<Object> params = new ArrayList<>();
params.add(certId);
params.add(accountId != null ? accountId : accountName);
params.add(lbRuleId);
api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/ListSslCertsCmd.java:54
- The
accountidparameter description says it is mutually exclusive withaccount, but the implementation also rejects combining it withdomainid. The API docs should reflect thataccountidcannot be used together withaccount/domainid.
@Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, required = false, description = "Account ID and " + ApiConstants.ACCOUNT + " are mutually exclusive.")
private Long accountId;
api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/ListSslCertsCmd.java:61
- The new parameters are marked
since = "4.24", but the current project version is4.23.0.0-SNAPSHOT(pom.xml). Using a future version here makes the generated API docs misleading; setsinceto the release version that will actually contain this change.
@Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, since = "4.24", description = "Account owning the SSL certificate")
private String accountName;
@Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, since = "4.24", entityType = DomainResponse.class, description = "Domain ID of the account owning the SSL certificate")
private Long domainId;
Unlike in `listSslCerts`, where we have to make a distinction because of the additional `accountId`, we use `finalizeOwner` early on so that the checks it contains are called. `caller` is returned as a last resort by `finalzieOwner`.
resmo
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (3)
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:298
- When
accountidis provided for a non-admin caller, the method will return an empty list without any access check if that account currently has no certs. This allows probing other account IDs to distinguish "no certs" vs "permission denied" (when certs exist). Enforce that non-admin callers can only use their ownaccountidbefore querying.
final List<SslCertVO> certVOList = _sslCertDao.listByAccountId(accountId != null ? accountId : owner.getId());
if (certVOList == null || certVOList.isEmpty()) {
return certResponseList;
}
_accountMgr.checkAccess(caller, SecurityChecker.AccessType.UseEntry, true, certVOList.get(0));
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:201
domainidcan currently be provided withoutaccount, and blankaccountvalues are counted as a filter because the validation checks only for null. This can lead to requests that silently ignoredomainid(listing caller certs) or incorrectly fail mutual-exclusion checks. Normalizeaccountto null when blank, and rejectdomainidwithoutaccountbefore doing the mutually-exclusive / single-filter validation.
final String accountName = listSslCertCmd.getAccountName();
final Long domainId = listSslCertCmd.getDomainId();
if (accountId != null && (StringUtils.isNotBlank(accountName) || domainId != null)) {
throw new InvalidParameterValueException("The accountid and account/domainid are mutually exclusive");
api/src/main/java/org/apache/cloudstack/api/command/user/loadbalancer/ListSslCertsCmd.java:61
- The new
account/domainidparameters are described as independent, but the implementation requires them to be used together and also treats them as mutually exclusive withaccountid. Updating the parameter descriptions makes the API contract clearer (and aligns with the server-side validation).
@Parameter(name = ApiConstants.ACCOUNT_ID, type = CommandType.UUID, entityType = AccountResponse.class, required = false, description = "Account ID and " + ApiConstants.ACCOUNT + " are mutually exclusive.")
private Long accountId;
@Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, since = "4.24", description = "Account owning the SSL certificate")
private String accountName;
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18820 |
Description
While implementing an ansible module for ssl cert (ngine-io/ansible-collection-cloudstack#178). I faced this api and experienced this issue. (As a side note: The ssl cert api is IMHO not consistent with other cloudstack apis: e.g. there are no domainId with accountName param but an accountId.)
SSL cert service requires to set the account id (if no project id or lb id), however, this is inconsistent to other cloudstack APIs where (AFAICS) the caller account name is used instead as a fallack.
UPDATE:
I added another commit on top to streamline the api by adding
accountanddomainidto the list api. Let's discuss which way to go.This change aligns with this behaviour.
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?