Skip to content

security: add authorization checks to WorkflowHttpHandler#16156

Open
adilburaksen wants to merge 1 commit into
cdapio:developfrom
adilburaksen:fix/workflow-handler-authorization
Open

security: add authorization checks to WorkflowHttpHandler#16156
adilburaksen wants to merge 1 commit into
cdapio:developfrom
adilburaksen:fix/workflow-handler-authorization

Conversation

@adilburaksen

Copy link
Copy Markdown

The workflow run endpoints under /v3/namespaces/{namespace-id}/apps/{app-id}/workflows/{workflow-id}/runs/{run-id} accessed workflow run data and workflow run state using only the namespace, application and workflow names from the request path, without enforcing any access control on the selected workflow program.

This change injects ContextAccessEnforcer into the handler and enforces access on the workflow ProgramReference before each operation:

  • Read endpoints (workflow token, node-level token, node states, local datasets) enforce StandardPermission.GET.
  • State-changing endpoints (suspend, resume, delete local datasets) enforce StandardPermission.UPDATE.

This mirrors the program-level enforcement already performed by the sibling program runtime handler and the other authorization PRs in this area. Hardening only; no functional change for authorized callers.

The workflow run endpoints under
/v3/namespaces/{namespace-id}/apps/{app-id}/workflows/{workflow-id}/runs/{run-id}
read workflow run data (workflow token, node-level token, node states,
local datasets) and changed workflow run state (suspend, resume, delete
local datasets) using only the namespace, application and workflow names
supplied in the request path, without enforcing any access control on
the selected workflow program. A caller could therefore read run data
from, or change the run state of, workflow programs it is not authorized
to access.

Enforce access on the workflow ProgramReference via the injected
ContextAccessEnforcer before each read or state change: StandardPermission.GET
for the read endpoints and StandardPermission.UPDATE for the state-changing
endpoints, mirroring the program-level enforcement already performed by the
sibling program runtime handler.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request integrates authorization checks into WorkflowHttpHandler by injecting ContextAccessEnforcer and enforcing permissions (UPDATE or GET) across various workflow endpoints. However, a critical compilation issue was identified: the contextAccessEnforcer.enforce method throws a checked exception (AccessException), but enforceWorkflowAccess and several calling endpoints do not declare or handle this exception, which will prevent the code from compiling.

Comment on lines +124 to 129
private void enforceWorkflowAccess(String namespaceId, String appId, String workflowId,
StandardPermission permission) {
ProgramReference programReference = Ids.namespace(namespaceId).appReference(appId)
.program(ProgramType.WORKFLOW, workflowId);
contextAccessEnforcer.enforce(programReference, permission);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The contextAccessEnforcer.enforce method throws AccessException, which is a checked exception. However, enforceWorkflowAccess does not declare throws AccessException (or throws Exception) or catch it, which will cause a compilation error.

To resolve this, enforceWorkflowAccess should be updated to declare throws Exception (or throws AccessException). Additionally, all calling endpoint methods that do not already declare throws Exception (such as getWorkflowToken, getWorkflowNodeStates, getWorkflowLocalDatasets, and deleteWorkflowLocalDatasets) must also be updated to declare throws Exception in their method signatures to propagate the exception correctly.

Suggested change
private void enforceWorkflowAccess(String namespaceId, String appId, String workflowId,
StandardPermission permission) {
ProgramReference programReference = Ids.namespace(namespaceId).appReference(appId)
.program(ProgramType.WORKFLOW, workflowId);
contextAccessEnforcer.enforce(programReference, permission);
}
private void enforceWorkflowAccess(String namespaceId, String appId, String workflowId,
StandardPermission permission) throws Exception {
ProgramReference programReference = Ids.namespace(namespaceId).appReference(appId)
.program(ProgramType.WORKFLOW, workflowId);
contextAccessEnforcer.enforce(programReference, permission);
}

@adilburaksen

Copy link
Copy Markdown
Author

@sahusanket @vsethi09 — would one of you be able to take a look at this small security cluster when you have a moment?

These four PRs add per-resource authorization to external /v3 handlers that currently invoke no ContextAccessEnforcer, so any authenticated user can read or act on another namespace's resources. Each change mirrors the enforcement already present in the sibling handlers (e.g. ConnectionHandler, ProgramRuntimeHttpHandler, LineageHTTPHandler):

All four are small (an enforceOnParent(...) before the store operation) and the gemini-code-assist review feedback has been addressed. Happy to rebase or adjust to match your preferred enforcement pattern.

@adilburaksen

Copy link
Copy Markdown
Author

@sahusanket @bhardwaj-priyanshu — gentle follow-up on this small cross-namespace authz cluster (original ping Jun 2). This adds the missing ContextAccessEnforcer check to WorkflowHttpHandler: today a user with access only to their own namespace can read another namespace's workflow-run token. It's runtime-confirmed — in the PoC the real DefaultContextAccessEnforcer denies the test user on the victim namespace, yet the handler returns its secret (db.password) and never calls the enforcer (Mockito verifyNoInteractions). Sibling ProgramRuntimeHttpHandler already enforces, so this is a gap, not by-design. Companion fixes mirror the same pattern: #16155 (OperationsDashboard), #16157 (OperationHandler), #16158 (UsageHandler). All green/CLA-signed. Could one of you take a look when you have a moment? Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant