From 46c47f5b977ecb3ed781bf5b22eb35f054080405 Mon Sep 17 00:00:00 2001 From: Aaron Spratt <19199893+aspratt@users.noreply.github.com> Date: Tue, 20 Oct 2026 04:12:39 +0000 Subject: [PATCH 1/3] fix: list-roles falls back to assertion roles when sign-in redirects (#14) A single-role user is 302ed straight to the console, so the role chooser never renders and ParseAWSAccounts returned no accounts. When the parse yields nothing after a redirect, build accounts from the assertion's roles, grouped by account id. TestListRolesSingleRole is now required. --- aws_account.go | 81 ++++++++++++++++++++++++++++++++++++++++++-- aws_account_test.go | 62 +++++++++++++++++++++++++++++++++ test/e2e/README.md | 5 +-- test/e2e/e2e_test.go | 11 +++--- 4 files changed, 148 insertions(+), 11 deletions(-) diff --git a/aws_account.go b/aws_account.go index 4b630bd41..19d10d604 100644 --- a/aws_account.go +++ b/aws_account.go @@ -2,10 +2,12 @@ package saml2aws import ( "bytes" + "encoding/base64" "fmt" "io" "net/http" "net/url" + "strings" "github.com/PuerkitoBio/goquery" "github.com/pkg/errors" @@ -19,7 +21,17 @@ type AWSAccount struct { // ParseAWSAccounts extract the aws accounts from the saml assertion func ParseAWSAccounts(audience string, samlAssertion string) ([]*AWSAccount, error) { - res, err := http.PostForm(audience, url.Values{"SAMLResponse": {samlAssertion}}) + // A user with a single role is redirected to the console instead of being + // shown the role chooser, so note any redirect for the fallback below. + redirected := false + client := &http.Client{ + CheckRedirect: func(req *http.Request, via []*http.Request) error { + redirected = true + return nil + }, + } + + res, err := client.PostForm(audience, url.Values{"SAMLResponse": {samlAssertion}}) if err != nil { return nil, errors.Wrap(err, "error retrieving AWS login form") } @@ -30,7 +42,72 @@ func ParseAWSAccounts(audience string, samlAssertion string) ([]*AWSAccount, err return nil, errors.Wrap(err, "error retrieving AWS login body") } - return ExtractAWSAccounts(data) + accounts, err := ExtractAWSAccounts(data) + if err != nil { + return nil, err + } + + if len(accounts) == 0 && redirected { + return accountsFromAssertion(samlAssertion) + } + + return accounts, nil +} + +// accountsFromAssertion builds the accounts from the roles in the SAML +// assertion. It is the fallback for when AWS redirects instead of serving the +// role chooser. The assertion carries no account alias, so accounts are named +// by id, as the chooser names accounts that have no alias. +func accountsFromAssertion(samlAssertion string) ([]*AWSAccount, error) { + data, err := base64.StdEncoding.DecodeString(samlAssertion) + if err != nil { + return nil, errors.Wrap(err, "error decoding saml assertion") + } + + roles, err := ExtractAwsRoles(data) + if err != nil { + return nil, errors.Wrap(err, "error parsing aws roles") + } + + awsRoles, err := ParseAWSRoles(roles) + if err != nil { + return nil, errors.Wrap(err, "error parsing aws roles") + } + + accounts := []*AWSAccount{} + byID := make(map[string]*AWSAccount) + for _, awsRole := range awsRoles { + id := accountIDFromRoleARN(awsRole.RoleARN) + + account, ok := byID[id] + if !ok { + account = &AWSAccount{Name: "Account: " + id} + byID[id] = account + accounts = append(accounts, account) + } + + account.Roles = append(account.Roles, &AWSRole{ + RoleARN: awsRole.RoleARN, + Name: roleNameFromRoleARN(awsRole.RoleARN), + }) + } + + return accounts, nil +} + +// accountIDFromRoleARN returns the account id from an ARN of the form +// arn:partition:service:region:account-id:resource +func accountIDFromRoleARN(roleARN string) string { + parts := strings.Split(roleARN, ":") + if len(parts) < 5 { + return "" + } + return parts[4] +} + +// roleNameFromRoleARN returns the role name, the last path segment of the ARN +func roleNameFromRoleARN(roleARN string) string { + return roleARN[strings.LastIndex(roleARN, "/")+1:] } // ExtractAWSAccounts extract the accounts from the AWS html page diff --git a/aws_account_test.go b/aws_account_test.go index b5c86c43b..66a3fd4d7 100644 --- a/aws_account_test.go +++ b/aws_account_test.go @@ -1,12 +1,74 @@ package saml2aws import ( + "encoding/base64" + "net/http" + "net/http/httptest" "os" + "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +// singleRoleAssertion returns testdata/assertion.xml, base64 encoded, with the +// second role removed, so the assertion carries exactly one role. +func singleRoleAssertion(t *testing.T) string { + t.Helper() + data, err := os.ReadFile("testdata/assertion.xml") + assert.Nil(t, err) + xml := strings.Replace(string(data), + "\n arn:aws:iam::123123123123:saml-provider/ExampleADFS,arn:aws:iam::123123123123:role/AWS-Admin-CloudOPSNonProd", "", 1) + assert.NotEqual(t, string(data), xml) + return base64.StdEncoding.EncodeToString([]byte(xml)) +} + +// A single-role user: AWS answers the sign-in POST with a 302 to the console +// instead of the role chooser, so the chooser page is never served. +func TestParseAWSAccountsSingleRoleRedirect(t *testing.T) { + assertion := singleRoleAssertion(t) + + mux := http.NewServeMux() + mux.HandleFunc("/saml", func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, http.MethodPost, r.Method) + assert.Equal(t, assertion, r.FormValue("SAMLResponse")) + http.Redirect(w, r, "/console", http.StatusFound) + }) + mux.HandleFunc("/console", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte("AWS Management Console")) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + accounts, err := ParseAWSAccounts(srv.URL+"/saml", assertion) + require.NoError(t, err) + require.Len(t, accounts, 1) + + account := accounts[0] + assert.Equal(t, "Account: 123123123123", account.Name) + require.Len(t, account.Roles, 1) + assert.Equal(t, "arn:aws:iam::123123123123:role/AWS-Admin-CloudOPSBuild", account.Roles[0].RoleARN) + assert.Equal(t, "AWS-Admin-CloudOPSBuild", account.Roles[0].Name) +} + +// Regression guard: a multi-role user gets the chooser page, which is parsed +// as before. +func TestParseAWSAccountsChooserPage(t *testing.T) { + data, err := os.ReadFile("testdata/saml.html") + assert.Nil(t, err) + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write(data) + })) + defer srv.Close() + + accounts, err := ParseAWSAccounts(srv.URL, singleRoleAssertion(t)) + require.NoError(t, err) + require.Len(t, accounts, 2) + assert.Equal(t, "Account: account-alias (000000000001)", accounts[0].Name) +} + func TestExtractAWSAccounts(t *testing.T) { data, err := os.ReadFile("testdata/saml.html") assert.Nil(t, err) diff --git a/test/e2e/README.md b/test/e2e/README.md index 9b99b8dec..7fcf47315 100644 --- a/test/e2e/README.md +++ b/test/e2e/README.md @@ -92,7 +92,7 @@ driver from npm (`playwright-core@1.47.2` + Node 20) at |---|---| | `TestLoginWritesCredentials` | `login --skip-prompt` writes the `[e2e]` profile with key, secret and session token; `x_principal_arn` is the assumed-role ARN; expiry is about 1h out; the credentials work: `GetCallerIdentity` returns the assumed role | | `TestScriptPrintsLoggedInCredentials` | `script --shell=env` prints the stored credentials | -| `TestListRolesSingleRole` | `list-roles` shows the role from a one-role assertion. **Known bug, skipped** unless `E2E_KNOWN_BUGS=1` (see below) | +| `TestListRolesSingleRole` | `list-roles` shows the role from a one-role assertion (fallback to the assertion's roles when sign-in redirects) | | `TestWrongPasswordFailsWithErrorElementConfig` | a wrong password fails fast and writes no credentials, with `kc_auth_error_element` set for Keycloak 26 | | `TestWrongPasswordFailsDefaultConfig` | the same with the default config. **Known bug, skipped** unless `E2E_KNOWN_BUGS=1` (see below) | @@ -154,7 +154,8 @@ to it. but with one valid role AWS answers `302` to the console, so the chooser never appears. `ParseAWSAccounts` follows the redirect, finds no `fieldset > div.saml-account`, and saml2aws exits 0 with no output. - Verified against real AWS. Reproduce with `E2E_KNOWN_BUGS=1`. + Verified against real AWS. Fixed: when sign-in redirects and no accounts + are parsed, `list-roles` now builds the accounts from the assertion's roles. 6. `script` validates the IdP account (it needs `--url` and the provider) even though it only prints stored credentials. diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 7f55b2d62..742d0b7bb 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -143,14 +143,11 @@ func TestScriptPrintsLoggedInCredentials(t *testing.T) { require.Contains(t, out, "AWS_ACCESS_KEY_ID="+key) } -// Known bug: list-roles reads account names from the AWS sign-in role -// chooser, but with a single role AWS skips the chooser and redirects to the -// console. saml2aws follows the redirect, finds no accounts and prints -// nothing, without an error. Set E2E_KNOWN_BUGS=1 to run it. +// list-roles reads account names from the AWS sign-in role chooser, but with a +// single role AWS skips the chooser and redirects to the console. Once the +// redirect finds no accounts, list-roles falls back to the roles in the +// assertion (fork issue aspratt/saml2aws#14). func TestListRolesSingleRole(t *testing.T) { - if os.Getenv("E2E_KNOWN_BUGS") == "" { - t.Skip("known bug: list-roles prints nothing when the assertion has one role") - } home := t.TempDir() out, err := run(t, home, append(append([]string{"list-roles"}, idpArgs(t)...), "--password="+mustEnv(t, "E2E_PASSWORD"))...) From 94c33fa3d02e798d9ad71cabb9bfc8f54bc154ce Mon Sep 17 00:00:00 2001 From: Aaron Spratt <19199893+aspratt@users.noreply.github.com> Date: Tue, 20 Oct 2026 04:20:12 +0000 Subject: [PATCH 2/3] fix: guard the list-roles fallback against error pages and redirect loops (#14) Fall back to assertion roles only when the final response is < 400, and restore Go's 10-redirect cap that the custom CheckRedirect dropped. --- aws_account.go | 8 +++++++- aws_account_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/aws_account.go b/aws_account.go index 19d10d604..aad26aee1 100644 --- a/aws_account.go +++ b/aws_account.go @@ -26,6 +26,10 @@ func ParseAWSAccounts(audience string, samlAssertion string) ([]*AWSAccount, err redirected := false client := &http.Client{ CheckRedirect: func(req *http.Request, via []*http.Request) error { + // Keep the same cap as Go's default client. + if len(via) >= 10 { + return errors.New("stopped after 10 redirects") + } redirected = true return nil }, @@ -47,7 +51,9 @@ func ParseAWSAccounts(audience string, samlAssertion string) ([]*AWSAccount, err return nil, err } - if len(accounts) == 0 && redirected { + // Only a successful sign-in can be a single-role redirect; an error page + // after a redirect is not. + if len(accounts) == 0 && redirected && res.StatusCode < 400 { return accountsFromAssertion(samlAssertion) } diff --git a/aws_account_test.go b/aws_account_test.go index 66a3fd4d7..9c7ff07c9 100644 --- a/aws_account_test.go +++ b/aws_account_test.go @@ -138,3 +138,47 @@ func TestLocateRole(t *testing.T) { assert.Equal(t, "arn:aws:iam::000000000001:role/Development", role.RoleARN) } + +// A redirect that lands on an error page must not be read as a single-role +// user: the fallback only applies to a successful sign-in. +func TestParseAWSAccountsRedirectToErrorPage(t *testing.T) { + assertion := singleRoleAssertion(t) + + mux := http.NewServeMux() + mux.HandleFunc("/saml", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "/forbidden", http.StatusFound) + }) + mux.HandleFunc("/forbidden", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte("Access denied")) + }) + srv := httptest.NewServer(mux) + defer srv.Close() + + accounts, err := ParseAWSAccounts(srv.URL+"/saml", assertion) + require.NoError(t, err) + assert.Empty(t, accounts) +} + +// A redirect loop must stop after Go's default cap of 10 redirects, not +// follow forever. +func TestParseAWSAccountsRedirectLoop(t *testing.T) { + assertion := singleRoleAssertion(t) + + hits := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits++ + if hits > 50 { + // Bound the test if the cap is missing; the assertion below then fails. + _, _ = w.Write([]byte("")) + return + } + http.Redirect(w, r, "/saml", http.StatusFound) + })) + defer srv.Close() + + _, err := ParseAWSAccounts(srv.URL+"/saml", assertion) + require.Error(t, err) + assert.Contains(t, err.Error(), "stopped after 10 redirects") + assert.LessOrEqual(t, hits, 11) +} From a419587c17b6aea8cab4dbe16af22fa0414ff0ed Mon Sep 17 00:00:00 2001 From: Aaron Spratt <19199893+aspratt@users.noreply.github.com> Date: Tue, 20 Oct 2026 04:35:16 +0000 Subject: [PATCH 3/3] test(e2e): run TestListRolesSingleRole only in the e2e-aws lane list-roles always posts to the real AWS sign-in page, which the offline moto lane can't reach, so the test skips unless E2E_TARGET=aws and stays required in e2e-aws. --- test/e2e/e2e_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 742d0b7bb..238f06ed5 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -148,6 +148,11 @@ func TestScriptPrintsLoggedInCredentials(t *testing.T) { // redirect finds no accounts, list-roles falls back to the roles in the // assertion (fork issue aspratt/saml2aws#14). func TestListRolesSingleRole(t *testing.T) { + // list-roles always posts to the real AWS sign-in page, which moto doesn't + // stand in for and the offline lane can't reach, so this runs in e2e-aws only. + if mustEnv(t, "E2E_TARGET") != "aws" { + t.Skip("needs the real AWS sign-in page; runs in the e2e-aws lane only") + } home := t.TempDir() out, err := run(t, home, append(append([]string{"list-roles"}, idpArgs(t)...), "--password="+mustEnv(t, "E2E_PASSWORD"))...)