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

added password_2_hash script #463

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

TheAlgorythm
Copy link
Contributor

I added it as script like discussed in #458.

@TheAlgorythm TheAlgorythm marked this pull request as ready for review October 1, 2022 14:55
@TheAlgorythm
Copy link
Contributor Author

TheAlgorythm commented Oct 1, 2022

The same CI failures again.

@grooverdan
Copy link
Member

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
Copy link
Collaborator

fauust commented Oct 17, 2022

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 #458 and docker-library/docs#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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

+ 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
Copy link
Collaborator

fauust commented Oct 18, 2022

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

 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
Copy link
Contributor Author

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
Copy link
Member

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

@TheAlgorythm
Copy link
Contributor Author

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

@TheAlgorythm
Copy link
Contributor Author

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.

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

Successfully merging this pull request may close these issues.

3 participants