Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add privilege check to render-template script #1728

Merged
merged 11 commits into from
Jan 31, 2024

Conversation

jdeanwallace
Copy link
Contributor

@jdeanwallace jdeanwallace commented Jan 25, 2024

Resolves https://github.com/tiny-pilot/tinypilot-pro/issues/1190

Please refer to https://github.com/tiny-pilot/tinypilot-pro/issues/1190#issue-2094765706 for PR rationale.

Executing the render-template script with root privileges with now fail. For example, as part of the TinyPilot installation:

$ apt-get install -y ./tinypilot_20240128105411_armhf.deb
...
Preparing to unpack .../tinypilot_20240128105411_armhf.deb ...
Unpacking tinypilot (20240128105411) over (20240128094116) ...
Setting up tinypilot (20240128105411) ...
Warning: The home dir /home/tinypilot you specified already exists.
The system user `tinypilot' already exists. Exiting.
/opt/tinypilot /

This script doesn't require root privileges.
Please re-run as tinypilot:
  runuser tinypilot --command './scripts/render-template'

dpkg: error processing package tinypilot (--configure):
 installed tinypilot package post-installation script subprocess returned error exit status 1
Errors were encountered while processing:
 tinypilot
E: Sub-process /usr/bin/dpkg returned an error code (1)
+ clean_up
+ umount --lazy /mnt/tinypilot-installer
+ rm -rf /opt/tinypilot-updater /mnt/tinypilot-installer

Notes

  1. We had to expand the previously used render-template command into multiple independent commands because the exit code was being ignored/swallowed.

  2. To execute render-template as the tinypilot user, we use the runuser command instead of su. Based on the runuser manual, runuser can only be used by root users which is currently the only way we use render-template:

    The difference between the commands runuser and su is that runuser does not ask
    for a password (because it may be executed by the root user only)
    and it uses a different PAM configuration.

Review on CodeApprove

@jotaen4tinypilot
Copy link
Contributor

Fyi, the e2e tests are green again on master.

@jdeanwallace jdeanwallace marked this pull request as ready for review January 28, 2024 11:31
Copy link
Contributor Author

Automated comment from CodeApprove ➜

@jotaen4tinypilot please review this Pull Request

Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated comment from CodeApprove ➜

⏳ Approval Pending (2 unresolved comments)
Approval will be granted automatically when all comments are resolved


In: scripts/render-template:
Shall we put a comment here for future reference? I’m thinking, we could just add the link to https://github.com/tiny-pilot/tinypilot-pro/issues/1190.


In: scripts/render-template:

> Line 30
if os.geteuid() == 0:

Question: I’m wondering what the difference is between putting this check here (at file top level) vs. putting it into main? Intuitively, I might have put it into main (although it probably also doesn’t matter that much, though).


👀 @jdeanwallace it's your turn please take a look

Copy link
Contributor Author

@jdeanwallace jdeanwallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated comment from CodeApprove ➜

In: scripts/render-template:
Oh that's an interesting thought 🤔 Like you said, it probably doesn't matter because the program is always only run as the main program, but then technically it should then be in the main function. Furthermore, the main function doesn't deal with parsing args, so perhaps it should rather be within the if __name__ == '__main__' block where we parse the other (non-existent) args? I went for the latter for now.


In: scripts/render-template:
Ah yes, done.

Copy link
Contributor

@jotaen4tinypilot jotaen4tinypilot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated comment from CodeApprove ➜

Approved: I have approved this change on CodeApprove and all of my comments have been resolved.

@jotaen4tinypilot jotaen4tinypilot dismissed their stale review January 30, 2024 08:21

Review approved on CodeApprove

@jdeanwallace jdeanwallace merged commit 6f6bc42 into master Jan 31, 2024
13 checks passed
@jdeanwallace jdeanwallace deleted the render-template-privileges branch January 31, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants