Closed dhaiducek closed 9 months ago
@dhaiducek are there any security repercussions with this approach? I'm thinking in particular that when running in App Sub, the Helm execution would end up using App Sub's service account but don't know for sure.
@dhaiducek are there any security repercussions with this approach? I'm thinking in particular that when running in App Sub, the Helm execution would end up using App Sub's service account but don't know for sure.
I was wondering about input on security implications. The risks feel similar to enabling Kustomize. I'm thinking it's toggled on by the user, and I think would call the internal Helm CLI, but that doesn't exist in App Sub, so it'd just fail in that case. (I just tried executing into the subscription container, and helm
came up empty.)
That said, I suppose I'll have to do some more digging around ArgoCD, but the situation there might be similar.
hi @dhaiducek .. this just "works" in argocd .. as it has helm and kustomize support already i.e. in the argocd-repo-server pod they have helm in /usr/local/bin/helm
This would be helpful. as stated above, this works in ArgoCD. Looks like a maintainer just needs to add a lgtm
label?
@dhaiducek I think we should approve it, I've a customer waiting on this and we reviewed it again and we do not see the risk, the customer would use ArgoCD and this would just use ArgoCD's Service Account. We could also doc any security related topics. What do you think?
@ch-stark and @dhaiducek , how about we require the user setting an environment variable to enable Helm in Kustomize? That way they opt-in to this feature and ensure it is secure to do so.
In Application Subscription, we can probably enable this by default and document it for ArgoCD.
@ch-stark and @dhaiducek , how about we require the user setting an environment variable to enable Helm in Kustomize? That way they opt-in to this feature and ensure it is secure to do so.
In Application Subscription, we can probably enable this by default and document it for ArgoCD.
That works for me--I've been thinking we just need to get this feature out there, and having a higher-level opt in sounds like a good way to go.
/hold for reviews
...and tests? Though that may be better handled in an integration test...
@dhaiducek could you please add documentation of the environment variable in the read me?
Updating the README.md
caused a really weird rebase (maybe because this PR was originally in stolostron
?), but I think it's sorted now...
/unhold
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: dhaiducek, mprahl
The full list of commands accepted by this bot can be found here.
The pull request process is described here
Allows passing flags equivalent to
--enable-helm
via the environment variablePOLICY_GEN_ENABLE_HELM
ref: https://issues.redhat.com/browse/ACM-9060
Closes #105