Source peer principal as a policy input variable

[tjons]

Resolves #710

I'm debatable if we should actually merge the functionality change here. We currently send the full check request to the interpreter for policy evaluation. However, a user recently expressed the desire to be able to validate SPIFFE principal identities via the plugin. The plugin technically supports this today, but it's a) not clearly documented and b) requires a significant amount of understanding of both how the plugin and Envoy work.

This PR adds a new input variable, source_principal that extracts the source peer principal and makes it available as a top level input for use in policy evaluation. I didn't even realize that we do indeed send the entire proto until I had completed the work, which is probably a good indicator that the UX here can be improved, since I'm pretty deep in the plugin.

I'm open to thoughts about whether this should, or shouldn't, be added. If we decide not to surface it as a top-level input variable, I can remove that part and keep the authorization test, as I think that at least has some value for others looking through the codebase.

[johanfylling]

Regarding a) not clearly documented: what's the discoverability of the new attribute? Do we need to also update documentation/examples? If so, would it be more complicated to simply describe how to retrieve the principal from where it's available today?

That said, I don't see a reason for not making it easier for policy authors by surfacing a new input attribute, even if it introduces some redundancy. This is the case for some of the other top-level input attributes, right?

[tjons]

@johanfylling thanks for your feedback! I'm glad you see some value here, I've spent some time thinking about it and I'm more convinced of my own work now too :)

That said, I don't see a reason for not making it easier for policy authors by surfacing a new input attribute, even if it introduces some redundancy. This is the case for some of the other top-level input attributes, right?

somewhat, but we transform all of them in one way or another, like the parsed body and parsed path variables: https://github.com/open-policy-agent/opa-envoy-plugin/blob/main/envoyauth/request.go#L74. I think this would be the first fully aliased input, but I think it's probably a significant improvement to usability and discoverability, and I'm thinking that it might be worth thinking about adding more of these to provide a gentler input API for new users in the future.

[remeric]

Thanks everyone for working on this and all your input, this will be really helpful!

I did want to follow up again and see if the destination principal would also be possible to send as well. There was a little conversation on the original Istio ticket but wasn’t sure if it was ever decided if that was possible or should be done.

Thanks!

[johanfylling]

@tjons, what's the status on this PR? Are you waiting for further input from me? Have you given further thought on my comments regarding also supporting ext_authz_v2.CheckRequest and to default input.source_principal to undefined?

[tjons]

@johanfylling thanks for the ping! I think this is ready for a review, I've added support for the v2 API and converted the input.source_principal variable to an ast.Value

[tjons]

Thanks everyone for working on this and all your input, this will be really helpful!

I did want to follow up again and see if the destination principal would also be possible to send as well. There was a little conversation on the original Istio ticket but wasn’t sure if it was ever decided if that was possible or should be done.

Thanks!

Hey @remeric, I'll take a look at that. We may not promote it to a top-level input variable, but it is available today (if sent) with input.request.attributes.destination.principal. I'll be adding some examples once this is all done, including the Istio side, so we'll be sure to better document this in the future.

[johanfylling]

Thank you 😃 👍

Just a pair of nits, but no blockers.

[tjons]

@johanfylling nits addressed! I think we're good to go here

[johanfylling]

Looks great 👍
Thank you!

[ashutosh-narkar]

Thanks everyone for working on this and all your input, this will be really helpful!
I did want to follow up again and see if the destination principal would also be possible to send as well. There was a little conversation on the original Istio ticket but wasn’t sure if it was ever decided if that was possible or should be done.
Thanks!

Hey @remeric, I'll take a look at that. We may not promote it to a top-level input variable, but it is available today (if sent) with input.request.attributes.destination.principal. I'll be adding some examples once this is all done, including the Istio side, so we'll be sure to better document this in the future.

Thanks for working on this @tjons! When do you think we can have some examples to try out? In the meantime, if you could provide some pointers or examples around how destination.principal can be extracted that would be really helpful. Thanks!

[tjons]

@ashutosh-narkar I'll try to find some time this week, been a bit busy the last few days. The whole v3.CheckRequest is sent as input.request, so simply referencing input.request.attributes.destination.principal should be enough to access it, if and only if it's there. This depends on the stage in the Envoy filter chain that the extauthz filter is applied - if it's before destination is determined, it won't be set, but if it's applied after endpoint selection (painting with a broad brush here), it should be present.

[ashutosh-narkar]

Thanks for your response @tjons!

cc @remeric