Add keyserver publishing foundation#37
Conversation
Add the local keyserver service, CLI publish/login/fetch commands, compose smoke wiring, and tests as a reviewable baseline for the security hardening work stacked above it. Constraint: Security fixes for issues #30-#34 target files that are not yet present on main. Rejected: Fold all security fixes into one PR | would make review and issue closure harder. Confidence: medium Scope-risk: broad Directive: Review this foundation before merging stacked hardening PRs into main. Tested: PYTHONPATH=. python3 -m unittest discover keyserver/tests; env -u GOROOT -u GOPATH /opt/homebrew/bin/go test ./... Not-tested: Docker compose smoke test was not run locally.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security and Stability Review
This PR adds keyserver functionality but introduces critical security vulnerabilities that block merge. The implementation requires immediate fixes before it can be safely deployed.
Critical Issues Found (5)
Security Vulnerabilities:
- SSRF vulnerability in
auth.py- External network requests are not validated, enabling access to internal services and AWS metadata endpoints - Timing attack vulnerability in
server.py- Admin token comparison is not constant-time, allowing token discovery - OAuth phishing risk in
keyserver_login_cmd.go- OAuth endpoints are not validated, enabling credential theft
Crash Risks:
4. JSON parsing in server.py - Missing exception handling causes server crashes on malformed requests
5. Port configuration in server.py - Invalid port values crash server on startup
Required Actions
All 5 critical issues must be resolved before merge. Code suggestions have been provided for the crash risks. The security vulnerabilities require design changes to properly validate URLs and implement constant-time comparisons.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| func deviceFlowConfig(provider, clientID, issuer string) (deviceEndpoint, tokenEndpoint, scope string, err error) { | ||
| switch provider { | ||
| case "github": | ||
| if clientID == "" { | ||
| return "", "", "", fmt.Errorf("--client-id is required for GitHub login") | ||
| } | ||
| return "https://github.com/login/device/code", "https://github.com/login/oauth/access_token", "read:user user:email", nil | ||
| case "okta": | ||
| if clientID == "" { | ||
| return "", "", "", fmt.Errorf("--client-id is required for Okta login") | ||
| } | ||
| issuer = strings.TrimRight(strings.TrimSpace(issuer), "/") | ||
| if issuer == "" { | ||
| return "", "", "", fmt.Errorf("--issuer is required for Okta login") | ||
| } | ||
| return issuer + "/v1/device/authorize", issuer + "/v1/token", "openid profile email", nil | ||
| default: | ||
| return "", "", "", fmt.Errorf("unsupported provider: %s", provider) | ||
| } |
There was a problem hiding this comment.
🛑 Security Vulnerability: Hardcoded OAuth endpoints for github.com and okta enable attackers to phish credentials by manipulating the issuer parameter to redirect to malicious look-alike domains. An attacker could set issuer to "" and capture user credentials through a fake OAuth flow.
| def _get_json(url: str, headers: dict[str, str]) -> dict: | ||
| req = urllib.request.Request(url, headers=headers) | ||
| with urllib.request.urlopen(req, timeout=10) as resp: | ||
| body = resp.read() |
There was a problem hiding this comment.
🛑 Security Vulnerability: Unvalidated external network request enables Server-Side Request Forgery (SSRF)1. The _get_json function uses urllib.request.urlopen without validating the URL or IP address, allowing attackers to access internal services or AWS metadata endpoints by controlling the issuer parameter in Okta authentication.
Footnotes
-
CWE-918: Server-Side Request Forgery (SSRF) - https://cwe.mitre.org/data/definitions/918.html ↩
| raise ValueError("requested peer id does not match SSO identity mapping") | ||
| if requested_peer_id and not mapped and requested_peer_id != subject and requested_peer_id != subject.split("@", 1)[0]: | ||
| raise ValueError("requested peer id does not match SSO identity") | ||
| return peer_id | ||
|
|
||
|
|
||
| def _mapped_peer_id(provider: str, subject: str, email: str) -> str: | ||
| raw = os.getenv("ENDE_KEYSERVER_IDENTITY_MAP", "") |
There was a problem hiding this comment.
🛑 Security Vulnerability: Timing attack vulnerability in token comparison1. Using direct string comparison (==) at line 86 allows attackers to determine valid tokens character-by-character through response time analysis. The admin token should be compared using a constant-time comparison function.
Footnotes
-
CWE-208: Observable Timing Discrepancy - https://cwe.mitre.org/data/definitions/208.html ↩
|
|
||
| def main() -> None: | ||
| host = os.getenv("ENDE_KEYSERVER_HOST", "127.0.0.1") | ||
| port = int(os.getenv("ENDE_KEYSERVER_PORT", "8765")) |
There was a problem hiding this comment.
🛑 Crash Risk: Missing exception handling for invalid port number will crash the server on startup. If ENDE_KEYSERVER_PORT is set to a non-integer value, int() will raise ValueError and the server will fail to start.
| port = int(os.getenv("ENDE_KEYSERVER_PORT", "8765")) | |
| port = int(os.getenv("ENDE_KEYSERVER_PORT", "8765") or "8765") |
| def _read_json(self) -> dict[str, Any]: | ||
| length = int(self.headers.get("Content-Length", "0")) | ||
| raw = self.rfile.read(length) | ||
| data = json.loads(raw.decode("utf-8")) | ||
| if not isinstance(data, dict): | ||
| raise ValueError("request body must be a JSON object") | ||
| return data |
There was a problem hiding this comment.
🛑 Crash Risk: Missing exception handling for json.loads and int conversion causes server to crash on malformed input. Line 141 will crash if Content-Length is not a valid integer, and line 143 will crash on invalid JSON, leaving the connection in a broken state and potentially causing the entire request handler thread to fail.
| def _read_json(self) -> dict[str, Any]: | |
| length = int(self.headers.get("Content-Length", "0")) | |
| raw = self.rfile.read(length) | |
| data = json.loads(raw.decode("utf-8")) | |
| if not isinstance(data, dict): | |
| raise ValueError("request body must be a JSON object") | |
| return data | |
| def _read_json(self) -> dict[str, Any]: | |
| try: | |
| length = int(self.headers.get("Content-Length", "0")) | |
| except ValueError: | |
| raise ValueError("invalid Content-Length header") | |
| raw = self.rfile.read(length) | |
| try: | |
| data = json.loads(raw.decode("utf-8")) | |
| except (json.JSONDecodeError, UnicodeDecodeError) as e: | |
| raise ValueError(f"invalid JSON: {e}") | |
| if not isinstance(data, dict): | |
| raise ValueError("request body must be a JSON object") | |
| return data |
|
Closing because this was opened against the wrong repository. The security triage is being redone against companyjupiter/quarkify. |
1 participant
Summary
This is a prerequisite draft PR for the security hardening issues #30-#34 because those issues target files that are not present on
mainyet.Validation
PYTHONPATH=. python3 -m unittest discover keyserver/testsenv -u GOROOT -u GOPATH /opt/homebrew/bin/go test ./...Notes
The security hardening changes are intentionally split into stacked PRs so each issue can be reviewed independently.