Fix/security and correctness - #15
Open
Will-Hellinger wants to merge 9 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on tightening security and improving correctness across the service by enabling proper TLS verification for OpenBao, adding health reporting, and supporting regional Snyk instances, while updating API request shapes and associated tests/docs.
Changes:
- Add
/healthendpoint + container healthcheck, and update auth middleware exclusions accordingly. - Add
instancesupport for Snyk OAuth/token/app-registration flows and shift credential endpoints to JSON bodies (including GET → POST for credential retrieval). - Improve OpenBao security/correctness (TLS verify options, auto-unseal helpers, raise-on-failure semantics) and update tests/docs/scripts to match new operational setup.
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| snykey/tests/test_v1_endpoints.py | Updates endpoint tests for JSON bodies, POST credential retrieval, and instance-aware token refresh. |
| snykey/tests/test_snyk.py | Adds coverage for regional auth URL hostnames and instance normalization. |
| snykey/tests/test_redis.py | Aligns Redis tests with simplified get/delete behavior (no exists precheck). |
| snykey/tests/test_openbao.py | Updates expectations for OpenBao error handling (raise vs. return False). |
| snykey/services/snyk.py | Adds instance normalization and instance-aware Snyk URLs for OAuth/token/app endpoints. |
| snykey/services/redis.py | Simplifies Redis get/delete semantics; stores instance in PKCE data. |
| snykey/services/openbao.py | Enables TLS verification configuration, adds unseal helpers, and changes some failures to raise. |
| snykey/main.py | Registers new health router. |
| snykey/core/config.py | Moves dotenv loading earlier; adds OpenBao CA/unseal settings and excludes /health & callback from API-key middleware. |
| snykey/api/v1/endpoints.py | Switches credential/register endpoints to Pydantic body models; adds instance plumbing and auto-unseal gating. |
| snykey/api/health.py | Introduces a health endpoint checking Redis and OpenBao. |
| scripts/setup.py | Updates created cert directory structure and setup instructions. |
| scripts/requirements.txt | Removes script-specific requirements. |
| scripts/generate_certs.py | Removes certificate generation script. |
| README.md | Adds top-level README pointing to MkDocs documentation. |
| mkdocs.yml | Updates navigation (adds guide; removes cert-gen script page). |
| docs/scripts/setup.md | Updates setup docs to reflect TLS cert placement step. |
| docs/scripts/generate-certificates.md | Removes cert generation documentation page. |
| docs/index.md | Updates quick start and test running instructions. |
| docs/guides/before-and-after.md | Adds narrative guide explaining motivation and usage. |
| docs/getting-started/installation.md | Updates installation steps for TLS certs and OpenBao init/unseal workflow. |
| docs/getting-started/configuration.md | Documents new OPENBAO_CA_CERT and improves configuration table. |
| Dockerfile | Installs curl to support container healthcheck. |
| docker-compose.yml | Adds app healthcheck, passes OPENBAO_CA_CERT, and removes Redis/OpenBao published ports. |
| configs/redis_config.conf | Adjusts Redis logging level. |
| configs/bao_config.hcl | Adjusts OpenBao lease TTL and logging level. |
| .gitignore | Ignores private key/cert file extensions. |
| .github/workflows/site.yml | Uses uv for installing MkDocs dependencies. |
| .env_example | Adds OPENBAO_CA_CERT documentation/default path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment on lines
+84
to
+91
| try: | ||
| if not await check_vault_sealed(): | ||
| return True | ||
| except Exception: | ||
| return False | ||
|
|
||
| logger.info("Vault is sealed, attempting automatic unseal") | ||
| return await unseal_vault() |
Comment on lines
+54
to
+60
| if not OPENBAO_UNSEAL_KEY: | ||
| logger.error("No unseal key configured, cannot unseal Vault") | ||
| return False | ||
|
|
||
| url: str = f"{OPENBAO_ADDR}/v1/sys/unseal" | ||
| headers: dict[str, str] = {"X-Vault-Token": OPENBAO_TOKEN} | ||
|
|
| ) | ||
|
|
||
| response = client.put("/v1/credentials", params=store_req) | ||
| response = client.put("/v1/credentials", json=store_req) |
Comment on lines
62
to
65
| try: | ||
| result: dict = await snyk.refresh_snyk_token( | ||
| client_id, client_secret, refresh_key | ||
| client_id, client_secret, refresh_key, instance | ||
| ) |
Comment on lines
142
to
145
| try: | ||
| result: dict = await snyk.refresh_snyk_token( | ||
| client_id, client_secret, refresh_key | ||
| client_id, client_secret, refresh_key, instance | ||
| ) |
Comment on lines
276
to
281
| try: | ||
| result = await snyk.register_snyk_app( | ||
| name, scopes_list, redirect_uris_list, org_id, auth_token | ||
| name, scopes_list, redirect_uris_list, org_id, auth_token, instance=instance | ||
| ) | ||
| except Exception as e: | ||
| logger.error("Failed to register Snyk app: %s", e) |
Comment on lines
391
to
395
| client_secret=client_secret, | ||
| redirect_uri=redirect_uri, | ||
| code_verifier=code_verifier, | ||
| instance=instance, | ||
| ) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
No description provided.