feat: github app env-var fallback for AWS Secrets Manager compatibility - #495
Conversation
WalkthroughRefactors GitHub App secret provisioning from Vault-only to a backend-agnostic flow via SecretsBackend.build_system_ref; private key is read from the active backend (cached), webhook secret is read from backend with an environment-variable fallback, and tests/docs updated for Vault and AWS Secrets Manager. ChangesBackend-Agnostic GitHub App Secrets
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The approach needs to change. Right now the PR makes GitHub App work on AWS SM by falling back to a GITHUB_APP_PRIVATE_KEY env var when the backend can't handle the hardcoded vault: ref. That means the private key never actually lives in AWS Secrets Manager — it's just an env var, and AWS SM is bypassed. We want the key stored in and read from the active secrets backend (Vault or AWS SM), the same way every other connector is backend-agnostic. So: drop the env-var fallback and make the system-secret reference be built by the active backend.
server/utils/secrets/base.py — add to SecretsBackend: server/utils/secrets/vault_backend.py — override (this reproduces today's exact Vault ref, so existing Vault deployments are unaffected): server/utils/secrets/aws_sm_backend.py — add a system prefix constant near AWSSM_REF_PREFIX and override: def build_system_ref(self, logical_name: str) -> str:
server/connectors/github_connector/vault_keys.py:
On AWS SM deployments, the operator creates the two secrets — note the aurora/system/... prefix (NOT aurora/users/AWS_SM_PREFIX) and the region must match AWS_SM_REGION:
Verification (all three GitHub auth methods)
|
…he secrets backend
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/tests/secrets/test_github_app_system_secret.py`:
- Line 25: The test fixture _FAKE_PEM contains real PEM-like headers/footers
which trigger secret scanners; replace its value with a non-key sentinel string
(e.g. "FAKE_PEM_CONTENT" or any opaque sample text) so tests still have sample
content but no RSA/PEM markers; update the _FAKE_PEM assignment in the test
module (referenced as _FAKE_PEM) to the sanitized sentinel string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d538006-7db1-4992-935f-147bac14ac9c
📒 Files selected for processing (8)
server/connectors/github_connector/vault_keys.pyserver/tests/secrets/test_github_app_system_secret.pyserver/utils/auth/github_app_jwt.pyserver/utils/secrets/aws_sm_backend.pyserver/utils/secrets/base.pyserver/utils/secrets/vault_backend.pywebsite/docs/configuration/environment.mdwebsite/docs/integrations/connectors.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/tests/secrets/test_github_app_system_secret.py (2)
167-205: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding a caching test for webhook secrets.
The private key test suite includes
test_result_is_cached(lines 154–159) to verify that repeated calls only trigger a single backend read. The webhook secret uses the same caching mechanism (snippet 2 shows_cached_webhook_secret), but this class has no equivalent test.Adding a test that calls
get_app_webhook_secret()twice and assertsbackend.get_secretwas invoked only once would provide symmetric coverage and guard against future regressions in the caching logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/secrets/test_github_app_system_secret.py` around lines 167 - 205, Add a caching test analogous to the private key suite: in TestGetAppWebhookSecret create a test (e.g., test_result_is_cached_for_webhook_secret) that configures a mock backend via get_secrets_backend, ensures backend.available=True and backend.get_secret returns "whsec", then call vault_keys.get_app_webhook_secret() twice and assert backend.get_secret was called exactly once; this verifies the _cached_webhook_secret caching behavior used by get_app_webhook_secret().
81-89: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider testing the round-trip without accessing private methods.
Line 89 directly calls
backend._parse_ref(), which couples the test to an implementation detail. If_parse_refis later refactored or renamed, this test will break even if the public contract remains stable.While verifying that the extracted secret ID matches expectations adds confidence, consider whether
can_handle_refalone provides sufficient validation of the round-trip behavior, or whether the AWS backend should expose a public method for testing ref parsing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/tests/secrets/test_github_app_system_secret.py` around lines 81 - 89, The test test_aws_sm_ref_roundtrips_through_parse_ref is relying on the private method backend._parse_ref which couples the test to implementation details; change the test to avoid calling the private method by either asserting public behavior only (use backend.can_handle_ref(ref) plus any public accessor that returns the SecretId) or add a public parse method on AWSSecretsManagerBackend (e.g., parse_ref or extract_secret_id) and use that instead of _parse_ref; update the test to call backend.build_system_ref(_PRIVATE_KEY_LOGICAL), assert backend.can_handle_ref(ref) and then call the new public method (or otherwise observe the SecretId via public API) to verify the extracted "aurora/system/github-app/private-key".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/tests/secrets/test_github_app_system_secret.py`:
- Around line 167-205: Add a caching test analogous to the private key suite: in
TestGetAppWebhookSecret create a test (e.g.,
test_result_is_cached_for_webhook_secret) that configures a mock backend via
get_secrets_backend, ensures backend.available=True and backend.get_secret
returns "whsec", then call vault_keys.get_app_webhook_secret() twice and assert
backend.get_secret was called exactly once; this verifies the
_cached_webhook_secret caching behavior used by get_app_webhook_secret().
- Around line 81-89: The test test_aws_sm_ref_roundtrips_through_parse_ref is
relying on the private method backend._parse_ref which couples the test to
implementation details; change the test to avoid calling the private method by
either asserting public behavior only (use backend.can_handle_ref(ref) plus any
public accessor that returns the SecretId) or add a public parse method on
AWSSecretsManagerBackend (e.g., parse_ref or extract_secret_id) and use that
instead of _parse_ref; update the test to call
backend.build_system_ref(_PRIVATE_KEY_LOGICAL), assert
backend.can_handle_ref(ref) and then call the new public method (or otherwise
observe the SecretId via public API) to verify the extracted
"aurora/system/github-app/private-key".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59fe6b4b-81e7-48cc-8529-3a30ccbe08bb
📒 Files selected for processing (1)
server/tests/secrets/test_github_app_system_secret.py
3 participants
Add can_handle_ref() check so the active backend gracefully declines incompatible secret references
Add GITHUB_APP_PRIVATE_KEY env var as fallback when Vault is not the secrets backend
Align get_app_private_key() with the try-backend-then-env pattern already used by get_app_webhook_secret()
Summary by CodeRabbit
New Features
Documentation
Tests