Skip to content
This repository has been archived by the owner on Dec 12, 2024. It is now read-only.

added utility on new key store service from keystorage #294

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/service/keystore/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,22 @@ func (s Service) Config() config.KeyStoreServiceConfig {
return s.config
}

// NewKeyStoreFromKeystoreStorage uses a keystore service directly from storage object
func NewKeyStoreFromKeystoreStorage(config config.KeyStoreServiceConfig, keyStoreStorage *Storage) (*Service, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where would this key store be coming from in practice?

Copy link
Contributor Author

@andorsk andorsk Feb 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@decentralgabe so it's a good question. I was writing some personal stuff for testing around this and I needed an entry point that did not generate a new service key and salt, which is why I needed a method to support this action. I'm happy to just support this entrypoint on my own fork of this, or if there's a better way to handle this I'm open to it.

my main problem was with this line

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about adding a parameter to the config to not generate a new key and instead look for an existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@decentralgabe I think that's separate concerns. How it's managed in the config (optional/default behavior) is certainly important, but in the end of the day you'd still need to provide the logic in the NewKeyStoreService function to ignore re-generating the service keys, and/or to load the existing one. You then need to address the behavior of how they are loaded internally, etc, which was out of scope for this PR. Certainly a valid point, just not sure if that's addressed best somewhere else.

Copy link
Contributor Author

@andorsk andorsk Feb 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW: How I did it, I checked if ssi-service key was set, and if it was, unmarshalled it into a ServiceKey, and created a Keystore Service using the method in this PR. There are other possible ways to handle this, which is why I didn't want to tackle the logic around configuration/loading existing keys in this PR. The blocker I was having was specifically that there needed to be separation between ServiceKey generation/the keystore service and the rest of the NewKeyStoreService logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameters of this new constructor makes sense to me. I recommend only having a single constructor, instead of 2. It seems to me that it makes sense to update all callers to use the new constructor, and remove the old one.

Keeping a single constructor can then be named NewKeyStoreService.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea..@andresuribe87 that works and I think is a reasonable way forward.

if keyStoreStorage == nil {
return nil, errors.New("no storage provided")
}

service := Service{
storage: keyStoreStorage,
config: config,
}
if !service.Status().IsReady() {
return nil, errors.New(service.Status().Message)
}
return &service, nil
}

func NewKeyStoreService(config config.KeyStoreServiceConfig, s storage.ServiceStorage) (*Service, error) {
// First, generate a service key
serviceKey, serviceKeySalt, err := GenerateServiceKey(config.ServiceKeyPassword)
Expand Down
62 changes: 62 additions & 0 deletions pkg/service/keystore/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,65 @@ func TestStoreAndGetKey(t *testing.T) {
assert.NoError(t, err)
assert.NotEmpty(t, signer)
}

func TestStoreAndGetKeyWithExistingKeystorage(t *testing.T) {

file, err := os.CreateTemp("", "bolt")
require.NoError(t, err)
name := file.Name()
assert.NoError(t, file.Close())
bolt, err := storage.NewStorage(storage.Bolt, name)
assert.NoError(t, err)
assert.NotEmpty(t, bolt)

// remove the db file after the test
t.Cleanup(func() {
_ = bolt.Close()
_ = os.Remove(bolt.URI())
})

config := config.KeyStoreServiceConfig{
BaseServiceConfig: &config.BaseServiceConfig{
Name: "test-keyStore",
},
ServiceKeyPassword: "test-password",
}

serviceKey, serviceKeySalt, err := GenerateServiceKey(config.ServiceKeyPassword)

// Next, instantiate the key storage
keyStoreStorage, err := NewKeyStoreStorage(bolt, ServiceKey{
Base58Key: serviceKey,
Base58Salt: serviceKeySalt,
})

keyStore, err := NewKeyStoreFromKeystoreStorage(
config,
keyStoreStorage,
)

assert.NoError(t, err)
assert.NotEmpty(t, keyStore)

// store the key
_, privKey, err := crypto.GenerateEd25519Key()
assert.NoError(t, err)
err = keyStore.StoreKey(context.Background(), StoreKeyRequest{
ID: "test-id",
Type: crypto.Ed25519,
Controller: "test-controller",
PrivateKeyBase58: base58.Encode(privKey),
})
assert.NoError(t, err)

// get it back
keyResponse, err := keyStore.GetKey(context.Background(), GetKeyRequest{ID: "test-id"})
assert.NoError(t, err)
assert.NotEmpty(t, keyResponse)
assert.Equal(t, privKey, keyResponse.Key)

// make sure can create a signer properly
signer, err := crypto.NewJWTSigner("kid", keyResponse.Key)
assert.NoError(t, err)
assert.NotEmpty(t, signer)
}