-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Grafana Token API Access #1917
base: main
Are you sure you want to change the base?
Grafana Token API Access #1917
Changes from all commits
8cdeed0
5c89dd3
960e54f
11dc065
c20da91
827bce0
5ac3527
0bc8348
3488587
74642e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
package database | ||
|
||
import ( | ||
"crypto/rand" | ||
"crypto/sha256" | ||
"encoding/hex" | ||
"fmt" | ||
"strings" | ||
"time" | ||
|
||
"github.com/jellydator/ttlcache/v3" | ||
"github.com/pkg/errors" | ||
"golang.org/x/crypto/bcrypt" | ||
"gorm.io/gorm" | ||
) | ||
|
||
var DirectorDB *gorm.DB | ||
|
||
type GrafanaApiKey struct { | ||
ID string `gorm:"primaryKey;column:id;type:text;not null"` | ||
Name string `gorm:"column:name;type:text"` | ||
HashedValue string `gorm:"column:hashed_value;type:text;not null"` | ||
Scopes string `gorm:"column:scopes;type:text"` | ||
ExpiresAt time.Time | ||
CreatedAt time.Time | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two "gotchas" about the TTL cache:
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 |
||
|
||
func generateSecret(length int) (string, error) { | ||
bytes := make([]byte, length) | ||
_, err := rand.Read(bytes) | ||
if err != nil { | ||
return "", err | ||
} | ||
return hex.EncodeToString(bytes), nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do a hex encoding here? I'd prefer to keep things as |
||
} | ||
|
||
func generateTokenID(secret string) string { | ||
hash := sha256.Sum256([]byte(secret)) | ||
return hex.EncodeToString(hash[:])[:5] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the birthday paradox, what's the likelihood of collision if we use 5 characters and have, say, 1,000 tokens? |
||
} | ||
func VerifyGrafanaApiKey(apiKey string) (bool, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here -- and throughout the PR -- drop This is simply an API key. Just so happens that the first use case will be Grafana. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're verifying here, we should also return any scopes that the key has. |
||
parts := strings.Split(apiKey, ".") | ||
if len(parts) != 2 { | ||
return false, errors.New("invalid API key format") | ||
} | ||
id := parts[0] | ||
secret := parts[1] | ||
|
||
item := verifiedKeysCache.Get(id) | ||
if item != nil { | ||
cachedToken := item.Value() | ||
beforeExpiration := time.Now().Before(cachedToken.ExpiresAt) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By default, if |
||
matches := bcrypt.CompareHashAndPassword([]byte(cachedToken.HashedValue), []byte(secret)) == nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm -- I think this logic is missing the point of doing a cache of verified tokens. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On further thought, since you want to do a lookup for deletion by the ID, maybe the correct cache setup is something like:
|
||
if beforeExpiration && matches { | ||
return true, nil | ||
} | ||
} | ||
|
||
var token GrafanaApiKey | ||
result := DirectorDB.First(&token, "id = ?", id) | ||
if result.Error != nil { | ||
if errors.Is(result.Error, gorm.ErrRecordNotFound) { | ||
return false, nil // token not found | ||
} | ||
return false, errors.Wrap(result.Error, "failed to retrieve the Grafana API key") | ||
} | ||
|
||
if time.Now().After(token.ExpiresAt) { | ||
return false, nil | ||
} | ||
|
||
err := bcrypt.CompareHashAndPassword([]byte(token.HashedValue), []byte(secret)) | ||
if err != nil { | ||
return false, nil | ||
} | ||
|
||
verifiedKeysCache.Set(id, token, ttlcache.DefaultTTL) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the remaining time before token expiration is less than the DefaultTTL, shorten the overall lifetime. That is, don't cache it for longer than it will be valid! |
||
return true, nil | ||
} | ||
|
||
func CreateGrafanaApiKey(name, createdBy, scopes string) (string, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Expiration time should be caller-controlled. (This will likely be a drop-down in the web UI with "no expiration" as one option). |
||
expiresAt := time.Now().Add(time.Hour * 24 * 30) // 30 days | ||
for { | ||
secret, err := generateSecret(32) | ||
if err != nil { | ||
return "", errors.Wrap(err, "failed to generate a secret") | ||
} | ||
|
||
id := generateTokenID(secret) | ||
|
||
hashedValue, err := bcrypt.GenerateFromPassword([]byte(secret), bcrypt.DefaultCost) | ||
if err != nil { | ||
return "", errors.Wrap(err, "failed to hash the secret") | ||
} | ||
|
||
apiKey := GrafanaApiKey{ | ||
ID: id, | ||
Name: name, | ||
HashedValue: string(hashedValue), | ||
Scopes: scopes, | ||
ExpiresAt: expiresAt, | ||
CreatedAt: time.Now(), | ||
CreatedBy: createdBy, | ||
} | ||
result := DirectorDB.Create(apiKey) | ||
if result.Error != nil { | ||
isConstraintError := result.Error.Error() == "UNIQUE constraint failed: tokens.id" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't do string comparison, check to see if the error is of type |
||
if !isConstraintError { | ||
return "", errors.Wrap(result.Error, "failed to create a new Grafana API key") | ||
} | ||
// If the ID is already taken, try again | ||
continue | ||
} | ||
return fmt.Sprintf("%s.%s", id, secret), nil | ||
} | ||
} | ||
|
||
func DeleteGrafanaApiKey(id string) error { | ||
result := DirectorDB.Delete(&GrafanaApiKey{}, "id = ?", id) | ||
if result.Error != nil { | ||
return errors.Wrap(result.Error, "failed to delete the Grafana API key") | ||
} | ||
if result.RowsAffected == 0 { | ||
return errors.New("API key not found") | ||
} | ||
// delete from cache so that we don't accidentally allow the deleted key to be used | ||
verifiedKeysCache.Delete(id) | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ import ( | |
"golang.org/x/sync/errgroup" | ||
|
||
"github.com/pelicanplatform/pelican/config" | ||
"github.com/pelicanplatform/pelican/database" | ||
"github.com/pelicanplatform/pelican/metrics" | ||
"github.com/pelicanplatform/pelican/param" | ||
"github.com/pelicanplatform/pelican/pelican_url" | ||
|
@@ -77,6 +78,11 @@ type ( | |
|
||
// Context key for the project name | ||
ProjectContextKey struct{} | ||
|
||
CreateGrafanaTokenReq struct { | ||
Name string `json:"name"` | ||
CreatedBy string `json:"created_by"` | ||
} | ||
) | ||
|
||
const ( | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned above -- there's nothing here Grafana or Director specific. Move this to the |
||
authOption := token.AuthOption{ | ||
Sources: []token.TokenSource{token.Cookie}, | ||
Issuers: []token.TokenIssuer{token.LocalIssuer}, | ||
Scopes: []token_scopes.TokenScope{token_scopes.WebUi_Access}, | ||
} | ||
|
||
status, ok, err := token.Verify(ctx, authOption) | ||
if !ok { | ||
log.Warningf("Cannot verify token: %v", err) | ||
ctx.JSON(status, server_structs.SimpleApiResp{ | ||
Status: server_structs.RespFailed, | ||
Msg: err.Error(), | ||
}) | ||
return | ||
} | ||
// marshall body into struct | ||
var req CreateGrafanaTokenReq | ||
err = ctx.ShouldBindJSON(&req) | ||
if err != nil { | ||
ctx.JSON(http.StatusBadRequest, server_structs.SimpleApiResp{ | ||
Status: server_structs.RespFailed, | ||
Msg: fmt.Sprintf("Invalid request body: %v", err), | ||
}) | ||
return | ||
} | ||
|
||
scopes := fmt.Sprintf("%s,%s", token_scopes.Monitoring_Query.String(), token_scopes.Monitoring_Scrape.String()) | ||
token, err := database.CreateGrafanaApiKey(req.Name, req.CreatedBy, scopes) | ||
if err != nil { | ||
log.Warning("Failed to create Grafana API key: ", err) | ||
ctx.JSON(status, server_structs.SimpleApiResp{ | ||
Status: server_structs.RespFailed, | ||
Msg: err.Error(), | ||
}) | ||
return | ||
} | ||
|
||
ctx.JSON(http.StatusOK, gin.H{"token": token}) | ||
} | ||
|
||
func deleteGrafanaToken(ctx *gin.Context) { | ||
id := ctx.Param("id") | ||
err := database.DeleteGrafanaApiKey(id) | ||
if err != nil { | ||
log.Warning("Failed to delete Grafana API key: ", err) | ||
ctx.JSON(http.StatusInternalServerError, server_structs.SimpleApiResp{ | ||
Status: server_structs.RespFailed, | ||
Msg: err.Error(), | ||
}) | ||
return | ||
} | ||
|
||
ctx.JSON(http.StatusOK, server_structs.SimpleApiResp{ | ||
Status: server_structs.RespOK, | ||
Msg: "Grafana API key deleted", | ||
}) | ||
} | ||
|
||
func getPrefixByPath(ctx *gin.Context) { | ||
pathParam := ctx.Param("path") | ||
if pathParam == "" || pathParam == "/" { | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe we have an OpenAPI / Swagger schema document, right? These should be documented there for completness. (Also renamed to remove "grafana" as elsewhere) |
||
directorAPIV1.DELETE("/deleteGrafanaToken/:id", deleteGrafanaToken) | ||
} | ||
|
||
directorAPIV2 := router.Group("/api/v2.0/director") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
-- +goose Up | ||
-- +goose StatementBegin | ||
SELECT 'up SQL query'; | ||
-- +goose StatementEnd | ||
|
||
CREATE TABLE IF NOT EXISTS server_downtimes ( | ||
uuid TEXT PRIMARY KEY, | ||
name TEXT NOT NULL UNIQUE, | ||
filter_type TEXT NOT NULL, | ||
created_at DATETIME NOT NULL, | ||
updated_at DATETIME NOT NULL | ||
); | ||
|
||
CREATE TABLE grafana_api_keys ( | ||
id TEXT PRIMARY KEY, | ||
name TEXT, | ||
hashed_value TEXT NOT NULL, | ||
scopes TEXT, | ||
expires_at DATETIME, | ||
created_at DATETIME DEFAULT CURRENT_TIMESTAMP, | ||
created_by TEXT | ||
); | ||
|
||
-- +goose Down | ||
-- +goose StatementBegin | ||
SELECT 'down SQL query'; | ||
-- +goose StatementEnd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not being familiar with GORM, does this also add a uniqueness requirement to the column?