kuadrant-operator icon indicating copy to clipboard operation
kuadrant-operator copied to clipboard

Some custom response return codes in AuthPolicy do not work

Open averevki opened this issue 1 year ago • 14 comments

When I set custom response return codes in AuthPolicy, I'm not sure why, but it can't send the response back now. Curl shows Unsupported HTTP/1 subversion in response as there is a wrong HTTP protocol version used. httpcore.RemoteProtocolError: illegal status line: bytearray(b'HTTP/1.1 0 Unknown') this is from the python library.

Seems like the issue remains only when specifying code that doesn't have any purpose by the standard (e.g. code 431 works, but 430 does not)

AuthPolicy:

spec:
  rules:
    authentication:
      default:
        credentials:
          authorizationHeader:
            prefix: Bearer
        jwt:
          issuerUrl: 'http://1.2.3.4:8080/realms/realm-averevki--hbxe'
          ttl: 0
        metrics: false
        priority: 0
    authorization:
      Whitelist:
        metrics: false
        patternMatching:
          patterns:
            - operator: eq
              selector: context.request.http.path
              value: /allow
        priority: 0
    response:
      unauthenticated:
        code: 333
      unauthorized:
        code: 444
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: route-averevki--lhhk
Gateway
spec:
  gatewayClassName: istio
  listeners:
    - allowedRoutes:
        namespaces:
          from: Same
      hostname: '*.apps.kua.redhat.com'
      name: api
      port: 80
      protocol: HTTP
HTTPRoute
spec:
  hostnames:
    - hostname-averevki--zwt-kuadrant.apps.kua.redhat.com
  parentRefs:
    - group: gateway.networking.k8s.io
      kind: Gateway
      name: gw-averevki--st7
  rules:
    - backendRefs:
        - group: ''
          kind: Service
          name: httpbin-averevki--v8n
          namespace: kuadrant
          port: 8080
          weight: 1
      matches:
        - path:
            type: PathPrefix
            value: /

averevki avatar Nov 14 '24 12:11 averevki

What is your expected behavior? Should the user be allowed to send custom status codes that out of spec as a response? Or is the issue we allow a user to define status codes that are out of spec?

Boomatang avatar Nov 14 '24 14:11 Boomatang

What is your expected behavior? Should the user be allowed to send custom status codes that out of spec as a response? Or is the issue we allow a user to define status codes that are out of spec?

I expect it to return 333 and 444 instead of the error. I'm not sure what range should be there. I'd limit the values you can put there so it won't provoke unexpected behavior if you leave it as it is implemented now. But I like the option to define any code for the response better (from 100 to 600)

I know it worked before, and now it is an unexpected change. So I guess it became a bug

averevki avatar Nov 14 '24 15:11 averevki

I was re-reading the spec and 2 things come to mind:

  1. Envoy specify a closed set of supported HTTP codes and behaviour is not clear for codes described as "Unasigned". Authorino narrows the list even further by accepting only code within the 300-599 range.
  2. There's now another layer in between Authorino and the setting of the response code, the wasm-shim. Somebody should look at that end too.

guicassolato avatar Nov 14 '24 17:11 guicassolato

  1. There's now another layer in between Authorino and the setting of the response code, the wasm-shim. Somebody should look at that end too.

I'll take a look at the wasm-shim side

adam-cattermole avatar Nov 15 '24 08:11 adam-cattermole

From what I see, Kuadrant is working as expected. You told it to return 333 and it is returning, indeed, 333. It is your clients, either curl or python clients that complain not understanding that non standard return code.

So the right question here is: Should kuadrant allow configuration that specifies non standard return codes?

eguzki avatar Nov 15 '24 08:11 eguzki

From the wasm-shim side - when a request is denied we try to create the response using the status/headers/body from authorino. If authorino is configured to use status codes that are not from the valid list from envoy we end up getting back a StatusCode::Empty = 0 which we try to send over causing the error that appears in the response.

I'd say we either restrict the set of status codes to those valid ones.. or alternatively we could "silently" override the code to a different one if we receive an empty status code, but I prefer the first option.

adam-cattermole avatar Nov 15 '24 16:11 adam-cattermole

@adam-cattermole I want to confirm what your saying in your comment. This issue is related to changes that was done in the wasm-shim?, meaning it is not an issue directly with the kuadrant operator?

Boomatang avatar Jan 09 '25 14:01 Boomatang

@adam-cattermole I want to confirm what your saying in your comment. This issue is related to changes that was done in the wasm-shim?, meaning it is not an issue directly with the kuadrant operator?

The issue is not so much a change that needs to be made in the wasm-shim, more on making a decision as to what valid status codes are. Envoy will not return us a status code in the wasm filter outside of those in the supported list, even if this is supported by Authorino. This means if a user configures to respond with one that is not on that list and authorino returns this in a response, we will not know what it is (as we get back StatusCode::Empty = 0 instead).

adam-cattermole avatar Jan 09 '25 15:01 adam-cattermole

Ok, Thanks @adam-cattermole, so if the user was to use a different Gateway and not Envoy, the wasm filter would return the supported status code for the different Gateway. If this is the case I think a document change to say the supported status codes is dependent on the Gateway provider that is used. Would you agree?

Boomatang avatar Jan 09 '25 16:01 Boomatang

if the user was to use a different Gateway and not Envoy, the wasm filter would return the supported status code for the different Gateway

Sorry for confusion, when I mention envoy here I mean the proxy (that our wasm filter interacts with) - this is unrelated to the gateway provider itself, and the issue is present on all of our current supported installation methods

adam-cattermole avatar Jan 09 '25 17:01 adam-cattermole

What is the proxy in this case? What I am trying to work out is can we fix this issue within our repos? or is it an external change?

Boomatang avatar Jan 13 '25 15:01 Boomatang

Yep it is handled by us, referring to my comment above we have to decide whether:

  • We document that we only support the same set of response codes that we can retrieve from the proxy
  • or we actually restrict these via the CRD to prevent non-valid codes (and I guess document as well)
  • (or we silently ignore invalid response codes and provide the default valid one)

adam-cattermole avatar Jan 13 '25 15:01 adam-cattermole

This took me to long to get my head around. It is the envoy proxy crate that is used with in the wasm shim that is causing the limitation. I would be in favor of limiting the values a user can have within the CRDs, linking to the envoy proxy crate maybe enough for documentation.

I would like to generate the list of values from the crate. But because the kube builder and how it generates the CRDs I think this could be problematic.

Boomatang avatar Jan 13 '25 15:01 Boomatang

or we actually restrict these via the CRD to prevent non-valid codes (and I guess document as well)

This + documentation 👍

azgabur avatar Mar 11 '25 19:03 azgabur