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

Tweak how ssh user is copied in usr/share/rear/rescue/default/500_ssh.sh #1489

Merged
merged 2 commits into from Sep 14, 2017

Conversation

N3WWN
Copy link
Contributor

@N3WWN N3WWN commented Sep 12, 2017

This PR was originally included in my YUM PR, but I at the request of @jsmeix , I pulled it out and am submitting it separately.

In my testing, I would see multiple passwd entries created if the string 'ssh' occurred in more than just the sshd daemon user's entry. While this didn't actually break anything, these tweaks should make it cleaner.

Copy link
Member

@schlomo schlomo left a comment

Choose a reason for hiding this comment

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

Seems OK, could be improved to make it more consistent with the other recent developments.

I also noticed that we check for $1$ in the ssh password which is probably not working any more. Probably better to check for $?$ or so to catch all encryption formats.

@@ -24,10 +24,14 @@ if has_binary sshd; then
Log "Adding required libfreeblpriv3.so to LIBS"

# copy ssh user
if PASSWD_SSH=$(grep ssh /etc/passwd) ; then
PASSWD_SSH=$(getent passwd ssh sshd)
Copy link
Member

Choose a reason for hiding this comment

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

Can it happen that you get two entries back? Will the code still work?

Copy link
Member

Choose a reason for hiding this comment

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

As fas as I see the

IFS=: read user ex uid gid gecos homedir junk <<<"$PASSWD_SSH"

ensures that only the first "getent passwd ssh sshd"
entry gets actually used.

Copy link
Member

Choose a reason for hiding this comment

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

@N3WWN
perhaps an explanatory comment why it works
if "getent passwd ssh sshd" results two entries
could avoid future doubts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsmeix
Explanatory comment added as well as searching for 'sshd' account before 'ssh' account.

I can't provide any examples where the account is named 'ssh', but that's what ReaR was looking for previously. Instead of breaking the account search for distros that I'm not familiar with by changing 'ssh' to 'sshd', I search for 'sshd' (which is the username on every system that I've checked) first and then ssh.

This also allows this code to work on systems that have 'sshd' as the system account and a non-system or secondary 'ssh' account.

IFS=: read user ex uid gid gecos homedir junk <<<"$PASSWD_SSH"
# skip if this user exists already in the restore system
if ! egrep -q "^$user:" $TARGET_FS_ROOT/etc/passwd ; then
Copy link
Member

Choose a reason for hiding this comment

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

Why not add it to CLONE_USERS similar to the ssh group?

Copy link
Member

Choose a reason for hiding this comment

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

I think the purpose of this 500_ssh.sh script should be kept
separated from the CLONE_USERS functionality
what I call "KSIS" Keep Separated Issues Separarated
(cf. RFC 1925 item 5 ;-)

…ssh user. Include comments on what happens if getent returns more than one entry.
@jsmeix jsmeix merged commit dd0316a into rear:master Sep 14, 2017
@jsmeix jsmeix self-assigned this Sep 14, 2017
@jsmeix jsmeix added enhancement Adaptions and new features fixed / solved / done labels Sep 14, 2017
@jsmeix jsmeix added this to the ReaR v2.3 milestone Sep 14, 2017
@jsmeix
Copy link
Member

jsmeix commented Sep 14, 2017

@schlomo in
#1489 (review)
you wrote that in rescue/default/500_ssh.sh

... we check for $1$ in the ssh password
which is probably not working any more.
Probably better to check for $?$ or so
to catch all encryption formats.

I am afraid I do not understand what that means.
In particular I do not understand what '$1$' or '$?$' stands for.
Apparently this seems to be really basic knowledge
that everybody "just knows" - except me :-(
and no comment explains what that code is about :-((
My blind guess is that '$1$' is perhaps meant to detect
if $SSH_ROOT_PASSWORD is already encrypted but
e.g. my SLES11 /etc/shadow password entries look like

...:$2y$...
...:$2a$...

so that neither '$1$' nor '$?$' seem to match - but I could be
totally wrong because I do not understand what goes on here.

@N3WWN N3WWN deleted the tweak-500_ssh.sh branch September 14, 2017 13:26
@N3WWN
Copy link
Contributor Author

N3WWN commented Sep 14, 2017

@jsmeix
Looks like your SLES11 has at least one more encryption method (or variant) than my CentOS7 box 😁

From crypt(3):

   The glibc2 version of this function supports additional encryption algorithms.

   If salt is a character string starting with the characters "$id$" followed by a string terminated by "$":

          $id$salt$encrypted

   then instead of using the DES machine, id identifies the encryption method used and this then determines how the rest of the password string is interpreted.  The following values of id are supported:

          ID  | Method
          ─────────────────────────────────────────────────────────
          1   | MD5
          2a  | Blowfish (not in mainline glibc; added in some
              | Linux distributions)
          5   | SHA-256 (since glibc 2.7)
          6   | SHA-512 (since glibc 2.7)

   So $5$salt$encrypted is an SHA-256 encoded password and $6$salt$encrypted is an SHA-512 encoded one.

   "salt" stands for the up to 16 characters following "$id$" in the salt.  The encrypted part of the password string is the actual computed password.  The size of this string is fixed:

   MD5     | 22 characters
   SHA-256 | 43 characters
   SHA-512 | 86 characters

   The characters in "salt" and "encrypted" are drawn from the set [a–zA–Z0–9./].  In the MD5 and SHA implementations the entire key is significant (instead of only the first 8 bytes in DES).

If we have extglob enabled (i.e. shopt -s extglob), I can match DES, MD5, Blowfish and SHA with the following:

# Set the SSH root password; if pw is hashed just copy it otherwise use openssl (for backward compatibility)
if [[ "$SSH_ROOT_PASSWORD" ]] ; then
    case "$SSH_ROOT_PASSWORD" in
    \$[0-9]?([a-z])\$*) echo "ALREADY ENC:  root:$SSH_ROOT_PASSWORD:::::::" ;;
    *     ) echo "NOT ENC:  root:$(echo $SSH_ROOT_PASSWORD | openssl passwd -1 -stdin):::::::" ;;
    esac
fi

I don't see any format restrictions, but going off the of what we know (single digit or single digit+single char), I tested the following successfully:

  1. SSH_ROOT_PASSWORD='unencryptedpasswd' # pass -> detected as unencrypted
  2. SSH_ROOT_PASSWORD='$1$encryptedpasswd' # pass -> detected as encrypted
  3. SSH_ROOT_PASSWORD='$6$encryptedpasswd' # pass -> detected as encrypted
  4. SSH_ROOT_PASSWORD='$2a$encryptedpasswd' # pass -> detected as encrypted
  5. SSH_ROOT_PASSWORD='$2az$encryptedpasswd' # pass -> detected as unencrypted because of the extra char in the crypt ID

This may not adhere to the coding standards of the project as I just whipped up this test, but it may serve as a good start. We could add a comment directing folks to check out the crypt(3) man page, too.

@jsmeix
Copy link
Member

jsmeix commented Sep 15, 2017

@N3WWN
many thanks for your explanation in
#1489 (comment)

If you like could you do another pull request to enhance it
so that it also works for more encryption methods?

@jsmeix
Copy link
Member

jsmeix commented Sep 15, 2017

@N3WWN
regarding "If we have extglob enabled" see

$ find usr/sbin/rear usr/share/rear/ | xargs grep 'extglob'
usr/sbin/rear:# The extglob shell option enables several extended pattern matching operators.
usr/sbin/rear:shopt -s nullglob extglob

i.e. ReaR has extglob enabled
and nullglob which requires to be careful,
e.g. see my commenta bout 'nullglob' in
prep/NETFS/default/070_set_backup_archive.sh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adaptions and new features fixed / solved / done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants