[backport/1.4] core: move setup_ssh() over to startup_blueos_update#3898
Open
patrickelectric wants to merge 1 commit intobluerobotics:1.4-devfrom
Open
[backport/1.4] core: move setup_ssh() over to startup_blueos_update#3898patrickelectric wants to merge 1 commit intobluerobotics:1.4-devfrom
patrickelectric wants to merge 1 commit intobluerobotics:1.4-devfrom
Conversation
Reviewer's GuideBackports automatic SSH key generation and authorized_keys setup into the BlueOS startup update script, and wires it into the script entrypoint so SSH is configured on startup. Sequence diagram for BlueOS startup SSH setup and main executionsequenceDiagram
actor System
participant PythonInterpreter as Python_interpreter
participant Module as blueos_startup_update
participant SSH as setup_ssh
participant Main as main
System->>PythonInterpreter: Invoke blueos_startup_update.py
PythonInterpreter->>Module: Load module
PythonInterpreter->>SSH: Call setup_ssh
SSH-->>PythonInterpreter: bool (success or failure)
PythonInterpreter->>Module: Log error if setup_ssh raised
PythonInterpreter->>Main: Call main
Main-->>PythonInterpreter: int exit_code
PythonInterpreter-->>System: Exit with code
Flow diagram for setup_ssh logic in BlueOS startup updateflowchart TD
A["Start setup_ssh"] --> B["Log 'Setting up SSH...'"]
B --> C["Set key_path to /root/.config/.ssh"]
C --> D["Set private_key to key_path/id_rsa"]
D --> E["Set public_key to private_key with .pub suffix"]
E --> F["Read SSH_USER or default pi"]
F --> G["Read USER_GID or default 1000 and convert to int"]
G --> H["Read USER_UID or default 1000 and convert to int"]
H --> I["Set authorized_keys path to /home/user/.ssh/authorized_keys"]
I --> J["Create key_path directory (parents, exist_ok)"]
J --> K{"Does public_key file exist?"}
K -- "No" --> L["Run ssh-keygen to create RSA key pair"]
L --> M["Log 'Generated new SSH key pair'"]
K -- "Yes" --> N["Skip key generation"]
M --> O["Read public_key text (UTF-8)"]
N --> O
O --> P{"Can read authorized_keys?"}
P -- "Yes" --> Q["Read authorized_keys text (UTF-8)"]
P -- "FileNotFoundError" --> R["Log missing authorized_keys file"]
R --> S["Set authorized_keys_text to empty string"]
Q --> T{"Does authorized_keys_text contain public_key_text?"}
S --> T
T -- "No" --> U{"Does authorized_keys_text end with newline?"}
U -- "No" --> V["Append newline to authorized_keys_text"]
U -- "Yes" --> W["Skip newline append"]
V --> X["Append public_key_text to authorized_keys_text"]
W --> X
X --> Y["Write authorized_keys_text back to authorized_keys (UTF-8)"]
Y --> Z["Log 'Added public key to authorized_keys'"]
T -- "Yes" --> AA["Skip updating authorized_keys"]
Z --> AB["Change owner of authorized_keys to uid,gid"]
AA --> AB
AB --> AC["Set permissions of authorized_keys to 600"]
AC --> AD["Return True"]
subgraph ErrorHandling
AE["Any Exception raised"] --> AF["Log 'Error setting up ssh:' with exception"]
AF --> AG["Return False"]
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider ensuring the parent directory for
authorized_keys(e.g./home/<user>/.ssh) exists and has appropriate permissions before writing the file, otherwiseauthorized_keys.write_text()andos.chown()may fail on a fresh system. - The broad
except Exceptioninsidesetup_ssh()combined with another broadtry/exceptaroundsetup_ssh()in__main__is redundant; you could either letsetup_ssh()raise and handle it at the call site, or handle/log everything insidesetup_ssh()and remove the outer try/except. - Since
setup_ssh()returns a boolean to indicate success, consider using that return value in__main__(or removing the return type) to avoid suggesting callers can rely on a success/failure signal that is currently ignored.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider ensuring the parent directory for `authorized_keys` (e.g. `/home/<user>/.ssh`) exists and has appropriate permissions before writing the file, otherwise `authorized_keys.write_text()` and `os.chown()` may fail on a fresh system.
- The broad `except Exception` inside `setup_ssh()` combined with another broad `try/except` around `setup_ssh()` in `__main__` is redundant; you could either let `setup_ssh()` raise and handle it at the call site, or handle/log everything inside `setup_ssh()` and remove the outer try/except.
- Since `setup_ssh()` returns a boolean to indicate success, consider using that return value in `__main__` (or removing the return type) to avoid suggesting callers can rely on a success/failure signal that is currently ignored.
## Individual Comments
### Comment 1
<location path="core/tools/blueos_startup_update/blueos_startup_update.py" line_range="678" />
<code_context>
+ user = os.environ.get("SSH_USER", "pi")
+ gid = int(os.environ.get("USER_GID", 1000))
+ uid = int(os.environ.get("USER_UID", 1000))
+ authorized_keys = Path(f"/home/{user}/.ssh/authorized_keys")
+
+ try:
</code_context>
<issue_to_address>
**issue:** Writing to `authorized_keys` assumes the parent `.ssh` directory exists, which can cause failures on fresh systems.
On a fresh setup `/home/{user}/.ssh` may not exist, so this will raise `FileNotFoundError` despite the missing file handling above. Consider creating the parent directory first with `authorized_keys.parent.mkdir(parents=True, exist_ok=True)` (and then applying the appropriate `chown`/`chmod`) before writing the file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| user = os.environ.get("SSH_USER", "pi") | ||
| gid = int(os.environ.get("USER_GID", 1000)) | ||
| uid = int(os.environ.get("USER_UID", 1000)) | ||
| authorized_keys = Path(f"/home/{user}/.ssh/authorized_keys") |
There was a problem hiding this comment.
issue: Writing to authorized_keys assumes the parent .ssh directory exists, which can cause failures on fresh systems.
On a fresh setup /home/{user}/.ssh may not exist, so this will raise FileNotFoundError despite the missing file handling above. Consider creating the parent directory first with authorized_keys.parent.mkdir(parents=True, exist_ok=True) (and then applying the appropriate chown/chmod) before writing the file.
Member
Author
|
I would hold this PR until we do the stable release for RadCam. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a backport of #3050
Summary by Sourcery
New Features: