OIDC Policy

[didierofrivia]

Closes #1148
Based on Kuadrant/architecture#114

This PR introduces the Meta Policy OIDCPolicy using the extensions "framework" and consuming the Kuadrant context for the subscriptions to particular DAG object props changes. Mind that at the moment is a basic implementation of the OIDC flow, following heuristics and definitely open to change :)

Notes

• Introducing new CRDs on v1alpha1
• Currently the only OIDC flow supported is the browser app one via the Authorization Code Flow out of the box
• For Native Apps, or tokenSource other than cookie, the storage of the token should be stored by the client
• Verification steps require a Gitlab account
• Status might need definition on the relevant info expected
• So far, it also behaves as an Override policy

Verification Steps

1. Local cluster with Kuadrant WITH_EXTENSIONS set to true

WITH_EXTENSIONS=true  make local-setup

2. Patch Kuadrant deployment and apply Kuadrant CR

kubectl -n kuadrant-system patch deployment kuadrant-operator-controller-manager \
  --type='json' \
  -p='[{"op": "add", "path": "/spec/template/spec/containers/0/env/-", "value": {"name": "AUTH_SERVICE_TIMEOUT", "value": "5s"}}]'

kubectl set env -n kuadrant-system deployments/kuadrant-operator-controller-manager LOG_LEVEL=debug WITH_EXTENSIONS="true"

kubectl create -n kuadrant-system -f - <<EOF
apiVersion: kuadrant.io/v1beta1
kind: Kuadrant
metadata:
  name: kuadrant
spec: {}
EOF

3. Configure default ingress gateway

export INGRESS_IP=$(kubectl get gateway/kuadrant-ingressgateway -n gateway-system -o jsonpath='{.status.addresses[0].value}')

kubectl apply -n gateway-system -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: kuadrant-ingressgateway
spec:
  gatewayClassName: istio
  listeners:
  - allowedRoutes:
      namespaces:
        from: All
    name: http
    hostname: bakery.$INGRESS_IP.nip.io
    port: 80
    protocol: HTTP
EOF

4. Deploy the demo app

kubectl apply -f - <<EOF
apiVersion: apps/v1
kind: Deployment
metadata:
  name: baker
spec:
  selector:
    matchLabels:
      app: baker
  template:
    metadata:
      labels:
        app: baker
    spec:
      containers:
      - name: baker-app
        image: quay.io/kuadrant/authorino-examples:baker-app
        imagePullPolicy: IfNotPresent
        ports:
        - containerPort: 8000
  replicas: 1
---
apiVersion: v1
kind: Service
metadata:
  name: baker
spec:
  selector:
    app: baker
  ports:
    - port: 8000
      protocol: TCP
EOF

5. Attach Route to the ingress GW

kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: baker-route
spec:
  parentRefs:
  - kind: Gateway
    name: kuadrant-ingressgateway
    namespace: gateway-system
  rules:
  - matches:
    - path:
        value: /baker
    backendRefs:
    - kind: Service
      name: baker
      port: 8000
EOF

5.1 . (optional) Check for connectivity

curl http://bakery.$INGRESS_IP.nip.io/baker -i

6. Apply the OIDC Policy

kubectl apply -f -<<EOF
apiVersion: kuadrant.io/v1alpha1
kind: OIDCPolicy
metadata:
  name: baker-auth
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: baker-route
  provider:
    issuerURL: "https://gitlab.com"
    clientID: "e1628246f292c90068d6307a631c17a9fec533b718a162c94c03c5a7db07e0d8"
EOF

7. Open http://bakery.$INGRESS_IP.nip.io/baker in browser and login to gitlab

Verifying Authorization:

Share with me your Gitlab username, and I'll add it to the group. Then you can apply the following OIDCPolicy:

kubectl apply -f -<<EOF
apiVersion: kuadrant.io/v1alpha1
kind: OIDCPolicy
metadata:
  name: baker-auth
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: baker-route
  provider:
    issuerURL: "https://gitlab.com"
    clientID: "e1628246f292c90068d6307a631c17a9fec533b718a162c94c03c5a7db07e0d8"
  auth:
    claims:
      groups_direct: "evil-genius-cupcakes"
EOF

TODO

• Get the gateway protocol (http, https) from Resolve DAG
• Review update of AuthPolicies created by the OIDCPolicy
• Set Cookie needs to check on ingressGatewayInfo for the protocol, and add Secure if it's https
• Define Authorization groups or settings
• Reconcile status
• Expand with RFC configuration
• Check subscriptions not triggering updates - Extensions: fix subscription response trigger #1408

Next:

• Review options for storage/manipulation of token source other than cookie (will aid the native apps)
• Document other OIDC Auth flows (native apps)
• More Docs (?) -- This will be internal for a while, so not sure when would be the right time for publish the docs

[codecov]

Codecov Report

❌ Patch coverage is 63.24786% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.46%. Comparing base (b498648) to head (a24507a).
⚠️ Report is 529 commits behind head on main.

Files with missing lines	Patch %	Lines
api/v1alpha1/oidcpolicy_types.go	67.61%	26 Missing and 8 partials ⚠️
internal/reconcilers/base_reconciler.go	0.00%	6 Missing ⚠️
pkg/extension/controller/controller.go	0.00%	2 Missing ⚠️
internal/controller/state_of_the_world.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1356      +/-   ##
==========================================
- Coverage   80.58%   78.46%   -2.13%     
==========================================
  Files          93       98       +5     
  Lines        8815     9574     +759     
==========================================
+ Hits         7104     7512     +408     
- Misses       1420     1744     +324     
- Partials      291      318      +27     

Flag	Coverage Δ
bare-k8s-integration	21.66% <1.73%> (-0.87%)	⬇️
controllers-integration	62.62% <1.73%> (-3.04%)	⬇️
envoygateway-integration	36.49% <1.73%> (-0.92%)	⬇️
gatewayapi-integration	17.32% <1.73%> (-0.76%)	⬇️
istio-integration	47.24% <1.73%> (-2.79%)	⬇️
unit	21.88% <63.24%> (+0.50%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api (u)	78.17% <65.30%> (-3.34%)	⬇️
internal (u)	79.69% <56.66%> (-2.17%)	⬇️
pkg (u)	49.41% <0.00%> (-0.30%)	⬇️

Files with missing lines	Coverage Δ
internal/extension/reconciler.go	88.18% <100.00%> (+0.33%)	⬆️
internal/controller/state_of_the_world.go	79.62% <0.00%> (+0.27%)	⬆️
pkg/extension/controller/controller.go	25.36% <0.00%> (-0.25%)	⬇️
internal/reconcilers/base_reconciler.go	39.82% <0.00%> (-2.24%)	⬇️
api/v1alpha1/oidcpolicy_types.go	67.61% <67.61%> (ø)

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[didierofrivia]

Commented because currently is not supported by Authorino

[didierofrivia]

Commented because of lacking definition: Kuadrant/architecture#114 (comment)

[adam-cattermole]

So far looking good.

I'm a little lost on the one-of for IssuerURL and JWKSURL, I don't see any uses of the JWKS variant and all the rest of the logic heavily depends on Issuer being present? I guess this is a TODO?

Only other bits I see would be handling updates for the callback policy (and maybe route) in the case the gateway hostname changes. What about deletion?

[adam-cattermole]

This kubebuilder validation seems confusing; if jwksURL is configured as omitempty, won't it always be a non-empty string if it is set, and has(self.jwksURL) and self.jwksURL != '' are equivalent?

Maybe instead we could set both fields to omitempty and just check that one of them is not empty and both are not set has(self.jwksURL) != has(self.issuerURL)

[didierofrivia]

I wanted to match how it was defined in Authorino. And yes, kind of confusing since is not implemented because we haven't replicated the AuthConfig changes in the AuthPolicy. TODO

[adam-cattermole]

Gotcha, if we're not going to support the JWKSURL field in the OIDCPolicy in this PR, perhaps we should comment it out/remove and have the validation ensure the issuer is set to a non-empty string, as that's required for most of the flow?

Then a follow up can add it back in when it's supported, wdyt?

[adam-cattermole]

I think what @Boomatang means is to re-order to have local, external, kuadrant-operator imports:

The import checker only ensures things are separate logical groups but not formatted nicely overall.

Suggested change

	"context"
	"encoding/json"
	"fmt"
	"net/url"
	"reflect"
	"strings"
	"github.com/kuadrant/limitador-operator/pkg/helpers"
	"k8s.io/apimachinery/pkg/api/meta"
	extcontroller "github.com/kuadrant/kuadrant-operator/pkg/extension/controller"
	"k8s.io/apimachinery/pkg/runtime"
	"github.com/kuadrant/policy-machinery/machinery"
	"k8s.io/utils/ptr"
	ctrl "sigs.k8s.io/controller-runtime"
	"sigs.k8s.io/controller-runtime/pkg/client"
	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
	gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
	gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
	"github.com/kuadrant/kuadrant-operator/internal/reconcilers"
	"github.com/go-logr/logr"
	authorinov1beta3 "github.com/kuadrant/authorino/api/v1beta3"
	"k8s.io/apimachinery/pkg/api/errors"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	kuadrantv1 "github.com/kuadrant/kuadrant-operator/api/v1"
	kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1"
	"github.com/kuadrant/kuadrant-operator/pkg/extension/types"
	"github.com/kuadrant/kuadrant-operator/pkg/extension/utils"
	"sigs.k8s.io/controller-runtime/pkg/reconcile"
	)
	"context"
	"encoding/json"
	"fmt"
	"net/url"
	"reflect"
	"strings"
	"github.com/go-logr/logr"
	authorinov1beta3 "github.com/kuadrant/authorino/api/v1beta3"
	"github.com/kuadrant/limitador-operator/pkg/helpers"
	"github.com/kuadrant/policy-machinery/machinery"
	"k8s.io/apimachinery/pkg/api/errors"
	"k8s.io/apimachinery/pkg/api/meta"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/apimachinery/pkg/runtime"
	"k8s.io/utils/ptr"
	ctrl "sigs.k8s.io/controller-runtime"
	"sigs.k8s.io/controller-runtime/pkg/client"
	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
	"sigs.k8s.io/controller-runtime/pkg/reconcile"
	gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
	gatewayapiv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
	kuadrantv1 "github.com/kuadrant/kuadrant-operator/api/v1"
	kuadrantv1alpha1 "github.com/kuadrant/kuadrant-operator/api/v1alpha1"
	"github.com/kuadrant/kuadrant-operator/internal/reconcilers"
	extcontroller "github.com/kuadrant/kuadrant-operator/pkg/extension/controller"
	"github.com/kuadrant/kuadrant-operator/pkg/extension/types"
	"github.com/kuadrant/kuadrant-operator/pkg/extension/utils"
	)

[guicassolato]

Should we first bump the version of the AuthConfig the Kuadrant Operator depends upon?

[didierofrivia]

Yes, that's the idea, so then we can implement it here

[guicassolato]

Ideally this would be a reference to a Kubernetes Secret.

[didierofrivia]

I'll leave it as a TODO for the next iteration if that's OK with you

[guicassolato]

Are we back to reading directly from the cluster in the middle of a reconciliation call? It feels like breaking the state of the world principle.

[alexsnaps]

that's is not the Kuadrant controller, that's just a "regular" reconciler, with some magic from our end. This wouldn't even have access to the SotW.

[guicassolato]

Problem is the pattern. There's no guarantee this Get will return the generation of the resource as it was when reconciliation was triggered. More importantly, a second Get further ahead could introduce yet another state, while decisions have been possibly made already in the same reconciliation call. This is the main motivation of SOTW and a problem the Policy Machinery offers tools to solve. That's why, IMO, new policy controllers should adopt that pattern, not this one.

[alexsnaps]

This is targeted at "any other k8s developer", that uses Kuadrant. "The pattern" here is: you get your resource (your metapolicy) once and use the Kuadrant Context to interact with the SotW (which would go thru the kuadrant controller over gRPC, to use the policy machinery and "a DAG"). The Context then also takes care of the subscription part to what you care about (and restart the reconciliation for you).

It might be confusing, but this isn't one or the other, it's "the good ol' way" for the metapolicy itself (which the SotW knows nothing about), alongside SotW querying/subscription model for the "Gateway API target objects".

[guicassolato]

I need to educate myself in the whole policy extension mechanism, but this smells a lot how we use to do reconciliation in the past and I do not like it. I want to make sure we are not making concurrent calls to a reconcile func that reads again directly from the cluster, that touches so many other resources, based on events centred in the policy object. I bet the next step here is adding mapping functions from target objects to policies ones, and that could be the final breaking of the state of the world principle. Sorry. Not happy with it.

[alexsnaps]

This only reads user defined CRs from the cluster, not targets or anything else, those are read from the SotW, through the "kuadrant context", as CEL expression e.g. self.findGateways()[0].metadata.name (or whatever it was/is), which is resolved by the Kuadrant controller (and for which then follow up updates are sent to this controller).

[alexsnaps]

Can we get these files to be under cmd/extension/ or somewhere specific? Wondering if we could get all files related to a single extension under one single directory?

[didierofrivia]

It's not quite possible without adding specific targets to use the build tools from kustomize/operator-sdk

[alexsnaps]

Probably will be slightly iterated further as we go, but this seems like a reasonable starting place.