Load public keys from auth service instead of hardcoding them in configuration #1623
Conversation
|
Looks fine, overall. |
alexeykazakov
changed the title
|
The PR is ready for review/merge. It's not WIP anymore. |
token/token.go
Outdated
| if devModeURL != "" { | ||
| remoteKeys, err = authtoken.FetchKeys(fmt.Sprintf("%s/protocol/openid-connect/certs", devModeURL)) | ||
| if err != nil { | ||
| log.Error(nil, map[string]interface{}{ |
There was a problem hiding this comment.
why do you log the error and return it ? what about wrapping the underlying error with a new message, instead ?
| tm.publicKeys = append(tm.publicKeys, &PublicKey{KeyID: remoteKey.KeyID, Key: remoteKey.Key}) | ||
| log.Info(nil, map[string]interface{}{ | ||
| "kid": remoteKey.KeyID, | ||
| }, "Public key added") |
There was a problem hiding this comment.
message should start with lowercase
There was a problem hiding this comment.
It's true for error messages in "error" (because they can be combined with other error messages). But here we just printing an info message.
| tm.publicKeys = append(tm.publicKeys, &PublicKey{KeyID: remoteKey.KeyID, Key: remoteKey.Key}) | ||
| log.Info(nil, map[string]interface{}{ | ||
| "kid": remoteKey.KeyID, | ||
| }, "Public key added") |
There was a problem hiding this comment.
message should start with lowercase
There was a problem hiding this comment.
It's true for error messages inside error (because they can be combined with other error messages). But here we just printing an info message.
token/token.go
Outdated
| token, err := jwt.Parse(tokenString, func(token *jwt.Token) (interface{}, error) { | ||
| return mgm.publicKey, nil | ||
| }) | ||
| remoteKeys, err := authtoken.FetchKeys(config.GetKeysEndpoint()) |
There was a problem hiding this comment.
would it make sense to rename authtoken to authclient in the WIT codebase ?
| log.Error(nil, map[string]interface{}{ | ||
| "keys_url": config.GetKeysEndpoint(), | ||
| }, "unable to load public keys from remote service") | ||
| return nil, errors.New("unable to load public keys from remote service") |
There was a problem hiding this comment.
why do you log the error and return it ? what about wrapping the underlying error with a new message, instead ?
There was a problem hiding this comment.
The main reason to log errors inside some function is to add parameters to the error log message which the calling function doesn't care or doesn't have to be aware of.
In this case we are logging keys_uri.
| if !token.Valid { | ||
| return nil, errors.New("Token not valid") | ||
| devModeURL := config.GetKeycloakDevModeURL() | ||
| if devModeURL != "" { |
There was a problem hiding this comment.
aren't you doing the same work as above, with another KC URL ?
There was a problem hiding this comment.
Not exactly. Main difference is that here we are operating with different PublicKey type. (fabric8-auth/PublicKey vs. fabric8-wit/PublicKey). We could re-use this code in both places but it would require to introduce some common interface which can be tricky. So, anyway, it would require us to write more code to re-use this small block in two places. Which doesn't make sense.
| } | ||
|
|
||
| // NewManagerWithPublicKey returns a new token Manager for handling tokens with the only public key | ||
| func NewManagerWithPublicKey(id string, key *rsa.PublicKey) Manager { |
There was a problem hiding this comment.
is this going to be used in dev mode, when the public key for the test realm is loaded from the configuration ?
There was a problem hiding this comment.
This constructor is used by tests only. The tests that don't care about keys from the test realm.
token/token.go
Outdated
| kid := token.Header["kid"] | ||
| if kid == nil { | ||
| log.Error(ctx, map[string]interface{}{}, "There is no 'kid' header in the token") | ||
| return nil, errors.New("There is no 'kid' header in the token") |
There was a problem hiding this comment.
message should start with lower case
token/token.go
Outdated
| // ParseToken parses token claims | ||
| func (mgm *tokenManager) ParseToken(ctx context.Context, tokenString string) (*TokenClaims, error) { | ||
| token, err := jwt.ParseWithClaims(tokenString, &TokenClaims{}, func(token *jwt.Token) (interface{}, error) { | ||
| kid := token.Header["kid"] |
There was a problem hiding this comment.
what about kid, found := token.Header["kid"] ?
token/token.go
Outdated
| } | ||
| key := mgm.PublicKey(fmt.Sprintf("%s", kid)) | ||
| if key == nil { | ||
| log.Error(ctx, map[string]interface{}{ |
There was a problem hiding this comment.
same comment as above: what about wrapping the error with the new message ?
There was a problem hiding this comment.
I log it here too just to log the key ID parameter.
|
I've addressed some @xcoulon comments. |
Related to fabric8-services/fabric8-auth#30
Tasks to be completed before it can be merged: