Fernet key rotation removing keys early

[Chris Stone]

When setting up Fernet key rotation with a maximum number of active of keys set to 25, it turned out that 'keystone-manage fernet_rotate' started deleting two keys once there reached 13 existing keys. It would waver between 12 and 13 keys every time it was rotated. It looks like this might be related to the range of keys to remove being negative :

excess_keys = ( keys[:len(key_files) - CONF.fernet_tokens.max_active_keys + 1])
.. ends up being excess_keys = ( keys[:-11] )
.. which seems to be dipping back into the range of keys that should still be good and removing those.

Adding something like: "if len(key_files) - CONF.fernet_tokens.max_active_keys + 1 >= 0" for the purge excess keys section seemed to allow us to generate all 25 keys, then rotate as normal. Once we hit the full 25 keys, this additional line was no longer needed.

Attaching some log information showing the available keys going from 12, 13, 12, 13.

[Lance Bragstad (lbragstad)]

Hi Chris,

Thanks for the report. I am able to confirm. For me, the first key (/etc/keystone/fernet-keys/1) is being pruned when I perform a rotate after there are 12 keys in the repository. So, once the 13th key is added, the first key is prematurely removed. Sounds like what you were experiencing.

I'll take a look into the code, thanks!

[Lance Bragstad (lbragstad)]

Oh, looks like I'm seeing keys 1 and 2 being purged when the 13th key is added.

[Lance Bragstad (lbragstad)]

I think I found the issue. I'm not 100% sure but I think it's possible that the logic that determines how many excess keys are in the repository doesn't account for negative slices in the list [0].

Here is a paste of what I was playing with [1]. I'm going to tinker with that logic.

[0] https://github.com/openstack/keystone/blob/1ce6a87738be1d6694c3feaf048547f7c58a1604/keystone/token/providers/fernet/utils.py#L212
[1] http://cdn.pasteraw.com/bsaknota9ex7cz7pua1c3koaezagjh2

[Lance Bragstad (lbragstad)]

The issue is that we are processing negatives as slices in the list, which gives a behavior similar to:

http://cdn.pasteraw.com/i2bc865nfdmva65vckdkt3sqioc2i9s

In our case, I don't think we'd want to bother continuing if the slice is negative, since that would mean that the repository contains less keys than what CONF.fernet_tokens.max_active_keys has set.

We could propose something like:

http://cdn.pasteraw.com/b46zmja0n61un7qamaofbrpbk4shr1j

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/207506

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/212945

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/212945
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=7498154ad8c1ee24a10e8577c69045fd19627530
Submitter: Jenkins
Branch: stable/kilo

commit 7498154ad8c1ee24a10e8577c69045fd19627530
Author: Lance Bragstad <email address hidden>
Date: Wed Jun 17 17:05:37 2015 +0000

    Fix Fernet key rotation

    Previously, depending on the size of CONF.fernet_tokens.max_active_keys, keys
    would be pruned depending on if excess_keys was a negative slice. This caused
    the key repository to never actually reach CONF.fernet_tokens.max_active_keys
    in the repository in some cases.

    Change-Id: Icf47d6612ef0e98e876334f56c993666df081b50
    Closes-Bug: 1465444
    (cherry picked from commit 46f56119a0d19f0c4075aaf0dd29f2d8cf76fa54)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/207506
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=17b7dbc81eb9b173c776b25b9b2aff6b2f9320ed
Submitter: Jenkins
Branch: stable/kilo

commit 17b7dbc81eb9b173c776b25b9b2aff6b2f9320ed
Author: Dolph Mathews <email address hidden>
Date: Wed Jun 17 17:15:42 2015 +0000

    Add unit test to exercise key rotation

    This reproduces the issue reported in bug 1465444 when applied directly
    to master (and actually fails with max_active_keys=4), but passes when
    applied to Lance's patch:

      https://review.openstack.org/#/c/192782/

    Change-Id: I045bc97b047dc18983757db052ad6e5bdad11329
    Related-Bug: 1465444
    (cherry picked from commit a422444fc4bb56e65cc4cc3fda8cf0f13cc079a5)