Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evaluate resource ordering to ensure authn/authz cannot be circumvented in failure modes #519

Open
rjferguson21 opened this issue Jun 28, 2024 · 0 comments
Labels
operator Issues pertaining to the UDS Operator (Pepr) security sso Issues related to the SSO stack (Keycloak/Authservice)

Comments

@rjferguson21
Copy link
Contributor

rjferguson21 commented Jun 28, 2024

Blake pointed out that as we add authn/authz functionality within Istio via authservice we need to be mindful of how unexpected failures and in general resource ordering could potentially leave applications exposed.

A straightforward way we might be able to somewhat alleviate this would be applying authorization policies (keycloak + authservice functionality) before the Istio resource generation. This would likely be an improvement for first reconciliation but we'd have to probably be more creative for cases where the VirtualService already existed.

"There is a potential security issue where this can fail open:

  1. Create a Package without groups, and wait for UDS operator to create the VirtualService
  2. Update the Package to have authservice (and optionally groups).
    • If the operator has an error during processing of either of these SSO functions, these functions will throw an error and halt the reconcile... but the old VirtualService will stay around. This means that it fails open.

(similarly you could go from Package+authservice → Package+authservice+groups and get an error in group application and get similar bad fail-open)

I'm unsure what the best solution is. Should we maybe catch errors in the authservice code and tear down the virtual services? That would mean the operator fails closed? Perhaps move istioResources() below this code and pass it a bool destroy option that triggers teardown (this allows better code reuse)?"

Originally posted by @bburky in #201 (comment)

@mjnagel mjnagel added security operator Issues pertaining to the UDS Operator (Pepr) labels Jul 2, 2024
@mjnagel mjnagel added the sso Issues related to the SSO stack (Keycloak/Authservice) label Jul 15, 2024
@mjnagel mjnagel added this to the 0.25.0 milestone Jul 22, 2024
@mjnagel mjnagel modified the milestones: 0.25.0, 0.26.0 Aug 2, 2024
@mjnagel mjnagel removed this from the 0.26.0 milestone Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
operator Issues pertaining to the UDS Operator (Pepr) security sso Issues related to the SSO stack (Keycloak/Authservice)
Projects
None yet
Development

No branches or pull requests

2 participants