refactor!: use RSA directly, drop PGPy #1
Conversation
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces significant changes to the poorman_handshake library, including the removal of various GitHub Actions workflows related to version management and publishing. The project transitions from PGP-based cryptography to RSA-based cryptography, with updates to utility functions and example scripts. Additionally, new workflows for automating label addition and stable releases are added, while several version management scripts are deleted. Changes
Sequence DiagramsequenceDiagram
participant Alice
participant Bob
participant RSA_Util
Alice->>RSA_Util: Generate Key Pair
Bob->>RSA_Util: Generate Key Pair
Alice->>Bob: Generate Handshake (Encrypt Secret)
Bob->>RSA_Util: Verify Handshake
Bob->>Bob: Decrypt Shared Secret
Bob-->>Alice: Confirm Handshake
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
poorman_handshake/symmetric/utils.py (1)
5-7: Docstring mismatch forgenerate_iv.
The docstring states “Generate a random string of letters and digits,” but the implementation returns random bytes. Consider updating the docstring to reflect the actual return type and usage.def generate_iv(key_length=8): - """Generate a random string of letters and digits """ + """Generate random bytes of specified length""" return get_random_bytes(key_length)poorman_handshake/asymmetric/utils.py (3)
9-26: Potential file I/O error handling inexport_RSA_key.
Currently, if the file path is invalid or unwritable, the function will raise an exception. Consider adding try-except or clarifying in docstrings that an I/O exception may occur.
28-40: Encourage error handling for file not found inload_RSA_key.
Similar toexport_RSA_key, you may want to add a brief note on possible I/O exceptions when the file does not exist or is unreadable.
121-147:verify_RSA: Balanced approach to signature verification.
The logic for adjustingsignature_sizedepending on the form ofpublic_keyis clean. Consider clarifying the docstring around possibleValueErrororTypeError.poorman_handshake/asymmetric/__init__.py (5)
54-62:export_private_key: Delegates to utilities properly.
Implementation is consistent. Could consider clarifying docstrings about overwriting existing files.
65-75:pubkeyproperty returns PEM string.
Looks great. Suggest raising an exception or returning an empty string ifself.private_keyis missing. Current code returnsNonein that scenario.
114-133:verifymethod: Good separation of signature from ciphertext.
Implementation is clean. As a future improvement, consider returning an error message or raising an exception if verification fails.
134-145:receive_and_verify: Good orchestrator method.
Callsverifyfirst, thenreceive_handshake. Consider returning the handshake result or success/failure for better clarity in external usage.
152-161:HalfHandShake.receive_handshake: Decrypts without XOR.
This approach replacesself.secretdirectly, in contrast to the parent class’s XOR usage. The difference in design seems intentional but might create confusion. Document each approach’s difference explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/publish_alpha.yml(1 hunks)examples/half_handshake.py(1 hunks)examples/key_handling.py(0 hunks)examples/simple_handshake.py(1 hunks)examples/simple_mitm.py(1 hunks)examples/static_handshake.py(1 hunks)examples/static_mitm.py(1 hunks)examples/tofu_handshake.py(1 hunks)examples/tofu_mitm.py(1 hunks)poorman_handshake/asymmetric/__init__.py(1 hunks)poorman_handshake/asymmetric/utils.py(1 hunks)poorman_handshake/symmetric/utils.py(2 hunks)requirements.txt(1 hunks)scripts/bump_alpha.py(0 hunks)scripts/bump_build.py(0 hunks)scripts/bump_major.py(0 hunks)scripts/bump_minor.py(0 hunks)scripts/remove_alpha.py(0 hunks)
💤 Files with no reviewable changes (6)
- examples/key_handling.py
- scripts/bump_alpha.py
- scripts/remove_alpha.py
- scripts/bump_build.py
- scripts/bump_major.py
- scripts/bump_minor.py
✅ Files skipped from review due to trivial changes (3)
- requirements.txt
- .github/workflows/publish_alpha.yml
- examples/simple_mitm.py
🔇 Additional comments (19)
poorman_handshake/symmetric/utils.py (2)
1-2: Use consistent imports for Cryptodome.
Looks good. The transition from hashlib to Cryptodome.Hash and usage of get_random_bytes is consistent with the new RSA-based design.
18-18: Verify usage of SHA256.new.
Using SHA256.new(iv + text.encode("utf-8")) is appropriate, but confirm that all upstream calls handle the resulting digest properly and that the code does not rely on the old hashlib approach.
poorman_handshake/asymmetric/utils.py (6)
1-7: Imports alignment and library usage.
The chosen Cryptodome modules and typing utilities look properly imported. This sets up the RSA-based workflow correctly.
42-56: Key size configurability in create_RSA_key.
Providing a default key size of 2048 is good. For specialized use cases (e.g., heavier security or performance constraints), users can pass a different size. This function looks clean.
58-77: encrypt_RSA: Potential large message handling caution.
RSA encryption has inherent limits on input size (must be smaller than key size minus padding overhead). Consider clarifying that large messages should be encrypted with a symmetric key instead, or docstring clarification.
79-98: decrypt_RSA: Good direct counterpart to encrypt_RSA.
Implementation is straightforward and consistent with the PKCS1_OAEP usage. No major concerns.
100-119: sign_RSA: Good use of PSS.
The usage of PSS for digital signatures is a recommended secure practice. Implementation is sound.
149-157: Demonstration block.
The example usage of RSA generation, encryption, decryption, and signing flows nicely. It might be helpful to print or log each step for educational purposes in the demonstration.
poorman_handshake/asymmetric/__init__.py (5)
28-35: Constructor handles new key creation or loading well.
Using isfile(path) is a sensible check. If the path does not exist, a fresh key is generated. Ensure that any I/O and file permission errors are surfaced or handled.
45-51: load_private: Good delegation to load_RSA_key.
Straightforward approach. No concerns beyond potential file I/O issues.
76-91: generate_handshake: Creates and signs handshake.
Encryption + signing is an excellent approach. Ensure the caller has loaded the correct public key into self.target_key if pub is not provided.
92-100: load_public: Simple public key import.
Implementation is correct. Could also add error handling or docstring notes if the key is invalid.
101-113: receive_handshake: XOR usage highlight.
You’re XORing the newly decrypted secret with self.secret. Confirm that this “combined secret” design is intentional, as it changes each handshake step. If the code intended to replace self.secret, remove the XOR.
examples/simple_handshake.py (1)
28-28: Hex-encoding secrets.
Printing bob.secret.hex() improves clarity and ensures raw binary data won’t break terminals. This looks good.
examples/half_handshake.py (1)
26-26: Printing alice.secret in hex.
Consistent with other examples. This is a helpful change for readability and debugging.
examples/static_handshake.py (1)
29-29: **Using .hex() for consistent secret output **
Printing the secret in hexadecimal format improves readability and consistency with other examples.
examples/static_mitm.py (1)
29-29: **Hexadecimal display standardizes secret representation **
Converting the secret to hex ensures a uniform output convention across the codebase.
examples/tofu_handshake.py (1)
41-41: **Aligning secret output format with .hex() usage **
This minor change is consistent with the broader approach of printing secrets in a standardized hexadecimal form.
examples/tofu_mitm.py (1)
42-42: **Hexadecimal format maintains consistency and readability **
Using .hex() here complements the rest of the updated handshake examples, ensuring uniform secret output.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
.github/workflows/conventional-label.yaml (1)
10-10: Consider these minor improvements:
- Pin to a specific SHA for better reproducibility
- Add a newline at the end of file
Apply this diff:
- - uses: bcoe/conventional-release-labels@v1 + - uses: bcoe/[email protected] # or specific SHA +🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/release_workflow.yml (1)
1-108: Well-structured release automation workflow with room for improvement.The workflow effectively handles the complete release process from alpha to stable versions. The job dependencies and conditions are properly set up, ensuring a sequential and controlled release process.
Consider these architectural improvements:
- Add status checks before PyPI publishing
- Include automated changelog generation
- Add automated version validation
- Consider implementing rollback procedures
Would you like me to provide implementation details for any of these improvements?
🧰 Tools
🪛 actionlint (1.7.4)
26-26: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
42-42: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
47-47: the runner of "actions/setup-python@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
54-54: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
71-71: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
76-76: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
82-82: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
94-94: shellcheck reported issue in this script: SC2034:warning:2:1: BRANCH_NAME appears unused. Verify use (or export if used externally)
(shellcheck)
.github/workflows/publish_stable.yml (1)
1-58: Consider adding explicit permissions.For enhanced security, consider adding explicit permissions to limit the workflow's scope:
Add this configuration at the workflow level:
name: Stable Release on: push: branches: [master] workflow_dispatch: +permissions: + contents: write # for checkout and pushing changes + packages: write # for publishing packages + pull-requests: read # for reading PR information🧰 Tools
🪛 actionlint (1.7.4)
22-22: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-27: the runner of "actions/setup-python@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
34-34: workflow command "set-output" was deprecated. use
echo "{name}={value}" >> $GITHUB_OUTPUTinstead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions(deprecated-commands)
50-50: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 58-58: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/conventional-label.yaml(1 hunks).github/workflows/dev2master.yml(0 hunks).github/workflows/publish_alpha.yml(0 hunks).github/workflows/publish_build.yml(0 hunks).github/workflows/publish_major.yml(0 hunks).github/workflows/publish_minor.yml(0 hunks).github/workflows/publish_stable.yml(1 hunks).github/workflows/release_workflow.yml(1 hunks)
💤 Files with no reviewable changes (5)
- .github/workflows/publish_build.yml
- .github/workflows/publish_alpha.yml
- .github/workflows/dev2master.yml
- .github/workflows/publish_minor.yml
- .github/workflows/publish_major.yml
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/conventional-label.yaml
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/publish_stable.yml
[error] 58-58: no new line character at the end of file
(new-line-at-end-of-file)
🪛 actionlint (1.7.4)
.github/workflows/release_workflow.yml
26-26: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
42-42: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
47-47: the runner of "actions/setup-python@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
54-54: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
71-71: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
76-76: the runner of "actions/setup-python@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
82-82: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
94-94: shellcheck reported issue in this script: SC2034:warning:2:1: BRANCH_NAME appears unused. Verify use (or export if used externally)
(shellcheck)
.github/workflows/publish_stable.yml
22-22: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
27-27: the runner of "actions/setup-python@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
34-34: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
50-50: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (4)
.github/workflows/conventional-label.yaml (2)
1-10: LGTM! The workflow configuration is secure and well-structured.
The workflow is correctly configured to automatically label PRs using conventional commit standards. The use of pull_request_target is appropriate here as the workflow only uses a trusted action for labeling.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 10-10: no new line character at the end of file
(new-line-at-end-of-file)
3-4: Verify the event trigger configuration.
The workflow uses pull_request_target which runs with repository token access. While this is safe for the current usage (only labeling), ensure that any future modifications to this workflow maintain security best practices.
Run this script to check for other workflows using pull_request_target:
✅ Verification successful
Let me check the actual implementation of the workflow to ensure it's following security best practices.
pull_request_target usage is secure in this workflow
The workflow is correctly configured and follows security best practices:
- It's the only workflow using
pull_request_targetin the repository - It exclusively uses a trusted third-party action (
bcoe/conventional-release-labels@v1) for labeling - The workflow doesn't checkout any code from the pull request
- No repository contents are processed directly, eliminating the risk of executing untrusted code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other workflows using pull_request_target to ensure consistent security practices
# Search for pull_request_target in workflow files
echo "Checking other workflows using pull_request_target:"
rg -l "pull_request_target" .github/workflows/
Length of output: 201
Script:
#!/bin/bash
# Check the implementation details of the conventional-label workflow
cat .github/workflows/conventional-label.yaml
Length of output: 266
.github/workflows/release_workflow.yml (1)
1-7: LGTM! Well-structured workflow trigger configuration.
The workflow trigger is properly configured to run only when PRs are closed (merged) to the dev branch.
.github/workflows/publish_stable.yml (1)
1-6: LGTM: Workflow triggers are appropriately configured.
The workflow triggers are well-configured for stable releases, allowing both automated (push to master) and manual (workflow_dispatch) execution.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
poorman_handshake/asymmetric/__init__.py(1 hunks)
🔇 Additional comments (1)
poorman_handshake/asymmetric/__init__.py (1)
95-99: Verify the cryptographic protocol implementation.
The handshake protocol implementation has potential security considerations:
- The shared secret generation uses a secure random number generator (good!)
- However, the protocol might be vulnerable to replay attacks as it lacks a timestamp or nonce
- The signature is concatenated with the ciphertext without a clear delimiter
Consider enhancing the protocol security by:
- Adding a nonce or timestamp
- Using a standardized format (like ASN.1) for message structure
- Implementing perfect forward secrecy
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
poorman_handshake/asymmetric/__init__.py (2)
31-38: Validate RSA key size against security standardsThe default key size of 2048 bits meets minimum security requirements, but consider:
- Adding validation to prevent keys smaller than 2048 bits
- Documenting that 4096 bits is recommended for long-term security
def __init__(self, path: str = None, key_size: int = 2048): + if key_size < 2048: + raise ValueError("RSA key size must be at least 2048 bits for security") self.private_key = None
53-54: Use exception chaining for better error trackingWhen re-raising exceptions, use exception chaining to preserve the original traceback.
- except Exception as e: - raise ValueError(f"Failed to handle invalid key file: {e}") + except Exception as e: + raise ValueError(f"Failed to handle invalid key file") from e🧰 Tools
🪛 Ruff (0.8.2)
54-54: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
poorman_handshake/asymmetric/__init__.py(1 hunks)requirements.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🪛 Ruff (0.8.2)
poorman_handshake/asymmetric/__init__.py
54-54: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
poorman_handshake/asymmetric/__init__.py (2)
126-130: Review the secret combination logic
The XOR operation for combining secrets might not provide the best security properties:
- XOR is sensitive to bit flips
- If an attacker obtains one secret, they can influence the final combined secret
Consider using a more robust key derivation function:
-self.secret = bytes(a ^ b for a, b in zip(self.secret, decrypted))
+from cryptography.hazmat.primitives import hashes
+from cryptography.hazmat.primitives.kdf.hkdf import HKDF
+self.secret = HKDF(
+ algorithm=hashes.SHA256(),
+ length=32,
+ salt=None,
+ info=b"handshake-key",
+).derive(self.secret + decrypted)166-168: Document security implications of HalfHandShake
The HalfHandShake class skips the secret combination step, which has security implications:
- The docstring should explicitly warn about the reduced security properties
- Consider adding a warning when this class is instantiated
class HalfHandShake(HandShake):
"""
A simpler handshake implementation where the shared secret is directly decrypted.
+
+ Warning:
+ This implementation provides weaker security guarantees compared to the full HandShake:
+ - No perfect forward secrecy
+ - Vulnerable to replay attacks
+ - Should only be used in specific scenarios where these trade-offs are acceptable
"""
+
+ def __init__(self, *args, **kwargs):
+ import warnings
+ warnings.warn(
+ "HalfHandShake provides reduced security. "
+ "Use the full HandShake implementation unless you have specific requirements.",
+ category=SecurityWarning
+ )
+ super().__init__(*args, **kwargs)
Summary by CodeRabbit
New Features
HandShakeclass with RSA encryption and signatures.Bug Fixes
Refactor
Dependency Changes
PGPylibrary.pycryptodomexlibrary.