From 32e9e053e63ad05e99aae90f6d341d741b682393 Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Thu, 22 Feb 2024 13:50:34 +0000 Subject: [PATCH 1/5] Added scope checking to API keys --- cmd/root.go | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 84836654..bde075b1 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -146,12 +146,33 @@ func readLocalToken(homeDir string, expectedScopes []string) (string, []string, return token.AccessToken, currentScopes, nil } +// Checkw whether or not a token has all of the required scopes. Returns a +// boolean and an error which will be populated if we couldn't read the token +func tokenHasAllScopes(token string, requiredScopes []string) (bool, error) { + claims, err := extractClaims(token) + + if err != nil { + return false, fmt.Errorf("error extracting claims from token: %w", err) + } + + // Check that the token has the right scopes + for _, scope := range requiredScopes { + if !claims.HasScope(scope) { + return false, nil + } + } + + return true, nil +} + // ensureToken +// TODO: This needs to complain if the permissions aren't right when using an API key func ensureToken(ctx context.Context, requiredScopes []string) (context.Context, error) { // get a token from the api key if present if viper.GetString("api-key") != "" { log.WithContext(ctx).Debug("using provided token for authentication") apiKey := viper.GetString("api-key") + var accessToken string if strings.HasPrefix(apiKey, "ovm_api_") { // exchange api token for JWT client := UnauthenticatedApiKeyClient(ctx) @@ -164,11 +185,22 @@ func ensureToken(ctx context.Context, requiredScopes []string) (context.Context, return ctx, fmt.Errorf("error authenticating the API token: %w", err) } log.WithContext(ctx).Debug("successfully authenticated") - apiKey = resp.Msg.GetAccessToken() + accessToken = resp.Msg.GetAccessToken() } else { return ctx, errors.New("OVM_API_KEY does not match pattern 'ovm_api_*'") } - return context.WithValue(ctx, sdp.UserTokenContextKey{}, apiKey), nil + + // Validate that the API_KEY has the required scopes + ok, err := tokenHasAllScopes(accessToken, requiredScopes) + + if err != nil { + return ctx, fmt.Errorf("error checking token scopes in API KEY token: %w", err) + } else if !ok { + return ctx, fmt.Errorf("authenticated successfully, but your API key doesn't have the required permissions: %v", requiredScopes) + } else { + // In this case the token is good + return context.WithValue(ctx, sdp.UserTokenContextKey{}, accessToken), nil + } } var localScopes []string @@ -302,16 +334,16 @@ func getChangeUuid(ctx context.Context, expectedStatus sdp.ChangeStatus, errNotF // Finally look through all open changes to find one with a matching ticket link client := AuthenticatedChangesClient(ctx) - var maybeChangeUuid *uuid.UUID changesList, err := client.ListChangesByStatus(ctx, &connect.Request[sdp.ListChangesByStatusRequest]{ Msg: &sdp.ListChangesByStatusRequest{ Status: expectedStatus, }, }) if err != nil { - return uuid.Nil, fmt.Errorf("failed to searching for existing changes: %w", err) + return uuid.Nil, fmt.Errorf("failed to search for existing changes: %w", err) } + var maybeChangeUuid *uuid.UUID for _, c := range changesList.Msg.GetChanges() { if c.GetProperties().GetTicketLink() == ticketLink { maybeChangeUuid = c.GetMetadata().GetUUIDParsed() From 8cb5654765b3e1993d94ebcb259bd052f78dfe81 Mon Sep 17 00:00:00 2001 From: Dylan Date: Thu, 22 Feb 2024 13:54:39 +0000 Subject: [PATCH 2/5] Update spelling Co-authored-by: David Schmitt <118179693+DavidS-ovm@users.noreply.github.com> --- cmd/root.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/root.go b/cmd/root.go index bde075b1..6331b694 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -146,7 +146,7 @@ func readLocalToken(homeDir string, expectedScopes []string) (string, []string, return token.AccessToken, currentScopes, nil } -// Checkw whether or not a token has all of the required scopes. Returns a +// Check whether or not a token has all of the required scopes. Returns a // boolean and an error which will be populated if we couldn't read the token func tokenHasAllScopes(token string, requiredScopes []string) (bool, error) { claims, err := extractClaims(token) From e37ace372fe38efb7a32e0db27e8b6b88aa7d19b Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Thu, 22 Feb 2024 14:13:38 +0000 Subject: [PATCH 3/5] Refactor auth to make a lot more sense --- cmd/root.go | 202 ++++++++++++++++++++++++++++------------------------ 1 file changed, 107 insertions(+), 95 deletions(-) diff --git a/cmd/root.go b/cmd/root.go index 6331b694..84c1a218 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -165,44 +165,35 @@ func tokenHasAllScopes(token string, requiredScopes []string) (bool, error) { return true, nil } -// ensureToken -// TODO: This needs to complain if the permissions aren't right when using an API key -func ensureToken(ctx context.Context, requiredScopes []string) (context.Context, error) { - // get a token from the api key if present - if viper.GetString("api-key") != "" { - log.WithContext(ctx).Debug("using provided token for authentication") - apiKey := viper.GetString("api-key") - var accessToken string - if strings.HasPrefix(apiKey, "ovm_api_") { - // exchange api token for JWT - client := UnauthenticatedApiKeyClient(ctx) - resp, err := client.ExchangeKeyForToken(ctx, &connect.Request[sdp.ExchangeKeyForTokenRequest]{ - Msg: &sdp.ExchangeKeyForTokenRequest{ - ApiKey: apiKey, - }, - }) - if err != nil { - return ctx, fmt.Errorf("error authenticating the API token: %w", err) - } - log.WithContext(ctx).Debug("successfully authenticated") - accessToken = resp.Msg.GetAccessToken() - } else { - return ctx, errors.New("OVM_API_KEY does not match pattern 'ovm_api_*'") - } - - // Validate that the API_KEY has the required scopes - ok, err := tokenHasAllScopes(accessToken, requiredScopes) - +// Gets a token using an API key +func getAPIKeyToken(ctx context.Context, apiKey string) (string, error) { + log.WithContext(ctx).Debug("using provided token for authentication") + + var accessToken string + + if strings.HasPrefix(apiKey, "ovm_api_") { + // exchange api token for JWT + client := UnauthenticatedApiKeyClient(ctx) + resp, err := client.ExchangeKeyForToken(ctx, &connect.Request[sdp.ExchangeKeyForTokenRequest]{ + Msg: &sdp.ExchangeKeyForTokenRequest{ + ApiKey: apiKey, + }, + }) if err != nil { - return ctx, fmt.Errorf("error checking token scopes in API KEY token: %w", err) - } else if !ok { - return ctx, fmt.Errorf("authenticated successfully, but your API key doesn't have the required permissions: %v", requiredScopes) - } else { - // In this case the token is good - return context.WithValue(ctx, sdp.UserTokenContextKey{}, accessToken), nil + return "", fmt.Errorf("error authenticating the API token: %w", err) } + log.WithContext(ctx).Debug("successfully authenticated") + accessToken = resp.Msg.GetAccessToken() + } else { + return "", errors.New("OVM_API_KEY does not match pattern 'ovm_api_*'") } + return accessToken, nil +} + +// Gets a token from Oauth with the required scopes. This method will also cache +// that token locally for use later, and will use the cached token if possible +func getOauthToken(ctx context.Context, requiredScopes []string) (string, error) { var localScopes []string // Check for a locally saved token in ~/.overmind @@ -214,92 +205,113 @@ func ensureToken(ctx context.Context, requiredScopes []string) (context.Context, if err != nil { log.WithContext(ctx).Debugf("Error reading local token, ignoring: %v", err) } else { - return context.WithValue(ctx, sdp.UserTokenContextKey{}, localToken), nil + // If we already have the right scopes, return the token + return localToken, nil } } + // If we need to get a new token, request the required scopes on top of + // whatever ones the current local, valid token has so that we don't + // keep replacing it + // Check to see if the URL is secure appurl := viper.GetString("url") parsed, err := url.Parse(appurl) if err != nil { log.WithContext(ctx).WithError(err).Error("Failed to parse --url") - return ctx, fmt.Errorf("error parsing --url: %w", err) - } - - if parsed.Scheme == "wss" || parsed.Scheme == "https" || parsed.Hostname() == "localhost" { - // If we need to get a new token, request the required scopes on top of - // whatever ones the current local, valid token has so that we don't - // keep replacing it - requestScopes := append(requiredScopes, localScopes...) - - // Authenticate using the oauth device authorization flow - config := oauth2.Config{ - ClientID: viper.GetString("cli-auth0-client-id"), - Endpoint: oauth2.Endpoint{ - AuthURL: fmt.Sprintf("https://%v/authorize", viper.GetString("cli-auth0-domain")), - TokenURL: fmt.Sprintf("https://%v/oauth/token", viper.GetString("cli-auth0-domain")), - DeviceAuthURL: fmt.Sprintf("https://%v/oauth/device/code", viper.GetString("cli-auth0-domain")), - }, - Scopes: requestScopes, - } + return "", fmt.Errorf("error parsing --url: %w", err) + } - deviceCode, err := config.DeviceAuth(ctx, oauth2.SetAuthURLParam("audience", "https://api.overmind.tech")) - if err != nil { - return ctx, fmt.Errorf("error getting device code: %w", err) - } + if !(parsed.Scheme == "wss" || parsed.Scheme == "https" || parsed.Hostname() == "localhost") { + return "", fmt.Errorf("target URL (%v) is insecure", parsed) + } + // If we need to get a new token, request the required scopes on top of + // whatever ones the current local, valid token has so that we don't + // keep replacing it + requestScopes := append(requiredScopes, localScopes...) + + // Authenticate using the oauth device authorization flow + config := oauth2.Config{ + ClientID: viper.GetString("cli-auth0-client-id"), + Endpoint: oauth2.Endpoint{ + AuthURL: fmt.Sprintf("https://%v/authorize", viper.GetString("cli-auth0-domain")), + TokenURL: fmt.Sprintf("https://%v/oauth/token", viper.GetString("cli-auth0-domain")), + DeviceAuthURL: fmt.Sprintf("https://%v/oauth/device/code", viper.GetString("cli-auth0-domain")), + }, + Scopes: requestScopes, + } + + deviceCode, err := config.DeviceAuth(ctx, oauth2.SetAuthURLParam("audience", "https://api.overmind.tech")) + if err != nil { + return "", fmt.Errorf("error getting device code: %w", err) + } + + fmt.Printf("Go to %v and verify this code: %v\n", deviceCode.VerificationURIComplete, deviceCode.UserCode) + + token, err := config.DeviceAccessToken(ctx, deviceCode) + if err != nil { + fmt.Printf(": %v\n", err) + return "", fmt.Errorf("Error exchanging Device Code for for access token: %w", err) + } - fmt.Printf("Go to %v and verify this code: %v\n", deviceCode.VerificationURIComplete, deviceCode.UserCode) + log.WithContext(ctx).Info("Authenticated successfully ✅") - token, err := config.DeviceAccessToken(ctx, deviceCode) + // Save the token locally + if home, err := os.UserHomeDir(); err == nil { + // Create the directory if it doesn't exist + err = os.MkdirAll(filepath.Join(home, ".overmind"), 0700) if err != nil { - fmt.Printf(": %v\n", err) - return ctx, fmt.Errorf("Error exchanging Device Code for for access token: %w", err) + log.WithContext(ctx).WithError(err).Error("Failed to create ~/.overmind directory") } - // Check that we actually got the claims we asked for. If you don't have - // permission auth0 will just not assign those scopes rather than fail - claims, err := extractClaims(token.AccessToken) - + // Write the token to a file + path := filepath.Join(home, ".overmind", "token.json") + file, err := os.Create(path) if err != nil { - return ctx, fmt.Errorf("error extracting claims from token: %w", err) + log.WithContext(ctx).WithError(err).Errorf("Failed to create token file at %v", path) } - for _, scope := range requiredScopes { - if !claims.HasScope(scope) { - return ctx, fmt.Errorf("authenticated successfully, but you don't have the required permission: '%v'", scope) - } + // Encode the token + err = json.NewEncoder(file).Encode(token) + if err != nil { + log.WithContext(ctx).WithError(err).Errorf("Failed to encode token file at %v", path) } - log.WithContext(ctx).Info("Authenticated successfully ✅") + log.WithContext(ctx).Debugf("Saved token to %v", path) + } - // Save the token locally - if home, err := os.UserHomeDir(); err == nil { - // Create the directory if it doesn't exist - err = os.MkdirAll(filepath.Join(home, ".overmind"), 0700) - if err != nil { - log.WithContext(ctx).WithError(err).Error("Failed to create ~/.overmind directory") - } + return token.AccessToken, nil +} - // Write the token to a file - path := filepath.Join(home, ".overmind", "token.json") - file, err := os.Create(path) - if err != nil { - log.WithContext(ctx).WithError(err).Errorf("Failed to create token file at %v", path) - } +// ensureToken +// TODO: This needs to complain if the permissions aren't right when using an API key +func ensureToken(ctx context.Context, requiredScopes []string) (context.Context, error) { + var accessToken string + var err error - // Encode the token - err = json.NewEncoder(file).Encode(token) - if err != nil { - log.WithContext(ctx).WithError(err).Errorf("Failed to encode token file at %v", path) - } + // get a token from the api key if present + if apiKey := viper.GetString("api-key"); apiKey != "" { + accessToken, err = getAPIKeyToken(ctx, apiKey) + } else { + accessToken, err = getOauthToken(ctx, requiredScopes) + } - log.WithContext(ctx).Debugf("Saved token to %v", path) - } + // Check that we actually got the claims we asked for. If you don't have + // permission auth0 will just not assign those scopes rather than fail + claims, err := extractClaims(accessToken) - // Set the token - return context.WithValue(ctx, sdp.UserTokenContextKey{}, token.AccessToken), nil + if err != nil { + return ctx, fmt.Errorf("error extracting claims from token: %w", err) } - return ctx, fmt.Errorf("no OVM_API_KEY configured and target URL (%v) is insecure", parsed) + + for _, scope := range requiredScopes { + if !claims.HasScope(scope) { + return ctx, fmt.Errorf("authenticated successfully, but you don't have the required permission: '%v'", scope) + } + } + + // Add the token to the context + return context.WithValue(ctx, sdp.UserTokenContextKey{}, accessToken), nil } // getChangeUuid returns the UUID of a change, as selected by --uuid or --change, or a state with the specified status and having --ticket-link From b80e48269d5bf38c21c17a63673d766a417dacd3 Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Thu, 22 Feb 2024 14:15:07 +0000 Subject: [PATCH 4/5] Remove TODO --- cmd/root.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/root.go b/cmd/root.go index 84c1a218..d08c3d4c 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -284,7 +284,6 @@ func getOauthToken(ctx context.Context, requiredScopes []string) (string, error) } // ensureToken -// TODO: This needs to complain if the permissions aren't right when using an API key func ensureToken(ctx context.Context, requiredScopes []string) (context.Context, error) { var accessToken string var err error From 376eb4dd216c62c9acb49e3dc66f0aa0c3a80d99 Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Thu, 22 Feb 2024 14:15:48 +0000 Subject: [PATCH 5/5] Added more error handling --- cmd/root.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmd/root.go b/cmd/root.go index d08c3d4c..3809c66c 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -295,6 +295,10 @@ func ensureToken(ctx context.Context, requiredScopes []string) (context.Context, accessToken, err = getOauthToken(ctx, requiredScopes) } + if err != nil { + return ctx, fmt.Errorf("error getting token: %w", err) + } + // Check that we actually got the claims we asked for. If you don't have // permission auth0 will just not assign those scopes rather than fail claims, err := extractClaims(accessToken)