Potential fix for code scanning alert no. 1: Uncontrolled command line#10
Potential fix for code scanning alert no. 1: Uncontrolled command line#10SimoleS wants to merge 1 commit into
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR adds regex-based input validation to ChangesSSH Signer Input Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/app/services/ssh_signer.py`:
- Around line 31-32: Add an upper-bound TTL check alongside the existing
positive check in the function that validates ttl_seconds (the block that raises
SSHSigningError("ttl_seconds must be positive")), e.g., enforce ttl_seconds <=
31536000 (1 year) or a configurable MAX_TTL_SECONDS constant; if the value
exceeds the limit, raise SSHSigningError with a clear message referencing the
limit so callers know why it failed. Ensure to define or reference a
MAX_TTL_SECONDS constant near the validation and update any related input docs
or error handling to reflect the new bound.
- Around line 36-40: The error thrown when validating principals (in the loop
over principals that builds validated_principals and uses _SAFE_PRINCIPAL_RE) is
too generic; update the SSHSigningError to include a clearer message that
identifies the failing principal (if safe) or at least the expected format/regex
so users can debug (e.g., "invalid ssh principal: '<principal>'" or "invalid ssh
principal format — must match <pattern>"); modify the raise inside the loop to
reference the current principal and/or _SAFE_PRINCIPAL_RE so the message is
descriptive while avoiding logging sensitive data.
- Around line 13-14: The regexes _SAFE_PRINCIPAL_RE and _SAFE_KEY_ID_RE
currently restrict characters used for ssh_principal (passed to ssh-keygen -n)
and key_id (passed to ssh-keygen -I) but do not guarantee that request_id in the
incoming payload matches the allowed charset, which can cause "invalid key_id
format" failures; update the code to validate request_id early (before building
key_id) against the same safe character set used by _SAFE_KEY_ID_RE (or
constrain/normalize request_id to that set) and/or update the regexes for
clarity (e.g., move or escape '-' in _SAFE_PRINCIPAL_RE and _SAFE_KEY_ID_RE) and
add a concise comment/docstring explaining the expected format for ssh_principal
and request_id so callers know the allowed characters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35bc1855-4c81-4b6b-9114-e8b8b4a27643
📒 Files selected for processing (1)
platform/app/services/ssh_signer.py
| _SAFE_PRINCIPAL_RE = re.compile(r"^[A-Za-z0-9._@:-]+$") | ||
| _SAFE_KEY_ID_RE = re.compile(r"^[A-Za-z0-9._@:=;-]+$") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for SSH principal and key_id usage patterns in tests or docs
# Look for test files that might show example principals/key_ids
rg -n -C3 'sign_user_certificate|principals.*=|key_id.*=' --type=py -g '!**/ssh_signer.py'
# Search for any SSH documentation
fd -e md -e txt -e rst | xargs rg -l -i 'ssh.*principal|key.*id' 2>/dev/null || echo "No docs found"Repository: PlayUPSK/Keyward
Length of output: 7198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect ssh_signer.py around the regex definitions and where they’re used
sed -n '1,120p' platform/app/services/ssh_signer.py
echo "---- usages ----"
rg -n "_SAFE_PRINCIPAL_RE|_SAFE_KEY_ID_RE" platform/app/services/ssh_signer.py
# 2) Look for principal/key_id examples in code/docs beyond the earlier search
echo "---- ssh_principal usage examples ----"
rg -n "ssh_principal|cert_key_id|key_id=" platform -S
echo "---- docs search (broader) ----"
rg -n -i "ssh.*principal|principal.*ssh|cert_key_id|key id|key_id" README.md platform -S || trueRepository: PlayUPSK/Keyward
Length of output: 11460
🌐 Web query:
ssh-keygen -n principals allowed characters comma separated list certificate
💡 Result:
When using the ssh-keygen command to sign a certificate, the -n flag allows you to specify one or more principals (user or host names) to be included in the certificate [1][2]. Multiple principals must be provided as a comma-separated list [1][3]. Regarding formatting and allowed characters: - Comma-Separated List: You should provide the list as a single string of comma-separated values without spaces between them (e.g., -n principal1,principal2,principal3) [1][3]. - Allowed Characters: While there is no rigid, single character-set specification in the OpenSSH manual, principals are treated as strings representing identities (users or hosts). For interoperability and to avoid parsing issues, it is strongly recommended to stick to standard alphanumeric characters, underscores, hyphens, and dots. Avoid using commas (except as the separator), spaces, or shell-metacharacters that might be misinterpreted by the SSH client or server during processing. Principals are ultimately matched against patterns defined on the server side in either an authorized_principals file (specified by AuthorizedPrincipalsFile in sshd_config) or directly within an authorized_keys file [1][3][4]. Because these files support pattern matching (including wildcards like *), using characters that have special meaning in glob patterns or regex could lead to unexpected behavior [1][2][3]. It is best practice to keep principal names simple and predictable [3].
Citations:
- 1: https://man7.org/linux/man-pages/man1/ssh-keygen.1.html
- 2: https://man.archlinux.org/man/ssh-keygen.1
- 3: https://skife.org/b4/ssh-certificates-notes/
- 4: https://www.ssh.com/academy/ssh/authorized-keys-openssh
🌐 Web query:
ssh-keygen -I key ID allowed characters format certificate
💡 Result:
In OpenSSH, the ssh-keygen “-I key_id” argument is a free-form “key identifier” for certificates. OpenSSH’s certificate protocol specifies that the certificate’s “key id” is a free-form text field filled in by the CA at signing time; it’s used for identification and is logged in server logs. [1][2] Allowed characters / restrictions: the sources above explicitly describe key_id/key id as free-form text and do not define a character whitelist or a specific character-format constraint in the documented interface. [1][2] Therefore, there is no authoritative published “allowed characters” set for “-I key ID”; any practical restrictions would come from parsing/quoting and any downstream log/format usage, not from a documented character whitelist. [1][2] Practical note: treat key_id as an arbitrary string suitable for the shell/command line you provide, but do not assume it will be validated against a strict regex like “alnum + dash + underscore” because OpenSSH documents it as free-form. [1][2] Answering your exact query terms: - “ssh-keygen -I key ID allowed characters”: none are specified by OpenSSH docs/protocol; it is free-form. [1][2] - “format certificate”: -I is used when signing to set the certificate’s key identifier field; the underlying certificate format includes “string key id” as a string field. [2][1]
Citations:
- 1: https://github.com/openssh/openssh-portable/blob/0075511e27e5394faa28edca02bfbf13b9a6693e/PROTOCOL.certkeys
- 2: https://man7.org/linux/man-pages/man1/ssh-keygen.1.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show README section around the "SSH principal" mention
rg -n "SSH principal" README.md
python3 - <<'PY'
import re, pathlib
p=pathlib.Path("README.md")
lines=p.read_text(encoding="utf-8").splitlines()
for i,l in enumerate(lines,1):
if "SSH principal" in l:
start=max(1,i-10); end=min(len(lines), i+40)
for j in range(start,end+1):
print(f"{j}:{lines[j-1]}")
print("----")
PY
# Show admin form input placeholder for ssh_principal (may hint allowed examples)
rg -n "name=\"ssh_principal\"|SSH Principal" platform/app/templates/admin/policies.html
sed -n '560,640p' platform/app/templates/admin/policies.htmlRepository: PlayUPSK/Keyward
Length of output: 5951
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,220p' platform/app/services/ssh_ca.py
sed -n '1,120p' platform/app/services/policy_engine.py
sed -n '180,260p' platform/app/services/policy_engine.pyRepository: PlayUPSK/Keyward
Length of output: 8962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect where request_id is validated/constructed in ssh_ca and related flows
rg -n "request_id" platform/app/services/ssh_ca.py platform/app -S
rg -n "payload\\[\"request_id\"\\]|\\brequest_id\\b" platform/app/services/ssh_ca.py -n
sed -n '1,120p' platform/app/services/ssh_ca.py
sed -n '120,260p' platform/app/services/ssh_ca.pyRepository: PlayUPSK/Keyward
Length of output: 12389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "request_id" platform/tests platform/app -SRepository: PlayUPSK/Keyward
Length of output: 2178
Document and align the SSH principal/key_id validation with expected inputs
_SAFE_PRINCIPAL_REis applied viafullmatchtopayload["ssh_principal"]before passing it tossh-keygen -nas a single principal (comma-separated list is handled by the code). The whitelist blocks commas/spaces/metacharacters and matches the repo’s documented/UI examples (alice,deploy,root,ubuntu)._SAFE_KEY_ID_REis applied viafullmatchtokey_idpassed tossh-keygen -I. Sincekey_idis built asrequest=<request_id>;user=<uuid>;device=<uuid>;server=<uuid>, the pattern’s allowance for=,;,-, etc. is necessary and currently covers that format. Howeverrequest_idis otherwise free-form in the request payload, so any unexpected characters will triggerinvalid key_id formatand fail issuance—validaterequest_idearlier to the same character set or document the required format.- Optional clarity: the trailing
-in[A-Za-z0-9._@:-]/[A-Za-z0-9._@:=;-]already behaves as a literal hyphen, but moving-to the start/escaping it would make the intent clearer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/app/services/ssh_signer.py` around lines 13 - 14, The regexes
_SAFE_PRINCIPAL_RE and _SAFE_KEY_ID_RE currently restrict characters used for
ssh_principal (passed to ssh-keygen -n) and key_id (passed to ssh-keygen -I) but
do not guarantee that request_id in the incoming payload matches the allowed
charset, which can cause "invalid key_id format" failures; update the code to
validate request_id early (before building key_id) against the same safe
character set used by _SAFE_KEY_ID_RE (or constrain/normalize request_id to that
set) and/or update the regexes for clarity (e.g., move or escape '-' in
_SAFE_PRINCIPAL_RE and _SAFE_KEY_ID_RE) and add a concise comment/docstring
explaining the expected format for ssh_principal and request_id so callers know
the allowed characters.
| if ttl_seconds <= 0: | ||
| raise SSHSigningError("ttl_seconds must be positive") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
TTL validation looks good; consider adding an upper bound.
The positive check prevents invalid TTL values. You might also consider adding an upper bound to prevent unexpectedly long-lived certificates (e.g., ttl_seconds > 31536000 for certificates longer than 1 year), depending on your security policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/app/services/ssh_signer.py` around lines 31 - 32, Add an upper-bound
TTL check alongside the existing positive check in the function that validates
ttl_seconds (the block that raises SSHSigningError("ttl_seconds must be
positive")), e.g., enforce ttl_seconds <= 31536000 (1 year) or a configurable
MAX_TTL_SECONDS constant; if the value exceeds the limit, raise SSHSigningError
with a clear message referencing the limit so callers know why it failed. Ensure
to define or reference a MAX_TTL_SECONDS constant near the validation and update
any related input docs or error handling to reflect the new bound.
| validated_principals: list[str] = [] | ||
| for principal in principals: | ||
| if not principal or not _SAFE_PRINCIPAL_RE.fullmatch(principal): | ||
| raise SSHSigningError("invalid ssh principal format") | ||
| validated_principals.append(principal) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider more descriptive error message.
The error "invalid ssh principal format" doesn't indicate which principal failed or what format is expected. This could make debugging difficult for users.
📝 Suggested improvement for error clarity
validated_principals: list[str] = []
for principal in principals:
if not principal or not _SAFE_PRINCIPAL_RE.fullmatch(principal):
- raise SSHSigningError("invalid ssh principal format")
+ raise SSHSigningError(
+ f"invalid ssh principal format: {principal!r}. "
+ "Principals must contain only alphanumeric, dot, underscore, @, colon, and hyphen."
+ )
validated_principals.append(principal)Note: Only include the actual principal value in the error if it's safe to log (i.e., doesn't contain sensitive data). Alternatively, just improve the general error message without including the value.
📝 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.
| validated_principals: list[str] = [] | |
| for principal in principals: | |
| if not principal or not _SAFE_PRINCIPAL_RE.fullmatch(principal): | |
| raise SSHSigningError("invalid ssh principal format") | |
| validated_principals.append(principal) | |
| validated_principals: list[str] = [] | |
| for principal in principals: | |
| if not principal or not _SAFE_PRINCIPAL_RE.fullmatch(principal): | |
| raise SSHSigningError( | |
| f"invalid ssh principal format: {principal!r}. " | |
| "Principals must contain only alphanumeric, dot, underscore, @, colon, and hyphen." | |
| ) | |
| validated_principals.append(principal) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/app/services/ssh_signer.py` around lines 36 - 40, The error thrown
when validating principals (in the loop over principals that builds
validated_principals and uses _SAFE_PRINCIPAL_RE) is too generic; update the
SSHSigningError to include a clearer message that identifies the failing
principal (if safe) or at least the expected format/regex so users can debug
(e.g., "invalid ssh principal: '<principal>'" or "invalid ssh principal format —
must match <pattern>"); modify the raise inside the loop to reference the
current principal and/or _SAFE_PRINCIPAL_RE so the message is descriptive while
avoiding logging sensitive data.
1 participant
Potential fix for https://github.com/PlayUPSK/Keyward/security/code-scanning/1
The best fix is to validate and constrain all untrusted values that become
ssh-keygenarguments, especiallyprincipals,key_id, andttl_seconds, before constructingcmd. Keep the hardcoded binary and list-stylesubprocess.run, but reject unsafe characters and empty/invalid values. This preserves behavior for valid inputs while blocking malicious or malformed inputs.Implement in
platform/app/services/ssh_signer.pyinsidesign_user_certificate:ttl_seconds> 0[A-Za-z0-9._@:-]+)key_id: non-empty and limited to safe printable chars used by your existing format (allow= ; _ - . @ :and alnum)SSHSigningErroron invalid input.-n.No functional change for normal valid requests; only unsafe values are rejected early.
Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by CodeRabbit