Introduce wrapper type for rsa.PrivateKey with JWK #100
Conversation
| @@ -0,0 +1,53 @@ | |||
| package private | |||
There was a problem hiding this comment.
I needed to work around a cyclic dependency with the mock packages which is why I introduced this sub-package. Open to suggestions of a better name or location. I've tried to make the package a natural part of the member names when references as is suggestd in go style guidelines
silasdavis
force-pushed
the
precompute-jwk
branch
3 times, most recently
from
d4f7df3
to
e54c966
Compare
app/data/key_store.go
Outdated
| Keys() []*private.Key | ||
| } | ||
|
|
||
| // Implements authn-go JWKProvider |
There was a problem hiding this comment.
This may fit better in authn-go, since authn-server has no reason to know about the interface.
If it stays here, though, I'd ask to move it into a new file.
There was a problem hiding this comment.
Agreed on move... I went to add this into authn-go, but I thought it was kind of nice that authn-go doesn't depend directly on a particular version of authn-server, which it would do if we import KeyStore. It means they won't get into a dependency fight for a module that depends on both, though it seems unlikely that you wouldn't want to upgrade one or the other to get around that.
If you prefer a hard dependency then I can move it, but for now i've put it in its own file
There was a problem hiding this comment.
Ahh, I see. This is probably the better compromise.
There's a third option though: leave the code in an integration layer that already depends on both authn-go and authn-server. In this case it would be your app. One downside is that no one else would have access to it.
Can you think of other downsides?
Do you expect more integration code like this?
There was a problem hiding this comment.
I think the third way makes sense. There probably will be some more, but we could always factor that out into some kind of 'lib-keratin' once we can prove they make sense. So happy to factor this out now if you'd prefer.
silasdavis
force-pushed
the
precompute-jwk
branch
from
e54c966
to
3f835ed
Compare
|
Made requested changes and added to changelog |
In various places we compute the JWK thumbprint and in get_jwks.go we use the entire key. We compute these as needed - possibly multiple times. This change introduces a wrapper type private.Key that wraps up the rsa.PrivateKey. It allows: - Single computation of JWK thumbprint and key - Simplifies logic in get_jwk.go - Centralises logic for generating RSA key - Makes it easy to add a KeyStore backed JWKProvider (useful for using authn-go's verifier when using authn-server as a library) - Makes JWK available from KeyStore (when using as library) - Provides a potential place to abstract over different key types (e.g. ed25519) Signed-off-by: Silas Davis <silas@monax.io>
silasdavis
force-pushed
the
precompute-jwk
branch
from
3f835ed
to
dfe2683
Compare
|
Removed |
This PR is motivated by using the KeyStore as a JWKProvider (see: keratin/authn-go#9) but I think it improves the structure of the code slightly too. It also makes gives better access to JWK generation when used as a library.
In various places we compute the JWK thumbprint and in get_jwks.go we
use the entire key. We compute these as needed - possibly multiple
times. This change introduces a wrapper type private.Key that wraps up
the rsa.PrivateKey. It allows:
authn-go's verifier when using authn-server as a library)
ed25519)
Signed-off-by: Silas Davis silas@monax.io