Implement support for renew in client #20
Conversation
WalkthroughThe pull request introduces significant enhancements to the Changes
Possibly related PRs
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: 1
🧹 Outside diff range and nitpick comments (3)
nodeman/client.py (3)
79-79: Fix typo in--passwordhelp text.There is a typo in the help text for the
--passwordargument. It should read"Admin password"instead of"Admin psername".Apply this diff to correct the typo:
-parser.add_argument("--password", metavar="password", help="Admin psername") +parser.add_argument("--password", metavar="password", help="Admin password")
96-96: Update the help text for the--serverargument to reflect its purpose.The help text
"Aggregate receiver"for the--serverargument is misleading. It should accurately describe that this argument specifies the Node Manager server URL.Consider updating the help text as follows:
-parser.add_argument("--server", metavar="URL", help="Aggregate receiver", default="http://127.0.0.1:8080") +parser.add_argument("--server", metavar="URL", help="Node Manager server URL", default="http://127.0.0.1:8080")
110-110: Simplify theopenfunction by removing the unnecessary mode parameter'r'.In Python, the default mode for
open()is'r', so specifying it explicitly is redundant.Apply this diff to simplify the code:
-with open(args.data_jwk_file, "r") as fp: +with open(args.data_jwk_file) as fp:🧰 Tools
🪛 Ruff (0.8.0)
110-110: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
nodeman/client.py(4 hunks)nodeman/nodes.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
nodeman/client.py
110-110: Unnecessary open mode parameters
Remove open mode parameters
(UP015)
🔇 Additional comments (1)
nodeman/nodes.py (1)
189-189: Ensure consistent response by excluding fields with None values.
The addition of response_model_exclude_none=True in the @router.post decorator for the enroll_node function ensures that fields with None values are excluded from the response. This improves the API response by not including unnecessary null fields.
| from nodeman.x509 import generate_x509_csr | ||
|
|
||
| PrivateKey = ec.EllipticCurvePrivateKey | rsa.RSAPublicKey | Ed25519PrivateKey | Ed448PrivateKey |
There was a problem hiding this comment.
Correct the use of RSAPublicKey to RSAPrivateKey in PrivateKey type alias.
The PrivateKey type alias includes rsa.RSAPublicKey, which is incorrect since we are dealing with private keys for generating CSRs. It should be rsa.RSAPrivateKey.
Apply this diff to fix the issue:
-PrivateKey = ec.EllipticCurvePrivateKey | rsa.RSAPublicKey | Ed25519PrivateKey | Ed448PrivateKey
+PrivateKey = ec.EllipticCurvePrivateKey | rsa.RSAPrivateKey | Ed25519PrivateKey | Ed448PrivateKey📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| PrivateKey = ec.EllipticCurvePrivateKey | rsa.RSAPublicKey | Ed25519PrivateKey | Ed448PrivateKey | |
| PrivateKey = ec.EllipticCurvePrivateKey | rsa.RSAPrivateKey | Ed25519PrivateKey | Ed448PrivateKey |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
nodeman/client.py (3)
47-67: Refactor common code into utility functions.The
renewfunction shares significant code withenroll. Consider extracting common functionality into utility functions to improve maintainability and reduce duplication.+def _create_jws_payload(x509_key: PrivateKey, name: str, data_key: JWK) -> str: + """Create JWS payload with CSR and public key.""" + x509_csr = generate_x509_csr(key=x509_key, name=name).public_bytes(serialization.Encoding.PEM).decode() + return json.dumps({"x509_csr": x509_csr, "public_key": data_key.export_public(as_dict=True)}) + +def _send_request(url: str, payload: str, timeout: float = 30.0) -> dict: + """Send HTTP request with error handling.""" + try: + response = httpx.post(url, json=payload, timeout=timeout) + response.raise_for_status() + return response.json() + except httpx.TimeoutException: + raise ConnectionError("Request timed out") + except httpx.HTTPStatusError as e: + if e.response.status_code == 401: + raise ValueError("Unauthorized: Invalid credentials") + elif e.response.status_code == 409: + raise ValueError("Resource conflict") + else: + raise ConnectionError(f"Request failed: {e}") def renew(name: str, server: str, data_key: JWK, x509_key: PrivateKey) -> NodeCertificate: """Renew existing node""" + if not name or not server: + raise ValueError("Name and server URL are required") + + try: + httpx.URL(server) + except httpx.InvalidURL as e: + raise ValueError(f"Invalid server URL: {e}") data_alg = jwk_to_alg(data_key) - x509_csr = generate_x509_csr(key=x509_key, name=name).public_bytes(serialization.Encoding.PEM).decode() - jws_payload = json.dumps({"x509_csr": x509_csr, "public_key": data_key.export_public(as_dict=True)}) + jws_payload = _create_jws_payload(x509_key, name, data_key) jws = JWS(payload=jws_payload) jws.add_signature(key=data_key, alg=data_alg, protected={"alg": data_alg}) renewal_request = jws.serialize() url = urljoin(server, f"/api/v1/node/{name}/renew") - response = httpx.post(url, json=renewal_request) - response.raise_for_status() - renewal_response = response.json() + renewal_response = _send_request(url, renewal_request)
79-79: Fix typo in password argument help text.The help text for the password argument contains a typo: "psername" should be "password".
- parser.add_argument("--password", metavar="password", help="Admin psername") + parser.add_argument("--password", metavar="password", help="Admin password")
154-158: Add error handling for file operations.The file writing operations should include proper error handling to catch and report IO errors.
if args.tls_cert_file: - with open(args.tls_cert_file, "w") as fp: - fp.write(result.x509_certificate) + try: + with open(args.tls_cert_file, "w") as fp: + fp.write(result.x509_certificate) + except IOError as e: + logging.error("Failed to write certificate file: %s", str(e)) + raise SystemExit(1) if args.tls_ca_file: - with open(args.tls_ca_file, "w") as fp: - fp.write(result.x509_ca_certificate) + try: + with open(args.tls_ca_file, "w") as fp: + fp.write(result.x509_ca_certificate) + except IOError as e: + logging.error("Failed to write CA certificate file: %s", str(e)) + raise SystemExit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
nodeman/client.py(4 hunks)
🔇 Additional comments (1)
nodeman/client.py (1)
19-19:
Fix incorrect key type in PrivateKey type alias.
The PrivateKey type alias incorrectly includes rsa.RSAPublicKey instead of rsa.RSAPrivateKey. This is a critical issue as the type is used for private key operations like CSR generation.
-PrivateKey = ec.EllipticCurvePrivateKey | rsa.RSAPublicKey | Ed25519PrivateKey | Ed448PrivateKey
+PrivateKey = ec.EllipticCurvePrivateKey | rsa.RSAPrivateKey | Ed25519PrivateKey | Ed448PrivateKey| if not (args.username and args.password): | ||
| logging.error("Admin username & password required") | ||
| sys.exit(-1) | ||
| try: | ||
| auth = (args.username, args.password) | ||
| response = httpx.post(urljoin(args.server, "/api/v1/node"), auth=auth) | ||
| response.raise_for_status() | ||
| except httpx.HTTPError as exc: | ||
| logging.error("Failed to create node: %s", str(exc)) | ||
| raise SystemExit(1) from exc | ||
| create_response = response.json() | ||
| name = create_response["name"] | ||
| secret = create_response["secret"] | ||
| logging.info("Got name=%s secret=%s", name, secret) |
There was a problem hiding this comment.
Improve security and error handling for admin authentication.
The current implementation has several issues:
- Password is exposed in command line arguments
- Inconsistent error handling (mixing
sys.exitandSystemExit) - No validation of admin credentials before use
Consider:
- Using environment variables or a config file for admin credentials
- Consistent error handling approach
- Validating credentials format
+def get_admin_credentials() -> tuple[str, str]:
+ """Get admin credentials from environment variables."""
+ username = os.getenv("NODEMAN_ADMIN_USERNAME")
+ password = os.getenv("NODEMAN_ADMIN_PASSWORD")
+ if not username or not password:
+ raise ValueError("Admin credentials must be set in NODEMAN_ADMIN_USERNAME and NODEMAN_ADMIN_PASSWORD")
+ return username, password
- if not (args.username and args.password):
- logging.error("Admin username & password required")
- sys.exit(-1)
+ try:
+ username, password = get_admin_credentials()
+ except ValueError as e:
+ logging.error(str(e))
+ raise SystemExit(1)
try:
- auth = (args.username, args.password)
+ auth = (username, password)
response = httpx.post(urljoin(args.server, "/api/v1/node"), auth=auth)
response.raise_for_status()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not (args.username and args.password): | |
| logging.error("Admin username & password required") | |
| sys.exit(-1) | |
| try: | |
| auth = (args.username, args.password) | |
| response = httpx.post(urljoin(args.server, "/api/v1/node"), auth=auth) | |
| response.raise_for_status() | |
| except httpx.HTTPError as exc: | |
| logging.error("Failed to create node: %s", str(exc)) | |
| raise SystemExit(1) from exc | |
| create_response = response.json() | |
| name = create_response["name"] | |
| secret = create_response["secret"] | |
| logging.info("Got name=%s secret=%s", name, secret) | |
| try: | |
| username, password = get_admin_credentials() | |
| except ValueError as e: | |
| logging.error(str(e)) | |
| raise SystemExit(1) | |
| try: | |
| auth = (username, password) | |
| response = httpx.post(urljoin(args.server, "/api/v1/node"), auth=auth) | |
| response.raise_for_status() | |
| except httpx.HTTPError as exc: | |
| logging.error("Failed to create node: %s", str(exc)) | |
| raise SystemExit(1) from exc | |
| create_response = response.json() | |
| name = create_response["name"] | |
| secret = create_response["secret"] | |
| logging.info("Got name=%s secret=%s", name, secret) |
| def enroll(name: str, server: str, hmac_key: JWK, data_key: JWK, x509_key: PrivateKey) -> NodeConfiguration: | ||
| """Enroll new node""" | ||
|
|
||
| hmac_alg = "HS256" | ||
| data_alg = jwk_to_alg(data_key) | ||
| x509_csr = generate_x509_csr(key=x509_key, name=name).public_bytes(serialization.Encoding.PEM).decode() | ||
|
|
||
| jws_payload = json.dumps({"x509_csr": x509_csr, "public_key": data_key.export_public(as_dict=True)}) | ||
|
|
||
| jws = JWS(payload=jws_payload) | ||
| jws.add_signature(key=hmac_key, alg=hmac_alg, protected={"alg": hmac_alg}) | ||
| jws.add_signature(key=data_key, alg=data_alg, protected={"alg": data_alg}) | ||
| enrollment_request = jws.serialize() | ||
|
|
||
| url = urljoin(server, f"/api/v1/node/{name}/enroll") | ||
| response = httpx.post(url, json=enrollment_request) | ||
| response.raise_for_status() | ||
|
|
||
| enrollment_response = response.json() | ||
|
|
||
| logging.debug("Response: %s", json.dumps(enrollment_response, indent=4)) | ||
|
|
||
| return NodeConfiguration(**enrollment_response) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and improve error handling.
The enroll function should validate input parameters and include better error handling:
- Validate server URL format
- Add request timeout
- Add more specific error handling for different HTTP status codes
def enroll(name: str, server: str, hmac_key: JWK, data_key: JWK, x509_key: PrivateKey) -> NodeConfiguration:
"""Enroll new node"""
+ if not name or not server:
+ raise ValueError("Name and server URL are required")
+
+ try:
+ # Validate server URL
+ httpx.URL(server)
+ except httpx.InvalidURL as e:
+ raise ValueError(f"Invalid server URL: {e}")
hmac_alg = "HS256"
data_alg = jwk_to_alg(data_key)
x509_csr = generate_x509_csr(key=x509_key, name=name).public_bytes(serialization.Encoding.PEM).decode()
jws_payload = json.dumps({"x509_csr": x509_csr, "public_key": data_key.export_public(as_dict=True)})
jws = JWS(payload=jws_payload)
jws.add_signature(key=hmac_key, alg=hmac_alg, protected={"alg": hmac_alg})
jws.add_signature(key=data_key, alg=data_alg, protected={"alg": data_alg})
enrollment_request = jws.serialize()
url = urljoin(server, f"/api/v1/node/{name}/enroll")
- response = httpx.post(url, json=enrollment_request)
- response.raise_for_status()
+ try:
+ response = httpx.post(url, json=enrollment_request, timeout=30.0)
+ response.raise_for_status()
+ except httpx.TimeoutException:
+ raise ConnectionError("Enrollment request timed out")
+ except httpx.HTTPStatusError as e:
+ if e.response.status_code == 401:
+ raise ValueError("Unauthorized: Invalid credentials")
+ elif e.response.status_code == 409:
+ raise ValueError("Node already exists")
+ else:
+ raise ConnectionError(f"Enrollment failed: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def enroll(name: str, server: str, hmac_key: JWK, data_key: JWK, x509_key: PrivateKey) -> NodeConfiguration: | |
| """Enroll new node""" | |
| hmac_alg = "HS256" | |
| data_alg = jwk_to_alg(data_key) | |
| x509_csr = generate_x509_csr(key=x509_key, name=name).public_bytes(serialization.Encoding.PEM).decode() | |
| jws_payload = json.dumps({"x509_csr": x509_csr, "public_key": data_key.export_public(as_dict=True)}) | |
| jws = JWS(payload=jws_payload) | |
| jws.add_signature(key=hmac_key, alg=hmac_alg, protected={"alg": hmac_alg}) | |
| jws.add_signature(key=data_key, alg=data_alg, protected={"alg": data_alg}) | |
| enrollment_request = jws.serialize() | |
| url = urljoin(server, f"/api/v1/node/{name}/enroll") | |
| response = httpx.post(url, json=enrollment_request) | |
| response.raise_for_status() | |
| enrollment_response = response.json() | |
| logging.debug("Response: %s", json.dumps(enrollment_response, indent=4)) | |
| return NodeConfiguration(**enrollment_response) | |
| def enroll(name: str, server: str, hmac_key: JWK, data_key: JWK, x509_key: PrivateKey) -> NodeConfiguration: | |
| """Enroll new node""" | |
| if not name or not server: | |
| raise ValueError("Name and server URL are required") | |
| try: | |
| # Validate server URL | |
| httpx.URL(server) | |
| except httpx.InvalidURL as e: | |
| raise ValueError(f"Invalid server URL: {e}") | |
| hmac_alg = "HS256" | |
| data_alg = jwk_to_alg(data_key) | |
| x509_csr = generate_x509_csr(key=x509_key, name=name).public_bytes(serialization.Encoding.PEM).decode() | |
| jws_payload = json.dumps({"x509_csr": x509_csr, "public_key": data_key.export_public(as_dict=True)}) | |
| jws = JWS(payload=jws_payload) | |
| jws.add_signature(key=hmac_key, alg=hmac_alg, protected={"alg": hmac_alg}) | |
| jws.add_signature(key=data_key, alg=data_alg, protected={"alg": data_alg}) | |
| enrollment_request = jws.serialize() | |
| url = urljoin(server, f"/api/v1/node/{name}/enroll") | |
| try: | |
| response = httpx.post(url, json=enrollment_request, timeout=30.0) | |
| response.raise_for_status() | |
| except httpx.TimeoutException: | |
| raise ConnectionError("Enrollment request timed out") | |
| except httpx.HTTPStatusError as e: | |
| if e.response.status_code == 401: | |
| raise ValueError("Unauthorized: Invalid credentials") | |
| elif e.response.status_code == 409: | |
| raise ValueError("Node already exists") | |
| else: | |
| raise ConnectionError(f"Enrollment failed: {e}") | |
| enrollment_response = response.json() | |
| logging.debug("Response: %s", json.dumps(enrollment_response, indent=4)) | |
| return NodeConfiguration(**enrollment_response) |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Nonevalues in the enrollment response.