profile-controller: preserve unrelated IAM trust policy statements when configuring IRSA
- Dominant language
- TypeScript
- Stars
- 19
- Forks
- 68
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 10
Description
### Checks
- [x] I have searched the existing issues.
- [x] My request is related to one of the components in the `kubeflow/dashboard` repository.
### Motivation
The Profile Controller's `AwsIamForServiceAccount` plugin can remove unrelated statements from an existing AWS IAM role trust policy when adding or removing a Kubeflow service account.
This can break other workloads or identity providers that legitimately use the same IAM role.
### What would you like to happen?
The Profile Controller should modify only the trust-policy statement/conditions belonging to the Kubeflow IRSA integration and preserve all unrelated statements and fields in the existing IAM trust policy.
In particular, the implementation should not assume that the relevant trust statement is always `Statement[0]`.
### What is happening currently?
In `components/profile-controller/controllers/plugin_iam.go`, the current implementation explicitly selects the first statement:
```go
oidcRoleArn := gjson.Get(policyDocument, "Statement.0.Principal.Federated").String()
...
statement := statements[0]
```
The code also documents this assumption:
```go
// We assume we only operator on first statement, don't add/remove new statement
```
After modifying that statement, the code reconstructs the policy using:
```go
newAssumeRolePolicyDocument := MakePolicyDocument(document)
```
`MakePolicyDocument()` creates a new policy whose `Statement` field contains only the statements passed to it.
In this path, only the newly reconstructed statement is passed, so an existing policy such as:
```json
{
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Principal": {
"Federated": "arn:aws:iam::123456789012:oidc-provider/example"
},
"Action": "sts:AssumeRoleWithWebIdentity"
},
{
"Effect": "Allow",
"Principal": {
"AWS": "arn:aws:iam::123456789012:role/another-workload"
},
"Action": "sts:AssumeRole"
}
]
}
```
can be rewritten to contain only the reconstructed first statement.
The second, unrelated trust relationship is therefore lost.
The same reconstruction pattern exists in `removeServiceAccountInAssumeRolePolicy()`, so removing a Kubeflow Profile/service account can have the same effect.
### Impact
This can cause unrelated identities to lose the ability to assume an IAM role.
For example, a shared IAM role may contain trust relationships for:
- Kubeflow/EKS IRSA;
- GitHub Actions OIDC;
- another Kubernetes cluster;
- another AWS account;
- another federated identity provider;
- an AWS IAM principal using `sts:AssumeRole`.
If the Profile Controller updates that role, unrelated trust statements can be removed and those workloads may subsequently fail with `AccessDenied`.
This is particularly risky because the change is performed automatically during Profile reconciliation.
### Reproduction
Create or use an IAM role whose assume-role policy contains at least two independent statements, for example:
```json
{
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Principal": {
"Federated": "arn:aws:iam::123456789012:oidc-provider/example"
},
"Action": "sts:AssumeRoleWithWebIdentity",
"Condition": {
"StringEquals": {
"example:aud": "sts.amazonaws.com"
}
}
},
{
"Effect": "Allow",
"Principal": {
"AWS": "arn:aws:iam::123456789012:role/another-workload"
},
"Action": "sts:AssumeRole"
}
]
}
```
Configure a Profile with the `AwsIamForServiceAccount` plugin using that role.
When the Profile Controller reconciles the Profile, inspect the role's assume-role policy again.
Expected:
```text
Statement[0] -> updated Kubeflow IRSA trust
Statement[1] -> unchanged
```
Current behavior:
```text
Statement[0] -> reconstructed Kubeflow IRSA trust
Statement[1] -> removed
```
The same behavior should be reproducible through the removal path.
### Additional concern
The problem is not limited to the number of statements.
The controller reconstructs the selected statement rather than modifying it in place. Therefore, existing attributes of that statement that are not explicitly reproduced by `MakeAssumeRoleWithWebIdentityPolicyDocument()` can also be lost.
The current helper reconstructs only the relevant:
- `Effect`
- `Action`
- `Principal.Federated`
- `Condition`
An existing statement containing additional policy attributes is therefore not necessarily preserved.
### Suggested fix
Instead of constructing a new trust policy from `Statement[0]`, preserve the complete existing policy and update only the relevant Kubeflow-managed statement.
The implementation should:
1. Parse the complete existing trust policy.
2. Locate the appropriate OIDC trust statement based on its provider/contents rather than its position.
3. Modify only the Kubeflow service-account condition.
4. Preserve all other statements.
5. Preserve unrelated fields in the target statement where possible.
6. Apply the same approach when removing a service account.
If the Profile Controller intentionally supports only a single Kubeflow-managed statement, the controller should explicitly validate that assumption and fail safely instead of silently replacing the existing trust policy.
### Regression tests
Please add regression coverage for:
- An IAM trust policy containing multiple statements.
- Adding a Kubeflow service account while an unrelated trust statement exists.
- Removing a Kubeflow service account while an unrelated trust statement exists.
- Multiple OIDC/federated trust statements.
- Preservation of unrelated statements.
- Preservation of existing fields in the target statement.
- Idempotent reconciliation.
Contributor guide
Assessment
This issue has not been assessed yet.