added utility on new key store service from keystorage #294
Conversation
andorsk
requested review from
decentralgabe,
nitro-neal and
andresuribe87
as code owners
Codecov Report
@@ Coverage Diff @@
## main #294 +/- ##
==========================================
+ Coverage 24.57% 24.67% +0.09%
==========================================
Files 32 32
Lines 3182 3194 +12
==========================================
+ Hits 782 788 +6
- Misses 2281 2285 +4
- Partials 119 121 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -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) { | |||
There was a problem hiding this comment.
where would this key store be coming from in practice?
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
what about adding a parameter to the config to not generate a new key and instead look for an existing one?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yea..@andresuribe87 that works and I think is a reasonable way forward.
|
Would like to get @andresuribe87 's perspective before continuing |
|
@andorsk are you still interested in pushing this along? if so what's needed? |
|
@decentralgabe thanks. This is probably still a good idea, but I'm OK with backlogging it for now. Currently it's not a priority with me. |
|
Thanks @andorsk I'll keep it open - agree it's useful. |
Overview
Helper to build a keystorage service form an existing keystore.
Description
I wanted to use an existing keystore.keyStorage for
ssi-service. The main issue is that https://github.com/TBD54566975/ssi-service/blob/main/pkg/service/keystore/service.go#L51 generates a new service key, and I wanted to read an existing service key stored in a db. It wasn't clear how to spin up a KeystoreService with an existing ServiceKey, and the storage and config methods are privately scoped, so other applications can't read it. Therefore, I decided to add a helper method that adds a way to create a new keystore service with an existing storage configuration.How Has This Been Tested?
Checklist
Before submitting this PR, please make sure: