-
Notifications
You must be signed in to change notification settings - Fork 6
Custom ca-bundle + verify ssl skip features #30
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,15 +1,39 @@ | ||||||||||||||||
| import io | ||||||||||||||||
| import os | ||||||||||||||||
| import time | ||||||||||||||||
| from typing import Optional | ||||||||||||||||
| from typing import Optional, Union | ||||||||||||||||
|
||||||||||||||||
| from typing import Optional, Union | |
| from typing import Optional |
Copilot
AI
Dec 22, 2025
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.
The class name SSLAdapter might be confusing since it's actually an HTTPAdapter that handles SSL configuration. Consider renaming to CustomCAAdapter or CABundleAdapter to better reflect its specific purpose of handling custom CA certificates.
Copilot
AI
Dec 22, 2025
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.
The class docstring says "allows merging custom CA certificates with the system default certificates" but the implementation in init_poolmanager may not actually merge them properly. The comment on line 31 says "Load both system CAs and custom CAs" but only calls load_verify_locations with the custom CA file, which might not preserve the system CAs depending on the SSL context state.
Copilot
AI
Dec 22, 2025
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.
The SSL context created by create_urllib3_context() already loads system default CA certificates. However, calling ctx.load_verify_locations(cafile=self.ca_bundle) may replace the default certificates rather than adding to them, depending on the SSL implementation. To ensure both system CAs and custom CAs are trusted, you should also explicitly load the default CA bundle using ctx.load_default_certs() or load both the system CA bundle and the custom CA bundle.
| # Load both system CAs and custom CAs | |
| # Ensure system default CAs are loaded, then add custom CAs | |
| ctx.load_default_certs() |
Copilot
AI
Dec 22, 2025
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.
Missing input validation for the ca_bundle parameter. If a non-existent file path is provided, the error will only surface when making HTTPS requests, making debugging harder. Consider validating that the file exists and is readable during initialization.
Copilot
AI
Dec 22, 2025
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.
Type hint mismatch: verify is declared as Union[bool, str] in pycaprio.py and http_adapter.py, but as bool in RetryableInceptionClient. The implementation only handles boolean values. If string values (typically CA bundle paths) are intended to be supported, the implementation needs to handle them. Otherwise, the type hints should be consistently bool across all three files.
Copilot
AI
Dec 22, 2025
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.
The logic for setting self.session.verify is redundant. Lines 59-64 explicitly set self.session.verify based on the verify parameter, but this is the default behavior for requests.Session(). The code can be simplified by just setting self.session.verify = verify unless there's a specific reason for the if/else structure.
| if verify is True: | |
| # If verify is True, use the SSLAdapter | |
| self.session.verify = True | |
| else: | |
| # If verify is False, disable verification | |
| self.session.verify = False | |
| self.session.verify = verify |
Copilot
AI
Dec 22, 2025
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.
When verify=False is used to disable SSL verification, urllib3 will emit warnings. Consider adding a warning suppression or at least documenting this behavior. Additionally, disabling SSL verification without proper warnings could lead to security issues in production environments.
Copilot
AI
Dec 22, 2025
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.
The condition verify is not False uses identity comparison instead of equality comparison. This will fail to detect False-like values (e.g., 0, empty strings). Since verify can be Union[bool, str], use verify != False or better yet, verify is True since you only want to mount the adapter when verification is explicitly enabled.
| # Mount custom SSL adapter if we have a custom CA bundle | |
| if ca_bundle and verify is not False: | |
| # Mount custom SSL adapter if we have a custom CA bundle and verification is explicitly enabled | |
| if ca_bundle and verify is True: |
Copilot
AI
Dec 22, 2025
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.
The SSLAdapter is mounted on both "https://" and "http://" schemes. However, SSL/TLS is only used for HTTPS connections. Mounting the SSL adapter on HTTP is unnecessary and could be misleading since HTTP connections don't use SSL/TLS at all.
| self.session.mount("http://", adapter) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import os | ||
| from typing import Optional | ||
| from typing import Optional, Union | ||
|
|
||
| from pycaprio.core.adapters.http_adapter import HttpInceptionAdapter | ||
| from pycaprio.core.exceptions import ConfigurationNotProvided | ||
|
|
@@ -17,13 +17,20 @@ def __init__( | |
| inception_host: Optional[str] = None, | ||
| authentication: Optional[authentication_type] = None, | ||
| local_projects_dir: Optional[str] = None, | ||
| ca_bundle: Optional[str] = None, | ||
| verify: Union[bool, str] = True, | ||
| ): | ||
| """ | ||
| Initializes Pycaprio in either remote or local mode. | ||
|
|
||
| :param inception_host: Hostname of the INCEpTION instance. | ||
| :param authentication: Tuple of username and password for INCEpTION instance. | ||
| :param local_projects_dir: Directory containing exported INCEpTION projects in ZIP format. | ||
| :param ca_bundle: Path to a custom CA certificate file to trust. | ||
| This is the recommended way to support self-signed certificates. | ||
| :param verify: Controls SSL verification behavior: | ||
| - True (default): Verify SSL certificates using system CAs + ca_bundle if provided | ||
| - False: Disable SSL verification (not recommended for production) | ||
|
Comment on lines
+31
to
+33
|
||
| """ | ||
| inception_host = inception_host or os.getenv("INCEPTION_HOST") | ||
| if inception_host: | ||
|
|
@@ -33,8 +40,8 @@ def __init__( | |
| "Authentication was not provided. " | ||
| "You can set it via environment variables as 'INCEPTION_USERNAME' and 'INCEPTION_PASSWORD'" | ||
| ) | ||
| self.api = HttpInceptionAdapter(inception_host, authentication) | ||
|
|
||
| self.api = HttpInceptionAdapter(inception_host, authentication, ca_bundle=ca_bundle, verify=verify) | ||
| elif local_projects_dir: | ||
| self.api = LocalInceptionAdapter(local_projects_dir) | ||
| else: | ||
|
|
||
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.
Import of 'os' is not used.