Skip to content

Conversation

hc2p
Copy link
Contributor

@hc2p hc2p commented Sep 19, 2025

Since the AWS organization will be dissolved soon we will lose SSO access via the old account and roles. LabLabs already created new roles in the prod account.

These changes should be shared with all customers and ensured they ran successfully, before the old org can be dissolved.

Summary by CodeRabbit

  • Chores
    • Expanded role trust relationships to include an additional trusted account for engineer and admin access, consistent with existing SSO patterns. This enables authorized cross-account role assumption while preserving current trust and permissions.
    • No changes to resources, attachments, or role permission scopes; only trust entries were extended.
    • No downtime or application changes expected; existing users remain unaffected.

@hc2p hc2p requested a review from m1so September 19, 2025 15:40
Copy link

coderabbitai bot commented Sep 19, 2025

📝 Walkthrough

Walkthrough

  • Expanded AWS IAM assume_role_policy for two roles: engineer_role and admin_role.
  • Added an additional trusted principal for AWS account 318911662267 while keeping the existing trust for account 978928340082 intact.
  • New trust entries include ArnLike conditions targeting SSO role ARNs:
    • engineer_role: adds conditions referencing ProductionEngineer_81bae8be82beff06* and ProductionAdmin_e7ae0d41f57593ac* patterns.
    • admin_role: adds conditions referencing ProductionAdmin_e7ae0d41f57593ac* pattern.
  • No other Terraform resources, attachments, outputs, or public interfaces were changed.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant UserA as User (acct 978928340082)
  participant UserB as User (acct 318911662267)
  participant STS as AWS STS
  participant IAM as IAM Role (engineer_role / admin_role)

  rect rgb(245,245,255)
  note left of IAM: assume_role_policy contains two trust blocks\n- trust for root 978928340082\n- trust for root 318911662267 with ArnLike conditions
  end

  UserA->>STS: AssumeRole(RoleArn)
  STS->>IAM: Evaluate assume_role_policy
  IAM-->>STS: Check Principal == 978928340082 root
  alt Conditions pass (978...)
    STS-->>UserA: Temporary credentials
  else
    STS-->>UserA: AccessDenied
  end

  UserB->>STS: AssumeRole(RoleArn)
  STS->>IAM: Evaluate assume_role_policy
  IAM-->>STS: Check Principal == 318911662267 root
  IAM-->>STS: Check ArnLike PrincipalArn conditions\n- engineer_role: ProductionEngineer_81bae8be82beff06*, ProductionAdmin_e7ae0d41f57593ac*\n- admin_role: ProductionAdmin_e7ae0d41f57593ac*
  alt Conditions pass (3189... + ArnLike)
    STS-->>UserB: Temporary credentials
  else
    STS-->>UserB: AccessDenied
  end
Loading

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix: Change assume role policies to new org and roles" accurately and concisely reflects the PR's main change—updating assume_role_policy entries to reference a new AWS account and SSO roles—and aligns with the PR objectives and file summaries. It is specific to the type of change and clear for a teammate scanning history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 54e4466 and 88d16db.

📒 Files selected for processing (1)
  • main.tf (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:47:05.573Z
Learnt from: hc2p
PR: deepnote/terraform-aws-deepnote-access#1
File: main.tf:163-167
Timestamp: 2025-09-19T15:47:05.573Z
Learning: The terraform-aws-deepnote-access module is scoped only for standard AWS partitions and won't target AWS GovCloud or China partitions, so hardcoded "aws" partition in ARNs is acceptable.

Applied to files:

  • main.tf
🔇 Additional comments (2)
main.tf (2)

217-231: Approve: admin_role trust for 318911662267 is correct

Mirrors engineer_role and contains the AWSReservedSSO_ProductionAdmin ARN patterns (main.tf lines 225–228).


170-187: LGTM — add trust for 318911662267 on engineer_role; retain legacy 978928340082 until cutover

Verified both ARN patterns for 978928340082 and 318911662267 exist in main.tf.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
main.tf (1)

189-203: Mirror the hardening for admin trust policy.

Use name‑based wildcards and bind the principal account.

         Condition = {
-          ArnLike = {
-            "aws:PrincipalArn" = [
-              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/*/AWSReservedSSO_ProductionAdmin_e7ae0d41f57593ac*",
-              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_ProductionAdmin_e7ae0d41f57593ac*"
-            ]
-          }
+          ArnLike = {
+            "aws:PrincipalArn" = [
+              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/*/AWSReservedSSO_ProductionAdmin_*",
+              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_ProductionAdmin_*"
+            ]
+          }
+          StringEquals = {
+            "aws:PrincipalAccount" = "318911662267"
+          }
         }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between e8a2986 and 54e4466.

📒 Files selected for processing (1)
  • main.tf (2 hunks)
🔇 Additional comments (1)
main.tf (1)

151-170: Harden engineer trust policy — wildcard SSO permission‑set names & bind account.

Use name wildcards (avoid fragile GUID prefixes) and add aws:PrincipalAccount for defense‑in‑depth. aws CLI is unavailable here — verify actual ARNs in account 318911662267 and apply the change below.

File: main.tf Lines: 151-170

         Condition = {
-          ArnLike = {
-            "aws:PrincipalArn" = [
-              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/*/AWSReservedSSO_ProductionEngineer_81bae8be82beff06*",
-              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_ProductionEngineer_81bae8be82beff06*",
-              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/*/AWSReservedSSO_ProductionAdmin_e7ae0d41f57593ac*",
-              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_ProductionAdmin_e7ae0d41f57593ac*"
-            ]
-          }
+          ArnLike = {
+            "aws:PrincipalArn" = [
+              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/*/AWSReservedSSO_ProductionEngineer_*",
+              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_ProductionEngineer_*",
+              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/*/AWSReservedSSO_ProductionAdmin_*",
+              "arn:aws:iam::318911662267:role/aws-reserved/sso.amazonaws.com/AWSReservedSSO_ProductionAdmin_*"
+            ]
+          }
+          StringEquals = {
+            "aws:PrincipalAccount" = "318911662267"
+          }
         }

Verify actual ARNs in the account (run where aws CLI is installed/configured for 318911662267):

aws iam list-roles \
  --query 'Roles[?starts_with(RoleName, `AWSReservedSSO_Production`)].{Name:RoleName,Arn:Arn}' \
  --output table

@hc2p hc2p merged commit 8513796 into main Sep 24, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants