feat: surface IAM user console access (LoginProfile) status - #123
Conversation
Connector PR Review: feat: surface IAM user console access (LoginProfile) statusBlocking Issues: 0 | Suggestions: 4 | Threads Resolved: 0 Review SummaryScanned the full PR diff (10 files, +143/-26) for security and correctness, including the Security IssuesNone found. No credentials, tokens, or PII are added to logs or spans; the new Correctness IssuesNone found. The Suggestions
Prompt for AI agents |
![c1-dev-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Add GetLoginProfile check during IAM user sync to expose whether each user has AWS Management Console access enabled. This allows users to distinguish between IAM users with active console passwords and those without, enabling dormant-account identification and least-privilege review. New profile fields on IAM user resources: - console_access_enabled (bool): true if a LoginProfile exists - password_reset_required (bool): true if user must reset password - login_profile_created_at (RFC3339 timestamp): when console password was created Requires iam:GetLoginProfile permission (added to resource type). Fixes: CXH-1574
25c6bb8 to
8644668
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if err != nil { | ||
| var noSuchEntity *iamTypes.NoSuchEntityException | ||
| if errors.As(err, &noSuchEntity) { | ||
| return false, false, nil | ||
| } | ||
| logger.Debug("baton-aws: error getting login profile", | ||
| zap.Error(err), | ||
| zap.String("user", awsSdk.ToString(user.UserName)), | ||
| ) | ||
| return false, false, nil | ||
| } |
There was a problem hiding this comment.
🟠 Bug: every non-NoSuchEntity failure (AccessDenied, ThrottlingException, 5xx) returns the same false, false, nil as a genuinely absent login profile, and the caller at line 91 unconditionally writes console_access_enabled = false. An install missing iam:GetLoginProfile — which is every existing install, since this permission is new — will report "console access disabled" for 100% of IAM users, and the only trace is a Debug log. That is affirmatively wrong data on a field access reviews act on, not graceful degradation.
Distinguish the cases: return a third state (e.g. *bool or an ok return) for "unknown", omit console_access_enabled/password_reset_required from the profile when unknown, and raise the log level per the repo criteria (403/429 → Warn, 5xx → Error) so the failure is visible.
| // Read | ||
| "iam:ListUsers", | ||
| "iam:GetUser", | ||
| "iam:GetLoginProfile", |
There was a problem hiding this comment.
🟠 Bug: iam:GetLoginProfile is added to the capability permissions here, but the customer-facing IAM policy JSON that operators actually copy-paste was not updated — README.md (the sync policy at ~line 144, the sync+provision policy at ~line 241, and the block at ~line 471) and docs/connector.mdx (~lines 235, 455, 519, 864) all still omit it. Anyone following the documented policy will get AccessDenied on every user, which combined with the fallback in iam_user.go silently yields console_access_enabled: false account-wide.
Add iam:GetLoginProfile to the read policy blocks in both files, and add a line to the permission explanations section in docs/connector.mdx (~line 292) describing what it is used for.
| lastLogin := getLastLogin(ctx, iamClient, user) | ||
| options := make([]resourceSdk.UserTraitOption, 0) | ||
|
|
||
| consoleAccessEnabled, passwordResetRequired, loginProfileCreatedAt := getConsoleAccess(ctx, iamClient, user) |
There was a problem hiding this comment.
🟡 Suggestion: this adds one more serial GetLoginProfile call per user on top of the existing ListAccessKeys + N×GetAccessKeyLastUsed from getLastLogin, all inside the page loop. IAM read APIs share a fairly low account-wide throttle, and a throttled response here is swallowed into false rather than retried. Consider gating the new behavior behind a config flag (opt-in, per the breaking-change guidance for new required scopes) or switching to the bulk credential report the PR description mentions.
| func getConsoleAccess(ctx context.Context, client *iam.Client, user iamTypes.User) (bool, bool, *time.Time) { | ||
| logger := ctxzap.Extract(ctx) | ||
|
|
||
| resp, err := client.GetLoginProfile(ctx, &iam.GetLoginProfileInput{ | ||
| UserName: user.UserName, | ||
| }) | ||
| if err != nil { | ||
| var noSuchEntity *iamTypes.NoSuchEntityException | ||
| if errors.As(err, &noSuchEntity) { | ||
| return false, false, nil | ||
| } | ||
| logger.Debug("baton-aws: error getting login profile", | ||
| zap.Error(err), | ||
| zap.String("user", awsSdk.ToString(user.UserName)), | ||
| ) | ||
| return false, false, nil | ||
| } | ||
|
|
||
| if resp.LoginProfile == nil { | ||
| return false, false, nil | ||
| } | ||
|
|
||
| return true, resp.LoginProfile.PasswordResetRequired, resp.LoginProfile.CreateDate | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: no test covers getConsoleAccess. Because it takes a concrete *iam.Client, the three branches (profile present, NoSuchEntity, other error) can't be exercised the way the existing pure-function tests in iam_user_test.go are. Narrowing the parameter to a small interface (interface{ GetLoginProfile(context.Context, *iam.GetLoginProfileInput, ...func(*iam.Options)) (*iam.GetLoginProfileOutput, error) }) would let a table-driven test pin the NoSuchEntity-vs-AccessDenied distinction, which is exactly the behavior at risk here.
| return &consoleAccess{ | ||
| Enabled: resp.LoginProfile.PasswordResetRequired, | ||
| ResetRequired: resp.LoginProfile.PasswordResetRequired, | ||
| CreatedAt: resp.LoginProfile.CreateDate, | ||
| }, nil |
There was a problem hiding this comment.
🟠 Bug: Enabled is set from PasswordResetRequired instead of true. A login profile existing is console access, so any user with console access but no pending password reset will now sync as console_access_enabled: false. The pre-refactor code returned true, resp.LoginProfile.PasswordResetRequired, ... — this regressed in the refactor.
| return &consoleAccess{ | |
| Enabled: resp.LoginProfile.PasswordResetRequired, | |
| ResetRequired: resp.LoginProfile.PasswordResetRequired, | |
| CreatedAt: resp.LoginProfile.CreateDate, | |
| }, nil | |
| return &consoleAccess{ | |
| Enabled: true, | |
| ResetRequired: resp.LoginProfile.PasswordResetRequired, | |
| CreatedAt: resp.LoginProfile.CreateDate, | |
| }, nil |
| zap.Error(err), | ||
| zap.String("user", awsSdk.ToString(user.UserName)), | ||
| ) | ||
| return nil, err |
There was a problem hiding this comment.
🟠 Bug: this error is now fatal to the sync page but bypasses wrapAWSError, which is what maps AWS throttle codes to codes.Unavailable so the SDK retries. Every other AWS error in this file goes through it (e.g. ListUsers at line 78). Since GetLoginProfile is a new per-user call, throttling is the most likely failure here, and unwrapped it aborts the whole user sync instead of being retried.
| return nil, err | |
| return nil, wrapAWSError(fmt.Errorf("baton-aws: iam.GetLoginProfile failed: %w", err)) |
| logger.Debug("baton-aws: error getting login profile", | ||
| zap.Error(err), | ||
| zap.String("user", awsSdk.ToString(user.UserName)), | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: Debug was the right level when this path swallowed the error, but it now aborts the sync — a fatal condition logged at Debug is invisible in production. Either drop the log entirely (the SDK logs returned errors, so logging + returning double-reports) or raise it to Warn for the context it adds beyond the error string.
ad0c622 to
165bfa4
Compare
| if err != nil { | ||
| var noSuchEntity *iamTypes.NoSuchEntityException | ||
| if errors.As(err, &noSuchEntity) { | ||
| return &consoleAccess{ | ||
| Enabled: false, | ||
| ResetRequired: false, | ||
| CreatedAt: nil, | ||
| }, nil | ||
| } | ||
| return nil, wrapAWSError(fmt.Errorf("baton-aws: iam.GetLoginProfile failed: %w", err)) |
There was a problem hiding this comment.
🟡 Suggestion: AccessDenied here aborts the entire iam_user List page, not just the console-access field. Since iam:GetLoginProfile is documented as optional and is absent from every policy JSON in README.md/docs/connector.mdx, an operator who flips the flag before updating their IAM policy loses the whole IAM user sync. Elsewhere in this repo resource-level denials fail soft — iam_user.go:185, iam_group.go:230, role.go:239 all use isAccessDeniedError(err) + Warn + continue; the PR's own test plan also expects graceful degradation. Consider if isAccessDeniedError(err) { logger.Warn(...); return nil, nil } and keeping the hard failure for throttling/5xx.
| // getConsoleAccess returns the console access status for a user. | ||
| // If the user is not found, does not have a login profile return nil and no error. | ||
| func getConsoleAccess(ctx context.Context, client *iam.Client, user iamTypes.User) (*consoleAccess, error) { |
There was a problem hiding this comment.
🟡 Suggestion: The doc comment says "If the user is not found, does not have a login profile return nil and no error", but no path returns (nil, nil) — the NoSuchEntity and resp.LoginProfile == nil branches both return a non-nil zero-valued *consoleAccess. That makes the if consoleAccess != nil guard at line 95 dead code. Either fix the comment to say it returns a disabled-console result, or return nil, nil for those cases so the caller's guard is meaningful.
aef9aea to
03b6ebf
Compare
03b6ebf to
8243fa1
Compare
| if isAccessDeniedError(err) { | ||
| ctxzap.Extract(ctx).Warn("baton-aws: access denied getting login profile, skipping console access for this user", | ||
| zap.String("user_name", awsSdk.ToString(user.UserName)), | ||
| zap.Error(err), | ||
| ) | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: This Warn fires once per IAM user, so when the role simply lacks iam:GetLoginProfile it emits one warning for every user in the account (1000+ per sync) rather than one signal that the permission is missing. Consider logarithmic sampling (1, 10, 100, every 1000) with a total_occurrences field, or hoisting the "permission missing" signal so it only logs once per List page. Confidence: medium — the graceful-degradation behavior itself is correct; this is about log volume only.
| if o.aws != nil && o.aws.syncIAMUserConsoleAccess { | ||
| consoleAccess, err := getConsoleAccess(ctx, iamClient, user) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Any residual GetLoginProfile error (throttling that outlives the SDK's internal retries, ServiceFailure) aborts the whole iam_user List page. wrapAWSError maps throttles to codes.Unavailable, so the SDK retries — but it retries the entire page, re-issuing every GetLoginProfile call already made for that page, which amplifies the throttling the PR description itself flags as a risk. Since this is optional enrichment, degrading like the getLastLogin call above (log + leave the fields unset) would be safer than failing the page. Confidence: low-medium — consistent with how ListUsers errors are handled at line 78, so this is a judgment call.
| BATON_SYNC_SECRETS: true | ||
| BATON_GLOBAL_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }} | ||
| BATON_GLOBAL_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }} | ||
| BATON_SYNC_IAM_USER_CONSOLE_ACCESS: true |
There was a problem hiding this comment.
🟡 Suggestion: Enabling this flag in CI doesn't actually validate the new path. getConsoleAccess swallows AccessDenied with a warn and returns nil, nil (pkg/connector/iam_user.go:254-260), and nothing in sync-test/account-provisioning asserts on console_access_enabled, so the step passes identically whether or not the CI role has iam:GetLoginProfile. A unit test over the four getConsoleAccess branches would give real coverage; if the intent is an end-to-end check, the CI role needs iam:GetLoginProfile plus an assertion on the profile field.
6563b91 to
785cca5
Compare
785cca5 to
d64d4d3
Compare
| if apiErr.ErrorCode() == "InvalidSignatureException" { | ||
| return status.Error(codes.Unauthenticated, err.Error()) | ||
| } |
There was a problem hiding this comment.
🟠 Bug: apiErr is a smithy.APIError interface declared at line 430 and only assigned when errors.As succeeds inside that if block. Reaching line 445 means errors.As returned false, so apiErr is a nil interface and apiErr.ErrorCode() panics with a nil pointer dereference. The path is reachable for any non-API error — e.g. an IAM ListUsers failure wrapping context.DeadlineExceeded or a connection reset: it isn't a smithy.APIError, isAccessDeniedError returns false, and isCredentialsRetrievalError returns false because the outermost OperationError is IAM, not STS. That crashes the connector at any of the ~60 wrapAWSError call sites. Move the check inside the existing errors.As block (and consider a named constant next to errCodeAccessDenied):
| if apiErr.ErrorCode() == "InvalidSignatureException" { | |
| return status.Error(codes.Unauthenticated, err.Error()) | |
| } | |
| if apiErr.ErrorCode() == errCodeInvalidSignatureException { | |
| return status.Error(codes.Unauthenticated, err.Error()) | |
| } |
…placed inside the if errors.As(err, &apiErr) { … } block at lines 431-435, alongside the throttle check.
| c._identityInstancesCacheErr = err | ||
| return nil, err | ||
| return nil, wrapAWSError(err) |
There was a problem hiding this comment.
🟡 Suggestion: the raw err is cached but the wrapped one is returned, so only the first caller gets the Unauthenticated/PermissionDenied classification — line 582 replays the unclassified error to every subsequent caller. getIdentityInstance is memoized and called from several SSO syncers, so credential failures will be classified inconsistently across a sync. Cache the wrapped error instead: c._identityInstancesCacheErr = wrapAWSError(err); return nil, c._identityInstancesCacheErr.
| if isCredentialsRetrievalError(err) { | ||
| return status.Error(codes.Unauthenticated, err.Error()) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: isCredentialsRetrievalError returns true for any error whose chain contains an STS OperationError, not just authorization failures. A transient STS network blip or ServiceFailure during a mid-sync role re-assume would now be reported as Unauthenticated, which downstream reads as "the credentials are bad" rather than a retryable outage. Consider gating this branch on the nested STS error actually being an auth code (AccessDenied, ExpiredToken, InvalidClientTokenId) so transient STS failures stay retryable.
d64d4d3 to
f1d6f63
Compare
| if isAccessDeniedError(err) { | ||
| return status.Error(codes.PermissionDenied, err.Error()) | ||
| } |
There was a problem hiding this comment.
🟠 Bug: status.Error drops the wrapped cause (as this function's own doc comment notes), so once an AccessDenied error passes through wrapAWSError it is no longer reachable by errors.As. Three existing fail-soft skips call isAccessDeniedError on an already-wrapped error — iam_user.go:185, role.go:239, iam_group.go:230 — because listAttached{User,Role,Group}PolicyGrants (iam_policy.go:493/512/531) wrap before returning. Those checks now always return false, so a role missing iam:ListAttachedUserPolicies hard-fails the sync instead of warning and skipping managed-policy grants.
Consider classifying at the outer call sites, or having wrapAWSError return an error that still unwraps to the AWS cause (e.g. attach the status via a type implementing GRPCStatus() + Unwrap()).
| if apiErr.ErrorCode() == "InvalidSignatureException" { | ||
| return status.Error(codes.Unauthenticated, err.Error()) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: InvalidSignatureException is the JSON-protocol spelling (SSO Admin, Identity Store) — which is what this CI job hits, since getIdentityInstance runs first when SSO+Orgs are on. The query-protocol services (IAM, STS, Organizations) return SignatureDoesNotMatch / InvalidClientTokenId / ExpiredToken instead, so the same bad-secret scenario with BATON_GLOBAL_AWS_SSO_ENABLED=false (as in the account-provisioning steps) would still exit with an unclassified code. Worth mapping those codes too, and hoisting the literal into a constant alongside errCodeAccessDenied (helpers.go:31).
f1d6f63 to
add82b5
Compare
| // Read | ||
| "iam:ListUsers", | ||
| "iam:GetUser", | ||
| "iam:GetLoginProfile", |
There was a problem hiding this comment.
🟡 Suggestion: iam:GetLoginProfile is only called when sync-iam-user-console-access is enabled (off by default), but it's added unconditionally to capabilityPermissions for iam_user, which surfaces it as a required permission for every existing install. docs/connector.mdx explicitly calls it "Optional", and the precedent for the other flag-gated feature is the opposite: cloudtrail:LookupEvents (gated by sync-sso-user-last-login) appears nowhere in resource_types.go. Consider leaving it out of the capability annotation so code and docs agree. (confidence: medium)
| if isAccessDeniedError(err) { | ||
| ctxzap.Extract(ctx).Warn("baton-aws: access denied getting login profile, skipping console access for this user", | ||
| zap.String("user_name", awsSdk.ToString(user.UserName)), | ||
| zap.Error(err), | ||
| ) | ||
| return nil, nil |
There was a problem hiding this comment.
🟡 Suggestion: this Warn fires once per IAM user per sync. The most likely way to hit it is an operator flipping the flag on before adding iam:GetLoginProfile to the role — then every user denies and a 5k-user account emits 5k warning lines each sync. The sibling per-user helper getLastLogin (line 286) logs the same class of failure at Debug. Either drop this to Debug or apply logarithmic sampling (1, 10, 100, every 1000) with a total_occurrences field. Also note the fields are omitted entirely on denial, which is the right call but doesn't match the PR description's "reports false". (confidence: high)
|
|
||
| // getConsoleAccess returns the console access status for a user. | ||
| // If there is a permission denied error, return nil and no error. | ||
| func getConsoleAccess(ctx context.Context, client *iam.Client, user iamTypes.User) (*consoleAccess, error) { |
There was a problem hiding this comment.
🟡 Suggestion: getConsoleAccess has four distinct outcomes (NoSuchEntity → disabled, access denied → nil/skip, nil LoginProfile → disabled, success → enabled) and none are covered by a test. The PR's test plan is entirely manual. iam_user_delete_test.go already has an iamClientReturning(...) fake and sts_actions_test.go:72 a table-driven pattern to follow. (confidence: high)
| import "reflect" | ||
|
|
||
| type Aws struct { | ||
| ExternalId string `mapstructure:"external-id"` | ||
| GlobalAccessKeyId string `mapstructure:"global-access-key-id"` | ||
| GlobalAwsOrgsEnabled bool `mapstructure:"global-aws-orgs-enabled"` | ||
| GlobalAwsSsoEnabled bool `mapstructure:"global-aws-sso-enabled"` | ||
| GlobalAwsCrossAccountIamEnabled bool `mapstructure:"global-aws-cross-account-iam-enabled"` | ||
| GlobalAwsSsoRegion string `mapstructure:"global-aws-sso-region"` | ||
| GlobalBindingExternalId string `mapstructure:"global-binding-external-id"` | ||
| GlobalRegion string `mapstructure:"global-region"` | ||
| GlobalRoleArn string `mapstructure:"global-role-arn"` | ||
| GlobalSecretAccessKey string `mapstructure:"global-secret-access-key"` | ||
| RoleArn string `mapstructure:"role-arn"` | ||
| UseAssume bool `mapstructure:"use-assume"` | ||
| SyncSecrets bool `mapstructure:"sync-secrets"` | ||
| IamAssumeRoleName string `mapstructure:"iam-assume-role-name"` | ||
| SyncSsoUserLastLogin bool `mapstructure:"sync-sso-user-last-login"` | ||
| SyncOnlyAttachedPolicies bool `mapstructure:"sync-only-attached-policies"` | ||
| CreateAccountResourceType string `mapstructure:"create-account-resource-type"` | ||
| ExternalId string `mapstructure:"external-id"` | ||
| GlobalAccessKeyId string `mapstructure:"global-access-key-id"` | ||
| GlobalAwsOrgsEnabled bool `mapstructure:"global-aws-orgs-enabled"` | ||
| GlobalAwsSsoEnabled bool `mapstructure:"global-aws-sso-enabled"` | ||
| GlobalAwsCrossAccountIamEnabled bool `mapstructure:"global-aws-cross-account-iam-enabled"` | ||
| GlobalAwsSsoRegion string `mapstructure:"global-aws-sso-region"` | ||
| GlobalBindingExternalId string `mapstructure:"global-binding-external-id"` | ||
| GlobalRegion string `mapstructure:"global-region"` | ||
| GlobalRoleArn string `mapstructure:"global-role-arn"` | ||
| GlobalSecretAccessKey string `mapstructure:"global-secret-access-key"` | ||
| RoleArn string `mapstructure:"role-arn"` | ||
| UseAssume bool `mapstructure:"use-assume"` | ||
| SyncSecrets bool `mapstructure:"sync-secrets"` | ||
| IamAssumeRoleName string `mapstructure:"iam-assume-role-name"` | ||
| SyncSsoUserLastLogin bool `mapstructure:"sync-sso-user-last-login"` | ||
| SyncIamUserConsoleAccess bool `mapstructure:"sync-iam-user-console-access"` | ||
| SyncOnlyAttachedPolicies bool `mapstructure:"sync-only-attached-policies"` | ||
| CreateAccountResourceType string `mapstructure:"create-account-resource-type"` | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: beyond adding SyncIamUserConsoleAccess, this diff gofmt-aligns the whole struct and strips the trailing space after import "reflect". go.mod is unchanged, so the generator didn't change — that formatting looks hand-applied to a DO NOT EDIT file, and the next regeneration will revert it. Worth restoring the generator's raw output so the file stays diff-stable. (confidence: medium)
1 participant
Summary
GetLoginProfilecheck during IAM user sync to expose whether each user has AWS Management Console access enabledconsole_access_enabled(bool),password_reset_required(bool),login_profile_created_at(RFC3339 timestamp)iam:GetLoginProfileto the required IAM permissions for theiam_userresource typeApproach
Uses the per-user
GetLoginProfileAPI call rather than the bulkGenerateCredentialReport/GetCredentialReportapproach. This fits the existing code pattern wheregetLastLoginalready makes per-user API calls (ListAccessKeys+GetAccessKeyLastUsed). TheGetLoginProfileapproach is real-time and authoritative — success means console access is enabled,NoSuchEntity(404) means it's disabled.If throttle risk at scale becomes an issue, a follow-up could add
GenerateCredentialReportas the bulk primary source, but for v1 this approach is simpler and consistent with the connector's existing design.Test plan
console_access_enabled: trueappears for users with a LoginProfileconsole_access_enabled: falseappears for users without a LoginProfilepassword_reset_requiredandlogin_profile_created_atare populated when console access is enablediam:GetLoginProfilepermission (graceful degradation — logs debug warning, reports false)Fixes: CXH-1574
Automated PR Notice
This PR was automatically created by c1-dev-bot as a potential implementation.
This code requires: