New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend acra-keys export/import subcommand #629
Conversation
Extend acra-keys export/import subcommand with V1 keystore support
Added comments for eported types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add description to PR what actually was done from the general view? like logic of acra-backup which ere implemented only for v1, moved to acra-keys import/export. Keystore v1 extended with implementation of interface X1, X2. Keystore v2 extended with Y1, Y2...
keystore/keystore.go
Outdated
// Export only public key data. | ||
ExportPublicOnly ExportMode = 0 | ||
// Export private and public key data. | ||
ExportPrivateKeys = (1 << iota) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, what the logic of such assigning?) 0, 1, 4... it's 000, 001, 100 in binary format..
if it just moved from acra-backup here and these values weren't dumped somewhere and used only internally, we can use more predictable values for that. Just iota or 1 << iota
if we want use them as bit masks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, will fix the mask creation
Extend acra-keys export to work with keyID and key paths
Fixed acra-keys configs
Fixed failing tests
cmd/acra-keys/keys/export_test.go
Outdated
}, | ||
{ | ||
KeyKind: keystore.KeyPath, | ||
ContextID: []byte("testclientid_storage_sym"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are cheater )) compile arguments programmatically to test both cases ))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add parsing from arguments)
Remove KeyPath KeyID
Added logging
Added additional tests
Fixed failing tests
Fixed general_validation tests
Added invalid cases tests
|
||
if len(exportIDs) != 0 { | ||
for _, exportID := range exportIDs { | ||
switch exportID.KeyKind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, add comment that we additionally verify public keys and do not for private keys due to they are encrypted and will be validated on the decryption step. because in the future a question will arise as why so and do we need validate private keys too
Fixed after reivew
# Conflicts: # CHANGELOG_DEV.md
|
||
// DescribeKeyFile describes key by its purpose path for V1 and V2 keystore | ||
func DescribeKeyFile(fileName string) (*keystore.KeyDescription, error) { | ||
description, ok, err := describeV2(fileName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool
What has been done in this PR?
Currently,
acra-keys
export/import
command supports keys fromV2
keystore only, the idea was to add support forV1
keystore as well under one tool. But we have different utility that has similar functionality for backup keys ofV1
keystore -acra-backup
. As the resultacra-backup
functionality was merged underacra-keys
export/import
Technically, there were introduced new interfaces
keystore.Exporter
forexport
subcommand andkeystore.Importer
forimport
subcommand. Corresponded_Backupers
were implemented for keystoresV1
andV2
.The important thing, is that
acra-keys
export
interface was the following - it could be possible to export all keys from keystore via--all
flags or export some special key by specifyingexportID
in the format like (client/client_test/storage) - which represent a relative path to the key in the keystore. Since all our subcommands insideacra-keys
tool operate with a generic key definition (e.gpoison-record
,client/{some_client}/symmetric
, etc) the same functionality was added foracra-keys
export
too, but specifying key as path was saved for backward compatibility reasons.Checklist
with new changes