Extend acra-keys destroy with specific rotated key #641
Conversation
Extended acra-keys destroy with destroying specific rotated key
| return ErrInvalidIndex | ||
| } | ||
|
|
||
| rotatedKey := rotatedKeyFiles[index-1] |
There was a problem hiding this comment.
do we sure that ReadDir returns files in the expected order? can it be changed in other filesystems?
or will be better to sort on keystore by name (that contains timestamp) side to be sure that it works similar in all environments?
There was a problem hiding this comment.
According to ReadDir it returns a list of directory entries sorted by filename. So we need to check the implementation of Storage interface to match that requirement
keystore/v2/keystore/hmac.go
Outdated
| return err | ||
| } | ||
| // Index represent virtual index of key | ||
| // 1 is always index of current key of the keystore |
There was a problem hiding this comment.
this comment confuse a bit because we decrement by one below... IMHO, we should describe here, that keyrings have internal index starting from 0 and we get index here that represents virtual index starting from 1.
p.s. does it work correctly with destroyed keys? What key will be destroyed, if we have current + destroyed + rotated + rotated and we call acra-keys destroy --index 2? will it destroy first rotated or destroy second time already destroyed? can we test that case?
Fixed after review
keystore/v2/keystore/keyStore.go
Outdated
| return ErrInvalidIndex | ||
| } | ||
| // 1 is always index of current key of the keystore, so we need to subtract 1 from search index | ||
| // as slice element enumeration starts from 0 subtract 1 again |
There was a problem hiding this comment.
lets add more explanation of why we decrease 2:
keyring internally stores keys from older to newer and the newest key has index
len(keys) -1. So we decrease once to start from thelen(keys)-1position. Incoming parameterindexrepresents the virtual index starting from 1 where 1 is the newest key. But slices work with 0-indexation, so we decrease one more time.
Extended comment above tricky part
Extended
acra-keysdestroywith destroying the specific rotated keys.--indexrepresenting the key to destroy.StorageRotatedKeyDestructionChecklist
with new changes