fix: Validation of XListenerSet certificateRefs#8168
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 canceled.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8168 +/- ##
==========================================
+ Coverage 73.68% 73.79% +0.10%
==========================================
Files 241 241
Lines 36569 36579 +10
==========================================
+ Hits 26946 26992 +46
+ Misses 7713 7681 -32
+ Partials 1910 1906 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
It's possible this solution (ed: the solution was updated based on this comment) isn't the right one given ReferenceGrant Semantics, which seems to imply the ReferenceGrant is not necessary when the ListenerSet references a secret in its own namespace:
If that's the case, I think the appropriate fix would be to skip the |
|
There was also a similar issue in kgateway. That was fixed by using the listener's parent's namespace rather than the gateway's namespace. I think the same fix would apply here given this line in internal/gatewayapi/validate.go: https://github.com/envoyproxy/gateway/blob/main/internal/gatewayapi/validate.go#L399 if certificateRef.Namespace != nil && string(*certificateRef.Namespace) != "" && string(*certificateRef.Namespace) != listener.gateway.Namespace {If the listener is an XListenerSet it should use the XListenerSet's namespace, not the gateway's namespace. That would allow an XListenerSet to refer to a secret in the same namespace as it. I think updating this code would also fix another issue I discovered, which is that an XListenerSet in one namespace can presently reference a secret that exists the Gateway's different namespace without a ReferenceGrant present, but it should not. This is because the listener's Gateway's namespace is used in the comparison instead of the namespace of the XListenerSet. |
7a1ab25 to
26e5a76
Compare
|
I updated the PR based on my comments above, and am marking it as ready for review. |
b3473e8 to
877a659
Compare
|
@krishicks Thanks for taking care of this. This PR looks good! Could we also move the secrets to another namespace in these two tests and add ReferenceGrants to cover the cross-ns secret references use case? https://github.com/envoyproxy/gateway/blob/main/test%2Fe2e%2Ftestdata%2Fxlistenerset-tls.yaml |
Yes, of course! I'm assuming you meant I pushed new changes. Please take a look at the e2e test for me; I think it's right but I'm not able to run it locally. |
|
@krishicks can you make CI happy? |
|
I understand the e2e failures and am working on an update. I also see how to run them locally, so I'll make sure they're green as well. |
|
@krishicks thanks for working this, your code looks good to me! e2e failure root cause is below During cleanup for each XListenerSet test, we delete so, we should move namespace and secret and XListenerSet (for HTTPS) into |
|
Thanks @kkk777-7 , that was my understanding of the failure. I chose to rename the existing XListenerSetTLS test to XListenerSetTLSPassthrough and add a new one, XListenerSetTLSTermination, which has its own Secret/ReferenceGrant/Namespace. I was able to run both of those e2e tests locally and see them pass, but the whole suite failed when I ran it in some other, non-XListenerSet area. |
Previously, validateTerminateModeAndGetTLSSecrets would always use the namespace of the listener's gateway when verifying a cross-namespace ref. This meant that if the listener were from an XListenerSet, whether or not the Secret associated with the certificateRef was in the same namespace as the XListenerSet, it would not be permitted. Additionally, and relatedly, this fixes an issue where an XListenerSet could reference a Secret in the gateway's namespace without a ReferenceGrant being present. With this change we add a new GetNamespace() method to gatewayapi.ListenerContext which returns the listener's gateway's namespace for a listener added directly to the gateway, or the XListenerSet's namespace otherwise. This is similar to some of the other methods that were added to ListenerContext in support of XListenerSets. The new method is used when creating the `crossNamespaceFrom` to determine if the certificateRef is permitted. If the Secret and XListenerSet are in the same namespace, it is permitted. If that is not the case a ReferenceGrant from the XListenerSet to the Secret will be properly searched for. Signed-off-by: Kris Hicks <khicks@nvidia.com>
|
This is ready for another round through CI. |
|
The test failures all seem unrelated to these changes. 🤔 |
|
/retest |
|
LGTM, thanks! |
Previously, validateTerminateModeAndGetTLSSecrets would always use the namespace of the listener's gateway when verifying a cross-namespace ref. This meant that if the listener were from an XListenerSet, whether or not the Secret associated with the certificateRef was in the same namespace as the XListenerSet, it would not be permitted. Additionally, and relatedly, this fixes an issue where an XListenerSet could reference a Secret in the gateway's namespace without a ReferenceGrant being present. With this change we add a new GetNamespace() method to gatewayapi.ListenerContext which returns the listener's gateway's namespace for a listener added directly to the gateway, or the XListenerSet's namespace otherwise. This is similar to some of the other methods that were added to ListenerContext in support of XListenerSets. The new method is used when creating the `crossNamespaceFrom` to determine if the certificateRef is permitted. If the Secret and XListenerSet are in the same namespace, it is permitted. If that is not the case a ReferenceGrant from the XListenerSet to the Secret will be properly searched for. Signed-off-by: Kris Hicks <khicks@nvidia.com>
Previously, validateTerminateModeAndGetTLSSecrets would always use the namespace of the listener's gateway when verifying a cross-namespace ref. This meant that if the listener were from an XListenerSet, whether or not the Secret associated with the certificateRef was in the same namespace as the XListenerSet, it would not be permitted. Additionally, and relatedly, this fixes an issue where an XListenerSet could reference a Secret in the gateway's namespace without a ReferenceGrant being present. With this change we add a new GetNamespace() method to gatewayapi.ListenerContext which returns the listener's gateway's namespace for a listener added directly to the gateway, or the XListenerSet's namespace otherwise. This is similar to some of the other methods that were added to ListenerContext in support of XListenerSets. The new method is used when creating the `crossNamespaceFrom` to determine if the certificateRef is permitted. If the Secret and XListenerSet are in the same namespace, it is permitted. If that is not the case a ReferenceGrant from the XListenerSet to the Secret will be properly searched for. Signed-off-by: krishicks <kris@krishicks.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
Previously, validateTerminateModeAndGetTLSSecrets would always use the namespace of the listener's gateway when verifying a cross-namespace ref. This meant that if the listener were from an XListenerSet, whether or not the Secret associated with the certificateRef was in the same namespace as the XListenerSet, it would not be permitted. Additionally, and relatedly, this fixes an issue where an XListenerSet could reference a Secret in the gateway's namespace without a ReferenceGrant being present. With this change we add a new GetNamespace() method to gatewayapi.ListenerContext which returns the listener's gateway's namespace for a listener added directly to the gateway, or the XListenerSet's namespace otherwise. This is similar to some of the other methods that were added to ListenerContext in support of XListenerSets. The new method is used when creating the `crossNamespaceFrom` to determine if the certificateRef is permitted. If the Secret and XListenerSet are in the same namespace, it is permitted. If that is not the case a ReferenceGrant from the XListenerSet to the Secret will be properly searched for. Signed-off-by: krishicks <kris@krishicks.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
* chore(docs): Update Azure Entra link in OIDC guide (#8167) Update Azure Entra link in OIDC guide Signed-off-by: Guy Daich <guy.daich@sap.com> * fix: continue processing the remaining xDS with invalid EnvoyPatchPolicies (#8153) continue processing the remaining xDS with invalid EnvoyPatchPolicies Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * build(deps): bump the actions group across 1 directory with 2 updates (#8178) Bumps the actions group with 2 updates in the / directory: [docker/login-action](https://github.com/docker/login-action) and [github/codeql-action](https://github.com/github/codeql-action). Updates `docker/login-action` from 3.6.0 to 3.7.0 - [Release notes](https://github.com/docker/login-action/releases) - [Commits](docker/login-action@5e57cd1...c94ce9f) Updates `github/codeql-action` from 4.32.0 to 4.32.1 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@b20883b...6bc82e0) --- updated-dependencies: - dependency-name: docker/login-action dependency-version: 3.7.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions - dependency-name: github/codeql-action dependency-version: 4.32.1 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: actions ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Isaac Wilson <10012479+jukie@users.noreply.github.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * fix: skip provision when IR Infra is invalid (#7754) * fix: do not trigger IR deletion when EnvoyProxy is invalid Signed-off-by: zirain <zirain2009@gmail.com> * add Invalid to ir.Infra Signed-off-by: zirain <zirain2009@gmail.com> * fix gen Signed-off-by: zirain <zirain2009@gmail.com> * add e2e Signed-off-by: zirain <zirain2009@gmail.com> * remove invalid Signed-off-by: zirain <zirain2009@gmail.com> * add comments Signed-off-by: zirain <zirain2009@gmail.com> * update Signed-off-by: zirain <zirain2009@gmail.com> * merge loop Signed-off-by: zirain <zirain2009@gmail.com> * move back Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * docs: add HTTP header and method based authentication task (#7990) * docs: add HTTP header and method based authentication task Signed-off-by: Aditya Sanskar Srivastav <161202916+Aditya7880900936@users.noreply.github.com> * docs: replace api-key examples with user header Signed-off-by: Aditya Sanskar Srivastav <161202916+Aditya7880900936@users.noreply.github.com> * docs: format header and method authentication examples Signed-off-by: Aditya Sanskar Srivastav <161202916+Aditya7880900936@users.noreply.github.com> * docs: add header and method based authorization examples Signed-off-by: Aditya Sanskar Srivastav <161202916+Aditya7880900936@users.noreply.github.com> --------- Signed-off-by: Aditya Sanskar Srivastav <161202916+Aditya7880900936@users.noreply.github.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * fix: Validation of XListenerSet certificateRefs (#8168) Previously, validateTerminateModeAndGetTLSSecrets would always use the namespace of the listener's gateway when verifying a cross-namespace ref. This meant that if the listener were from an XListenerSet, whether or not the Secret associated with the certificateRef was in the same namespace as the XListenerSet, it would not be permitted. Additionally, and relatedly, this fixes an issue where an XListenerSet could reference a Secret in the gateway's namespace without a ReferenceGrant being present. With this change we add a new GetNamespace() method to gatewayapi.ListenerContext which returns the listener's gateway's namespace for a listener added directly to the gateway, or the XListenerSet's namespace otherwise. This is similar to some of the other methods that were added to ListenerContext in support of XListenerSets. The new method is used when creating the `crossNamespaceFrom` to determine if the certificateRef is permitted. If the Secret and XListenerSet are in the same namespace, it is permitted. If that is not the case a ReferenceGrant from the XListenerSet to the Secret will be properly searched for. Signed-off-by: krishicks <kris@krishicks.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * fix: Remove whitespace for nodeSelector in deployment YAML - helm chart change (#8185) Remove whitespace for nodeSelector in deployment YAML Signed-off-by: Jess Belliveau <jess.belliveau@gmail.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * [release/v1.7.0] release notes (#8188) Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> --------- Signed-off-by: Guy Daich <guy.daich@sap.com> Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com> Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: zirain <zirain2009@gmail.com> Signed-off-by: Aditya Sanskar Srivastav <161202916+Aditya7880900936@users.noreply.github.com> Signed-off-by: krishicks <kris@krishicks.com> Signed-off-by: Jess Belliveau <jess.belliveau@gmail.com> Co-authored-by: Guy Daich <guy.daich@sap.com> Co-authored-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Isaac Wilson <10012479+jukie@users.noreply.github.com> Co-authored-by: zirain <zirain2009@gmail.com> Co-authored-by: Aditya Sanskar Srivastav <161202916+Aditya7880900936@users.noreply.github.com> Co-authored-by: krishicks <kris@krishicks.com> Co-authored-by: Jess Belliveau <jess.belliveau@gmail.com>
Previously, validateTerminateModeAndGetTLSSecrets would always use the namespace of the listener's gateway when verifying a cross-namespace ref. This meant that if the listener were from an XListenerSet, whether or not the Secret associated with the certificateRef was in the same namespace as the XListenerSet, it would not be permitted. Additionally, and relatedly, this fixes an issue where an XListenerSet could reference a Secret in the gateway's namespace without a ReferenceGrant being present. With this change we add a new GetNamespace() method to gatewayapi.ListenerContext which returns the listener's gateway's namespace for a listener added directly to the gateway, or the XListenerSet's namespace otherwise. This is similar to some of the other methods that were added to ListenerContext in support of XListenerSets. The new method is used when creating the `crossNamespaceFrom` to determine if the certificateRef is permitted. If the Secret and XListenerSet are in the same namespace, it is permitted. If that is not the case a ReferenceGrant from the XListenerSet to the Secret will be properly searched for. Signed-off-by: Kris Hicks <khicks@nvidia.com>
What this PR does / why we need it:
Previously,
validateTerminateModeAndGetTLSSecretswould always use the namespace of the listener's gateway when verifying a cross-namespace ref.This meant that if the listener were from an XListenerSet, whether or not the Secret associated with the certificateRef was in the same namespace as the XListenerSet, it would not be permitted.
Additionally, and relatedly, this fixes an issue where an XListenerSet could reference a Secret in the gateway's namespace without a ReferenceGrant being present.
With this change we add a new
GetNamespace()method to gatewayapi.ListenerContext which returns the listener's gateway's namespace for a listener added directly to the gateway, or the XListenerSet's namespace otherwise. This is similar to some of the other methods that were added to ListenerContext in support of XListenerSets.The new method is used when creating the
crossNamespaceFromto determine if the certificateRef is permitted. If the Secret and XListenerSet are in the same namespace, it is permitted. If that is not the case a ReferenceGrant from the XListenerSet to the Secret will be properly searched for.Release Notes: Yes