Skip to content

Commit

Permalink
oauthtoken: Implement Fallback backend (#37)
Browse files Browse the repository at this point in the history
This change adds two new token storage backends:
* a file-based backend for use in CI systems, as proposed by #34 
* a fallback backend, which consults multiple backends in order when
loading a token

The backend to use when storing tokens is controlled by the top-level
`--store` flag, which defaults to `keyring` (a secure option); usage in
CI will need to set `--store=file` if `keyring` is not expected to work
(e.g. inside a Docker container with no GUI libraries installed). Both
backends are consulted on `Load()` and `Delete()` operations, though
this shouldn't be insecure, as it doesn't leak token info into the
filesystem unless explicitly requested via the flag.

Tests are added for both backends; the fake backend implementation is
also fixed to obey the `LoadStorer` contract around returning
`fs.ErrNotExist`, which means also fixing `engflow_auth logout`
semantics when no token is stored for the current cluster.

Tested: some manual testing still TODO:
- [x] test that delete for cluster with no token returns error
- [x] test that default stores in keyring, token still fetchable
- [x] test that delete succeeds once and fails on subsequent attempts
(token not found)
- [x] test storing into file, token still fetchable
- [x] test that delete succeeds once and fails on subsequent attempts
(token not found)

Bug: linear/CUS-384
  • Loading branch information
minor-fixes authored Aug 23, 2024
1 parent e3972d1 commit 1363f11
Show file tree
Hide file tree
Showing 10 changed files with 615 additions and 45 deletions.
96 changes: 79 additions & 17 deletions cmd/engflow_auth/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@ import (
"encoding/json"
"errors"
"fmt"
"io/fs"
"net"
"net/url"
"os"
"os/signal"
"path/filepath"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -73,12 +76,50 @@ func (r *appState) build(cliCtx *cli.Context) error {
r.browserOpener = &browser.StderrPrint{}
}
if r.tokenStore == nil {
tokenStore, err := oauthtoken.NewKeyring()
keyring, err := oauthtoken.NewKeyring()
if err != nil {
return autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to open token store: %w", err)
return autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to open keyring-based token store: %w", err)
}

r.tokenStore = oauthtoken.NewCacheAlert(tokenStore, cliCtx.App.ErrWriter)
userConfigDir, err := os.UserConfigDir()
if err != nil {
return autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to discover user's config dir: %w", err)
}
tokensDir := filepath.Join(userConfigDir, "engflow_auth", "tokens")
fileStore, err := oauthtoken.NewFileTokenStore(tokensDir)
if err != nil {
return autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to open file-based token store: %w", err)
}

errorStore := &oauthtoken.FakeTokenStore{
StoreErr: fmt.Errorf("subcommand attempted invalid write to token storage"),
}

var writeStore oauthtoken.LoadStorer
switch writeStoreName := cliCtx.String("store"); writeStoreName {
case "keyring":
writeStore = keyring
case "file":
writeStore = fileStore
case "":
// Subcommands that don't have this flag defined will cause the flag
// value fetch to return empty (as opposed to a sane default value).
// These commands shouldn't write to token storage (else they should
// define the flag) so the corresponding token storage object errors
// on writes.
writeStore = errorStore
default:
return autherr.CodedErrorf(autherr.CodeBadParams, "unknown token store type %q", writeStoreName)
}

r.tokenStore =
oauthtoken.NewCacheAlert(
oauthtoken.NewFallback(
/* gets Store() operations */ writeStore,
/* gets Load() operations */ keyring, fileStore,
),
cliCtx.App.ErrWriter,
)
}
return nil
}
Expand Down Expand Up @@ -279,7 +320,9 @@ func (r *appState) logout(cliCtx *cli.Context) error {
return autherr.CodedErrorf(autherr.CodeBadParams, "invalid cluster: %w", err)
}

if err := r.tokenStore.Delete(clusterURL.Host); err != nil {
if err := r.tokenStore.Delete(clusterURL.Host); errors.Is(err, fs.ErrNotExist) {
return &autherr.CodedError{Code: autherr.CodeBadParams, Err: fmt.Errorf("no credentials found for cluster %q", clusterURL.Host)}
} else if err != nil {
return &autherr.CodedError{Code: autherr.CodeTokenStoreFailure, Err: err}
}
return nil
Expand All @@ -291,6 +334,23 @@ func (r *appState) version(cliCtx *cli.Context) error {
}

func makeApp(root *appState) *cli.App {
storeFlag := &cli.StringFlag{
Name: "store",
Usage: "Name of backend that should be used for token store operations",
Value: "keyring",
Action: func(ctx *cli.Context, s string) error {
allowedStoreBackends := []string{"keyring", "file"}
if !slices.Contains(allowedStoreBackends, s) {
return autherr.CodedErrorf(autherr.CodeBadParams, "invalid value %q for --store. Allowed values: %v", s, allowedStoreBackends)
}
return nil
},
}
aliasFlag := &cli.StringSliceFlag{
Name: "alias",
Usage: "Comma-separated list of alias hostnames for this cluster",
}

app := &cli.App{
Name: "engflow_auth",
Usage: "Authenticate to EngFlow remote build clusters",
Expand All @@ -312,16 +372,12 @@ credential helper protocol.`),
Usage: "Prints the currently-stored token for the specified cluster to stdout",
ArgsUsage: " CLUSTER_URL",
Action: root.export,
Flags: []cli.Flag{
&cli.StringSliceFlag{
Name: "alias",
Usage: "Comma-separated list of alias hostnames for this cluster",
},
},
Flags: []cli.Flag{aliasFlag},
},
{
Name: "import",
Usage: "Imports a data blob containing auth token(s) exported via `engflow_auth export` from stdin",
Flags: []cli.Flag{storeFlag},
Action: root.import_,
},
{
Expand All @@ -331,12 +387,7 @@ credential helper protocol.`),
Initiates an interactive OAuth2 flow to log into the cluster at
CLUSTER_URL.`),
Action: root.login,
Flags: []cli.Flag{
&cli.StringSliceFlag{
Name: "alias",
Usage: "Comma-separated list of alias hostnames for this cluster",
},
},
Flags: []cli.Flag{aliasFlag, storeFlag},
},
{
Name: "logout",
Expand All @@ -359,6 +410,17 @@ Erases the credentials for the named cluster from the local machine.`),
// we will take care of calling os.Exit().
},
}

// Ensure that all usage errors get an error code, for consistency with
// other error exit conditions.
usageErrWrapper := func(cliCtx *cli.Context, err error, isSubcommand bool) error {
return autherr.CodedErrorf(autherr.CodeBadParams, "%w", err)
}
app.OnUsageError = usageErrWrapper
for _, cmd := range app.Commands {
cmd.OnUsageError = usageErrWrapper
}

return app
}

Expand Down Expand Up @@ -418,7 +480,7 @@ func sanitizedURL(cluster string) (*url.URL, error) {
// - `Port` is optional, defaulting to whatever is implied by `Scheme`
// - All other fields are forbidden
if clusterURL.Scheme != "https" {
return nil, fmt.Errorf("illegal scheme %q; only 'https' is supported", clusterURL.Scheme)
return nil, fmt.Errorf("invalid scheme %q; only 'https' is supported", clusterURL.Scheme)
}
if clusterURL.Host == "" {
return nil, fmt.Errorf("cluster URL %q does not specify a host", clusterURL)
Expand Down
47 changes: 41 additions & 6 deletions cmd/engflow_auth/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,11 @@ func TestRun(t *testing.T) {
tokenStore oauthtoken.LoadStorer
browserOpener browser.Opener

wantCode int
wantErr string
wantCode int
wantErr string
// If the command fails CLI flag/arg parsing, fill in wantUsageErr
// instead of wantErr.
wantUsageErr string
wantStdoutContaining []string
wantStderrContaining []string
wantStored []string
Expand Down Expand Up @@ -258,7 +261,7 @@ func TestRun(t *testing.T) {
desc: "login with invalid scheme",
args: []string{"login", "grpcs://cluster.example.com:8080"},
wantCode: autherr.CodeBadParams,
wantErr: "illegal scheme",
wantErr: "invalid scheme",
},
{
desc: "login code fetch failure",
Expand Down Expand Up @@ -336,15 +339,47 @@ func TestRun(t *testing.T) {
wantCode: autherr.CodeTokenStoreFailure,
wantErr: "token_store_fail",
},
{
desc: "login with file-backed token storage",
args: []string{"login", "--store=file", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
},
},
{
desc: "login with keyring-backed token storage",
args: []string{"login", "--store=keyring", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
},
},
{
desc: "login with empty token storage type",
args: []string{"login", "--store=", "cluster.example.com"},
wantCode: autherr.CodeBadParams,
wantErr: `invalid value "" for --store`,
},
{
desc: "login with misplaced store flag",
args: []string{"--store=file", "login", "cluster.example.com"},
wantCode: autherr.CodeBadParams,
wantErr: "flag provided but not defined",
},
{
desc: "logout without cluster",
args: []string{"logout"},
wantCode: autherr.CodeBadParams,
wantErr: "expected exactly 1 positional argument",
},
{
desc: "logout with unknown cluster",
args: []string{"logout", "unknown.example.com"},
desc: "logout with unknown cluster",
args: []string{"logout", "unknown.example.com"},
wantCode: autherr.CodeBadParams,
wantErr: "no credentials found for cluster",
},
{
desc: "logout with cluster",
Expand Down Expand Up @@ -374,7 +409,7 @@ func TestRun(t *testing.T) {
desc: "export with invalid cluster URL",
args: []string{"export", "grpcs://cluster.example.com:8080"},
wantCode: autherr.CodeBadParams,
wantErr: "illegal scheme",
wantErr: "invalid scheme",
},
{
desc: "export when token not found",
Expand Down
6 changes: 4 additions & 2 deletions internal/oauthtoken/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ go_library(
"cache_alert.go",
"debug.go",
"fake.go",
"fallback.go",
"file.go",
"keyring.go",
"load_storer.go",
],
importpath = "github.com/EngFlow/auth/internal/oauthtoken",
visibility = ["//:__subpackages__"],
deps = [
"//internal/autherr",
"@com_github_golang_jwt_jwt_v5//:jwt",
"@com_github_zalando_go_keyring//:go-keyring",
"@org_golang_x_oauth2//:oauth2",
Expand All @@ -23,11 +24,12 @@ go_test(
name = "oauthtoken_test",
srcs = [
"cache_alert_test.go",
"fallback_test.go",
"keyring_test.go",
"load_storer_test.go",
],
embed = [":oauthtoken"],
deps = [
"//internal/autherr",
"@com_github_golang_jwt_jwt_v5//:jwt",
"@com_github_google_uuid//:uuid",
"@com_github_stretchr_testify//assert",
Expand Down
24 changes: 24 additions & 0 deletions internal/oauthtoken/fake.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package oauthtoken

import (
"fmt"
"io/fs"

"golang.org/x/oauth2"
)
Expand Down Expand Up @@ -58,6 +59,29 @@ func (f *FakeTokenStore) Delete(cluster string) error {
if f.DeleteErr != nil {
return f.DeleteErr
}
if _, ok := f.Tokens[cluster]; !ok {
return fs.ErrNotExist
}
delete(f.Tokens, cluster)
return nil
}

func (f *FakeTokenStore) WithToken(cluster string, token *oauth2.Token) *FakeTokenStore {
f.Tokens[cluster] = token
return f
}

func (f *FakeTokenStore) WithLoadErr(err error) *FakeTokenStore {
f.LoadErr = err
return f
}

func (f *FakeTokenStore) WithStoreErr(err error) *FakeTokenStore {
f.StoreErr = err
return f
}

func (f *FakeTokenStore) WithDeleteErr(err error) *FakeTokenStore {
f.DeleteErr = err
return f
}
Loading

0 comments on commit 1363f11

Please sign in to comment.