Extend acra-keys list with rotated keys #636
Conversation
Extend acra-keys list with historical keys listing
cmd/acra-keys/keys/list-keys_test.go
Outdated
| } | ||
|
|
||
| for i := 0; i < timesToGenerateHistoricalKeys; i++ { | ||
| if descriptions[i+3].CreationTime.String() != pubKeysTimes[i].String() { |
There was a problem hiding this comment.
| if descriptions[i+3].CreationTime.String() != pubKeysTimes[i].String() { | |
| if descriptions[i+timesToGenerateHistoricalKeys].CreationTime.String() != pubKeysTimes[i].String() { |
plus we can merge two cycles into one and add comment that public keys stored after private keys in the descriptions
There was a problem hiding this comment.
and can we compare content of the keys? at least public keys which are not encrypted to be sure that they returned properly according to names?
There was a problem hiding this comment.
Not sure we can compare since list subcommand just returns KeyDescription without its content.
cmd/acra-keys/keys/list-keys_test.go
Outdated
| t.Fatal(err) | ||
| } | ||
|
|
||
| if len(descriptions) != timesToGenerateHistoricalKeys { |
There was a problem hiding this comment.
description contains public and private keys together? can we check it and validate? for example, we can remember all keys generated above with an order and then compare them. Validate that first generated is last rotated key and last generated is first key. Also, we can compare public content of keys
There was a problem hiding this comment.
Yes, under V2 only one keyring - storage.keyring. Actually we cant compare the contents of the key as list subcommand just return KeyDescription without its content.
|
|
||
| for _, file := range files { | ||
| if file.IsDir() { | ||
| continue |
There was a problem hiding this comment.
is it valid situation? maybe we should return error in such cases because it is unexpected folder structure?
There was a problem hiding this comment.
Discussed to return error in such cases
keystore/utils.go
Outdated
| // PrintHistoricalKeysTable prints table which describes keys in a human readable format | ||
| // into the writer. | ||
| // Code is shared by `acra-keys list` and a couple of tests | ||
| func PrintHistoricalKeysTable(keys []KeyDescription, writer io.Writer) error { |
There was a problem hiding this comment.
can you add example of the expected output here? it simplifies understanding what the code does and what is expected
keystore/v2/keystore/keyStore.go
Outdated
| // | ||
| // client/${client_id}/storage | ||
| // | ||
| // And transport paths look like this, with an additional component: |
There was a problem hiding this comment.
now we don't have transport paths and we expected 3 parts of the path below. So, lets update comment to actual the state
There was a problem hiding this comment.
Agree, will fix in another method too.
keystore/v2/keystore/keyStore.go
Outdated
| //components := strings.Split(path, string(filepath.Separator)) | ||
| components := strings.Split(path, string(filepath.Separator)) | ||
| if len(components) == 3 { | ||
| if components[0] == clientPrefix && components[2] == storageSuffix { |
There was a problem hiding this comment.
lets use named indexes to simplify reading?
const (
_ = iota // I don't know how to name this part))
clientIDIndex
purposeIndex
)
if components[clientIDIndex] == ...
keystore/v2/keystore/keyStore.go
Outdated
| } | ||
|
|
||
| result := make([]keystore.KeyDescription, 0, len(keys)-1) | ||
| for i := len(keys) - 1; i > 0; i-- { |
There was a problem hiding this comment.
this complicated iteration pushed to think that we should test an order of result lists for rotated keys to be sure that we return and list them in the same order for v1 and v2 keystores
Fixed after review
Zhaars
changed the title
Fixed error text
Fixed general_validation step
Extend
acra-keyslistwith displaying historical keys for V1/V2. Introduce the new flag--historical-keysfor the list subcommand.Checklist
with new changes