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 14 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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13818 +/- ##
============================================
+ Coverage 19.64% 19.80% +0.15%
- Complexity 19790 20020 +230
============================================
Files 6368 6371 +3
Lines 574889 575980 +1091
Branches 70353 70527 +174
============================================
+ Hits 112962 114055 +1093
+ Misses 449656 449479 -177
- Partials 12271 12446 +175
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
|
@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 |
|
@resmo , fyc |
|
@DaanHoogland thanks, I misread |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:211
- listSslCerts(): domainid is accepted even when no account is specified, but it is then ignored (owner falls back to caller). This can silently return unexpected results; domainid should only be allowed together with the account parameter.
if (accountId != null && (StringUtils.isNotBlank(accountName) || domainId != null)) {
throw new InvalidParameterValueException("The accountid and account/domainid are mutually exclusive");
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Suppressed comments (6)
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:1
- The parameter exclusivity counting treats an empty
accountNamestring as a provided filter (non-null), which can incorrectly trigger the 'only one of ... can be specified' validation or treat the request as filtered. Consider normalizingaccountNameto null when blank (e.g., only counting it when isNotBlank), and ideally includedomainIdconsistently as part of the same account filter group.
// Licensed to the Apache Software Foundation (ASF) under one
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:139
- This change now calls finalizeOwner when an account name is provided even if domainId is null (and no projectId). That can make owner resolution ambiguous or fail depending on finalizeOwner’s expectations. Consider enforcing that
domainIdmust be provided whenaccountNameis set (unlessprojectIdis set), and throw an InvalidParameterValueException otherwise.
if (StringUtils.isNotBlank(certCmd.getAccountName()) || certCmd.getProjectId() != null) {
owner = _accountMgr.finalizeOwner(caller, certCmd.getAccountName(), certCmd.getDomainId(), certCmd.getProjectId());
} else {
owner = caller;
}
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:407
- This can throw a NullPointerException if
pubKey.getAlgorithm()returns null. Prefer a null-safe comparison (e.g., calling equals on the constant) to avoid unexpected failures in edge cases.
if (!pubKey.getAlgorithm().equals("RSA")) {
return;
}
server/src/test/java/org/apache/cloudstack/network/ssl/CertServiceTest.java:840
- This test registers a CallContext but does not unregister it afterward, which can leak state into subsequent tests and cause order-dependent failures. Use a try/finally to unregister (or restore previous context) after the assertions.
CallContext.unregister();
CallContext.register(user, callerAccount);
certService.listSslCerts(new ListSslCertsCmdExtn());
Mockito.verify(certService._sslCertDao).listByAccountId(callerAccountId);
}
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:211
- New/changed validation paths (mutual exclusivity and the single-filter constraint) are not covered by tests in this PR, while this test suite already exists for the service. Consider adding unit tests that assert: (1) accountId + account/domainId throws, (2) specifying >1 of certId/lbId/project/account throws, and (3)
domainIdwithoutaccountis rejected (once the implementation is fixed).
if (accountId != null && (StringUtils.isNotBlank(accountName) || domainId != null)) {
throw new InvalidParameterValueException("The accountid and account/domainid are mutually exclusive");
}
server/src/main/java/org/apache/cloudstack/network/ssl/CertServiceImpl.java:228
- New/changed validation paths (mutual exclusivity and the single-filter constraint) are not covered by tests in this PR, while this test suite already exists for the service. Consider adding unit tests that assert: (1) accountId + account/domainId throws, (2) specifying >1 of certId/lbId/project/account throws, and (3)
domainIdwithoutaccountis rejected (once the implementation is fixed).
if (nonNullIds > 1) {
throw new InvalidParameterValueException("Only one of certid, lbid, projectid, or accountid/account can be specified");
}
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:213
- In listSslCerts(), account/domainid validation is incomplete: (1)
accountNameis counted as a filter even when blank, because it’s added to the exclusivity check as a raw String; and (2)domainIdcan be provided withoutaccountName(or vice versa) and will be silently ignored, which is inconsistent with the usual "AccountName and domainId must be specified together" contract (see AccountManagerImpl.java:2762-2764). This can lead to confusing/incorrect parameter validation and behavior.
if (accountId != null && (StringUtils.isNotBlank(accountName) || 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
|
@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 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18828 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16740)
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| if (accountId != null && (StringUtils.isNotBlank(accountName) || domainId != null)) { | ||
| throw new InvalidParameterValueException("The accountid and account/domainid are mutually exclusive"); | ||
| } | ||
|
|
||
| if (certId == null && accountId == null && lbRuleId == null && projectId == null) { | ||
| throw new InvalidParameterValueException("Invalid parameters either certificate ID or Account ID or Loadbalancer ID or Project ID required"); | ||
| // 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"); | ||
| } |
|
|
||
| final List<SslCertResponse> certResponseList = new ArrayList<SslCertResponse>(); | ||
| if (accountId != null && (StringUtils.isNotBlank(accountName) || domainId != null)) { | ||
| throw new InvalidParameterValueException("The accountid and account/domainid are mutually exclusive"); |
|
|
||
| certService.listSslCerts(new ListSslCertsCmdExtn()); | ||
|
|
||
| Mockito.verify(certService._sslCertDao).listByAccountId(callerAccountId); |
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?