Deferred from #6 (review-findings-of-review-findings pass), flagged for triage before GA.
Where
MarkMonitorClient.CancelCertificateAsync (markmonitor-caplugin/Client/MarkMonitorClient.cs:963)
takes only orderId - no orgName parameter, no ownership check - and calls
PATCH /certs/v1/order/{orderId}/cancel directly.
Compare RevokeCertificateAsync (markmonitor-caplugin/Client/MarkMonitorClient.cs:1046), which
added the cross-org GUID-comparison check as part of #6:
public async Task<bool> RevokeCertificateAsync(string orderId, string orgName = null, uint reason = 0)
{
...
if (string.IsNullOrWhiteSpace(orgName)) { ... }
else
{
var expectedOrgId = await ResolveOrganizationIdAsync(orgName);
var order = await FetchOrderAsync(orderId);
var actualOrgIdParsed = Guid.TryParse(order.OrganizationId, out var actualOrgId) ? actualOrgId : (Guid?)null;
...
}
}
Problem
#6's summary judged this "low current risk, only ever called with orders this connector just
created" - true for today's callers (enrollment cleanup / TestConsole/IntegrationTests order
cleanup), where the order ID always comes from an enrollment this same connector instance just
performed. But that's a caller-discipline invariant, not something the method itself enforces - the
same class of gap that made the RevokeCertificateAsync fix in #6 necessary in the first place
(that one was reachable via RenewOrReissue's ICertificateDataReader-sourced order ID, not just
connector-created orders). Any future caller that passes an externally-sourced order ID to
CancelCertificateAsync - directly, or through a new enrollment/renewal path - would silently be
able to cancel an order belonging to a different organization.
Suggested fix
Give CancelCertificateAsync the same optional orgName parameter and ownership check as
RevokeCertificateAsync, for symmetry and to close off the gap before a future caller relies on an
invariant the method doesn't actually enforce.
Deferred from #6 (review-findings-of-review-findings pass), flagged for triage before GA.
Where
MarkMonitorClient.CancelCertificateAsync(markmonitor-caplugin/Client/MarkMonitorClient.cs:963)takes only
orderId- noorgNameparameter, no ownership check - and callsPATCH /certs/v1/order/{orderId}/canceldirectly.Compare
RevokeCertificateAsync(markmonitor-caplugin/Client/MarkMonitorClient.cs:1046), whichadded the cross-org GUID-comparison check as part of #6:
Problem
#6's summary judged this "low current risk, only ever called with orders this connector justcreated" - true for today's callers (enrollment cleanup /
TestConsole/IntegrationTestsordercleanup), where the order ID always comes from an enrollment this same connector instance just
performed. But that's a caller-discipline invariant, not something the method itself enforces - the
same class of gap that made the
RevokeCertificateAsyncfix in #6 necessary in the first place(that one was reachable via
RenewOrReissue'sICertificateDataReader-sourced order ID, not justconnector-created orders). Any future caller that passes an externally-sourced order ID to
CancelCertificateAsync- directly, or through a new enrollment/renewal path - would silently beable to cancel an order belonging to a different organization.
Suggested fix
Give
CancelCertificateAsyncthe same optionalorgNameparameter and ownership check asRevokeCertificateAsync, for symmetry and to close off the gap before a future caller relies on aninvariant the method doesn't actually enforce.