Emit ASSUMABLE_ROLE NHI type for IAM roles (NHI Phase-1 K3)#126
Conversation
Connector PR Review: Emit ASSUMABLE_ROLE NHI type for IAM roles (NHI Phase-1 K3)Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. This change classifies IAM roles into NHI types at sync time inside Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
ba8b3e5 to
5dc7ee3
Compare
f5d6965 to
8c8aab3
Compare
| - name: Install baton CLI from baton-sdk (conductorone/baton is archived) | ||
| run: | | ||
| set -euxo pipefail | ||
| BATON_VERSION=v0.11.0 | ||
| OS=$(uname -s | tr '[:upper:]' '[:lower:]') | ||
| ARCH=$(uname -m) | ||
| if [ "${ARCH}" = "x86_64" ]; then ARCH="amd64"; fi | ||
| FILENAME="baton-${BATON_VERSION}-${OS}-${ARCH}.tar.gz" | ||
| curl -fsSL -O "https://github.com/conductorone/baton-sdk/releases/download/${BATON_VERSION}/${FILENAME}" | ||
| tar xzf "${FILENAME}" | ||
| mv baton /usr/local/bin/baton | ||
| baton --version |
There was a problem hiding this comment.
This is probably unnecessary. I think this was addressed by ConductorOne/github-workflows#89
Implements NHI Phase-1 K3 type emission (per the NHI RFC §6 row 1); IAM roles (TRAIT_ROLE) are annotated WithNHIType(NHI_TYPE_ASSUMABLE_ROLE, detail). The trust-policy classification is parsed at sync time in List() so the type detail is set on every synced role. Detail strings follow the §2.8 convention <platform>.<object>[.<purpose>]: service-linked roles -> aws.role.service_linked; service-trusted -> aws.role.<service> (e.g. aws.role.lambda); federated -> aws.role.oidc/ saml/federated; cross-account -> aws.role.cross_account; else aws.role. Self-bumps baton-sdk -> v0.11.0 (interim; rebases after the baton-admin fleet bump). The existing AWS-principal grant path is unchanged; Service and Federated trust principals are now retained solely for classification. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…archived (v0.4.5) and can't resolve NonHumanIdentityTrait Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
…366) AWS service-linked roles are platform-custodied — the org doesn't control their trust policy, AWS does — so they map to NHI_TYPE_MANAGED_IDENTITY rather than ASSUMABLE_ROLE. Detected by the reserved /aws-service-role/ path or the AWSServiceRoleFor* name prefix; all other roles keep ASSUMABLE_ROLE with their existing trust-derived detail. Bumps baton-sdk to v0.11.1, which adds NHI_TYPE_MANAGED_IDENTITY=3. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
9270274 to
4172e5e
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| var singleARN string | ||
| err = json.Unmarshal(awsFieldData, &singleARN) | ||
| for field, dest := range map[string]*[]string{ | ||
| awsDisplayName: &principal.AWS, |
There was a problem hiding this comment.
🟡 Suggestion: Using awsDisplayName (the connector's "AWS" display-name constant from connector.go) as the JSON field key for the IAM Principal.AWS field couples trust-policy parsing to an unrelated UI constant. It works today only because both happen to be "AWS". If someone ever changes the display name (e.g. to "Amazon Web Services"), AWS principal parsing silently breaks and assume-role grants would no longer be detected. Use a literal "AWS" here instead.
| // Try string first | ||
| var singleARN string | ||
| err = json.Unmarshal(awsFieldData, &singleARN) | ||
| for field, dest := range map[string]*[]string{ |
There was a problem hiding this comment.
the loop handles AWS/Service/Federated but there's no branch for when Principal is the string "*" (wildcard trust). classifyRoleNHI falls back to base "aws.role", making a publicly-assumable role look the same as a same-account one. probably worth a distinct detail like "aws.role.public".
| return tp, nil | ||
| } | ||
|
|
||
| func hasAssumeRoleAction(actions Action) bool { |
There was a problem hiding this comment.
this uses strings.HasPrefix so it catches AssumeRoleWithWebIdentity and AssumeRoleWithSAML too, but extractTrustPrincipals (the one that drives actual grant emission) still uses slices.Contains(actions, "sts:AssumeRole") — exact match only. worth a comment explaining why they differ, or aligning them.
| func serviceTrustDetail(services []string) string { | ||
| short := make([]string, 0, len(services)) | ||
| for _, s := range services { | ||
| name, _, _ := strings.Cut(s, ".") // "lambda.amazonaws.com" -> "lambda" |
There was a problem hiding this comment.
strings.Cut(s, ".") on something like "accounts.google.com" gives "accounts", which passes the non-empty check and produces "aws.role.accounts". probably fine in practice since non-amazonaws.com values are rare in the Service field, but a guard checking that the string ends with .amazonaws.com before using the first segment would make it safer.
There was a problem hiding this comment.
not a valid concern, AWS only allows *.amazonaws.com
service principals in trust policies — it's enforced at policy creation time
| } | ||
|
|
||
| // arnAccountID extracts the 12-digit account ID from an ARN. | ||
| func arnAccountID(arnStr string) (string, bool) { |
There was a problem hiding this comment.
this duplicates AccountIdFromARN in arn.go — same arn.Parse call, just returns bool instead of error. could just be: id, err := AccountIdFromARN(arnStr); return id, err == nil
| // returns the AWS, Service, and Federated principals from Allow statements that | ||
| // grant an sts:AssumeRole* action (AssumeRole, AssumeRoleWithWebIdentity, | ||
| // AssumeRoleWithSAML). | ||
| func extractTrustPrincipalsByKind(policyDocument string) (trustPrincipals, error) { |
There was a problem hiding this comment.
extractTrustPrincipals (line 152) already does the same url.QueryUnescape + json.Unmarshal + Effect=Allow loop. now that this function exists, extractTrustPrincipals could just call it and return tp.aws, eliminating the duplication.
| func isCrossAccountTrust(roleAccountID string, awsPrincipals []string) bool { | ||
| for _, p := range awsPrincipals { | ||
| if acct, ok := arnAccountID(p); ok && acct != roleAccountID { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): isCrossAccountTrust relies on arnAccountID, which uses arn.Parse and so returns ok=false for a bare-account-ID principal (e.g. "AWS": "123456789012"). Such a principal is silently skipped, so a genuinely cross-account role expressed that way would fall back to aws.role instead of aws.role.cross_account. In practice IAM normalizes bare account IDs to arn:aws:iam::<acct>:root in the stored trust document, so this rarely manifests — but if you want full coverage, treat a 12-digit numeric principal as an account ID directly.
5 participants
Implements NHI Phase-1 K3 type emission (per the NHI RFC §6 row 1); IAM roles (
TRAIT_ROLE) →WithNHIType(NHI_TYPE_ASSUMABLE_ROLE, detail).Per §6 this is a mod: the trust-policy classification is parsed at sync time inside
roleResourceType.List()(pkg/connector/role.go), so the NHI type + detail land on every synced role with no extra API call (ListRolesalready returnsAssumeRolePolicyDocument+Pathinline).Detail strings follow the §2.8 convention
<platform>.<object>[.<purpose>](dotted lowercase), most-specific first:/aws-service-role/) →aws.role.service_linkedaws.role.<service>(e.g.aws.role.lambda,aws.role.ec2,aws.role.ecs_tasks)aws.role.oidc/aws.role.saml/aws.role.federatedaws.role.cross_accountaws.roleThe existing AWS-principal assume-role grant path is unchanged;
Service/Federatedtrust principals (previously discarded inPrincipal.UnmarshalJSON) are now retained solely for classification.Self-bumps
baton-sdk→ v0.11.0 (interim; rebases after the baton-admin fleet bump). No protogen. Verified the shipped v0.11.0 API isWithNHIType/NonHumanIdentityTrait_NHI_TYPE_ASSUMABLE_ROLE(the RFC'sWithNHISubtypewas a stale draft name).go build ./...,go test ./..., andgolangci-lint runall pass; addedTestClassifyRoleNHIDetailcovering each branch.🛰️ Built with pqprime.