Tokenize Credential Request#782
Draft
desmax74 wants to merge 1 commit into
Draft
Conversation
Reviewer's GuideAdds cloud credentials support via OpenShift CredentialsRequest resources, configurable per cloud provider, and wires the resulting secrets into the Helm deployment for AWS, plus a sample dev-time credential request manifest. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The Helm template for
credentialrequest.yamlhas inconsistent indentation and mixed static/dynamicstatementEntries/predefinedRolesblocks, which may produce invalid or unexpected YAML; consider simplifying to either fully static defaults or a cleanly mergedtoYamlblock with carefully aligned indentation. - The
deployment.yamlcurrently hardcodes an AWS-specific secret and environment variables; ifcloudProvidercan be GCP/Azure, it would be better to conditionally inject provider-specific env vars or secrets to avoid misconfigured deployments for non-AWS providers. - The values for AWS (
s3:*) and the hardcodeds3:GetObject/PutObject/ListBucketactions in the credentials request may be redundant or conflicting; aligning the default actions with what the template actually renders will make the permissions model clearer and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Helm template for `credentialrequest.yaml` has inconsistent indentation and mixed static/dynamic `statementEntries`/`predefinedRoles` blocks, which may produce invalid or unexpected YAML; consider simplifying to either fully static defaults or a cleanly merged `toYaml` block with carefully aligned indentation.
- The `deployment.yaml` currently hardcodes an AWS-specific secret and environment variables; if `cloudProvider` can be GCP/Azure, it would be better to conditionally inject provider-specific env vars or secrets to avoid misconfigured deployments for non-AWS providers.
- The values for AWS (`s3:*`) and the hardcoded `s3:GetObject`/`PutObject`/`ListBucket` actions in the credentials request may be redundant or conflicting; aligning the default actions with what the template actually renders will make the permissions model clearer and easier to maintain.
## Individual Comments
### Comment 1
<location path="helm-charts/redhat-trusted-profile-analyzer/values.yaml" line_range="82-85" />
<code_context>
+ cloudProvider: aws # aws | gcp | azure
+ cloudCredentials:
+ aws:
+ statementEntries:
+ - effect: Allow
+ action: [ "s3:*" ]
+ resource: "*"
+ gcp:
</code_context>
<issue_to_address>
**🚨 issue (security):** AWS credentials default to full S3 access, which is overly permissive
The default `cloudCredentials.aws.statementEntries` grant `"s3:*"` on all resources, which is effectively admin-level S3 access and may be unacceptable in locked-down environments. Please scope the default to specific buckets and limited actions, or require users to provide these permissions explicitly rather than shipping such broad defaults.
</issue_to_address>
### Comment 2
<location path="helm-charts/redhat-trusted-profile-analyzer/templates/credentialrequest.yaml" line_range="11-20" />
<code_context>
+ name: {{ .Release.Name }}-cloud-creds
+ namespace: {{ .Release.Namespace }}
+
+ {{- if eq .Values.cloudProvider "aws" }}
+ providerSpec:
+ apiVersion: cloudcredential.openshift.io/v1
+ kind: AWSProviderSpec
+ statementEntries:
+ - effect: Allow
+ action:
+ - "s3:GetObject"
+ - "s3:PutObject"
+ - "s3:ListBucket"
+ resource: "*"
+ {{- toYaml .Values.cloudCredentials.aws.statementEntries | nindent 6 }}
+ {{- else if eq .Values.cloudProvider "gcp" }}
</code_context>
<issue_to_address>
**issue (bug_risk):** Indentation and mixed static/dynamic `statementEntries` may yield invalid or duplicated YAML
The `providerSpec` block is over-indented relative to the `if`, and `statementEntries` is both hardcoded and redefined via `toYaml .Values.cloudCredentials.aws.statementEntries | nindent 6`. This can break the YAML structure or create a malformed `statementEntries` field. Please align `providerSpec` with its peers and use a single, consistently indented `statementEntries` definition (either fully templated or merged cleanly).
</issue_to_address>
### Comment 3
<location path="helm-charts/redhat-trusted-profile-analyzer/templates/credentialrequest.yaml" line_range="23-27" />
<code_context>
+ - "s3:ListBucket"
+ resource: "*"
+ {{- toYaml .Values.cloudCredentials.aws.statementEntries | nindent 6 }}
+ {{- else if eq .Values.cloudProvider "gcp" }}
+ providerSpec:
+ apiVersion: cloudcredential.openshift.io/v1
+ kind: GCPProviderSpec
+ predefinedRoles:
+ {{- toYaml .Values.cloudCredentials.gcp.permissions | nindent 6 }}
+ {{- end }}
</code_context>
<issue_to_address>
**issue (bug_risk):** GCP `predefinedRoles` seems to be used with permission strings instead of role names
Here `predefinedRoles` is populated from `cloudCredentials.gcp.permissions`, but those values look like permission strings (e.g. `compute.instances.list`) instead of IAM role IDs (e.g. `roles/...`). Since `GCPProviderSpec.predefinedRoles` requires role names, this is likely to fail or not grant the required access. Either switch to the appropriate field for listing permissions (e.g. `serviceAccountPermissions`) or map these permissions to suitable predefined roles before using this field.
</issue_to_address>
### Comment 4
<location path="helm-charts/redhat-trusted-profile-analyzer/templates/deployment.yaml" line_range="11-20" />
<code_context>
+ containers:
+ - name: my-operator
+ env:
+ - name: AWS_ACCESS_KEY_ID
+ valueFrom:
+ secretKeyRef:
+ name: {{ .Release.Name }}-cloud-creds
+ key: aws_access_key_id
+ - name: AWS_SECRET_ACCESS_KEY
+ valueFrom:
+ secretKeyRef:
+ name: {{ .Release.Name }}-cloud-creds
+ key: aws_secret_access_key
</code_context>
<issue_to_address>
**issue (bug_risk):** Deployment always expects AWS-style secrets even when using non-AWS providers
Env vars are always populated from `{{ .Release.Name }}-cloud-creds` using AWS-specific keys. For `cloudProvider: gcp` (or others), the credentialsRequest will likely create a different secret schema, so this pod won’t receive valid creds. Please gate these env vars on the provider, or introduce provider-specific secret layouts/templates so non-AWS providers don’t break this deployment.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+82
to
+85
| aws: | ||
| statementEntries: | ||
| - effect: Allow | ||
| action: [ "s3:*" ] |
There was a problem hiding this comment.
🚨 issue (security): AWS credentials default to full S3 access, which is overly permissive
The default cloudCredentials.aws.statementEntries grant "s3:*" on all resources, which is effectively admin-level S3 access and may be unacceptable in locked-down environments. Please scope the default to specific buckets and limited actions, or require users to provide these permissions explicitly rather than shipping such broad defaults.
Comment on lines
+11
to
+20
| {{- if eq .Values.cloudProvider "aws" }} | ||
| providerSpec: | ||
| apiVersion: cloudcredential.openshift.io/v1 | ||
| kind: AWSProviderSpec | ||
| statementEntries: | ||
| - effect: Allow | ||
| action: | ||
| - "s3:GetObject" | ||
| - "s3:PutObject" | ||
| - "s3:ListBucket" |
There was a problem hiding this comment.
issue (bug_risk): Indentation and mixed static/dynamic statementEntries may yield invalid or duplicated YAML
The providerSpec block is over-indented relative to the if, and statementEntries is both hardcoded and redefined via toYaml .Values.cloudCredentials.aws.statementEntries | nindent 6. This can break the YAML structure or create a malformed statementEntries field. Please align providerSpec with its peers and use a single, consistently indented statementEntries definition (either fully templated or merged cleanly).
Comment on lines
+23
to
+27
| {{- else if eq .Values.cloudProvider "gcp" }} | ||
| providerSpec: | ||
| apiVersion: cloudcredential.openshift.io/v1 | ||
| kind: GCPProviderSpec | ||
| predefinedRoles: |
There was a problem hiding this comment.
issue (bug_risk): GCP predefinedRoles seems to be used with permission strings instead of role names
Here predefinedRoles is populated from cloudCredentials.gcp.permissions, but those values look like permission strings (e.g. compute.instances.list) instead of IAM role IDs (e.g. roles/...). Since GCPProviderSpec.predefinedRoles requires role names, this is likely to fail or not grant the required access. Either switch to the appropriate field for listing permissions (e.g. serviceAccountPermissions) or map these permissions to suitable predefined roles before using this field.
Comment on lines
+11
to
+20
| - name: AWS_ACCESS_KEY_ID | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Release.Name }}-cloud-creds | ||
| key: aws_access_key_id | ||
| - name: AWS_SECRET_ACCESS_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Release.Name }}-cloud-creds | ||
| key: aws_secret_access_key |
There was a problem hiding this comment.
issue (bug_risk): Deployment always expects AWS-style secrets even when using non-AWS providers
Env vars are always populated from {{ .Release.Name }}-cloud-creds using AWS-specific keys. For cloudProvider: gcp (or others), the credentialsRequest will likely create a different secret schema, so this pod won’t receive valid creds. Please gate these env vars on the provider, or introduce provider-specific secret layouts/templates so non-AWS providers don’t break this deployment.
desmax74
force-pushed
the
occ
branch
2 times, most recently
from
a21b447 to
12dda7c
Compare
Signed-off-by: desmax74 <mdessi@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Assisted-by: Claude: Opus 4.6
Summary by Sourcery
Introduce cloud-provider-specific credential requests and wire them into the trusted profile analyzer Helm chart.
New Features: