Skip to content

CancelCertificateAsync has no cross-org ownership check (unlike RevokeCertificateAsync) #11

Description

@spbsoluble

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    before-gaNeeds triage before GA releasebugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions