fix: 3LO secrets by name, and provider config that actually renders - #35
Merged
Conversation
The Google/GitHub/Notion credential providers had two defects, one hiding the
other:
1. Client secrets travelled as plaintext (-c google_client_secret / env) and
were rendered verbatim into the synthesized template — the exposure class
the IdP secret fix already closed for module 4's federation path.
2. Worse, found while fixing (1): the provider config was passed as a raw dict
whose top-level key spelled OAuth with a capital A, which does not match
the CloudFormation model — the L1 mapping silently dropped the ENTIRE
block. Every 3LO provider synthesized with Oauth2ProviderConfigInput: {}.
They could never have worked. Same hazard class as the web-search
connector (project memory has the earlier instance).
Fixes, all following existing precedent in this repo:
- identity_stack takes *_client_secret_name and renders
{{resolve:secretsmanager:...}} dynamic references (auth-stack pattern);
a client_id without a secret name fails at synth with the same actionable
message shape as the auth stack.
- Typed L1 property classes replace the raw dicts, so a wrong key raises at
synth instead of vanishing. Google/GitHub use their vendor configs (the
Token Vault knows their endpoints); Notion publishes no OIDC discovery, so
it is CustomOauth2 with explicit authorization/token endpoints — the same
shape as the deployed-and-working gateway M2M provider. The scopes lists
were dead weight in the dropped dicts: scopes are requested per token via
@requires_access_token, not stored on the provider.
- app.py rejects the plaintext keys outright and reads names through cfg()
(the test_platform_config guard now allows only the rejection loop, which
deliberately probes keys that must NOT resolve).
- deploy.sh: upsert_oauth_secret generalizes upsert_idp_secret (trim,
bring-your-own name, plaintext unset, exported so context-arg-less CDK
calls like the bootstrap probe see it — that export was found live when
the first deploy failed only at the bootstrap step). upsert_3lo_secrets
covers the three vendors; names ride as context args.
Verified live on the test rig (deploy --stack agentcore-workshop-dev-identity
with GITHUB_CLIENT_ID + a secret carrying a trailing newline):
- provider created as GithubOauth2 with real vendor config (was {} before)
- Secrets Manager value is the trimmed 20 chars, newline gone
- the DEPLOYED CloudFormation template contains the dynamic reference and no
plaintext (checked via get-template)
- CfnOAuth2CredentialProvider accepts the dynamic reference (the task's open
question) — UPDATE_COMPLETE both ways
Rig restored: provider removed, test secret deleted, invoke.py green.
Checks: check (m) in check-deploy-config.sh (trim + naming + plaintext unset
across all three vendors); tests/test_3lo_providers.py guards the source
shape (no plaintext params, dynamic refs, no raw-dict key, deploy.sh passes
names only). 120 tests green, ruff clean, shellcheck clean.
|
Commit: Security Scan Results
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The Google/GitHub/Notion 3LO credential providers had two defects, one hiding the other.
The one we went looking for: client secrets travelled as plaintext (
-c google_client_secret/GOOGLE_CLIENT_SECRET) and were rendered verbatim into the synthesized template — the same exposure class already closed for the enterprise-IdP secret.The one we found underneath: the provider config was passed as a raw dict whose top-level key spelled OAuth with a capital A, which doesn't match the CloudFormation model. The L1 mapping silently dropped the entire block — every 3LO provider synthesized with
Oauth2ProviderConfigInput: {}. They could never have worked, and nothing failed: deploys were green, the providers just had no configuration.Fixes (all following existing precedent in this repo)
identity_stacktakes*_client_secret_nameand renders{{resolve:secretsmanager:...}}dynamic references, exactly like the auth stack's IdP secret. Aclient_idwithout a secret name fails at synth with an actionable message.CustomOauth2with explicit authorization/token endpoints — the same shape as the deployed-and-working gateway M2M provider.scopeslists were dead weight inside the dropped dicts: scopes are requested per token via@requires_access_token(scopes=[...]), not stored on the provider. Not carried over.app.pyrejects the plaintext keys outright and reads the names throughcfg(), soplatform.yamlparticipates.deploy.sh:upsert_oauth_secretgeneralizes the IdP upsert (whitespace trim, bring-your-own secret name, plaintext unset after storing) andupsert_3lo_secretscovers the three vendors. The secret name is exported, not just assigned — CDK invocations that carry no context args (the bootstrap probe synthesizes the app too) must see it; found live when the first deploy failed only at the bootstrap step.Verified live
Deployed to a test environment with
GITHUB_CLIENT_IDand a secret carrying a trailing newline:GithubOauth2with real vendor config{}before this changeget-template)CfnOAuth2CredentialProvideraccepts the dynamic referenceUPDATE_COMPLETE, both directionsEnvironment restored afterwards: provider removed, test secret deleted, health invoke green.
Checks
check-deploy-config.sh: trim + naming + plaintext-unset across all three vendors, asserting on the arguments passed to the stubbed CLI.tests/test_3lo_providers.py: static guards — no plaintext secret parameters, dynamic references present, the raw-dict key cannot come back,deploy.shpasses names only.test_platform_configcfg-guard tightened: its old allowlist entries for the 3LO lookups are gone (they now go throughcfg()); the only exception left is the plaintext-rejection loop, which deliberately probes keys that must not resolve.