Grafana Token API Access #1917
Grafana Token API Access #1917
Conversation
… function in token verification process
patrickbrophy
added
enhancement
| var DirectorDB *gorm.DB | ||
|
|
||
| type GrafanaApiKey struct { | ||
| ID string `gorm:"primaryKey;column:id;type:text;not null"` |
There was a problem hiding this comment.
Not being familiar with GORM, does this also add a uniqueness requirement to the column?
| CreatedBy string `gorm:"column:created_by;type:text"` | ||
| } | ||
|
|
||
| var verifiedKeysCache *ttlcache.Cache[string, GrafanaApiKey] = ttlcache.New[string, GrafanaApiKey]() |
There was a problem hiding this comment.
Two "gotchas" about the TTL cache:
- You need to set an explicit TTL, otherwise I believe it defaults to infinite.
- If you don't invoke
Starton the cache, no eviction is ever performed. You need to hook this into some server startup routine (probably associated with the web UI?).
Further, the verified key work doesn't seem to have much to do with a generic DB layer. I think it makes more sense in web_ui.
| hash := sha256.Sum256([]byte(secret)) | ||
| return hex.EncodeToString(hash[:])[:5] | ||
| } | ||
| func VerifyGrafanaApiKey(apiKey string) (bool, error) { |
There was a problem hiding this comment.
Here -- and throughout the PR -- drop Grafana from the method names.
This is simply an API key. Just so happens that the first use case will be Grafana.
| if err != nil { | ||
| return "", err | ||
| } | ||
| return hex.EncodeToString(bytes), nil |
There was a problem hiding this comment.
Why do a hex encoding here? I'd prefer to keep things as []byte and raw bytes so it's clear what is being dealt with here.
| item := verifiedKeysCache.Get(id) | ||
| if item != nil { | ||
| cachedToken := item.Value() | ||
| beforeExpiration := time.Now().Before(cachedToken.ExpiresAt) |
There was a problem hiding this comment.
By default, if item is not nil, then the item isn't expired in the cache. This check is redundant.
| @@ -1360,6 +1366,65 @@ func listNamespacesV2(ctx *gin.Context) { | |||
| ctx.JSON(http.StatusOK, namespacesAdsV2) | |||
| } | |||
|
|
|||
| func createGrafanaToken(ctx *gin.Context) { | |||
There was a problem hiding this comment.
As mentioned above -- there's nothing here Grafana or Director specific. Move this to the web_ui module.
| @@ -1510,6 +1575,9 @@ func RegisterDirectorAPI(ctx context.Context, router *gin.RouterGroup) { | |||
| // so that director can be our point of contact for collecting system-level metrics. | |||
| // Rename the endpoint to reflect such plan. | |||
| directorAPIV1.GET("/discoverServers", discoverOriginCache) | |||
|
|
|||
| directorAPIV1.POST("/createGrafanaToken", createGrafanaToken) | |||
There was a problem hiding this comment.
I believe we have an OpenAPI / Swagger schema document, right? These should be documented there for completness.
(Also renamed to remove "grafana" as elsewhere)
| @@ -243,6 +245,16 @@ func Verify(ctx *gin.Context, authOption AuthOption) (status int, verified bool, | |||
| } else { | |||
| return http.StatusOK, true, nil | |||
| } | |||
| case GrafanaTokenIssuer: | |||
There was a problem hiding this comment.
Ok, looks like there's still work to do here -- e.g., need to implement scope checking and whatnot. I'll let you work on that.
| @@ -243,6 +245,16 @@ func Verify(ctx *gin.Context, authOption AuthOption) (status int, verified bool, | |||
| } else { | |||
| return http.StatusOK, true, nil | |||
| } | |||
| case GrafanaTokenIssuer: | |||
| fmt.Println("GOT GRAFANA TOKEN") | |||
| ok, err := database.VerifyGrafanaApiKey(token) | |||
There was a problem hiding this comment.
Thought: to prevent absolute garbage from being passed through, let's do a simple check to see if the token is of the right format. No reason why we should push a JWT down to the DB layer.
| @@ -89,7 +89,7 @@ func promQueryEngineAuthHandler(av1 *route.Router) gin.HandlerFunc { | |||
| authOption := token.AuthOption{ | |||
| // Cookie for web user access and header for external service like Grafana to access | |||
| Sources: []token.TokenSource{token.Cookie, token.Header}, | |||
| Issuers: []token.TokenIssuer{token.LocalIssuer}, | |||
| Issuers: []token.TokenIssuer{token.LocalIssuer, token.GrafanaTokenIssuer}, | |||
There was a problem hiding this comment.
Honestly, I feel like we can start accepting the token.ApiTokenIssuer issuer in many more places than this!
This PR addresses #1796. This PR currently implements a new token with its purpose to simplify access to Prometheus metrics. This token is accessible via the
Authorizationheader. There are a new HTTP route at the director for creating and deleting tokens:/api/v1.0/director/createGrafanaToken/api/v1.0/director/deleteGrafanaTokenThis PR also creates a database package as a work around to dependency cycles create between the director package and token package. Its is very rudimentary and will be improved in #1868.