pomerium / ingress-controller

Pomerium Kubernetes Ingress Controller
https://pomerium.com
Apache License 2.0
23 stars 11 forks source link

ingress.pomerium.io/allowed_idp_claims doesn't work anymore #94

Closed sherifkayad closed 2 years ago

sherifkayad commented 2 years ago

What happened?

We were heavy users of the Pomerium Operator until it got deprecated in favor of the Ingress Controller. Everything was working smoothly and now we wanted to migrate to the Ingress Controller.

We have a setup consisting of Nginx as the main Ingress Controller and honestly we don't want to change that! .. So we used the Forward Auth mechanism and we want to keep using it.

Among the used annotations we have on the Nginx Ingress objects is the following:

ingress.pomerium.io/allowed_idp_claims: '"groups": ["Team X", "Team Y"]'

That basically instructed Pomerium to allow any members of the groups Team X or Team Y. The Ingress controller is refusing that annotation with the message below:

{"level":"error","ts":1638349309.9580674,"logger":"controller.ingress","msg":"Reconciler error","reconciler group":"networking.k8s.io","reconciler kind":"Ingress","name":"my-secure-ingress","namespace":"my-namespace","error":"upsert: upsert routes: parsing ingress: annotations: applying policy annotations: yaml: mapping values are not allowed in this context"}

What did you expect to happen?

The allowed IDP Claims e.g. to fetch groups mapping should work as expected (as per the Operator)

How'd it happen?

Refer to the the What Happened section above

What's your environment like?

What's your config.yaml?

autocert: false
dns_lookup_family: V4_ONLY
address: :80
grpc_address: :80
authenticate_service_url: https://pomerium.mydomain.com
authorize_service_url: http://pomerium-authorize.pomerium.svc.cluster.local
databroker_service_url: http://pomerium-databroker.pomerium.svc.cluster.local
idp_provider: oidc
idp_scopes: [openid profile email groups offline_access]
idp_provider_url: https://myidp.com
insecure_server: true
grpc_insecure: true

cookie_domain: mydomain.com
databroker_storage_connection_string: rediss://:password@amazon.elasticache.endpoint.com
databroker_storage_tls_skip_verify: true
databroker_storage_type: redis
forward_auth_url: https://forwardauth.mydomain.com
cookie_secret: secret
shared_secret: secret2
idp_client_id: secret3
idp_client_secret: secret4
idp_service_account: pomerium-authenticate
databroker_storage_tls_skip_verify: true    
routes:
  - allow_any_authenticated_user: true
    allow_spdy: true
    allow_websockets: true
    from: https://dashboard.mydomain.com
    kubernetes_service_account_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token
    to: http://kubernetes-dashboard.kube-system.svc:443
  - allow_any_authenticated_user: true
    allow_spdy: true
    allow_websockets: true
    from: https://kubernetes.mydomain.com
    kubernetes_service_account_token_file: /var/run/secrets/kubernetes.io/serviceaccount/token
    tls_skip_verify: true
    to: https://kubernetes.default.svc

What did you see in the logs?

{"level":"error","ts":1638349309.9580674,"logger":"controller.ingress","msg":"Reconciler error","reconciler group":"networking.k8s.io","reconciler kind":"Ingress","name":"my-secure-ingress","namespace":"my-namespace","error":"upsert: upsert routes: parsing ingress: annotations: applying policy annotations: yaml: mapping values are not allowed in this context"}

Additional context

My ingress definition:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: my-secure-ingress
  namespace: my-namespace
  labels:
    app: demo
  annotations:
    nginx.ingress.kubernetes.io/auth-signin: "https://forwardauth.mydomain.com/?uri=https://$host$request_uri"
    nginx.ingress.kubernetes.io/auth-url: "https://forwardauth.mydomain.com/verify?uri=https://$host$request_uri"
    ingress.pomerium.io/allowed_idp_claims: '"groups": ["Team X", "Team Y"]'
spec:
  ingressClassName: nginx
  rules:
  - host: demo.mydomain.com
    http:
      paths:
      - path: /
        pathType: ImplementationSpecific
        backend:
          service:
            name: demo
            port:
              number: 8080
sherifkayad commented 2 years ago

@travisgroth an update here: After some digging in the Controller code base, I found out that the probable right syntax is as follows:

ingress.pomerium.io/allowed_idp_claims: '{"groups": ["Team X", "Team Y"]}'

With the allowed IDP Claims as a JSON Object the controller admits that and doesn't complain. However, the problem now is that users of either group won't be authorized! .. There's definitely something wrong here

sherifkayad commented 2 years ago

Another update: Actually the simplest annotation ingress.pomerium.io/allow_any_authenticated_user: "true" doesn't work for me. I keep getting the infinite redirect loop indicating that it's not working.

My Ingress is as follows:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: my-secure-ingress
  namespace: my-namespace
  labels:
    app: demo
  annotations:
    nginx.ingress.kubernetes.io/auth-signin: "https://forwardauth.mydomain.com/?uri=https://$host$request_uri"
    nginx.ingress.kubernetes.io/auth-url: "https://forwardauth.mydomain.com/verify?uri=https://$host$request_uri"
    ingress.pomerium.io/allow_any_authenticated_user: "true"
spec:
  ingressClassName: nginx
  rules:
  - host: demo.mydomain.com
    http:
      paths:
      - path: /
        pathType: ImplementationSpecific
        backend:
          service:
            name: demo
            port:
              number: 8080
travisgroth commented 2 years ago

A few things going on in this issue.

  1. Change in annotation syntax
  2. Infinite redirect for unauthorized routes
  3. Authorization policy not applying as expected

In reverse order:

(3) The issue is that we're defaulting to Path based pathType behavior for ImplementationSpecific. The old behavior - because we didn't consider pathType at all - was implicitly Prefix. In testing locally requests for / worked fine but anything else gave me (2). Can you try changing your ingress pathType to Prefix and see if that resolves this particular problem?

(2) You identified here https://github.com/pomerium/pomerium/issues/2796. I'd like to track that separately and it already has it's own issue so let's continue there.

(1) We need to investigate a little more. Offhand that wasn't an intentional syntax change but the way we parse annotations is substantially different now.

sherifkayad commented 2 years ago

@travisgroth Indeed using Prefix as the PathType fixed (3)! .. Thanks so much. I didn't know that the PathType had any impact on the Pomerium Policy generated by the Controller.

With regards to (1) I don't find it a big deal that the syntax changes, I think we just need to ensure that this is reflected in the documentation of the annotations somewhere.

As for (2) Yeah let's track that in the separate issue I created. BTW the issue I created was meant for the Operator not the Controller, however, I see the same behavior with the Controller. Let's discuss in more details on the other ticket, I think I might have identified the root cause of that.

sherifkayad commented 2 years ago

@travisgroth I think we might have a (4) 😢 .. that's basically defining regexes in paths. I will open a separate ticket for that.

travisgroth commented 2 years ago

@travisgroth Indeed using Prefix as the PathType fixed (3)! .. Thanks so much. I didn't know that the PathType had any impact on the Pomerium Policy generated by the Controller.

Great. I think we can change this behavior to default to Prefix type matching. as that would more closely align with user expectations.

With regards to (1) I don't find it a big deal that the syntax changes, I think we just need to ensure that this is reflected in the documentation of the annotations somewhere.

Stay tuned on this.

wasaga commented 2 years ago

you need use allowed_groups annotation.

sherifkayad commented 2 years ago

@wasaga as pointed out above, the ingress.pomerium.io/allowed_idp_claims: '{"groups": ["Team X", "Team Y"]}' syntax works together with a Prefix Ingress PathType.

For me it's fine the documentation just needs to reflect that kinda new syntax with the JSON Object {} notation