Use PBKDF2HMAC() from cryptography for vault keys. #11765

Merged
merged 2 commits into from Aug 21, 2015

Conversation

Projects
None yet
5 participants
@ldx
Contributor

ldx commented Jul 28, 2015

When stretching the key for vault files, use PBKDF2HMAC() from the
cryptography package instead of pycrypto. This will speed up the opening
of vault files by ~10x.

The problem is here in lib/ansible/utils/vault.py:

hash_function = SHA256

# make two keys and one iv
pbkdf2_prf = lambda p, s: HMAC.new(p, s, hash_function).digest()

derivedkey = PBKDF2(password, salt, dkLen=(2 * keylength) + ivlength,
                    count=10000, prf=pbkdf2_prf)

PBKDF2() calls a Python callback function (pbkdf2_pr()) 10000 times.
If one has several vault files, this will cause excessive start times
with ansible or ansible-playbook (we experience ~15 second startup
times).

Testing the original implementation in 1.9.2 with a vault file:

In [2]: %timeit v.decrypt(encrypted_data)
1 loops, best of 3: 265 ms per loop

Having a recent OpenSSL version and using the vault.py changes in this commit:

In [2]: %timeit v.decrypt(encrypted_data)
10 loops, best of 3: 23.2 ms per loop

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Jul 28, 2015

Contributor

Heh, I was looking into a very similar change just this morning. Please note however that the code under v1/ is thoroughly dead, and your PR should be targeted to lib/ansible/parsing/vault/__init__.py now. Also, how about (in a separate commit) abstracting out the checks so that you don't have to change n different places?

Contributor

amenonsen commented Jul 28, 2015

Heh, I was looking into a very similar change just this morning. Please note however that the code under v1/ is thoroughly dead, and your PR should be targeted to lib/ansible/parsing/vault/__init__.py now. Also, how about (in a separate commit) abstracting out the checks so that you don't have to change n different places?

@ldx

This comment has been minimized.

Show comment
Hide comment
@ldx

ldx Jul 28, 2015

Contributor

@amenonsen thanks, I pushed a commit that implements the same in lib/ansible/parsing/vault/__init__.py.

Would you like me to refactor the HAS_* checks in v1? If v1 is outdated now, we can leave it as is. In lib/ansible/parsing/vault/__init__.py the check is already in a helper function.

Contributor

ldx commented Jul 28, 2015

@amenonsen thanks, I pushed a commit that implements the same in lib/ansible/parsing/vault/__init__.py.

Would you like me to refactor the HAS_* checks in v1? If v1 is outdated now, we can leave it as is. In lib/ansible/parsing/vault/__init__.py the check is already in a helper function.

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Jul 28, 2015

Member

please drop the changes to v1/ as it is there just to be able to quickly test if a bug was introduced in devel or was preexisting.

Member

bcoca commented Jul 28, 2015

please drop the changes to v1/ as it is there just to be able to quickly test if a bug was introduced in devel or was preexisting.

@ldx

This comment has been minimized.

Show comment
Hide comment
@ldx

ldx Jul 28, 2015

Contributor

I reverted the commit changing v1, and squashed the three commits in the branch into one.

Contributor

ldx commented Jul 28, 2015

I reverted the commit changing v1, and squashed the three commits in the branch into one.

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Jul 28, 2015

Contributor

@ldx looks good, but HAS_ANY_PBKDF2HMAC already includes HAS_PBKDF2 so you don't need to test for the latter separately in check_prereqs.

Contributor

amenonsen commented Jul 28, 2015

@ldx looks good, but HAS_ANY_PBKDF2HMAC already includes HAS_PBKDF2 so you don't need to test for the latter separately in check_prereqs.

Use PBKDF2HMAC() from cryptography for vault keys.
When stretching the key for vault files, use PBKDF2HMAC() from the
cryptography package instead of pycrypto. This will speed up the opening
of vault files by ~10x.

The problem is here in lib/ansible/utils/vault.py:

    hash_function = SHA256

    # make two keys and one iv
    pbkdf2_prf = lambda p, s: HMAC.new(p, s, hash_function).digest()

    derivedkey = PBKDF2(password, salt, dkLen=(2 * keylength) + ivlength,
                        count=10000, prf=pbkdf2_prf)

`PBKDF2()` calls a Python callback function (`pbkdf2_pr()`) 10000 times.
If one has several vault files, this will cause excessive start times
with `ansible` or `ansible-playbook` (we experience ~15 second startup
times).

Testing the original implementation in 1.9.2 with a vault file:

In [2]: %timeit v.decrypt(encrypted_data)
1 loops, best of 3: 265 ms per loop

Having a recent OpenSSL version and using the vault.py changes in this commit:

In [2]: %timeit v.decrypt(encrypted_data)
10 loops, best of 3: 23.2 ms per loop
@ldx

This comment has been minimized.

Show comment
Hide comment
@ldx

ldx Jul 28, 2015

Contributor

@amenonsen thanks, I fixed it and squashed again.

Contributor

ldx commented Jul 28, 2015

@amenonsen thanks, I fixed it and squashed again.

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Jul 28, 2015

Contributor

Looks good. Do you think we should mention this (i.e. the possibility to speed things up by doing pip install cryptography) somewhere in the documentation? I'm not sure if, and I'm not sure where.

Contributor

amenonsen commented Jul 28, 2015

Looks good. Do you think we should mention this (i.e. the possibility to speed things up by doing pip install cryptography) somewhere in the documentation? I'm not sure if, and I'm not sure where.

@ldx

This comment has been minimized.

Show comment
Hide comment
@ldx

ldx Jul 28, 2015

Contributor

Mentioning this in the documentation is definitely a good idea. I guess other vault users have also been frustrated with slow startup times.

I see vault is documented in docsite/rst/playbooks_vault.rst. Is it okay to add a note there about speeding up vault access via installing cryptography?

Contributor

ldx commented Jul 28, 2015

Mentioning this in the documentation is definitely a good idea. I guess other vault users have also been frustrated with slow startup times.

I see vault is documented in docsite/rst/playbooks_vault.rst. Is it okay to add a note there about speeding up vault access via installing cryptography?

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Jul 28, 2015

Member

i would not say it's OK, but that it is needed

Member

bcoca commented Jul 28, 2015

i would not say it's OK, but that it is needed

@ldx

This comment has been minimized.

Show comment
Hide comment
@ldx

ldx Jul 28, 2015

Contributor

I pushed a commit with a documentation update.

Contributor

ldx commented Jul 28, 2015

I pushed a commit with a documentation update.

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Jul 29, 2015

Contributor

Sorry to nitpick, but I'd prefer more imperative wording similar to the following:

By default, Ansible uses PyCrypto to encrypt and decrypt vault files. If you have many encrypted files, decrypting them at startup may cause a perceptible delay. To speed this up, install the cryptography package::

Contributor

amenonsen commented Jul 29, 2015

Sorry to nitpick, but I'd prefer more imperative wording similar to the following:

By default, Ansible uses PyCrypto to encrypt and decrypt vault files. If you have many encrypted files, decrypting them at startup may cause a perceptible delay. To speed this up, install the cryptography package::

@ldx

This comment has been minimized.

Show comment
Hide comment
@ldx

ldx Jul 29, 2015

Contributor

Amended the last commit.

Contributor

ldx commented Jul 29, 2015

Amended the last commit.

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Jul 30, 2015

Contributor

Cool, thanks. This looks good.

Contributor

amenonsen commented Jul 30, 2015

Cool, thanks. This looks good.

@amenonsen

This comment has been minimized.

Show comment
Hide comment
@amenonsen

amenonsen Aug 21, 2015

Contributor

Note to maintainers: this is a significant performance improvement for vault operations and should be in v2.

Contributor

amenonsen commented Aug 21, 2015

Note to maintainers: this is a significant performance improvement for vault operations and should be in v2.

@brainsik

This comment has been minimized.

Show comment
Hide comment
@brainsik

brainsik Aug 21, 2015

Note to maintainers: this is a significant performance improvement for vault operations and should be in v2.

On my setup I went from waiting a painful minute for all the vaults to open — and Ansible to start doing it's thing — to waiting just 2s. This is a game changer for usability when you have a lot of vault files.

Note to maintainers: this is a significant performance improvement for vault operations and should be in v2.

On my setup I went from waiting a painful minute for all the vaults to open — and Ansible to start doing it's thing — to waiting just 2s. This is a game changer for usability when you have a lot of vault files.

bcoca added a commit that referenced this pull request Aug 21, 2015

Merge pull request #11765 from ldx/vault_pbkdf2hmac
Use PBKDF2HMAC() from cryptography for vault keys.

@bcoca bcoca merged commit 144da7e into ansible:devel Aug 21, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ansibot ansibot added feature and removed feature_pull_request labels Mar 4, 2018

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