-
Notifications
You must be signed in to change notification settings - Fork 0
Adding CA certificates logic for verification #92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis change adds SSL/TLS certificate bundle support by replacing hardcoded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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 |
|
Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/handlers/test_handler_token.py (1)
147-158: Tests cover default and customssl_ca_bundle; consider also testingFalseThese tests nicely pin the default
Truebehavior and a custom CA bundle path. For completeness with the documented options, you could add a case wheressl_ca_bundleis explicitlyFalseto confirm verification can be disabled when desired.src/handlers/handler_token.py (1)
33-38: Config‑driven SSL verification for public key fetches looks correctUsing
SSL_CA_BUNDLE_KEYwith a default ofTrueand passingself.ssl_ca_bundletorequests.get(..., verify=...)matches the intended behavior (bool or CA‑bundle path) and restores a secure default TLS verification. As a small enhancement, you might log a warning whenssl_ca_bundleis explicitlyFalseso disabled verification is clearly visible in logs.Also applies to: 57-57, 88-88
src/event_gate_lambda.py (1)
26-27: S3 client now honorsssl_ca_bundlewith a secure default; consider improving testabilityDeriving
ssl_verify = config.get(SSL_CA_BUNDLE_KEY, True)and passing it toboto3.Session().resource("s3", verify=ssl_verify)correctly supports both boolean and CA‑bundle‑path verification and defaults to TLS verification instead of disabling it. Because this initialization happens at module import time, however, it’s awkward to unit‑test or override; extracting this into a small helper (e.g.,init_s3(config) -> aws_s3) that’s called from module scope would make the behavior easier to test and to mock in different environments.Also applies to: 31-31, 67-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)conf/config.json(1 hunks)scripts/notebook.ipynb(0 hunks)scripts/prepare.deplyoment.sh(0 hunks)src/event_gate_lambda.py(2 hunks)src/handlers/handler_token.py(3 hunks)src/utils/constants.py(1 hunks)tests/handlers/test_handler_token.py(1 hunks)tests/test_event_gate_lambda.py(2 hunks)
💤 Files with no reviewable changes (2)
- scripts/prepare.deplyoment.sh
- scripts/notebook.ipynb
🧰 Additional context used
🧬 Code graph analysis (1)
tests/handlers/test_handler_token.py (1)
src/handlers/handler_token.py (1)
HandlerToken(45-173)
🪛 Ruff (0.14.7)
src/utils/constants.py
22-22: Possible hardcoded password assigned to: "TOKEN_PROVIDER_URL_KEY"
(S105)
23-23: Possible hardcoded password assigned to: "TOKEN_PUBLIC_KEY_URL_KEY"
(S105)
24-24: Possible hardcoded password assigned to: "TOKEN_PUBLIC_KEYS_URL_KEY"
(S105)
src/event_gate_lambda.py
74-74: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
conf/config.json (1)
6-7: Sample config correctly exposesssl_ca_bundlekeyKey name and placement are consistent with the new constant and README docs; example path clearly signals where a custom CA bundle would go.
src/utils/constants.py (1)
21-25: NewSSL_CA_BUNDLE_KEYconstant is clear and consistentCentralizing the
ssl_ca_bundlekey here avoids hard‑coding the string across modules and matches the config/README naming.README.md (1)
85-100:ssl_ca_bundledocumentation matches the implementationThe example config and the three documented modes (
true,false, and custom path) are aligned with howssl_ca_bundleis read from config and passed into S3 and token public‑key HTTPS calls, withtrueas the secure default.
|
|
||
| import json | ||
| from unittest.mock import patch | ||
| from unittest.mock import patch, MagicMock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New boto3 S3 “tests” don’t exercise production code and give a false sense of coverage
Both tests only compute ssl_verify = config.get("ssl_ca_bundle", True) on a local dict and call mock_session_instance.resource directly; they never invoke src.event_gate_lambda or its S3 initialization, and the patched boto3.Session is unused inside the with blocks. As a result, these tests won’t catch regressions in how SSL_CA_BUNDLE_KEY is wired into boto3.Session().resource. Consider instead factoring S3 client creation into a helper in src.event_gate_lambda and unit‑testing that helper, or patching src.event_gate_lambda.boto3.Session and reloading the module so the real initialization path is exercised.
Also applies to: 235-260
🤖 Prompt for AI Agents
In tests/test_event_gate_lambda.py around line 18 (and similarly for lines
235-260), the boto3.Session patch is not exercised because the tests compute
ssl_verify on a local dict and call the mocked session.resource directly, so
production S3 initialization in src.event_gate_lambda is never invoked; update
the tests to either (a) factor S3 client creation into a helper inside
src/event_gate_lambda and unit‑test that helper directly, or (b) patch
src.event_gate_lambda.boto3.Session (not just boto3.Session in the test file),
reload or import the module after patching so the real initialization code runs,
and assert that boto3.Session().resource is called with ssl_verify set from
config["ssl_ca_bundle"] (including defaulting behavior) to ensure the
SSL_CA_BUNDLE_KEY wiring is covered.
Overview
This pull request introduces support for configurable SSL certificate verification for both AWS S3 access and JWT public key retrieval, improving security and flexibility for deployments. Configuration documentation and usage examples have been updated, and tests were added to ensure correct behavior for default and custom CA bundle settings.
Release Notes:
Related
Closes #60
Summary by CodeRabbit
New Features
ssl_ca_bundleconfiguration option for SSL verification settings.Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.