[tiagoek]

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

IASVerifier.from_env() silently built a verifier with client_id=None whenever a resolved IAS binding carried no clientid. With client_id=None, __call__ sets options["verify_aud"] = False — so tokens issued for any application were accepted. This is an audience (aud) bypass on the recommended zero-config path (the one StarletteIASTelemetryMiddleware and IASContextProvider use automatically).

Root cause: from_env() treated a missing clientid as "skip audience validation" instead of "misconfiguration".

from_env() now fails closed: it raises IASConfigError when a binding is resolved (VCAP_SERVICES, K8s secret mount, or IAS_URL) but has no clientid, instead of disabling audience validation silently.

The explicit constructor IASVerifier(ias_url=..., client_id=None) remains the single, deliberate opt-out for advanced cases (e.g. Istio/Kyma already verified the token), so the documented contract is preserved.

Related Issue

Tracked as FINDING-444 (HASI2026203 — JWT Audience Bypass via missing client_id). No GitHub issue; tracked in internal Jira.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Code refactoring
• Dependency update

How to Test

1. Check out fix/ias-verifier-require-audience.
2. Run pytest tests/ias/unit/test_verifier.py -v -k "no_client_id" — all three _raises tests should pass.
3. Run the full IAS suite: pytest tests/ias/ -v — 135 tests should pass.
4. Verify the opt-out still works: pytest tests/ias/unit/test_verifier.py::TestIASVerifierCall::test_skips_audience_when_no_client_id should pass (explicit client_id=None constructor still skips audience).

Checklist

• I have read the Contributing Guidelines
• I have verified that my changes solve the issue
• I have added/updated automated tests to cover my changes
• All tests pass locally
• I have verified that my code follows the Code Guidelines
• I have updated documentation (if applicable)
• I have added type hints for all public APIs
• My code does not contain sensitive information (credentials, tokens, etc.)
• I have followed Conventional Commits for commit messages
• (Security-sensitive changes only) I have run /security-review and addressed all ❌ findings

Behaviour change

Scenario	Before	After
Binding with clientid	Audience enforced	Audience enforced (unchanged)
Binding without clientid (auto-config)	verify_aud=False — any audience accepted	IASConfigError raised — fails closed
IAS_URL set, IAS_CLIENT_ID missing	verify_aud=False	IASConfigError raised
Explicit IASVerifier(ias_url=..., client_id=None)	Audience skipped	Audience skipped (deliberate opt-out, unchanged)

Both auto-config callers (StarletteIASTelemetryMiddleware, IASContextProvider) already wrap from_env() in try/except IASConfigError and degrade gracefully. So a misconfigured binding now results in no identity stamping (WARNING logged) rather than stamping identity from an unverified-audience token.

Context

Follow-up to #382, which landed IASVerifier + the telemetry/audit provenance fix (FINDING-447) but left this audience-bypass branch in from_env() live. Addresses FINDING-444 (JWT Audience Bypass via missing client_id, HASI2026203).

[tiagoek]

PR Review: #438 — fix(ias): fail closed when IAS binding has no clientid

Author: tiagoek Branch: fix/ias-verifier-require-audience → main
Verdict: ❌ Blocked
Summary: Code and security are sound; PR description does not follow the template (Type of Change, How to Test, and Checklist missing — including the mandatory security-sensitive item).

---

A: Process & Compliance

#	Criterion	Status	Finding
A1	PR template complete	❌ Fail	Missing: Type of Change checkbox, How to Test steps, 9-item checklist. Specifically: the *(Security-sensitive changes only)* I have run /security-review and addressed all ❌ findings item is required but absent. The body is thorough prose but skips the template entirely.
A2	Conventional Commits	✅ Pass	fix(ias): fail closed when IAS binding has no clientid — correct format; commit-validation CI passes.
A3	Issue linked	⚠️ Warning	FINDING-444 / HASI2026203 referenced in text but no Closes #N GitHub issue link. If the issue is Jira-only this is acceptable — note it.
A4	AI-generated code disclosure	➖ N/A	Single hand-crafted commit by tiagoek; no evidence of AI generation.

B: Security & Sensitive Data

#	Criterion	Status	Finding
B1	No sensitive data in code	✅ Pass	No hardcoded credentials or internal URLs. Test fixtures use obvious placeholders.
B2	No sensitive data in PR body	✅ Pass	No account-specific URLs, GUIDs, or tenant identifiers.

C: Code Quality

#	Criterion	Status	Finding
C1	CI checks passing	✅ Pass	10 jobs green. SDK review label gate is what this review resolves.
C2	Version bump	➖ N/A	No pyproject.toml changes.
C3	Type hints	✅ Pass	All modified public methods fully annotated.
C4	No hardcoded values	✅ Pass	_NO_CLIENT_ID_HINT constant defined at module level and reused across all three guard blocks.
C5	Import organization	✅ Pass	No new imports.
C6	Naming conventions	✅ Pass	_NO_CLIENT_ID_HINT follows _SCREAMING_SNAKE_CASE for private module constants.
C7	No unused code	✅ Pass	_NO_CLIENT_ID_HINT used in all three raise IASConfigError(...) sites.
C8	No unjustified new dependencies	➖ N/A	No dependency changes.
C9	Proto code freshness	➖ N/A	No proto files touched.

D: API & Design

#	Criterion	Status	Finding
D1	API future-proofing	✅ Pass	No new config surface; IASConfigError already part of the public contract.
D2	Public API hygiene	✅ Pass	IASConfigError already exported from sap_cloud_sdk.ias. No new public symbols.
D3	Breaking changes marked	✅ Pass	Behavior change is a security fix on a previously-buggy path. PR body includes a before/after table for all four scenarios.
D4	Pagination & tenant filtering	➖ N/A
D5	Telemetry instrumentation	➖ N/A	No new client methods.
D6	Multi-tenancy support	⚠️ Warning	Pre-existing gap surfaced by modifying user-guide.md: no ## Multi-tenancy section. See E2a.

E: Tests & Documentation

#	Criterion	Status	Finding
E1	Tests added/updated	✅ Pass	Three new security-tagged tests cover all three binding paths. Four existing tests correctly updated. All 135 IAS+telemetry tests pass per CI.
E2	Documentation quality	✅ Pass	user-guide.md explains fail-closed behavior, error condition, and explicit opt-out. from_env() docstring updated.
E2a	Multi-tenancy documentation	⚠️ Warning	user-guide.md modified in this PR but has no ## Multi-tenancy section. Required fields: Supported, Authentication, How to use, Further reading.
E3	Module structure compliance	➖ N/A	Not a new module.

---

❌ Blocking Issues

• [A1]: PR description does not follow the template. Missing: Type of Change checkbox (should be "Bug fix"), How to Test numbered steps, and the 9-item checklist — including the required *(Security-sensitive changes only)* I have run /security-review and addressed all ❌ findings item. Fix: update the PR body to fill in the template sections.

⚠️ Non-Blocking Suggestions

• [A3]: Add a Closes #NNN reference if a GitHub issue exists. If tracking is Jira-only, note that in the PR body.
• [E2a / D6]: Add a ## Multi-tenancy section to src/sap_cloud_sdk/ias/user-guide.md with: Supported (Yes), Authentication (IAS), How to use (via app_tid claim + IASVerifier audience enforcement), Further reading.

✅ Things Done Well

• Correct, minimal fix. Exactly the right change — three guard blocks in from_env(), zero changes to __init__/__call__. Explicit constructor opt-out preserved and documented.
• K8s binding initialization moved outside try/except. Subtle but important: partial mounts (url set, clientid read fails) now correctly trigger fail-closed instead of falling through.
• from None used consistently. Suppresses noisy exception chains while root cause stays in DEBUG logs.
• Test cascade handled correctly. test_k8s_mount_no_url_falls_through_to_env_var and test_mount_takes_precedence_over_env_var were silently broken under the new semantics and are correctly patched.
• test_skips_audience_when_no_client_id intentionally preserved. Tests the explicit constructor opt-out — the right call.