Skip to content

feat(aicore): clear AICORE_CLIENT_SECRET after token acquisition (AFSDK-4291) - #257

Closed
tiagoek wants to merge 2 commits into
feat/aicore-transparent-tlsfrom
feat/aicore-clear-client-secret
Closed

feat(aicore): clear AICORE_CLIENT_SECRET after token acquisition (AFSDK-4291)#257
tiagoek wants to merge 2 commits into
feat/aicore-transparent-tlsfrom
feat/aicore-clear-client-secret

Conversation

@tiagoek

@tiagoek tiagoek commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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

Stacked on #256 — merge that PR first, then retarget this one to main.

Tracking: AFSDK-4413 / HASI2026203 (CVE 9.9)

After the first successful litellm.completion() / litellm.acompletion() call, AICORE_CLIENT_SECRET is removed from os.environ. At that point LiteLLM has already captured the secret inside its token creator closure and no longer reads it from the environment. Removing it minimises the window of exposure to child processes and container introspection APIs.

Behaviour details


⚠️ BREAKING CHANGE

AICORE_CLIENT_SECRET is removed from os.environ after the first successful completion call.

Code that reads os.environ["AICORE_CLIENT_SECRET"] after calling completion() or acompletion() will receive a KeyError (or empty string via .get()).

Affected pattern:

set_aicore_config()
completion(...)                              # secret cleared here
secret = os.environ["AICORE_CLIENT_SECRET"]  # ❌ KeyError after this PR

Known affected agents — migration required before this PR merges:

Agent File Pattern
PMDRA / skills agent PMDRA/skills_agent.py reads AICORE_CLIENT_SECRET from env post-completion()
Billing anomaly agent billing-anomaly/_credentials.py reads AICORE_CLIENT_SECRET from env post-completion()
Finance agent file path TBD (scan in progress) manual AI Core credential rewrite, bypasses SDK

Issues have been opened in the respective repos with the migration path below. This PR stays draft until all three confirm migration or provide a timeline.

Migration path:

Remove any code that reads AICORE_CLIENT_SECRET directly from os.environ after set_aicore_config() / completion(). The secret is an implementation detail of the LiteLLM integration — application code should not depend on it.

# Before (breaks after this PR)
set_aicore_config()
response = completion(model="sap/gpt-4o", messages=[...])
secret = os.environ["AICORE_CLIENT_SECRET"]  # ❌

# After (correct pattern)
set_aicore_config()
response = completion(model="sap/gpt-4o", messages=[...])
# Do not read AICORE_CLIENT_SECRET — let the SDK manage it

If the secret is needed for a purpose outside LiteLLM (e.g. a separate HTTP call), read it directly from the mounted secret volume at /etc/secrets/appfnd/aicore/<instance>/clientsecret before calling set_aicore_config().


Related Issues

  • AFSDK-4413 — clear client_secret BLI
  • HASI2026203 (CVE 9.9 — internal tracking)

Type of Change

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

How to Test

python -m pytest tests/aicore/unit/ -v
# Expected: 73 passed

Manual verification:

  1. Call set_aicore_config() followed by a successful completion() call
  2. Assert os.environ.get("AICORE_CLIENT_SECRET") is None
  3. Make a second completion() call — verify it succeeds (LiteLLM uses its cached token)
  4. Simulate credential rotation: secret file updated, next call triggers AuthenticationError
  5. Verify completion() recovers: reloads credentials, clears secret again after retry

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 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

Additional Notes

This is the second in a series of stacked PRs addressing credential exposure in the aicore module:

  1. feat(aicore): transparent TLS mode and reactive credential reload #256 — reactive reload on AuthenticationError + proactive watcher (watch_aicore_config)
  2. This PR — clear CLIENT_SECRET from env after token acquisition
  3. feat(aicore): transparent proxy routing and BTP Destination Service mode (Option 3) #271 — Option 3: proxy routing + BTP Destination Service mode

When #256 is merged to main, this PR should be retargeted from feat/aicore-transparent-tls to main before merging.

… acquisition

After the first successful litellm.completion() call, AICORE_CLIENT_SECRET is
removed from os.environ. LiteLLM has captured the secret inside its token
creator closure at that point and no longer reads from the environment. Removing
it minimises the exposure window to child processes and container introspection
APIs (AFSDK-4291 / HASI2026203 SEC-309).

The flag is reset when credentials are reloaded (credential rotation flow) so
the secret is cleared again after the retry succeeds. No-op in transparent TLS
mode where the secret was never written.

Relates-to: AFSDK-4291
@tiagoek
tiagoek force-pushed the feat/aicore-clear-client-secret branch from 3e9ef43 to ed315de Compare August 26, 2026 17:37
PR #257 clears AICORE_CLIENT_SECRET from env after every successful
completion() call. The TestReactive401UpdatesEnv test asserts the env
value after the retry — patch _clear_client_secret as no-op so the
assertion can still verify set_aicore_config() wrote the rotated secret.
@tiagoek

tiagoek commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

SDK Module Review

Check Status Findings
bdd ✅ PASS 0
binding-shape ✅ PASS 0
commits ✅ PASS 0
concurrency ✅ PASS 0
constants ✅ PASS 0
deletion-hygiene ✅ PASS 0
deps-supply ✅ PASS 0
disclosure ✅ PASS 0
docs ✅ PASS 0
errors-logging ✅ PASS 0
hardcode ✅ PASS 0
http-hygiene ✅ PASS 0
license-spdx ✅ PASS 0
patterns ✅ PASS 0
pr-size ✅ PASS 0
quality-gate-parity ✅ PASS 0
secrets ✅ PASS 0
telemetry ✅ PASS 0
testing-depth ✅ PASS 0
versioning ✅ PASS 0

0 finding(s): 0 posted as inline comment(s) on the affected lines, 0 not tied to a code line (listed above).


Generated by sdk-review-skill · v1

@tiagoek

tiagoek commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Closing — approach not viable with current LiteLLM SAP provider

This PR assumes that after the first successful completion() call, LiteLLM has captured AICORE_CLIENT_SECRET in its token creator closure and no longer reads it from os.environ. That assumption is incorrect.

What actually happens

LiteLLM's SAP provider calls run_env_setupget_token_creatorvalidate_credentials on every completion() / acompletion() call, reading AICORE_CLIENT_SECRET from os.environ each time:

litellm/llms/sap/chat/transformation.py: run_env_setup
litellm/llms/sap/credentials.py: get_token_creator → validate_credentials
ValueError: SAP AI Core credentials are incomplete. Invalid credentials: provide exactly one of
            client_secret, (cert_str & key_str), or (cert_file_path & key_file_path).

Removing the env var after the first call causes every subsequent call to fail. Integration tests confirmed this — all three test_filtering_bdd.py tests and several telemetry tests fail with the error above.

Why there is no clean fix

Any "clear and restore" approach around each call introduces thread-safety issues and would be more fragile than the problem it tries to solve. Patching LiteLLM internals to read from a module-level store instead of os.environ would create an unmaintainable dependency on LiteLLM's private API.

What stays in place

PR #256 (feat/aicore-transparent-tls) addresses AFSDK-4306 without touching AICORE_CLIENT_SECRET visibility:

  • Reactive reload: AuthenticationError (401) is caught, set_aicore_config() reloads credentials from the mounted volume, and the call is retried transparently.
  • Proactive watcher: watch_aicore_config() polls the secret directory mtime and reloads before the OAuth token expires — avoiding 401s entirely.

The AICORE_CLIENT_SECRET exposure concern (AFSDK-4413 / CVE 9.9) requires a different resolution path — either upstream in the LiteLLM SAP provider (cache the token creator after first instantiation, stop re-reading env on every call) or at the platform level (restrict secret volume permissions so child processes cannot read /proc/self/environ).

@tiagoek tiagoek closed this Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant