Require explicit AWS credentials in the S3 sink

[Jonathan-Roboflow]

What does this PR do?

The S3 sink built its boto3 client with whatever credentials boto3 could resolve when the workflow omitted aws_access_key_id/aws_secret_access_key. Since /workflows/run accepts caller-supplied specifications, any workflow author could then write chosen content to a chosen bucket and key as the inference server's AWS identity.

create_s3_client now raises before reaching boto3.client() unless both credentials are present, so the ambient chain (env vars, ~/.aws/credentials, instance or task role) is never consulted. aws_region stays optional.

Related Issue(s):

Type of Change

• Breaking change

This is a breaking change for deployments that rely on an instance role for this block. Those workflows must now pass credentials explicitly, ideally via an Environment Secrets Store block.

Testing

• I have tested this change locally
• I have added/updated tests for this change

Test details:
test_create_s3_client_rejects_missing_credentials (test_s3_sink.py:265)
Passes both credentials as None and expects a ValueError. It also patches boto3.client and asserts it was never called, which is the difference between failing after boto3 resolved an identity and failing before it looked.

test_create_s3_client_passes_explicit_credentials (test_s3_sink.py:277)
Sends key, secret, and region through and asserts boto3.client got exactly those kwargs. Without it, the first test would pass against a function that raises on everything.

test_run_does_not_fall_back_to_server_aws_credentials (test_s3_sink.py:716)
Puts AWS credentials in the environment so the process looks like a server with an instance role, then calls the real S3SinkBlockV1.run() with a caller-chosen bucket and no workflow credentials. It asserts the run raises and that botocore.client.BaseClient._make_api_call was never reached, so no AWS request left the process. This is the test that covers the vulnerability itself; the other two only cover the helper.

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code where necessary, particularly in hard-to-understand areas
• My changes generate no new warnings or errors
• I have updated the documentation accordingly (if applicable)

[github-actions]

👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely.

🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits.

Warning

💸 The Claude reviewer bills in credits, not vibes

Automated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared.

Draft when unsure, Ready when you mean it:

• 🌱 Not sure the PR is in good shape yet? Keep it (or set it back) as a draft — drafts pause review, so you can push and iterate without burning credits on a moving target.
• 💪 Feel strong about the contents? Mark it Ready for review and the reviewer will take a look.

However you get there, arrive prepared:

• 🧱 Bring a SOLID, thorough PR. Point your local agent at our skills/ to tune it to our guidelines first — or, if you are one of those fabled carbon-based contributors, read them yourself. A half-baked diff costs exactly the same to review as a finished one.
• ✅ Resolve every comment before you re-request review. Re-requesting with threads still open means paying twice for the same conversation.
• 🔁 Do not use CI review as an inner loop for a local agent. The reviewer is not a step-by-step debugger — do the unfolding locally and arrive with the answer, not the search.
• 🙋 If something looks off, ask a human. One question to a maintainer is cheaper and faster than three rounds of agent re-review chasing a misread.

Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.

• Prefer to skip automated review entirely? Add the skip-claude-review label.

[github-actions]

🤖 Claude review started at commit 6fe0ffdb6e758ff1b8f17743e54567445e5707e5.

New commits are not auto-reviewed. Add the claude-review label to request a re-review — the label is consumed when the review starts, so just add it again next time.

[github-actions]

@Jonathan-Roboflow

⏳ This PR is on hold pending your answers — the review will not advance to sign-off until the IMPORTANT question below is answered.

The code change itself is correct and complete: create_s3_client now fails closed before boto3.client() is reached, the ambient chain is never consulted, and the three tests genuinely discriminate the fixed behavior (the run-level test at test_s3_sink.py:716 fails when v1.py is reverted). Patching v1 in place is the right call here — a new v2 would leave the vulnerable v1 reachable — so I am not asking for a new block version.

IMPORTANT question

The fix is a blanket removal of ambient-credential support for every deployment, but the threat model you describe is specific to the multi-tenant hosted /workflows/run surface. On a single-tenant self-hosted inference server, an EC2/ECS instance role or task role is AWS's recommended credential mechanism, not a vulnerability — and after this change those deployments break with no escape hatch (inference/core/workflows/core_steps/sinks/s3/v1.py:387).

Was an env-gated opt-in considered instead of a hard break — e.g. an ALLOW_S3_SINK_AMBIENT_CREDENTIALS flag defaulting to False (secure-by-default, closes the hosted vuln) that a trusted single-tenant operator can set to True to keep using an instance role? There is direct precedent for exactly this kind of security/deployment toggle for workflow blocks in inference/core/env.py (ALLOW_WORKFLOW_BLOCKS_ACCESSING_ENVIRONMENTAL_VARIABLES, ALLOW_LOADING_IMAGES_FROM_LOCAL_FILESYSTEM).

Why this changes the outcome: if maintainers intend to preserve the IAM-role pattern for trusted self-hosted deployments, this PR should ship the env-gated opt-in rather than the blanket break; if the decision is deliberately fail-closed everywhere (accepting the self-hosted regression as documented in the PR body), it can proceed as-is. Either answer is fine — but it needs to be an explicit, confirmed product decision, not an implicit side effect. Unanswered, this may keep the PR out of a release.

---

Re-review is not automatic: new commits are not auto-reviewed — add the claude-review label to request a re-review (the label is consumed when the review starts, so just add it again next time).

Reviewed at HEAD: 6fe0ffd

[github-actions]

Review summary

Skills: review-workflows-blocks, review-topic-backward-compat-and-versioning, review-topic-auth-and-tenant-security, review-topic-test-hygiene

No blocking code-level findings. The credential-isolation fix is implemented correctly, the only caller (run()) is covered, and the tests are meaningful — the run-level test fails against the reverted v1.py, so it discriminates on the vulnerability rather than just the helper.

This is not a merge-ready sign-off: one IMPORTANT product/security question is open (see the action-item comment above) about whether the blanket removal of ambient credentials — which regresses the AWS-recommended IAM instance-role pattern on trusted single-tenant self-hosted servers — is deliberate versus an env-gated opt-in. The review will not advance to sign-off until that is answered and confirmed.

No changelog/version companion is required: the change is confined to a single Workflow block and touches neither inference_models nor Execution Engine compile/run behavior.

_{Minor doubts (non-blocking): a missing-credentials ValueError now propagates out of run() as a StepExecutionError (failing the whole workflow) rather than returning {"error_status": True, ...} like the S3 auth/permission errors do — fail-hard is defensible for a config error, but the inconsistency is worth a conscious choice.}

Reviewed at HEAD: 6fe0ffd