Skip to content

Conversation

@tmikula-dev
Copy link
Collaborator

@tmikula-dev tmikula-dev commented Dec 2, 2025

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:

  • Configurable SSL certificate verification logic

Related

Closes #60

Summary by CodeRabbit

  • New Features

    • Added ssl_ca_bundle configuration option for SSL verification settings.
  • Documentation

    • Updated README with ssl_ca_bundle configuration details and expanded configuration key documentation.
  • Chores

    • Removed internal test notebook and deployment packaging scripts.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2025

Walkthrough

This change adds SSL/TLS certificate bundle support by replacing hardcoded verify=False SSL verification bypass with configurable SSL certificate verification. A new optional configuration key ssl_ca_bundle is added, along with updated error handling for AWS S3 client initialization, tests, and documentation.

Changes

Cohort / File(s) Summary
Documentation & Configuration
README.md, conf/config.json, src/utils/constants.py
Updated documentation with new ssl_ca_bundle configuration key and its usage; added example config entry; introduced public constant SSL_CA_BUNDLE_KEY = "ssl_ca_bundle"
AWS S3 Client
src/event_gate_lambda.py
Added SSL verification toggle via SSL_CA_BUNDLE_KEY config; wrapped S3 client initialization in try/except for BotoCoreError and NoCredentialsError; replaced hardcoded verify=False with configurable verify=ssl_verify from configuration
Token Handler
src/handlers/handler_token.py
Added ssl_ca_bundle configuration field; updated load_public_keys to use configurable verify=self.ssl_ca_bundle instead of hardcoded verify=False
Tests
tests/handlers/test_handler_token.py, tests/test_event_gate_lambda.py
Added test coverage for ssl_ca_bundle configuration behavior in both token handler and S3 client initialization; includes default True and custom CA bundle path scenarios
Build & Deployment
scripts/prepare.deployment.sh, scripts/notebook.ipynb
Removed deployment packaging sequence and test notebook; eliminated build and zip steps from deployment script

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Areas requiring attention:

  • Error handling logic in src/event_gate_lambda.py for boto3 client initialization—verify that BotoCoreError and NoCredentialsError are properly caught and logged
  • Verify that ssl_verify parameter correctly resolves from configuration (handling both boolean and file path values)
  • Test assertions mock boto3 correctly and cover both default and custom CA bundle scenarios
  • Configuration examples in README are clear and reflect the expected file path format

Poem

🐰 SSL certs, no more lies—
No more False to bypass,
Now we verify with pride,
Bundles configured just right,
Secure requests, secure skies!
🔐✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding CA certificate configuration for SSL verification across S3 and JWT key retrieval.
Linked Issues check ✅ Passed Changes fully implement issue #60 requirements: ssl_ca_bundle configuration added to enable configurable SSL verification instead of hardcoded verify=False in S3 client and JWT key loading.
Out of Scope Changes check ✅ Passed All changes directly support SSL certificate verification goal: configuration key added, code updated to use configurable verification, tests added, and deployment script removed appropriately for cleanup.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/60-add-ca-certificates

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Dec 2, 2025

Trivy has completed a full security repository scan ✅ You can find the analysis results for this PR branch on this overview.
Below is the summary of the findings:

TRIVY CRITICAL HIGH MEDIUM LOW TOTAL
vulnerability 0 0 0 0 0
secret 0 6 0 0 6
misconfiguration 0 0 1 10 11
license 0 0 0 0 0
➡️ Total 0 6 1 10 17

Copy link

@coderabbitai coderabbitai bot left a 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 custom ssl_ca_bundle; consider also testing False

These tests nicely pin the default True behavior and a custom CA bundle path. For completeness with the documented options, you could add a case where ssl_ca_bundle is explicitly False to confirm verification can be disabled when desired.

src/handlers/handler_token.py (1)

33-38: Config‑driven SSL verification for public key fetches looks correct

Using SSL_CA_BUNDLE_KEY with a default of True and passing self.ssl_ca_bundle to requests.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 when ssl_ca_bundle is explicitly False so 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 honors ssl_ca_bundle with a secure default; consider improving testability

Deriving ssl_verify = config.get(SSL_CA_BUNDLE_KEY, True) and passing it to boto3.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

📥 Commits

Reviewing files that changed from the base of the PR and between a9347e2 and 0596408.

📒 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 exposes ssl_ca_bundle key

Key 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: New SSL_CA_BUNDLE_KEY constant is clear and consistent

Centralizing the ssl_ca_bundle key here avoids hard‑coding the string across modules and matches the config/README naming.

README.md (1)

85-100: ssl_ca_bundle documentation matches the implementation

The example config and the three documented modes (true, false, and custom path) are aligned with how ssl_ca_bundle is read from config and passed into S3 and token public‑key HTTPS calls, with true as the secure default.


import json
from unittest.mock import patch
from unittest.mock import patch, MagicMock
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

@tmikula-dev tmikula-dev merged commit 13db508 into master Dec 3, 2025
13 of 15 checks passed
@tmikula-dev tmikula-dev deleted the feature/60-add-ca-certificates branch December 3, 2025 09:38
@coderabbitai coderabbitai bot mentioned this pull request Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CA certificates for requests

3 participants