From 0c1bfbf8f36760c3f8ce226eb6443ad8c2a5c8dd Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Sat, 31 Oct 2026 13:33:30 +0100 Subject: [PATCH 1/2] fix(identity): make team guardrails the permission set boundary and scope team access The javabin-team-{team} permission sets attached javabin-team-{team}-guardrails as a customer-managed policy. Its baseline "Allow *" therefore granted every action the guardrail Denies miss, so team members could write the Terraform state bucket, legacy buckets, untagged RDS/EC2/Lambda/IAM roles and Route53 zones. The inline StringEqualsIfExists allow added the same for untagged resources. Set the guardrails as the permissions boundary instead, detach the old policy references, attach ViewOnlyAccess for console browsing, and replace the inline policy with StringEquals on the team tag plus name-scoped allows for the team's parameters, log groups, buckets and the ECS Exec cluster half. Grant team-provisioner the SSO and IAM calls this needs. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 4 +- docs/lambda-functions.md | 14 ++ docs/platform-modules.md | 2 +- .../lambda-src/team_provisioner/handler.py | 197 ++++++++++++------ .../test_team_provisioner_memberships.py | 84 +++++++- terraform/platform/iam/boundary.tf | 5 +- terraform/platform/lambdas/main.tf | 28 ++- 7 files changed, 253 insertions(+), 81 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6e8b747..3e803ef 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -218,7 +218,7 @@ terraform/state/ | SSM params (app) | `/javabin/apps/{team}/{service}/{name}` | | ECS cluster | `javabin-platform` | | ECS execution roles | `javabin-ecs-exec-{team}` (platform-owned, reads only `/javabin/apps/{team}/*`) | -| SSO team guardrails | `javabin-team-{team}-guardrails` (developer boundary with the team written in, for `javabin-team-{team}` permission sets) | +| SSO team guardrails | `javabin-team-{team}-guardrails` (developer boundary with the team written in; the permissions boundary of `javabin-team-{team}` permission sets) | | ECR repos | `{team}-{service}` | | SNS topics | `javabin-{alerts,security,budget-enforcement}` | | Log groups | `/ecs/{team}/{service}` | @@ -325,7 +325,7 @@ Team group OWNERs are admin-managed and never removed by the sync; non-@java.no ### Known Issues - **ECS deploy stabilization**: platform-test-app task registers but service fails health check - **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform. It is replaced by the Terraform-managed Google OIDC IdP at cutover (docs/cognito-google-setup.md), then deleted -- **SSO sessions carry no `team` tag**: the Identity Center ABAC attribute (`${path:enterprise.team}`) is never set on users, so `${aws:PrincipalTag/team}` is empty for humans. Per-team permission sets (`javabin-team-{team}`) therefore use `javabin-team-{team}-guardrails` (the developer boundary rendered with the team name) instead. The ABAC-based `javabin-developer` permission set in `terraform/org` cannot reach team resources +- **SSO sessions carry no `team` tag**: the Identity Center ABAC attribute (`${path:enterprise.team}`) is never set on users, so `${aws:PrincipalTag/team}` is empty for humans. Per-team permission sets (`javabin-team-{team}`) therefore use `javabin-team-{team}-guardrails` (the developer boundary rendered with the team name) as their permissions boundary instead. It must stay a boundary: its baseline `Allow *` attached as a policy grants everything its Denies miss (every untagged resource). The ABAC-based `javabin-developer` permission set in `terraform/org` cannot reach team resources - **App databases share one network path** (fixed by `app-network`): service-rds now allows ingress only from the app's own `{team}-{app}-tasks` SG; each app moves off the shared ECS tasks SG on its next pipeline run - **Team provisioner Lambda**: All sync functions working (Google/GitHub/Budget/Cognito/Identity Center). Password-set flow deployed. - **`registered_app_repos` manually managed**: Being replaced with team-scoped IAM roles (repo→team resolved via GitHub API at runtime) diff --git a/docs/lambda-functions.md b/docs/lambda-functions.md index 8f490d9..580f8bc 100644 --- a/docs/lambda-functions.md +++ b/docs/lambda-functions.md @@ -127,6 +127,20 @@ more; read the current list there first and add to it rather than pasting this o `admin.directory.user.alias`, `gmail.send`, `gmail.settings.sharing`, `apps.groups.settings` (all under `https://www.googleapis.com/auth/`). +### Team permission set (`javabin-team-{team}`) + +`_ensure_team_permission_set` creates the Identity Center permission set for each registry team and assigns it to the team's group. It is the only AWS access team members get: + +| Part | What it is | Effect | +|------|-----------|--------| +| Permissions boundary | `javabin-team-{team}-guardrails` (platform Terraform, `iam/boundary.tf`) | The developer boundary's Denies with the team written in. A boundary only limits; its baseline `Allow *` grants nothing | +| AWS managed policy | `ViewOnlyAccess` | Console browsing (List/Describe, no object, parameter or secret reads), as `javabin-readonly` | +| Inline policy | `_team_inline_policy` | Everything on resources tagged `team={team}` (`StringEquals`, so untagged resources, S3 objects, Route53 records and creates are not included); by name: `/javabin/apps/{team}/*` parameters (read, overwrite), `/ecs/{team}/*` log groups, `{team}-*-{account}` buckets, the cluster half of ECS Exec; a few untagged reads for `javabin status`. Platform-managed parameters and new parameters stay denied | + +So a team member can read and overwrite their secrets (`javabin secret`), read their logs, push and pull their ECR repos, redeploy, stop or exec into their services, and browse the console. They cannot change shared or untagged resources (state bucket, legacy buckets and instances, Route53 zones, untagged roles and Lambdas) or create resources by hand: infrastructure goes through `app.yaml` and CI. + +Order on each run: set the boundary, then detach any `javabin-team-{team}-guardrails` or `javabin-developer-boundary` attached as a policy by older versions, attach `ViewOnlyAccess`, put the inline policy, provision. If the boundary cannot be set the permission set is left as it is and the result has an `error`; a `ViewOnlyAccess` failure is reported as `view_only` and the rest still applies. Teams without guardrails yet (`pending`) are not touched. + ### Atlassian group sync (`sync_atlassian`) Invoked by the registry's `provision-groups.yml` via `scripts/provision-atlassian.py`, which resolves diff --git a/docs/platform-modules.md b/docs/platform-modules.md index 4b958ea..b469002 100644 --- a/docs/platform-modules.md +++ b/docs/platform-modules.md @@ -107,7 +107,7 @@ SNS topics, EventBridge rules, Config, GuardDuty, Security Hub, account guardrai GuardDuty filters only affect new findings; archive existing matches once in the console. -Team SSO roles' `CredentialAccess:IAMUser/AnomalousBehavior` on `ssm:GetParameter` is deliberately not filtered. The `javabin-team-{team}` permission set is `Action "*"` scoped by `StringEqualsIfExists aws:ResourceTag/team`, so untagged parameters are readable too, and this is the finding a stolen SSO session produces. Check the source country and confirm with the user before archiving one. +Team SSO roles' `CredentialAccess:IAMUser/AnomalousBehavior` on `ssm:GetParameter` is deliberately not filtered. The `javabin-team-{team}` permission set can read its own team's parameters (`/javabin/apps/{team}/*`, see [team-provisioner](lambda-functions.md#team-permission-set-javabin-teamteam)), and this is the finding a stolen SSO session produces. Check the source country and confirm with the user before archiving one. ## lambdas diff --git a/terraform/lambda-src/team_provisioner/handler.py b/terraform/lambda-src/team_provisioner/handler.py index b5a4e10..8db7440 100644 --- a/terraform/lambda-src/team_provisioner/handler.py +++ b/terraform/lambda-src/team_provisioner/handler.py @@ -1450,6 +1450,7 @@ def sync_group_to_identity_center(group): DEVELOPER_BOUNDARY_NAME = f"{PROJECT}-developer-boundary" +VIEW_ONLY_POLICY_ARN = "arn:aws:iam::aws:policy/job-function/ViewOnlyAccess" def _create_account_assignment(ps_arn, group_id, label=""): @@ -1503,12 +1504,14 @@ def _create_account_assignment(ps_arn, group_id, label=""): def _ensure_team_permission_set(team_name, group_id): """Create a per-team permission set and assign it to the team's IC group. - Guardrails come from javabin-team-{team}-guardrails (platform Terraform): - the developer boundary's rules with the team name written in. The shared - developer boundary can't be used here: its rules compare resource tags - with ${aws:PrincipalTag/team}, and Identity Center sessions carry no team - tag, so it would deny every team-scoped resource. The inline policy - allows broad access scoped to resources tagged with the team name. + The permissions boundary is javabin-team-{team}-guardrails (platform + Terraform): the developer boundary's rules with the team name written + in. The shared developer boundary can't be used here: its rules compare + resource tags with ${aws:PrincipalTag/team}, and Identity Center sessions + carry no team tag, so it would deny every team-scoped resource. The + boundary only limits; access itself comes + from AWS ViewOnlyAccess (console browsing, no data reads) plus the inline + policy from _team_inline_policy (the team's own resources). """ ps_name = f"{PROJECT}-team-{team_name}" @@ -1534,7 +1537,10 @@ def _ensure_team_permission_set(team_name, group_id): if not ps_arn: return {"error": "could not create or find permission set"} - # Attach the team's guardrails; drop the shared boundary from older setups. + # The team's guardrails are the permissions boundary. Older setups had + # them (or the shared developer boundary) attached as a policy, where + # their baseline "Allow *" granted everything the Denies didn't cover; + # detach those only once the boundary is in place. # Only a policy platform Terraform created: platform CI may not have made # it yet for a brand-new team (or never, for non-app teams like # platform). Then leave the permission set as it is and report pending. @@ -1544,41 +1550,146 @@ def _ensure_team_permission_set(team_name, group_id): logger.warning("Not switching %s to %s: %s", ps_name, guardrails, state) return {"permission_set": ps_name, "guardrails": state} try: - sso_client.attach_customer_managed_policy_reference_to_permission_set( + sso_client.put_permissions_boundary_to_permission_set( InstanceArn=SSO_INSTANCE_ARN, PermissionSetArn=ps_arn, - CustomerManagedPolicyReference={"Name": guardrails}, + PermissionsBoundary={"CustomerManagedPolicyReference": {"Name": guardrails}}, ) - except sso_client.exceptions.ConflictException: - pass # already attached except Exception as e: - logger.error("Failed to attach %s to %s: %s", guardrails, ps_name, e) + logger.error("Failed to set %s as boundary of %s: %s", guardrails, ps_name, e) + return {"permission_set": ps_name, "error": str(e)[:200]} + for name in (guardrails, DEVELOPER_BOUNDARY_NAME): + try: + sso_client.detach_customer_managed_policy_reference_from_permission_set( + InstanceArn=SSO_INSTANCE_ARN, + PermissionSetArn=ps_arn, + CustomerManagedPolicyReference={"Name": name}, + ) + logger.info("Detached policy %s from %s", name, ps_name) + except sso_client.exceptions.ResourceNotFoundException: + pass # not attached + except Exception as e: + logger.error("Failed to detach %s from %s: %s", name, ps_name, e) + + view_only = _attach_view_only(ps_arn, ps_name) + policy = _team_inline_policy(team_name) + try: - sso_client.detach_customer_managed_policy_reference_from_permission_set( + sso_client.put_inline_policy_to_permission_set( InstanceArn=SSO_INSTANCE_ARN, PermissionSetArn=ps_arn, - CustomerManagedPolicyReference={"Name": DEVELOPER_BOUNDARY_NAME}, + InlinePolicy=json.dumps(policy), ) - logger.info("Detached %s from %s", DEVELOPER_BOUNDARY_NAME, ps_name) - except sso_client.exceptions.ResourceNotFoundException: - pass # not attached except Exception as e: - logger.error("Failed to detach %s from %s: %s", DEVELOPER_BOUNDARY_NAME, ps_name, e) + logger.error("Failed to set inline policy for %s: %s", ps_name, e) + return {"error": str(e)[:200]} - # Set inline policy — broad allow scoped to team-tagged resources - policy = { + # Assign to group and wait for completion + result = _create_account_assignment(ps_arn, group_id, ps_name) + result["permission_set"] = ps_name + result["view_only"] = view_only + # Push policy changes to the role in the account (needed for existing + # assignments; a new assignment provisions on creation) + result["provisioned"] = _provision_permission_set(ps_arn, ps_name) + return result + + +def _attach_view_only(ps_arn, ps_name): + """Attach AWS ViewOnlyAccess (the same read-only browsing as + javabin-readonly). 'ok', or the error: the inline policy is still set, + so the team keeps access to its own resources either way.""" + try: + sso_client.attach_managed_policy_to_permission_set( + InstanceArn=SSO_INSTANCE_ARN, + PermissionSetArn=ps_arn, + ManagedPolicyArn=VIEW_ONLY_POLICY_ARN, + ) + except sso_client.exceptions.ConflictException: + pass # already attached + except Exception as e: + logger.error("Failed to attach ViewOnlyAccess to %s: %s", ps_name, e) + return f"error: {str(e)[:200]}" + return "ok" + + +def _team_inline_policy(team_name): + """Inline policy of javabin-team-{team}: write access only to the team's + own resources, on top of ViewOnlyAccess and under the team guardrails. + + The tag allow is StringEquals, not StringEqualsIfExists: an action whose + request carries no aws:ResourceTag (S3 objects, Route53, creates, any + untagged legacy resource) is not allowed by it. The team's own resources + that can't be matched by tag are allowed by name instead: SSM parameters + (PutParameter has no aws:ResourceTag), log groups, buckets + ({team}-{name}-{account}) and ECS Exec on the shared cluster (the task is + checked by its propagated team tag). + """ + params = f"arn:aws:ssm:*:{ACCOUNT_ID}:parameter/{PROJECT}/apps/{team_name}" + buckets = f"arn:aws:s3:::{team_name}-*-{ACCOUNT_ID}" + return { "Version": "2012-10-17", "Statement": [ { - "Sid": "AllowTeamResources", + "Sid": "AllowTeamTaggedResources", "Effect": "Allow", "Action": "*", "Resource": "*", - "Condition": { - "StringEqualsIfExists": { - "aws:ResourceTag/team": team_name, - } - }, + "Condition": {"StringEquals": {"aws:ResourceTag/team": team_name}}, + }, + { + "Sid": "AllowTeamParameters", + "Effect": "Allow", + "Action": ["ssm:GetParameter", "ssm:GetParameters", "ssm:GetParametersByPath", + "ssm:GetParameterHistory", "ssm:PutParameter"], + "Resource": [params, f"{params}/*"], + }, + { + # Guardrails DenyXLogs already limits log reads to these groups + "Sid": "AllowTeamLogs", + "Effect": "Allow", + "Action": ["logs:GetLogEvents", "logs:FilterLogEvents", "logs:DescribeLogStreams", + "logs:StartQuery", "logs:StartLiveTail", "logs:GetLogGroupFields"], + "Resource": f"arn:aws:logs:*:{ACCOUNT_ID}:log-group:/ecs/{team_name}/*", + }, + { + "Sid": "AllowTeamBuckets", + "Effect": "Allow", + "Action": ["s3:ListBucket", "s3:ListBucketVersions", "s3:GetBucketLocation", + "s3:GetObject", "s3:GetObjectVersion", "s3:PutObject", + "s3:DeleteObject", "s3:DeleteObjectVersion", + "s3:AbortMultipartUpload", "s3:ListMultipartUploadParts"], + "Resource": [buckets, f"{buckets}/*"], + }, + { + # The cluster half of ECS Exec; the task half needs the team tag + "Sid": "AllowExecOnPlatformCluster", + "Effect": "Allow", + "Action": "ecs:ExecuteCommand", + "Resource": f"arn:aws:ecs:*:{ACCOUNT_ID}:cluster/{PROJECT}-platform", + }, + { + # Kept although ViewOnlyAccess covers most of it: javabin-cli + # (status, secret) must not depend on the managed attachment + "Sid": "AllowUntaggedReads", + "Effect": "Allow", + "Action": [ + "ecs:ListClusters", + "ecs:ListServices", + "ecs:ListTasks", + "ecs:DescribeServices", + "ecs:DescribeTasks", + "ecr:GetAuthorizationToken", + "elasticloadbalancing:Describe*", + "route53:ListHostedZones", + "logs:DescribeLogGroups", + "ssm:DescribeParameters", + "cloudwatch:ListMetrics", + "cloudwatch:GetMetricData", + "cloudwatch:GetMetricStatistics", + "ce:GetCostAndUsage", + "ce:GetCostForecast", + ], + "Resource": "*", }, { # Platform-managed parameters in the team path: Cognito @@ -1606,43 +1717,9 @@ def _ensure_team_permission_set(team_name, group_id): "Resource": f"arn:aws:ssm:*:{ACCOUNT_ID}:parameter/{PROJECT}/apps/*", "Condition": {"BoolIfExists": {"ssm:Overwrite": "false"}}, }, - { - "Sid": "AllowUntaggedReads", - "Effect": "Allow", - "Action": [ - "ecs:ListClusters", - "ecs:ListServices", - "ecr:GetAuthorizationToken", - "elasticloadbalancing:Describe*", - "route53:ListHostedZones", - "logs:DescribeLogGroups", - "cloudwatch:ListMetrics", - "ce:GetCostAndUsage", - "ce:GetCostForecast", - ], - "Resource": "*", - }, ], } - try: - sso_client.put_inline_policy_to_permission_set( - InstanceArn=SSO_INSTANCE_ARN, - PermissionSetArn=ps_arn, - InlinePolicy=json.dumps(policy), - ) - except Exception as e: - logger.error("Failed to set inline policy for %s: %s", ps_name, e) - return {"error": str(e)[:200]} - - # Assign to group and wait for completion - result = _create_account_assignment(ps_arn, group_id, ps_name) - result["permission_set"] = ps_name - # Push policy changes to the role in the account (needed for existing - # assignments; a new assignment provisions on creation) - result["provisioned"] = _provision_permission_set(ps_arn, ps_name) - return result - def _guardrails_state(policy_name): """'ok' if the team guardrails policy exists and was created by platform diff --git a/terraform/lambda-src/tests/test_team_provisioner_memberships.py b/terraform/lambda-src/tests/test_team_provisioner_memberships.py index 4465a22..36eeb65 100644 --- a/terraform/lambda-src/tests/test_team_provisioner_memberships.py +++ b/terraform/lambda-src/tests/test_team_provisioner_memberships.py @@ -63,7 +63,7 @@ def test_prune_removes_gone_groups_but_keeps_team_and_client_rows(self): class TeamPermissionSetTest(unittest.TestCase): - def test_team_permission_set_uses_team_guardrails_and_reprovisions(self): + def _provisioner(self): h = load(FakeDynamo([])) sso = mock.MagicMock() sso.exceptions.ConflictException = type("Conflict", (Exception,), {}) @@ -75,23 +75,86 @@ def test_team_permission_set_uses_team_guardrails_and_reprovisions(self): h._create_account_assignment = lambda *a, **k: {"assigned": True} h.time.sleep = lambda s: None h._guardrails_state = lambda name: "ok" + return h, sso + + def test_team_guardrails_are_the_boundary_not_an_attached_policy(self): + h, sso = self._provisioner() result = h._ensure_team_permission_set("cupcake", "group-1") - attach = sso.attach_customer_managed_policy_reference_to_permission_set.call_args.kwargs - self.assertEqual(attach["CustomerManagedPolicyReference"], {"Name": "javabin-team-cupcake-guardrails"}) - detach = sso.detach_customer_managed_policy_reference_from_permission_set.call_args.kwargs - self.assertEqual(detach["CustomerManagedPolicyReference"], {"Name": "javabin-developer-boundary"}) + boundary = sso.put_permissions_boundary_to_permission_set.call_args.kwargs + self.assertEqual(boundary["PermissionsBoundary"], + {"CustomerManagedPolicyReference": {"Name": "javabin-team-cupcake-guardrails"}}) + # Attached as a policy, the guardrails' baseline "Allow *" granted everything + sso.attach_customer_managed_policy_reference_to_permission_set.assert_not_called() + detached = {c.kwargs["CustomerManagedPolicyReference"]["Name"] + for c in sso.detach_customer_managed_policy_reference_from_permission_set.call_args_list} + self.assertEqual(detached, {"javabin-team-cupcake-guardrails", "javabin-developer-boundary"}) + view_only = sso.attach_managed_policy_to_permission_set.call_args.kwargs + self.assertEqual(view_only["ManagedPolicyArn"], "arn:aws:iam::aws:policy/job-function/ViewOnlyAccess") + self.assertEqual(result["view_only"], "ok") + self.assertEqual(result["provisioned"], {"status": "SUCCEEDED"}) policy = json.loads(sso.put_inline_policy_to_permission_set.call_args.kwargs["InlinePolicy"]) - sids = {st["Sid"] for st in policy["Statement"]} - self.assertTrue({"DenyPlatformManagedParameters", "DenyCreatingParameters"} <= sids) + self.assertEqual(policy, h._team_inline_policy("cupcake")) + + def test_boundary_failure_leaves_policies_alone(self): + h, sso = self._provisioner() + sso.put_permissions_boundary_to_permission_set.side_effect = RuntimeError("AccessDenied") + + result = h._ensure_team_permission_set("cupcake", "group-1") + + self.assertIn("AccessDenied", result["error"]) + sso.detach_customer_managed_policy_reference_from_permission_set.assert_not_called() + sso.put_inline_policy_to_permission_set.assert_not_called() + sso.provision_permission_set.assert_not_called() + + def test_view_only_failure_still_sets_the_team_policy(self): + h, sso = self._provisioner() + sso.attach_managed_policy_to_permission_set.side_effect = RuntimeError("AccessDenied") + + result = h._ensure_team_permission_set("cupcake", "group-1") + + self.assertTrue(result["view_only"].startswith("error")) + sso.put_inline_policy_to_permission_set.assert_called_once() self.assertEqual(result["provisioned"], {"status": "SUCCEEDED"}) - deny = next(st for st in policy["Statement"] if st["Sid"] == "DenyCreatingParameters") + + def test_inline_policy_allows_only_the_teams_own_resources(self): + h = load(FakeDynamo([])) + h.ACCOUNT_ID = "123456789012" + policy = h._team_inline_policy("cupcake") + st = {s["Sid"]: s for s in policy["Statement"]} + + # No IfExists: a request without the resource's team tag (untagged + # resources, S3 objects, Route53, creates) is not allowed by it + self.assertEqual(st["AllowTeamTaggedResources"]["Condition"], + {"StringEquals": {"aws:ResourceTag/team": "cupcake"}}) + for s in policy["Statement"]: + if s["Effect"] != "Allow" or s["Sid"] == "AllowTeamTaggedResources": + continue + self.assertNotEqual(s["Action"], "*", s["Sid"]) + for action in s["Action"] if isinstance(s["Action"], list) else [s["Action"]]: + self.assertNotIn(action.split(":")[1], ("*", "Delete*", "Put*"), s["Sid"]) + if s["Resource"] == "*": + self.assertEqual(s["Sid"], "AllowUntaggedReads") + self.assertTrue(all(a.split(":")[1].startswith(("List", "Describe", "Get")) + for a in st["AllowUntaggedReads"]["Action"])) + self.assertEqual(st["AllowTeamParameters"]["Resource"], [ + "arn:aws:ssm:*:123456789012:parameter/javabin/apps/cupcake", + "arn:aws:ssm:*:123456789012:parameter/javabin/apps/cupcake/*", + ]) + self.assertEqual(st["AllowTeamLogs"]["Resource"], "arn:aws:logs:*:123456789012:log-group:/ecs/cupcake/*") + # {team}-{name}-{account}: never a legacy bucket like javazone-backup + self.assertEqual(st["AllowTeamBuckets"]["Resource"], [ + "arn:aws:s3:::cupcake-*-123456789012", "arn:aws:s3:::cupcake-*-123456789012/*"]) + self.assertEqual(st["AllowExecOnPlatformCluster"]["Resource"], + "arn:aws:ecs:*:123456789012:cluster/javabin-platform") + deny = st["DenyCreatingParameters"] self.assertEqual(deny["Condition"], {"BoolIfExists": {"ssm:Overwrite": "false"}}) # app-auth-sync's outputs are readable but not writable by the team - managed = next(st for st in policy["Statement"] if st["Sid"] == "DenyPlatformManagedParameters") + managed = st["DenyPlatformManagedParameters"] self.assertTrue(any(r.endswith("/apps/*/*/auth-status") for r in managed["Resource"])) self.assertTrue(any(r.endswith("/apps/*/*/cognito-client-*") for r in managed["Resource"])) + self.assertLess(len(json.dumps(policy)), 32768) # permission set inline limit def _guardrails_with(self, tags=None, missing=False): h = load(FakeDynamo([])) @@ -118,7 +181,8 @@ def test_missing_guardrails_leave_the_permission_set_untouched(self): result = h._ensure_team_permission_set("newteam", "group-1") self.assertTrue(result["guardrails"].startswith("pending")) h.sso_client.detach_customer_managed_policy_reference_from_permission_set.assert_not_called() - h.sso_client.attach_customer_managed_policy_reference_to_permission_set.assert_not_called() + h.sso_client.put_permissions_boundary_to_permission_set.assert_not_called() + h.sso_client.put_inline_policy_to_permission_set.assert_not_called() if __name__ == "__main__": diff --git a/terraform/platform/iam/boundary.tf b/terraform/platform/iam/boundary.tf index c5ba24c..4fa598e 100644 --- a/terraform/platform/iam/boundary.tf +++ b/terraform/platform/iam/boundary.tf @@ -294,8 +294,9 @@ resource "aws_iam_policy" "developer_boundary" { # never set on users), so ${aws:PrincipalTag/team} is empty for humans and # DenyXTeam would deny everything team-scoped. Each per-team permission set # (javabin-team-{team}, created by team-provisioner) knows its team, so it -# attaches this rendered copy instead. Multi-team heroes pick the team by -# picking the permission set. +# uses this rendered copy as its permissions boundary instead. Never attach +# it as a policy: the baseline "Allow *" would then grant everything the +# Denies miss. Multi-team heroes pick the team by picking the permission set. # # Self-protected by DenyBndMod in the developer boundary (team CI roles # cannot create new versions). diff --git a/terraform/platform/lambdas/main.tf b/terraform/platform/lambdas/main.tf index 1606a02..1d7ec17 100644 --- a/terraform/platform/lambdas/main.tf +++ b/terraform/platform/lambdas/main.tf @@ -677,19 +677,24 @@ resource "aws_iam_role_policy" "team_provisioner" { "sso:DescribePermissionSet", "sso:CreatePermissionSet", "sso:PutInlinePolicyToPermissionSet", - "sso:AttachCustomerManagedPolicyReferenceToPermissionSet", "sso:DetachCustomerManagedPolicyReferenceFromPermissionSet", + "sso:AttachManagedPolicyToPermissionSet", + "sso:PutPermissionsBoundaryToPermissionSet", "sso:ProvisionPermissionSet", "sso:DescribePermissionSetProvisioningStatus", ] Resource = "*" }, { - # Provisioning a permission set reads its customer-managed policies - Sid = "ReadTeamGuardrails" - Effect = "Allow" - Action = ["iam:GetPolicy", "iam:GetPolicyVersion", "iam:ListPolicyTags"] - Resource = "arn:aws:iam::${var.aws_account_id}:policy/${var.project}-team-*-guardrails" + # Provisioning a permission set reads its policies (team guardrails, + # and the ViewOnlyAccess attached to javabin-team-{team}) + Sid = "ReadTeamGuardrails" + Effect = "Allow" + Action = ["iam:GetPolicy", "iam:GetPolicyVersion", "iam:ListPolicyTags"] + Resource = [ + "arn:aws:iam::${var.aws_account_id}:policy/${var.project}-team-*-guardrails", + "arn:aws:iam::aws:policy/job-function/ViewOnlyAccess", + ] }, { Sid = "IAMForSSOAssignment" @@ -714,6 +719,17 @@ resource "aws_iam_role_policy" "team_provisioner" { "arn:aws:iam::${var.aws_account_id}:saml-provider/AWSSSO_*", ] }, + { + # Provisioning javabin-team-{team} in this (management) account sets + # the team guardrails as boundary of the SSO role with our credentials + Sid = "SetTeamGuardrailsBoundary" + Effect = "Allow" + Action = "iam:PutRolePermissionsBoundary" + Resource = "arn:aws:iam::${var.aws_account_id}:role/aws-reserved/sso.amazonaws.com/*" + Condition = { + ArnLike = { "iam:PermissionsBoundary" = "arn:aws:iam::${var.aws_account_id}:policy/${var.project}-team-*-guardrails" } + } + }, ] }) } From 132f326982dddc00b1e8a9f2a40c054d097b7f40 Mon Sep 17 00:00:00 2001 From: Alexanderamiri Date: Sat, 31 Oct 2026 13:44:51 +0100 Subject: [PATCH 2/2] fix(identity): give team permission sets without guardrails read-only access javabin-team-platform has no guardrails policy, so the provisioner left it as it was: javabin-developer-boundary attached as a policy, whose Allow * granted everything its Denies miss. A team whose guardrails are pending or refused now gets ViewOnlyAccess plus a read-only inline policy, with the legacy attached policies removed. The happy-path test also checks that the boundary is set before the old policies are detached. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- docs/lambda-functions.md | 2 +- .../lambda-src/team_provisioner/handler.py | 90 +++++++++++-------- .../test_team_provisioner_memberships.py | 40 ++++++--- 4 files changed, 83 insertions(+), 51 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3e803ef..7a12f49 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -325,7 +325,7 @@ Team group OWNERs are admin-managed and never removed by the sync; non-@java.no ### Known Issues - **ECS deploy stabilization**: platform-test-app task registers but service fails health check - **Cognito**: pools deployed; internal pool's `GoogleSAML` IdP was created outside Terraform. It is replaced by the Terraform-managed Google OIDC IdP at cutover (docs/cognito-google-setup.md), then deleted -- **SSO sessions carry no `team` tag**: the Identity Center ABAC attribute (`${path:enterprise.team}`) is never set on users, so `${aws:PrincipalTag/team}` is empty for humans. Per-team permission sets (`javabin-team-{team}`) therefore use `javabin-team-{team}-guardrails` (the developer boundary rendered with the team name) as their permissions boundary instead. It must stay a boundary: its baseline `Allow *` attached as a policy grants everything its Denies miss (every untagged resource). The ABAC-based `javabin-developer` permission set in `terraform/org` cannot reach team resources +- **SSO sessions carry no `team` tag**: the Identity Center ABAC attribute (`${path:enterprise.team}`) is never set on users, so `${aws:PrincipalTag/team}` is empty for humans. Per-team permission sets (`javabin-team-{team}`) therefore use `javabin-team-{team}-guardrails` (the developer boundary rendered with the team name) as their permissions boundary instead. It must stay a boundary: its baseline `Allow *` attached as a policy grants everything its Denies miss (every untagged resource). A team with no guardrails policy (`platform`, or a new team before platform CI) gets read-only access only. The ABAC-based `javabin-developer` permission set in `terraform/org` cannot reach team resources - **App databases share one network path** (fixed by `app-network`): service-rds now allows ingress only from the app's own `{team}-{app}-tasks` SG; each app moves off the shared ECS tasks SG on its next pipeline run - **Team provisioner Lambda**: All sync functions working (Google/GitHub/Budget/Cognito/Identity Center). Password-set flow deployed. - **`registered_app_repos` manually managed**: Being replaced with team-scoped IAM roles (repo→team resolved via GitHub API at runtime) diff --git a/docs/lambda-functions.md b/docs/lambda-functions.md index 580f8bc..edc0116 100644 --- a/docs/lambda-functions.md +++ b/docs/lambda-functions.md @@ -139,7 +139,7 @@ more; read the current list there first and add to it rather than pasting this o So a team member can read and overwrite their secrets (`javabin secret`), read their logs, push and pull their ECR repos, redeploy, stop or exec into their services, and browse the console. They cannot change shared or untagged resources (state bucket, legacy buckets and instances, Route53 zones, untagged roles and Lambdas) or create resources by hand: infrastructure goes through `app.yaml` and CI. -Order on each run: set the boundary, then detach any `javabin-team-{team}-guardrails` or `javabin-developer-boundary` attached as a policy by older versions, attach `ViewOnlyAccess`, put the inline policy, provision. If the boundary cannot be set the permission set is left as it is and the result has an `error`; a `ViewOnlyAccess` failure is reported as `view_only` and the rest still applies. Teams without guardrails yet (`pending`) are not touched. +Order on each run: set the boundary, then detach any `javabin-team-{team}-guardrails` or `javabin-developer-boundary` attached as a policy by older versions, attach `ViewOnlyAccess`, put the inline policy, provision. If the boundary cannot be set the permission set is left as it is and the result has an `error`; a `ViewOnlyAccess` failure is reported as `view_only` and the rest still applies. A team without a Terraform-managed guardrails policy (`pending`, e.g. a brand-new team before platform CI or `platform` itself, or `refused`) gets read-only access instead: no boundary, the legacy policies detached, `ViewOnlyAccess`, and `_read_only_inline_policy` (only the untagged reads). Without a boundary, the team inline policy's tag allow would also cover platform-tagged resources. The result reports the state as `guardrails`, and the next run after platform CI creates the policy switches the team to full access. ### Atlassian group sync (`sync_atlassian`) diff --git a/terraform/lambda-src/team_provisioner/handler.py b/terraform/lambda-src/team_provisioner/handler.py index 8db7440..37e7c52 100644 --- a/terraform/lambda-src/team_provisioner/handler.py +++ b/terraform/lambda-src/team_provisioner/handler.py @@ -1543,21 +1543,25 @@ def _ensure_team_permission_set(team_name, group_id): # detach those only once the boundary is in place. # Only a policy platform Terraform created: platform CI may not have made # it yet for a brand-new team (or never, for non-app teams like - # platform). Then leave the permission set as it is and report pending. + # platform). Without a boundary the team policy's tag allow would cover + # platform-tagged resources too, so such a team gets read-only access + # (ViewOnlyAccess and _read_only_inline_policy) until the guardrails exist. guardrails = f"{PROJECT}-team-{team_name}-guardrails" state = _guardrails_state(guardrails) - if state != "ok": - logger.warning("Not switching %s to %s: %s", ps_name, guardrails, state) - return {"permission_set": ps_name, "guardrails": state} - try: - sso_client.put_permissions_boundary_to_permission_set( - InstanceArn=SSO_INSTANCE_ARN, - PermissionSetArn=ps_arn, - PermissionsBoundary={"CustomerManagedPolicyReference": {"Name": guardrails}}, - ) - except Exception as e: - logger.error("Failed to set %s as boundary of %s: %s", guardrails, ps_name, e) - return {"permission_set": ps_name, "error": str(e)[:200]} + if state == "ok": + try: + sso_client.put_permissions_boundary_to_permission_set( + InstanceArn=SSO_INSTANCE_ARN, + PermissionSetArn=ps_arn, + PermissionsBoundary={"CustomerManagedPolicyReference": {"Name": guardrails}}, + ) + except Exception as e: + logger.error("Failed to set %s as boundary of %s: %s", guardrails, ps_name, e) + return {"permission_set": ps_name, "error": str(e)[:200]} + policy = _team_inline_policy(team_name) + else: + logger.warning("No %s for %s (%s): read-only access", guardrails, ps_name, state) + policy = _read_only_inline_policy() for name in (guardrails, DEVELOPER_BOUNDARY_NAME): try: sso_client.detach_customer_managed_policy_reference_from_permission_set( @@ -1572,7 +1576,6 @@ def _ensure_team_permission_set(team_name, group_id): logger.error("Failed to detach %s from %s: %s", name, ps_name, e) view_only = _attach_view_only(ps_arn, ps_name) - policy = _team_inline_policy(team_name) try: sso_client.put_inline_policy_to_permission_set( @@ -1588,6 +1591,8 @@ def _ensure_team_permission_set(team_name, group_id): result = _create_account_assignment(ps_arn, group_id, ps_name) result["permission_set"] = ps_name result["view_only"] = view_only + if state != "ok": + result["guardrails"] = state # Push policy changes to the role in the account (needed for existing # assignments; a new assignment provisions on creation) result["provisioned"] = _provision_permission_set(ps_arn, ps_name) @@ -1612,6 +1617,38 @@ def _attach_view_only(ps_arn, ps_name): return "ok" +# Kept although ViewOnlyAccess covers most of it: javabin-cli (status, +# secret) must not depend on the managed attachment +_UNTAGGED_READS = { + "Sid": "AllowUntaggedReads", + "Effect": "Allow", + "Action": [ + "ecs:ListClusters", + "ecs:ListServices", + "ecs:ListTasks", + "ecs:DescribeServices", + "ecs:DescribeTasks", + "ecr:GetAuthorizationToken", + "elasticloadbalancing:Describe*", + "route53:ListHostedZones", + "logs:DescribeLogGroups", + "ssm:DescribeParameters", + "cloudwatch:ListMetrics", + "cloudwatch:GetMetricData", + "cloudwatch:GetMetricStatistics", + "ce:GetCostAndUsage", + "ce:GetCostForecast", + ], + "Resource": "*", +} + + +def _read_only_inline_policy(): + """Inline policy of a team permission set whose guardrails don't exist: + with no boundary to limit it, only reads (with ViewOnlyAccess).""" + return {"Version": "2012-10-17", "Statement": [_UNTAGGED_READS]} + + def _team_inline_policy(team_name): """Inline policy of javabin-team-{team}: write access only to the team's own resources, on top of ViewOnlyAccess and under the team guardrails. @@ -1667,30 +1704,7 @@ def _team_inline_policy(team_name): "Action": "ecs:ExecuteCommand", "Resource": f"arn:aws:ecs:*:{ACCOUNT_ID}:cluster/{PROJECT}-platform", }, - { - # Kept although ViewOnlyAccess covers most of it: javabin-cli - # (status, secret) must not depend on the managed attachment - "Sid": "AllowUntaggedReads", - "Effect": "Allow", - "Action": [ - "ecs:ListClusters", - "ecs:ListServices", - "ecs:ListTasks", - "ecs:DescribeServices", - "ecs:DescribeTasks", - "ecr:GetAuthorizationToken", - "elasticloadbalancing:Describe*", - "route53:ListHostedZones", - "logs:DescribeLogGroups", - "ssm:DescribeParameters", - "cloudwatch:ListMetrics", - "cloudwatch:GetMetricData", - "cloudwatch:GetMetricStatistics", - "ce:GetCostAndUsage", - "ce:GetCostForecast", - ], - "Resource": "*", - }, + _UNTAGGED_READS, { # Platform-managed parameters in the team path: Cognito # credentials and auth-status (app-auth-sync), the auth diff --git a/terraform/lambda-src/tests/test_team_provisioner_memberships.py b/terraform/lambda-src/tests/test_team_provisioner_memberships.py index 36eeb65..c899ccc 100644 --- a/terraform/lambda-src/tests/test_team_provisioner_memberships.py +++ b/terraform/lambda-src/tests/test_team_provisioner_memberships.py @@ -96,6 +96,11 @@ def test_team_guardrails_are_the_boundary_not_an_attached_policy(self): self.assertEqual(result["provisioned"], {"status": "SUCCEEDED"}) policy = json.loads(sso.put_inline_policy_to_permission_set.call_args.kwargs["InlinePolicy"]) self.assertEqual(policy, h._team_inline_policy("cupcake")) + # The old policies only go once the boundary is in place + calls = [c[0] for c in sso.mock_calls] + first_detach = calls.index("detach_customer_managed_policy_reference_from_permission_set") + self.assertLess(calls.index("put_permissions_boundary_to_permission_set"), first_detach) + self.assertLess(first_detach, calls.index("provision_permission_set")) def test_boundary_failure_leaves_policies_alone(self): h, sso = self._provisioner() @@ -173,17 +178,30 @@ def test_guardrails_must_exist_and_be_terraform_managed(self): self.assertTrue(self._guardrails_with(missing=True)._guardrails_state("p").startswith("pending")) self.assertTrue(self._guardrails_with({"managed-by": "me"})._guardrails_state("p").startswith("refused")) - def test_missing_guardrails_leave_the_permission_set_untouched(self): - h = load(FakeDynamo([])) - h.sso_client = mock.MagicMock() - h._resolve_permission_set_arn = lambda name: "arn:ps" - h._guardrails_state = lambda name: "pending: policy not created yet" - result = h._ensure_team_permission_set("newteam", "group-1") - self.assertTrue(result["guardrails"].startswith("pending")) - h.sso_client.detach_customer_managed_policy_reference_from_permission_set.assert_not_called() - h.sso_client.put_permissions_boundary_to_permission_set.assert_not_called() - h.sso_client.put_inline_policy_to_permission_set.assert_not_called() - + def test_missing_guardrails_give_read_only_access(self): + # e.g. javabin-team-platform: no guardrails, and the developer + # boundary attached as a policy by older versions (its "Allow *" + # granted everything). Without a boundary, no write access at all. + for state in ("pending: policy not created yet", "refused: not created by platform Terraform"): + h, sso = self._provisioner() + h._guardrails_state = lambda name, state=state: state + + result = h._ensure_team_permission_set("platform", "group-1") + + self.assertEqual(result["guardrails"], state) + sso.put_permissions_boundary_to_permission_set.assert_not_called() + detached = {c.kwargs["CustomerManagedPolicyReference"]["Name"] + for c in sso.detach_customer_managed_policy_reference_from_permission_set.call_args_list} + self.assertEqual(detached, {"javabin-team-platform-guardrails", "javabin-developer-boundary"}) + self.assertEqual(sso.attach_managed_policy_to_permission_set.call_args.kwargs["ManagedPolicyArn"], + "arn:aws:iam::aws:policy/job-function/ViewOnlyAccess") + policy = json.loads(sso.put_inline_policy_to_permission_set.call_args.kwargs["InlinePolicy"]) + self.assertEqual(policy, h._read_only_inline_policy()) + for st in policy["Statement"]: + self.assertEqual(st["Effect"], "Allow") + self.assertTrue(all(a.split(":")[1].startswith(("List", "Describe", "Get")) + for a in st["Action"]), st["Sid"]) + self.assertEqual(result["provisioned"], {"status": "SUCCEEDED"}) if __name__ == "__main__": unittest.main()