MariaDB / mariadb-docker

Docker Official Image packaging for MariaDB
https://mariadb.org
GNU General Public License v2.0
759 stars 438 forks source link

added password_2_hash script #463

Open TheAlgorythm opened 1 year ago

TheAlgorythm commented 1 year ago

I added it as script like discussed in #458.

TheAlgorythm commented 1 year ago

The same CI failures again.

grooverdan commented 1 year ago

I was on leave for a bit so sorry for the delay. Dockerfile missing COPY which is an easy fix. @fauust what do you think of the script as a password generator?

fauust commented 1 year ago

I was on leave for a bit so sorry for the delay. Dockerfile missing COPY which is an easy fix. @fauust what do you think of the script as a password generator?

After reading https://github.com/MariaDB/mariadb-docker/issues/458 and https://github.com/docker-library/docs/pull/2203 I still don't understand how/where this script would be called.

Bash-wise, and since it's a container environment, I would suggest you to check that openssl command is there with something like:

command -v openssl >/dev/null || {
  echo "openssl command not found"
  exit 1
}

I don't see a reason why openssl would not be in PATH nor installed but it costs nothing and it make the script more robust.

Also, instead of awk you could use bash parameter expansion ${var^^}, but I have no strong opinion on which is better here.

Finally, but again, I am absolutely not sure this is a pb since I don't understand how this works, what would happen with https://mariadb.com/kb/en/authentication-plugin-ed25519/, see https://mariadb.com/kb/en/password/:

For example, when used in conjunction with a user authenticated by the ed25519 plugin, the statement will create a longer hash:

TheAlgorythm commented 1 year ago

Bash-wise, and since it's a container environment, I would suggest you to check that openssl command is there

Did it. 👍

Also, instead of awk you could use bash parameter expansion ${var^^}, but I have no strong opinion on which is better here.

I used awk as I can pipe it directly into it and don't have to think about shell escaping. But I'm no expert in bash and therefore can't reason about if the parameter expansion works without problems. If I should change it to the bash parameter expansion I can do it. Well, as it hex it should be probably okay.

Finally, but again, I am absolutely not sure this is a pb since I don't understand how this works, what would happen with https://mariadb.com/kb/en/authentication-plugin-ed25519/, see https://mariadb.com/kb/en/password/:

This script creates hashes for the old SHA-1 based scheme. If the newer ed25519 scheme is used another script has to be created. But currently this project uses only the old scheme. In order to use the new scheme this would be the smallest change as the following changes are required:

  1. Make sure that the plugin is part of the container
  2. Provide an environment flag for the configuration to switch between the hashing schemes for legacy applications
  3. Change the user creation statements
  4. Create a new hashing script

I had this on my mind to look into but there was no time. Upgrading this scheme would be a pretty significant security improvement as SHA-1 isn't made for password hashing.

But this is unrelated to this PR as I haven't seen any efforts to transition to ed25519 and it doesn't weaken anything.

grooverdan commented 1 year ago

Adding a set -x -v shows the test case not passing:

$ ./password_2_hash.sh 

command -v openssl >/dev/null || {
  echo "openssl command not found"
  exit 1
}
+ command -v openssl

command -v awk >/dev/null || {
  echo "awk command not found"
  exit 1
}
+ command -v awk

function hash_pw() {
  openssl sha1 -binary | openssl sha1 -hex | awk '{print "*"toupper($0)}'
}

function test_hash() {
  gen_hash=$(echo -n "$1" | hash_pw)
  if [ "$gen_hash" != "$2" ]; then
    exit 1
  fi
}

test_hash 'mariadb' '*54958E764CE10E50764C2EECBB71D01F08549980'
+ test_hash mariadb '*54958E764CE10E50764C2EECBB71D01F08549980'
++ echo -n mariadb
++ hash_pw
++ openssl sha1 -hex
++ awk '{print "*"toupper($0)}'
++ openssl sha1 -binary
+ gen_hash='*SHA1(STDIN)= 54958E764CE10E50764C2EECBB71D01F08549980'
+ '[' '*SHA1(STDIN)= 54958E764CE10E50764C2EECBB71D01F08549980' '!=' '*54958E764CE10E50764C2EECBB71D01F08549980' ']'
+ exit 1
TheAlgorythm commented 1 year ago
+ gen_hash='*SHA1(STDIN)= 54958E764CE10E50764C2EECBB71D01F08549980'
+ '[' '*SHA1(STDIN)= 54958E764CE10E50764C2EECBB71D01F08549980' '!=' '*54958E764CE10E50764C2EECBB71D01F08549980' ']'
+ exit 1

Where does the SHA1(STDIN)= come from? I checked it and everything seems to work.

+ gen_hash='*54958E764CE10E50764C2EECBB71D01F08549980'
+ '[' '*54958E764CE10E50764C2EECBB71D01F08549980' '!=' '*54958E764CE10E50764C2EECBB71D01F08549980' ']'
fauust commented 1 year ago

Same here:

❯ bash -x password_2_hash.sh
+ set -eo pipefail
+ shopt -s nullglob
+ command -v openssl
+ command -v awk
+ test_hash mariadb '*54958E764CE10E50764C2EECBB71D01F08549980'
++ echo -n mariadb
++ hash_pw
++ openssl sha1 -binary
++ awk '{print "*"toupper($0)}'
++ openssl sha1 -hex
+ gen_hash='*(STDIN)= 54958E764CE10E50764C2EECBB71D01F08549980'
+ '[' '*(STDIN)= 54958E764CE10E50764C2EECBB71D01F08549980' '!=' '*54958E764CE10E50764C2EECBB71D01F08549980' ']'
+ exit 1
❯ bash --version
GNU bash, version 5.1.4(1)-release (x86_64-pc-linux-gnu)
Copyright (C) 2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>

This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
❯ openssl version
OpenSSL 1.1.1n  15 Mar 2022
❯ echo test | openssl sha1 -binary | openssl sha1 -hex | awk '{print "*"toupper($0)}'
*(STDIN)= 1368D90029BDFE40A49E57F8FD348CD0BFB6D61A

There is something wrong with the openssl command IMO, not sure what.

grooverdan commented 1 year ago

its just removing (STDIN)= out of the output from -hex (which doesn't seem easy by options), so work in with that awk command.

TheAlgorythm commented 1 year ago

The reason it worked on my MacBook is, that it uses LibreSSL and not OpenSSL. Therefore I would suggest, that the awk command should work with both implementations.

I am working on it.

TheAlgorythm commented 1 year ago

I'm using the -r openssl flag as it uses the postfix notation. Should I use head --bytes 40 or awk -F ' '?

As both work and I don't know which one is more clear.

grooverdan commented 1 year ago
 function hash_pw() {
-  openssl sha1 -binary | openssl sha1 -hex | awk '{print "*"toupper($0)}'
+  openssl sha1 -binary | openssl sha1 -hex | awk -F ' ' '{print "*"toupper($2)}'
 }

Seems to be it.

The bash read seems to have tty handling and prompt as the stty can error:

$ echo bla | ./password_2_hash.sh 
..
+ awk -F ' ' '{print "*"toupper($2)}'
stty: 'standard input': Inappropriate ioctl for device
*BE1BDEC0AA74B4DCB079943E70528096CCA985F8
TheAlgorythm commented 1 year ago
openssl sha1 -binary | openssl sha1 -hex | awk -F ' ' '{print "*"toupper($2)}'

This won't work with LibreSSL because of the different output formats. The following works with both ssl implementations:

openssl sha1 -binary | openssl sha1 -hex -r | awk -F ' ' '{print "*"toupper($1)}'

The stty error is expected as it disables the echo and this can only be done on an interactive shell and not a pipe. I could try to silence the error, but this could also silence an unexpected error. An alternative is to check wether the shell is interactive or not.

grooverdan commented 1 year ago

only openssl is in the container. What's the libressl use case? copy it out of the container and use it?

TheAlgorythm commented 1 year ago

Yes, so that the script can be used everywhere without any drawbacks.

TheAlgorythm commented 1 year ago

Would it make sense to add the execution of the script to the CI tests? If it should be done, I don't know where to put it. Otherwise this PR would be ready.