Conversation
…tilities - Implemented asymmetric cryptography utilities including key generation, encryption, and decryption. - Added synchronous and asynchronous functions for creating and managing SSL certificates. - Introduced symmetric encryption and decryption using AES-CBC. - Developed hashing functions supporting various algorithms. - Created JWT encoding and decoding functionalities. - Integrated password hashing and verification using Argon2id. - Enhanced logging and error handling across all modules.
There was a problem hiding this comment.
Pull request overview
Introduces a new potato_util.crypto utilities package (asymmetric/symmetric/SSL/JWT/password/hash) and updates dependency grouping and documentation to reflect the migration away from the old secure docs/examples.
Changes:
- Added new
src/potato_util/crypto/modules for crypto functionality, including async variants where applicable. - Added a
cryptooptional dependency group and a dedicatedrequirements/requirements.crypto.txt. - Updated docs navigation/content and removed old Secure Utilities docs + example usage.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/potato_util/crypto/symmetric.py | New AES-CBC decrypt helper (symmetric crypto utilities). |
| src/potato_util/crypto/ssl/_sync.py | New sync SSL key/cert generation utilities. |
| src/potato_util/crypto/ssl/_base.py | New Pydantic model for X509 attribute defaults/validation. |
| src/potato_util/crypto/ssl/_async.py | New async SSL key/cert generation utilities using aiofiles. |
| src/potato_util/crypto/ssl/init.py | Package export/conditional async import wiring for SSL utilities. |
| src/potato_util/crypto/password.py | New Argon2 password hashing + verification helpers. |
| src/potato_util/crypto/jwt.py | New JWT encode/decode helpers wrapping PyJWT. |
| src/potato_util/crypto/hash.py | Fix import path for HashAlgoEnum under new crypto package layout. |
| src/potato_util/crypto/asymmetric/_sync.py | New sync RSA key management utilities (create/read keys). |
| src/potato_util/crypto/asymmetric/_base.py | New RSA encrypt/decrypt primitives and keypair generation. |
| src/potato_util/crypto/asymmetric/_async.py | New async RSA key management utilities using aiofiles. |
| src/potato_util/crypto/asymmetric/init.py | Package export/conditional async import wiring for asymmetric utilities. |
| src/potato_util/crypto/init.py | Introduces the crypto package (currently empty __init__). |
| src/potato_util/constants/_enum.py | Adds HTTPSchemeEnum constant for http/https schemes. |
| requirements/requirements.crypto.txt | Adds crypto-related Python requirements. |
| pyproject.toml | Registers new crypto optional dependency group and includes it in all/test/dev. |
| examples/simple/main.py | Removes old secure example usage/imports. |
| README.md | Removes old secure example usage/imports from docs snippet. |
| docs/api-docs/secure.md | Removes the old Secure Utilities API page. |
| docs/api-docs/crypto/asymmetric.md | Adds API docs page for asymmetric crypto utilities. |
| docs/api-docs/crypto/symmetric.md | Adds API docs page for symmetric crypto utilities. |
| docs/api-docs/crypto/hash.md | Adds API docs page for hashing utilities. |
| docs/api-docs/crypto/password.md | Adds API docs page for password utilities. |
| docs/api-docs/crypto/jwt.md | Adds API docs page for JWT utilities. |
| docs/api-docs/crypto/ssl.md | Adds API docs page for SSL utilities. |
| docs/api-docs/crypto/.nav.yml | Adds nav config for the new crypto docs subsection. |
| docs/api-docs/crypto/.meta.yml | Adds metadata/tags for the new crypto docs subsection. |
| docs/api-docs/.nav.yml | Adds “Crypto Utilities” section to API docs navigation. |
| docs/api-docs/.meta.yml | Cleans up docs metadata by removing hidden tags config. |
| docs/getting-started/.meta.yml | Cleans up docs metadata by removing hidden tags config. |
| docs/dev/.meta.yml | Cleans up docs metadata by removing hidden tags config. |
| docs/about/.meta.yml | Cleans up docs metadata by removing hidden tags config. |
Comments suppressed due to low confidence (1)
pyproject.toml:61
- Docs builds (see
.github/workflows/publish-docs.yml) install onlyrequirements/requirements.docs.txt, but the newly added mkdocstrings pages underdocs/api-docs/crypto/will importpotato_util.crypto.*modules, which require crypto deps. To prevent the docs pipeline from failing, ensure the docs dependency set includes the crypto requirements (e.g., addrequirements.crypto.txtpackages torequirements.docs.txtor otherwise install thecryptoextra for docs builds).
build = { file = ["./requirements/requirements.build.txt"] }
docs = { file = ["./requirements/requirements.docs.txt"] }
dev = { file = [
"./requirements/requirements.async.txt",
"./requirements/requirements.crypto.txt",
"./requirements/requirements.fastapi.txt",
"./requirements/requirements.test.txt",
"./requirements/requirements.build.txt",
"./requirements/requirements.docs.txt",
"./requirements/requirements.dev.txt",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _meesage = f"Generating SSL key and cert files: ['{_key_path}', '{_cert_path}']..." | ||
| if warn_mode == WarnEnum.ALWAYS: | ||
| logger.info(_meesage) | ||
| elif warn_mode == WarnEnum.DEBUG: | ||
| logger.debug(_meesage) |
There was a problem hiding this comment.
Typo: _meesage should be _message (and the same variable name should be used consistently for readability/searchability).
| _meesage = f"Generating SSL key and cert files: ['{_key_path}', '{_cert_path}']..." | |
| if warn_mode == WarnEnum.ALWAYS: | |
| logger.info(_meesage) | |
| elif warn_mode == WarnEnum.DEBUG: | |
| logger.debug(_meesage) | |
| _message = f"Generating SSL key and cert files: ['{_key_path}', '{_cert_path}']..." | |
| if warn_mode == WarnEnum.ALWAYS: | |
| logger.info(_message) | |
| elif warn_mode == WarnEnum.DEBUG: | |
| logger.debug(_message) |
| _cipher = ciphers.Cipher( | ||
| algorithm=algorithms.AES(key=key), mode=modes.CBC(initialization_vector=iv) | ||
| ) | ||
| _decryptor = _cipher.decryptor() | ||
| _plaintext = _decryptor.update(data=ciphertext) + _decryptor.finalize() |
There was a problem hiding this comment.
AES-CBC decryption here returns raw decrypted blocks without removing padding (e.g., PKCS7), so callers will often get padded plaintext or decode errors when as_str=True. Consider unpadding the plaintext (or adding an explicit unpad/padding parameter and documenting the expected padding scheme).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| @@ -0,0 +1,3 @@ | |||
| argon2-cffi-bindings>=21.2.0,<30.0.0 | |||
| argon2-cffi>=25.1.0,<26.0.0 | |||
| PyJWT[crypto]>=2.10.1,<3.0.0 | |||
There was a problem hiding this comment.
requirements.crypto.txt doesn’t list cryptography, even though the new crypto modules (asymmetric/symmetric/ssl) import and require it. Relying on PyJWT[crypto] to pull cryptography transitively makes the dependency implicit and brittle; add an explicit cryptography requirement for clarity and to avoid breakage if transitive deps change.
| PyJWT[crypto]>=2.10.1,<3.0.0 | |
| PyJWT[crypto]>=2.10.1,<3.0.0 | |
| cryptography |
| key: SecretStr | AllowedPublicKeyTypes, | ||
| algorithm: str, | ||
| options: Options = {}, | ||
| ) -> dict[str, Any]: | ||
| """Decodes JWT token and returns payload. | ||
|
|
||
| Args: | ||
| token (str , required): JWT token to decode. | ||
| key (SecretStr | AllowedPublicKeyTypes, required): Secret key to decode token with. | ||
| algorithm (str , required): Algorithm to decode token with. | ||
| options (Options , optional): Options to decode token with. Defaults to {}. | ||
|
|
||
| Raises: | ||
| jwt.ExpiredSignatureError: If token is expired. | ||
| jwt.InvalidTokenError : If token is invalid. | ||
|
|
||
| Returns: | ||
| dict[str, Any]: Decoded payload from JWT token. | ||
| """ | ||
|
|
||
| if "require" not in options: | ||
| options["require"] = ["sub", "exp", "iat", "jti"] | ||
|
|
There was a problem hiding this comment.
options uses a mutable default ({}) and is mutated in-place (options["require"] = ...). This will leak state between calls and can cause surprising behavior. Use options: Options | None = None and create/copy a new dict inside the function before mutating it.
| x509_attrs: X509AttrsPM | dict[str, Any] = X509AttrsPM(), | ||
| force: bool = False, | ||
| warn_mode: WarnEnum = WarnEnum.DEBUG, | ||
| ) -> None: | ||
| """Generate and create SSL key and cert files. | ||
|
|
||
| Args: | ||
| ssl_dir (str , required): SSL directory path. | ||
| key_fname (str , required): Key file name. | ||
| cert_fname (str , required): Certificate file name. | ||
| key_size (int , required): Key size. | ||
| x509_attrs (X509AttrsPM | dict[str, Any], optional): X509 named attributes. Defaults to X509AttrsPM(). | ||
| force (bool , optional): Force to create SSL key and cert files. Defaults to False. | ||
| warn_mode (WarnEnum , optional): Warning mode. Defaults to WarnEnum.DEBUG. | ||
|
|
||
| Raises: | ||
| FileExistsError: When warning mode is set to ERROR and SSL key or cert files already exist. | ||
| OSError : If failed to create SSL key or cert files. | ||
| """ | ||
|
|
||
| assert isinstance( | ||
| x509_attrs, X509AttrsPM | ||
| ), f"`x509_attrs` argument type is invalid {type(x509_attrs)}, should be <X509AttrsPM>!" | ||
|
|
There was a problem hiding this comment.
x509_attrs is annotated as X509AttrsPM | dict[...], but the function relies on an assert isinstance(x509_attrs, X509AttrsPM) for runtime validation. assert statements are stripped with Python optimizations (-O), which would allow a dict through and later cause AttributeError when accessing x509_attrs.C, etc. Replace the assert with explicit validation/conversion (e.g., convert dicts into X509AttrsPM and raise a real exception on invalid input).
| async def async_create_ssl_certs( | ||
| ssl_dir: str, | ||
| cert_fname: str, | ||
| key_fname: str, | ||
| key_size: int, |
There was a problem hiding this comment.
Async and sync SSL cert helpers use different parameter ordering (create_ssl_certs(ssl_dir, key_fname, cert_fname, ...) vs async_create_ssl_certs(ssl_dir, cert_fname, key_fname, ...)). This inconsistency is easy to misuse and can silently swap key/cert filenames. Align the async signature with the sync one (or make args keyword-only).
| assert isinstance( | ||
| x509_attrs, X509AttrsPM | ||
| ), f"`x509_attrs` argument type is invalid {type(x509_attrs)}, should be <X509AttrsPM>!" |
There was a problem hiding this comment.
x509_attrs is annotated as X509AttrsPM | dict[...], but the function relies on an assert isinstance(x509_attrs, X509AttrsPM) for runtime validation. assert statements are stripped with Python optimizations (-O), which would allow a dict through and later cause AttributeError when accessing x509_attrs.C, etc. Replace the assert with explicit validation/conversion (e.g., convert dicts into X509AttrsPM and raise a real exception on invalid input).
| assert isinstance( | |
| x509_attrs, X509AttrsPM | |
| ), f"`x509_attrs` argument type is invalid {type(x509_attrs)}, should be <X509AttrsPM>!" | |
| if isinstance(x509_attrs, dict): | |
| try: | |
| x509_attrs = X509AttrsPM(**x509_attrs) | |
| except TypeError as exc: # pragma: no cover - defensive programming | |
| raise TypeError( | |
| f"`x509_attrs` dict contains invalid fields or values: {x509_attrs!r}" | |
| ) from exc | |
| elif not isinstance(x509_attrs, X509AttrsPM): | |
| raise TypeError( | |
| f"`x509_attrs` argument type is invalid {type(x509_attrs)}, should be <X509AttrsPM> or dict!" | |
| ) |
This pull request introduces a new modular crypto utilities package and updates documentation, dependencies, and example code to reflect this addition. The main changes are the migration of cryptographic functionality from the previous
secureutilities to a newcryptomodule, the addition of optional crypto dependencies, and the update of related documentation and configuration files.Crypto utilities modularization and documentation:
cryptopackage with submodules for asymmetric encryption, symmetric encryption, hashing, password management, JWT, and SSL, and added corresponding API documentation files (docs/api-docs/crypto/*.md,.meta.yml,.nav.yml). [1] [2] [3] [4] [5] [6] [7] [8]docs/api-docs/.nav.yml).Secure Utilitiesdocumentation page (docs/api-docs/secure.md).Dependency management:
cryptooptional dependency group inpyproject.tomland created a requirements file for crypto-related packages (requirements/requirements.crypto.txt). [1] [2]Code and example updates:
secureutilities fromREADME.mdandexamples/simple/main.pyto reflect the migration to the newcryptomodule. [1] [2] [3] [4]Constants update:
HTTPSchemeEnumtosrc/potato_util/constants/_enum.pyand included it in the module's__all__list. [1] [2]Documentation metadata cleanup:
hide: - tagsfield from multiple.meta.ymlfiles to simplify documentation metadata. [1] [2] [3]These changes improve the modularity and clarity of the crypto-related utilities, streamline dependency management, and update documentation to match the new structure.