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

Specify jailmaker directory irrespective of script location #238

Closed
wants to merge 2 commits into from

Conversation

jonct
Copy link
Contributor

@jonct jonct commented Jul 17, 2024

Per request… decoupling the jailmaker directory/dataset path from wherever the script gets installed. And taking a stab at filling related blanks in the readme.

For now I'd be happy with just the env var mechanics.

Unfortunately you'd almost need to train people to use sudo -E? There are other solutions, sorting out the dotfiles under /root, but it's such a "maze of twisty passages all alike" that I'd imagine that would (or should) make people squeamish.

With that in mind, I've told it to also respect a ~/.local/share/jailmaker.conf in the .iniformat…

[DEFAULT]
jailmaker_dir = /mnt/pool/jailmaker

… which on the surface is pretty inconvenient. But it would suddenly make all the sense if you added a jlmkr path [newpath] command or similar. (Without the second argument it outputs the current path.) I'm not monkeying with command-line args here; I also didn't add a --dir/-D argument. But if there were such an argument IMO it should take precedence over these other sources of truth.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 17, 2024

When creating a PR please don't break the tests we have in place.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 17, 2024

In light of this change, which may encourage users to create and manage multiple jailmaker directories and switch between them, I forsee some issues.

Jail names should be unique. Even across multiple jailmaker dirs. You can't have multiple jails with the same name running simultaneously. The check_jail_name_available() can't check this, as it's unknown where all the jailmaker directories are.

Running jlmkr list will show running jails, even if not NOT started from the currently specified jailmaker dir. This may confuse the user in the following scenario. 2 jailmaker dirs with 2 unique jails and each a debian jail (but the one in A is bullseye and the one in B is bookworm):

/mnt/tank/jailmakerA/jails/debian
/mnt/tank/jailmakerA/jails/uniquejailA
/mnt/tank/jailmakerB/jails/debian
/mnt/tank/jailmakerA/jails/uniquejailB

If the user specifies jailmakerA and starts debian it will show up as running in jlmkr list besides the not running uniquejailA jail. If the user then specifies jailmakerB and runs jlmkr list again it will then show the running debian jail from jailmakerA alongside uniquejailB. So even with jailmakerB specified the user can execute commands inside a jail from jailmakerA with jlmkr shell. If the user then decides to remove this jail, running jlmkr remove won't remove the jail the user expects. If the user doesn't remove the jail, but instead stops the debian jail, then after stopping jlmkr list will show that the os is debian bookworm, whereas before stopping it listed debian bullseye. Just to show how mixed up things can get.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 17, 2024

Creating a fresh admin2 user on Dragonfish-24.04.2 results in a home directory with a .profile file which adds both ~/bin and ~/.local/bin to the PATH.

admin2@nas:~$ ls -la
total 41
drwx------ 3 admin2 admin2    6 Jul 17 11:30 .
drwxr-xr-x 7 root   root      7 Jul 17 11:26 ..
-rw-r--r-- 1 admin2 admin2  220 Jul 17 11:30 .bash_logout
-rw-r--r-- 1 admin2 admin2 3526 Jul 17 11:30 .bashrc
-rw-r--r-- 1 admin2 admin2  807 Jul 17 11:30 .profile
drwx------ 2 admin2 admin2    3 Jul 17 11:30 .ssh
admin2@nas:~$ cat .profile 
# ~/.profile: executed by the command interpreter for login shells.
# This file is not read by bash(1), if ~/.bash_profile or ~/.bash_login
# exists.
# see /usr/share/doc/bash/examples/startup-files for examples.
# the files are located in the bash-doc package.

# the default umask is set in /etc/profile; for setting the umask
# for ssh logins, install and configure the libpam-umask package.
#umask 022

# if running bash
if [ -n "$BASH_VERSION" ]; then
    # include .bashrc if it exists
    if [ -f "$HOME/.bashrc" ]; then
	. "$HOME/.bashrc"
    fi
fi

# set PATH so it includes user's private bin if it exists
if [ -d "$HOME/bin" ] ; then
    PATH="$HOME/bin:$PATH"
fi

# set PATH so it includes user's private bin if it exists
if [ -d "$HOME/.local/bin" ] ; then
    PATH="$HOME/.local/bin:$PATH"
fi

And I can confirm calling executables placed in ~/bin works when selecting bash as the Shell in TrueNAS Edit User settings. It doesn't work out of the box when selecting zsh.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 17, 2024

Unfortunately I don't think the install instructions you suggested work. Installing jlmkr inside ~/bin doesn't allow it to be called with sudo by this admin2 user.

admin2@scale:~$ pwd
/mnt/tank/home/admin2

admin2@scale:~$ ls -la
total 42
drwx------ 5 admin2 admin2    9 Jul 17 11:50 .
drwxr-xr-x 8 root   root      8 Jul 17 11:36 ..
-rw------- 1 admin2 admin2   89 Jul 17 11:50 .bash_history
-rw-r--r-- 1 admin2 admin2  220 Jul 17 11:30 .bash_logout
-rw-r--r-- 1 admin2 admin2 3526 Jul 17 11:30 .bashrc
drwxr-xr-x 3 admin2 admin2    3 Jul 17 11:50 .local
-rw-r--r-- 1 admin2 admin2  807 Jul 17 11:30 .profile
drwx------ 2 admin2 admin2    3 Jul 17 11:30 .ssh
drwxr-xr-x 2 admin2 admin2    3 Jul 17 11:56 bin

admin2@scale:~$ printenv
SHELL=/usr/bin/bash
PWD=/mnt/tank/home/admin2
LOGNAME=admin2
HOME=/mnt/tank/home/admin2
LS_COLORS=rs=0:di=01[...]
SSH_CONNECTION=192.168.1.3 53051 192.168.1.2 22
TERM=xterm-256color
USER=admin2
SHLVL=1
SSH_CLIENT=192.168.1.3 53051 22
PATH=/mnt/tank/home/admin2/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games
MAIL=/var/mail/admin2
SSH_TTY=/dev/pts/3
_=/usr/bin/printenv

admin2@scale:~$ jlmkr
usage: jlmkr [-h] [--version]  ...

admin2@scale:~$ sudo printenv
LS_COLORS=rs=0:di=01[...]
TERM=xterm-256color
PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin
MAIL=/var/mail/root
LOGNAME=root
USER=root
HOME=/root
SHELL=/usr/bin/bash
SUDO_COMMAND=/bin/printenv
SUDO_USER=admin2
SUDO_UID=9000
SUDO_GID=9000

admin2@scale:~$ sudo jlmkr 
sudo: jlmkr: command not found

admin2@scale:~$ JAILMAKER_DIR=/tmp/test printenv | grep JAILMAKER
JAILMAKER_DIR=/tmp/test

admin2@scale:~$ sudo JAILMAKER_DIR=/tmp/test printenv | grep JAILMAKER
JAILMAKER_DIR=/tmp/test

Due to using sudo the jlmkr executable inside the admin2 ~/bin directory will no longer be found as the PATH changes. But interestingly enough environment variables do get carried over after sudo. This means I could do:

sudo PATH="$PATH" env jlmkr

Now it will run jlmkr installed in /mnt/tank/home/admin2/bin as root when invoked with sudo by admin2. It will even respect the JAILMAKER_DIR env var.

sudo PATH="$PATH" JAILMAKER_DIR=/tmp/test env jlmkr

But it's quite complicated...

@jonct
Copy link
Contributor Author

jonct commented Jul 17, 2024

When creating a PR please don't break the tests we have in place.

Well, that's frustrating. You just threw away the client-side tooling that we'd use to preflight all aspects of code quality — and could ultimately enforce through a commit hook.

@jonct
Copy link
Contributor Author

jonct commented Jul 17, 2024

In light of this change, which may encourage users to create and manage multiple jailmaker directories and switch between them, I forsee some issues.

I should probably question the premise here. 😬 As I see it, the tool "encourages" a single jailmaker directory, as does its documentation. Beyond that, neither should get in the user's way more than necessary.

I agree that it is possible to trip over any named machines which the user creates outside of the currently declared jailmaker directory. But I'll remind you that this is also the current status quo. 🧐

As you were right to caveat: the rest is a balance of anecdotal probabilities. 🤷

check_jail_name_available() can't check this, as it's unknown where all the jailmaker directories are.

I think it can. Your principle of No lock in states that after using jlmkr to create the jail, you should be able to throw jlmkr away and carry on directly with the built-in tools.

For me, what naturally follows is that we should implement based on hard constraints imposed or encountered by the system, and avoid conflating that with any new conventions suggested by the tool and its documentation.

So to avoid your delayed discovery scenario: jlmkr should inform the system of any jails that it creates, and check_jail_name_available should consult the system for any concerning conflicts in its namespace. Whether that's through machinectl list/list-images or walking the system directories, or… ? 🧠

Until such a change: the designated jailmaker directory is inherently just a slightly fragile convention — as it's always been.

@jonct
Copy link
Contributor Author

jonct commented Jul 17, 2024

Creating a fresh admin2 user on Dragonfish-24.04.2 results in a home directory with a .profile file which adds both ~/bin and ~/.local/bin to the PATH.

COOL! 😍 I'd been mulling another pestering Jira.

It doesn't work out of the box when selecting zsh.

OK, so I think that just means changing "is not/will not" to "may not have/might not".

IMO preestablished accounts (and fish, ksh, tcsh…) suggest that we shouldn't remove the general advisory. But it doesn't necessarily need to be spelled out so fully in this particular readme. More could be offloaded to a wiki or external webpage, where there's room to address more edge cases and options.

If we cover the topic concisely and effectively, maybe that should be the Jira.

@jonct
Copy link
Contributor Author

jonct commented Jul 17, 2024

Due to using sudo the jlmkr executable inside the admin2 ~/bin directory will no longer be found as the PATH changes.

Oh, wow. Yeah, for all my testing I'd still been invoking ./jlmkr inside the project directory. 🤦 What to do… what to do…

@Lockszmith-GH
Copy link
Contributor

Lockszmith-GH commented Jul 17, 2024

Moving commnet to #239

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 18, 2024

My bad, I shouldn't have asked you to work on this but I got carried away. All in all this reminds me that building something like this for SCALE means making compromises. Not all the sysadmin best practices apply here. Anyhow I shall keep the jailmaker executable inside the jailmaker directory. I don't want to migrate users from one workaround to other workarounds and I don't want to involve more locations or reintroduce an install command which was removed in v2. Especially not in light of the announcement that I shall stop maintaining jailmaker after October.

@Jip-Hop Jip-Hop closed this Jul 18, 2024
@jonct
Copy link
Contributor Author

jonct commented Jul 18, 2024

Does this have to be either/or?

It seems we've cleanly solved having Original Recipe be the default, with no penalties from allowing undocumented override to those who need it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants