unbreak password_hash('blowfish')

[jasperla]

SUMMARY

The used salt buffer was of incorrect length (16 instead of 22). Also the
passlib documentation recommends against generating your own salt. So unless
the salt was provided, let passlib generate it.

Also set the default bcrypto algorithm to '2b' which is the recommended format.

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

password_hash filter

ANSIBLE VERSION

ansible 2.3.0.0
  config file = /home/jasper/.ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.13 (default, May 24 2017, 16:43:59) [GCC 4.2.1 20070719 ]

ADDITIONAL INFORMATION

The upstream documentation for passlib can be found here.

About salt it writes:

salt (str) – Optional salt string. If not specified, one will be autogenerated (this is recommended). If specified, it must be 22 characters, drawn from the regexp range [./0-9A-Za-z].

and about the algorithm:

"2a" - some implementations suffered from rare security flaws, replaced by 2b.
[..]
"2b" - latest revision of the official BCrypt algorithm, current default.

Before:

fatal: [192.168.2.1]: FAILED! => {"failed": true, "msg": "the field 'args' has an invalid value ([]), and could not be converted to an dict. Error was: salt too small (bcrypt
+requires exactly 22 chars)\n\nThe error appears to have been in '/home/jirib/ansible/test.yml': line 11, column 7, but may\nbe elsewhere in the file depending on the exact syntax
+problem.\n\nThe offending line appears to be:\n\n  tasks:\n    - name: Create users from secret.yml\n      ^ here\n"}

After:

changed: [192.168.2.1]

More informatively I've ran a debug task (debug: msg="{{ 'emiel' | password_hash('blowfish') }}") that used to return:

TASK [debug] ************************************************************************************************************************************************************************
fatal: [localhost]: FAILED! => {"failed": true, "msg": "the field 'args' has an invalid value ([]), and could not be converted to an dict. Error was: salt too small (bcrypt requires exactly 22 chars)\n\nThe error appears to have been in '/private/tmp/ansible-blowfish/users.yml': line 8, column 5, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n  tasks:\n  - debug: msg=\"{{ 'emiel' | password_hash('blowfish') }}\"\n    ^ here\nWe could be wrong, but this one looks like it might be an issue with\nmissing quotes.  Always quote template expression brackets when they\nstart a value. For instance:\n\n    with_items:\n      - {{ foo }}\n\nShould be written as:\n\n    with_items:\n      - \"{{ foo }}\"\n"}

and now returns:

ok: [localhost] => {
    "changed": false,
    "msg": "$2b$12$yhShz4JEDMGWCZO3AFpBq..gn6TIq9ydFcxOtqlFcIxdQFJc4zHk2"
}

[abadger]

This doesn't match documentation. Why is it being changed?

[abadger]

          2a  | Blowfish (not in mainline glibc; added in some
              | Linux distributions)

from the glibc man page for crypt.

[abadger]

Okay, I see where the passlib documentation specifies a 2b. Since cryptmethod is only used in the crypt.crypt() path, I don't think we should change this code here.

[abadger]

Making this change here doesn't work because we aren't guaranteed to have passlib installed. The crypt.crypt() method will require that we have a salt set. You could move salt generation into the conditional for not HAS_PASSLIB.

[abadger]

I also noticed that the passlib import check we're doing is no longer sufficient... apparently there are now separate backend packages and passlib needs at least one of those installed to function.

Perhaps we need a try: except passlib.exc.MissingBackendError: in the passlib code and fallback to crypt if that fails as well.

[abadger]

Something like this perhaps:

encrypted = None

if HAS_PASSLIB:
    try:
        # passlib based code
    except passlib.exc.MissingBackendError:
        pass

if encrypted is None:
     if sys.platform.startswith('darwin'):
         raise errors.AnsibleFilterError('|password_hash requires the passlib python module to generate password hashes on Mac OS X/Darwin')
     saltstring = "$%s$%s" % (cryptmethod[hashtype], salt)
     encrypted = crypt.crypt(password, saltstring)

[abadger]

Note: bcoca recommends a different error message if a passlib backend is missing rather than the same message as missing passlib altogether.

[eli-collins]

Passlib author here - I just incidentally ran across this thread, and wanted to point out that crypt.crypt() is one of the backends that passlib tests for when bcrypt.hash() is called.

So if passlib throws MissingBackendError, it's reasonably certain that crypt.crypt() doesn't support bcrypt... or even worse, has broken support [1]. Because of that, if passlib is present but throws MissingBackendError, I'd recommend not using crypt.crypt() as a bcrypt fallback.

[1] For example, in cases like linux/glibc, crypt() may not even throw an error, but fall back to the super-insecure des-crypt algorithm: crypt.crypt("$2b$foo", "bar") --> "bac8SdYfKlb5."

[abadger]

Hmm... On my linux/glibc box, crypt.crypt does not fallback. It returns None (Would be better to raise an exception but a sentinel like None can also be programmed against).

[eli-collins]

For reference, I'm using Linux Mint 18.1, python 3.5.2; but I'm not at all surprised you have different behavior.

That's one of the things that started me coding passlib -- crypt()'s error/fallback behavior is hugely inconsistent at the C level, and python's wrapper just reflects that. Given the same input of crypt.crypt("$2b$foo", "bar"), other platforms may even return things like "*0" or ":" to indicate an error, rather than None.

Passlib's internal safe_crypt() helper is an example of the number of edge cases I've had to normalize for :(

[abadger]

#21215 has been merged which implements all of this except for the switch to 2b iff we know we're using passllib instead of crypt. If you'd like to make a PR for that aspect, feel free to create a new one which does that. Thanks!