search

crossplane-contrib / function-go-templating

A Go templating composition function
https://crossplane.io
Apache License 2.0
58 stars 37 forks source link

Remove access to Sprigs env and expandenv functions #67

Closed jake-ciolek closed 4 months ago

jake-ciolek commented 10 months ago

What happened?

Excluding Sprig's functions related to accessing the pod/host environment could enhance the security posture.

The current approach integrates all of Sprig's offerings, notably the "env" and "expandenv" functions. However, it's worth noting that other projects leveraging Sprig, such as Helm and ArgoCD, consciously omit these particular functions.

The Sprig documentation itself cautions against the potential for information leakage through these functions, as detailed here: Sprig Documentation.

Considering that typical use cases involving GoTemplate doesn't require fetching runtime environment variables, it seems prudent to reconsider including these capabilities.

I'd appreciate your thoughts on this matter. Thank you!

jake-ciolek commented 10 months ago

I've sent a small fix in #68

phisco commented 10 months ago

I believe there is less risk here because the code being executed should have been written by a Composition author or another highly privileged user (or at least installed in the cluster through a Package by an highly privileged user). However, I do think it would be better not to encourage such practices too.

j2L4e commented 9 months ago

Only allowing prefixed variable names (like FN_GO_TPL_MY_VAR) would be an alternative that's more secure but doesn't remove the possibility of using env vars. What do you think?

jake-ciolek commented 9 months ago

@j2L4e That approach sounds like an interesting middle-ground.

For reference, here you can see Helm explicitly removing these two:

https://github.com/helm/helm/blob/c7f318ca4c0687292e258474134afec691054cd1/pkg/engine/funcs.go#L46

And so does ArgoCD (argo also removes getHostByName):

https://github.com/argoproj/argo-cd/blob/3cc02779ca8c9711934f38ee43f612e18c756d0e/server/deeplinks/deeplinks.go#L24

It might be valuable to align with these and do a similar thing here. Are there any valid use cases for this function sourcing values from the environment?

@phisco I agree that usually it's a highly (highest?) privileged user doing the deployment. Might not be the case always though and then it's possible to use the composition function pod as a way of learning more about the compute environment.

I'm wondering if there's an example architectural use case of wanting to source things from the environment of the functions pod? It would be helpful to find some to reason about this more.