From 7ef63df5bdb71ffad46db9faea41a5023a4597fc Mon Sep 17 00:00:00 2001 From: Aaron Spratt <19199893+aspratt@users.noreply.github.com> Date: Tue, 20 Oct 2026 04:54:46 +0000 Subject: [PATCH 1/2] fix(okta): random per-install device token kept in the keychain (#23) The Okta device token was predictable (okta__saml2aws). Each install now generates a random token and stores it in the keychain. --- pkg/provider/okta/okta.go | 35 ++++++-- pkg/provider/okta/okta_devicetoken_test.go | 98 ++++++++++++++++++++++ 2 files changed, 128 insertions(+), 5 deletions(-) create mode 100644 pkg/provider/okta/okta_devicetoken_test.go diff --git a/pkg/provider/okta/okta.go b/pkg/provider/okta/okta.go index 52690b2f3..3833100e1 100644 --- a/pkg/provider/okta/okta.go +++ b/pkg/provider/okta/okta.go @@ -20,6 +20,7 @@ import ( "time" "github.com/PuerkitoBio/goquery" + "github.com/google/uuid" "github.com/marshallbrekka/go-u2fhost" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -360,10 +361,34 @@ func (oc *Client) getDeviceTokenFromOkta(loginDetails *creds.LoginDetails) (stri return "", fmt.Errorf("unable to get a device token from okta") } +// deviceToken returns the Okta device token for this install. A random UUID is +// generated on first use and kept in the credential store under +// /deviceToken, so Okta keeps remembering the device across runs. +// Without a credential store the legacy username-derived value is used, which +// keeps "remember this device" working for those installs. +func deviceToken(loginDetails *creds.LoginDetails) string { + legacy := fmt.Sprintf("okta_%s_saml2aws", loginDetails.Username) + if !credentials.SupportsStorage() { + logger.Debug("no credential store, using legacy okta device token") + return legacy + } + + key := loginDetails.URL + "/deviceToken" + if _, dt, err := credentials.CurrentHelper.Get(key); err == nil && dt != "" { + return dt + } + + dt := uuid.NewString() + if err := credentials.SaveCredentials(key, loginDetails.Username, dt); err != nil { + logger.Debugf("unable to store okta device token, using legacy value | err: %v", err) + return legacy + } + return dt +} + // setDeviceTokenCookie sets the DT cookie in the HTTP Client cookie jar -// using the okta__saml2aws, we reduce making an extra api call -// this func can be uplifted in the future to set custom device tokens or used with -// getDeviceTokenFromOkta function +// using a per-install device token (see deviceToken), we reduce making an extra api call +// this func can be uplifted in the future to use getDeviceTokenFromOkta func (oc *Client) setDeviceTokenCookie(loginDetails *creds.LoginDetails) error { // getDeviceTokenFromOkta is not used but doing this to keep the function code @@ -385,8 +410,8 @@ func (oc *Client) setDeviceTokenCookie(loginDetails *creds.LoginDetails) error { cookie := http.Cookie{ Name: "DT", Secure: true, - Expires: time.Now().Add(time.Hour * 24 * 30), // 30 Days -> this time might not matter as this cookie is set on every saml2aws login request - Value: fmt.Sprintf("okta_%s_saml2aws", loginDetails.Username), // Okta recommends using an UUID but this should be unique enough. Also, this is key to remembering Okta MFA device + Expires: time.Now().Add(time.Hour * 24 * 30), // 30 Days -> this time might not matter as this cookie is set on every saml2aws login request + Value: deviceToken(loginDetails), // key to remembering the Okta MFA device } cookies = append(cookies, &cookie) oc.client.Jar.SetCookies(baseURL, cookies) diff --git a/pkg/provider/okta/okta_devicetoken_test.go b/pkg/provider/okta/okta_devicetoken_test.go new file mode 100644 index 000000000..e42c995d0 --- /dev/null +++ b/pkg/provider/okta/okta_devicetoken_test.go @@ -0,0 +1,98 @@ +package okta + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/versent/saml2aws/v2/helper/credentials" +) + +// memHelper is an in-memory credentials store for tests. +type memHelper struct { + store map[string]*credentials.Credentials +} + +func (m *memHelper) Add(c *credentials.Credentials) error { + m.store[c.ServerURL] = c + return nil +} + +func (m *memHelper) Delete(serverURL string) error { + delete(m.store, serverURL) + return nil +} + +func (m *memHelper) Get(serverURL string) (string, string, error) { + c, ok := m.store[serverURL] + if !ok { + return "", "", credentials.ErrCredentialsNotFound + } + return c.Username, c.Secret, nil +} + +func (m *memHelper) SupportsCredentialStorage() bool { return true } + +func useMemHelper(t *testing.T) *memHelper { + t.Helper() + old := credentials.CurrentHelper + h := &memHelper{store: map[string]*credentials.Credentials{}} + credentials.CurrentHelper = h + t.Cleanup(func() { credentials.CurrentHelper = old }) + return h +} + +func dtCookie(t *testing.T, oc *Client, rawURL string) string { + t.Helper() + u, err := url.Parse(rawURL) + require.NoError(t, err) + for _, c := range oc.client.Jar.Cookies(&url.URL{Scheme: u.Scheme, Host: u.Host, Path: "/"}) { + if c.Name == "DT" { + return c.Value + } + } + t.Fatal("DT cookie not set") + return "" +} + +func TestDeviceTokenIsRandomAndStable(t *testing.T) { + ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer ts.Close() + h := useMemHelper(t) + + oc, loginDetails := setupTestClient(t, ts, "PUSH") + require.NoError(t, oc.setDeviceTokenCookie(loginDetails)) + first := dtCookie(t, oc, loginDetails.URL) + + assert.NotContains(t, first, loginDetails.Username, "device token must not be derived from the username") + assert.NotEqual(t, "okta_"+loginDetails.Username+"_saml2aws", first) + _, stored, err := h.Get(loginDetails.URL + "/deviceToken") + require.NoError(t, err, "device token must be persisted in the credential store") + assert.Equal(t, first, stored) + + // A second run (new client) reuses the stored token. + oc2, loginDetails2 := setupTestClient(t, ts, "PUSH") + require.NoError(t, oc2.setDeviceTokenCookie(loginDetails2)) + assert.Equal(t, first, dtCookie(t, oc2, loginDetails2.URL)) + + // A different install (empty store) gets a different token. + useMemHelper(t) + oc3, loginDetails3 := setupTestClient(t, ts, "PUSH") + require.NoError(t, oc3.setDeviceTokenCookie(loginDetails3)) + assert.NotEqual(t, first, dtCookie(t, oc3, loginDetails3.URL)) + assert.False(t, strings.HasPrefix(dtCookie(t, oc3, loginDetails3.URL), "okta_")) +} + +func TestDeviceTokenLegacyWithoutCredentialStore(t *testing.T) { + ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer ts.Close() + + // The default helper has no storage; keep the old value so remembered devices still work. + oc, loginDetails := setupTestClient(t, ts, "PUSH") + require.NoError(t, oc.setDeviceTokenCookie(loginDetails)) + assert.Equal(t, "okta_user@example.com_saml2aws", dtCookie(t, oc, loginDetails.URL)) +} From d2e7c3dcacc7e6492429c165fd1e4e46ad52f8f9 Mon Sep 17 00:00:00 2001 From: Aaron Spratt <19199893+aspratt@users.noreply.github.com> Date: Tue, 20 Oct 2026 05:44:43 +0000 Subject: [PATCH 2/2] fix(okta): file fallback for the device token; don't rotate on transient errors (#23) With no keychain, or when saving to it fails, the random device token is now kept in a 0600 file under the saml2aws config directory instead of falling back to the predictable okta__saml2aws value. A new token is generated only when none is found, and a failed save is reported to the user. --- pkg/provider/okta/okta.go | 71 +++++++++++-- pkg/provider/okta/okta_devicetoken_test.go | 111 ++++++++++++++++++++- pkg/provider/okta/okta_test.go | 4 +- 3 files changed, 171 insertions(+), 15 deletions(-) diff --git a/pkg/provider/okta/okta.go b/pkg/provider/okta/okta.go index 3833100e1..b59ede35b 100644 --- a/pkg/provider/okta/okta.go +++ b/pkg/provider/okta/okta.go @@ -3,8 +3,10 @@ package okta import ( "bytes" "context" + "crypto/sha256" "crypto/tls" "encoding/base64" + "encoding/hex" "encoding/json" "fmt" "html" @@ -13,6 +15,8 @@ import ( "net/http" "net/http/cookiejar" "net/url" + "os" + "path/filepath" "regexp" "strconv" "strings" @@ -361,31 +365,80 @@ func (oc *Client) getDeviceTokenFromOkta(loginDetails *creds.LoginDetails) (stri return "", fmt.Errorf("unable to get a device token from okta") } +// deviceTokenDir is where the device token file fallback lives. A variable so +// tests can point it elsewhere. +var deviceTokenDir = func() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(home, ".aws", "saml2aws"), nil +} + // deviceToken returns the Okta device token for this install. A random UUID is // generated on first use and kept in the credential store under // /deviceToken, so Okta keeps remembering the device across runs. -// Without a credential store the legacy username-derived value is used, which -// keeps "remember this device" working for those installs. +// Without a usable credential store the token is kept in a 0600 file under the +// saml2aws config directory instead. The token is never derived from the username. func deviceToken(loginDetails *creds.LoginDetails) string { - legacy := fmt.Sprintf("okta_%s_saml2aws", loginDetails.Username) if !credentials.SupportsStorage() { - logger.Debug("no credential store, using legacy okta device token") - return legacy + logger.Debug("no credential store, using okta device token file") + return fileDeviceToken(loginDetails.URL, uuid.NewString()) } key := loginDetails.URL + "/deviceToken" - if _, dt, err := credentials.CurrentHelper.Get(key); err == nil && dt != "" { + _, dt, err := credentials.CurrentHelper.Get(key) + switch { + case err == nil && dt != "": return dt + case err != nil && !credentials.IsErrCredentialsNotFound(err): + // Don't rotate a token we could not read; it may still be in the store. + logger.Warnf("unable to read okta device token from the credential store, using the device token file | err: %v", err) + return fileDeviceToken(loginDetails.URL, uuid.NewString()) } - dt := uuid.NewString() + dt = uuid.NewString() if err := credentials.SaveCredentials(key, loginDetails.Username, dt); err != nil { - logger.Debugf("unable to store okta device token, using legacy value | err: %v", err) - return legacy + logger.Warnf("unable to store okta device token in the credential store, using the device token file; Okta may ask for MFA again | err: %v", err) + return fileDeviceToken(loginDetails.URL, dt) } return dt } +// fileDeviceToken returns the device token kept in the fallback file for idpURL, +// creating the file with candidate if it does not exist yet. If the file can't +// be used, candidate is returned for this run only. +func fileDeviceToken(idpURL, candidate string) string { + dir, err := deviceTokenDir() + if err != nil { + logger.Warnf("unable to find the saml2aws config directory, okta device token will not be remembered | err: %v", err) + return candidate + } + sum := sha256.Sum256([]byte(idpURL)) + path := filepath.Join(dir, "okta-device-token-"+hex.EncodeToString(sum[:8])) + + if b, err := os.ReadFile(path); err == nil { + if dt := strings.TrimSpace(string(b)); dt != "" { + return dt + } + } else if !os.IsNotExist(err) { + logger.Warnf("unable to read okta device token file %s, device will not be remembered | err: %v", path, err) + return candidate + } + + if err := os.MkdirAll(dir, 0700); err != nil { + logger.Warnf("unable to create %s, okta device token will not be remembered | err: %v", dir, err) + return candidate + } + if err := os.WriteFile(path, []byte(candidate), 0600); err != nil { + logger.Warnf("unable to write okta device token file %s, device will not be remembered | err: %v", path, err) + return candidate + } + // WriteFile keeps the mode of an existing file; make sure it is private. + _ = os.Chmod(path, 0600) + return candidate +} + // setDeviceTokenCookie sets the DT cookie in the HTTP Client cookie jar // using a per-install device token (see deviceToken), we reduce making an extra api call // this func can be uplifted in the future to use getDeviceTokenFromOkta diff --git a/pkg/provider/okta/okta_devicetoken_test.go b/pkg/provider/okta/okta_devicetoken_test.go index e42c995d0..c149de34b 100644 --- a/pkg/provider/okta/okta_devicetoken_test.go +++ b/pkg/provider/okta/okta_devicetoken_test.go @@ -1,12 +1,18 @@ package okta import ( + "errors" "net/http" "net/http/httptest" "net/url" + "os" + "path/filepath" + "runtime" "strings" "testing" + "github.com/sirupsen/logrus" + logtest "github.com/sirupsen/logrus/hooks/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/versent/saml2aws/v2/helper/credentials" @@ -14,10 +20,15 @@ import ( // memHelper is an in-memory credentials store for tests. type memHelper struct { - store map[string]*credentials.Credentials + store map[string]*credentials.Credentials + getErr error + addErr error } func (m *memHelper) Add(c *credentials.Credentials) error { + if m.addErr != nil { + return m.addErr + } m.store[c.ServerURL] = c return nil } @@ -28,6 +39,9 @@ func (m *memHelper) Delete(serverURL string) error { } func (m *memHelper) Get(serverURL string) (string, string, error) { + if m.getErr != nil { + return "", "", m.getErr + } c, ok := m.store[serverURL] if !ok { return "", "", credentials.ErrCredentialsNotFound @@ -87,12 +101,101 @@ func TestDeviceTokenIsRandomAndStable(t *testing.T) { assert.False(t, strings.HasPrefix(dtCookie(t, oc3, loginDetails3.URL), "okta_")) } -func TestDeviceTokenLegacyWithoutCredentialStore(t *testing.T) { +func TestDeviceTokenFileFallbackWithoutCredentialStore(t *testing.T) { + ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer ts.Close() + dir := t.TempDir() + useDeviceTokenDir(t, dir) + + // The default helper has no storage, so the token goes to a private file. + oc, loginDetails := setupTestClient(t, ts, "PUSH") + require.NoError(t, oc.setDeviceTokenCookie(loginDetails)) + first := dtCookie(t, oc, loginDetails.URL) + assert.NotContains(t, first, loginDetails.Username) + + files, err := filepath.Glob(filepath.Join(dir, "okta-device-token-*")) + require.NoError(t, err) + require.Len(t, files, 1) + if runtime.GOOS != "windows" { + fi, err := os.Stat(files[0]) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0600), fi.Mode().Perm()) + } + + // A later run reads the same token back. + oc2, loginDetails2 := setupTestClient(t, ts, "PUSH") + require.NoError(t, oc2.setDeviceTokenCookie(loginDetails2)) + assert.Equal(t, first, dtCookie(t, oc2, loginDetails2.URL)) +} + +func TestDeviceTokenTransientGetErrorDoesNotRotate(t *testing.T) { ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) defer ts.Close() + h := useMemHelper(t) - // The default helper has no storage; keep the old value so remembered devices still work. oc, loginDetails := setupTestClient(t, ts, "PUSH") require.NoError(t, oc.setDeviceTokenCookie(loginDetails)) - assert.Equal(t, "okta_user@example.com_saml2aws", dtCookie(t, oc, loginDetails.URL)) + stored := dtCookie(t, oc, loginDetails.URL) + + // The keychain is locked or unavailable for this run. + h.getErr = errors.New("keychain locked") + hook := logtest.NewGlobal() + oc2, loginDetails2 := setupTestClient(t, ts, "PUSH") + require.NoError(t, oc2.setDeviceTokenCookie(loginDetails2)) + + _, still, err := (&memHelper{store: h.store}).Get(loginDetails.URL + "/deviceToken") + require.NoError(t, err) + assert.Equal(t, stored, still, "a transient Get error must not overwrite the stored token") + assert.True(t, hasWarning(hook), "a transient Get error must be reported as a warning") +} + +func TestDeviceTokenSaveErrorWarnsAndUsesFile(t *testing.T) { + ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer ts.Close() + h := useMemHelper(t) + h.addErr = errors.New("keychain write denied") + dir := t.TempDir() + useDeviceTokenDir(t, dir) + + hook := logtest.NewGlobal() + oc, loginDetails := setupTestClient(t, ts, "PUSH") + require.NoError(t, oc.setDeviceTokenCookie(loginDetails)) + first := dtCookie(t, oc, loginDetails.URL) + + assert.NotEqual(t, "okta_"+loginDetails.Username+"_saml2aws", first, "must not fall back to the predictable token") + assert.True(t, hasWarning(hook), "a failed save must be reported as a warning") + + // The token kept in the file is reused while saves keep failing. + oc2, loginDetails2 := setupTestClient(t, ts, "PUSH") + require.NoError(t, oc2.setDeviceTokenCookie(loginDetails2)) + assert.Equal(t, first, dtCookie(t, oc2, loginDetails2.URL)) +} + +func hasWarning(hook *logtest.Hook) bool { + for _, e := range hook.AllEntries() { + if e.Level == logrus.WarnLevel && strings.Contains(e.Message, "device token") { + return true + } + } + return false +} + +func useDeviceTokenDir(t *testing.T, dir string) { + t.Helper() + old := deviceTokenDir + deviceTokenDir = func() (string, error) { return dir, nil } + t.Cleanup(func() { deviceTokenDir = old }) +} + +// TestMain keeps every test in this package away from the real +// ~/.aws/saml2aws directory. +func TestMain(m *testing.M) { + dir, err := os.MkdirTemp("", "saml2aws-okta-test") + if err != nil { + panic(err) + } + deviceTokenDir = func() (string, error) { return dir, nil } + code := m.Run() + _ = os.RemoveAll(dir) + os.Exit(code) } diff --git a/pkg/provider/okta/okta_test.go b/pkg/provider/okta/okta_test.go index 06e8cd4f7..ca8b822b8 100644 --- a/pkg/provider/okta/okta_test.go +++ b/pkg/provider/okta/okta_test.go @@ -665,7 +665,7 @@ func TestSetDeviceTokenCookie(t *testing.T) { err = oc.setDeviceTokenCookie(loginDetails) assert.Nil(t, err) - expectedDT := fmt.Sprintf("okta_%s_saml2aws", loginDetails.Username) + legacyDT := fmt.Sprintf("okta_%s_saml2aws", loginDetails.Username) actualDT := "" for _, c := range oc.client.Jar.Cookies(&url.URL{Scheme: "https", Host: "idp.example.com", Path: "/abc"}) { if c.Name == "DT" { @@ -673,7 +673,7 @@ func TestSetDeviceTokenCookie(t *testing.T) { } } assert.NotEqual(t, actualDT, "") - assert.Equal(t, expectedDT, actualDT) + assert.NotEqual(t, legacyDT, actualDT) }