fix: RawExtension to string conversion

[guicassolato]

Fixes conversion of ValueOrSelector.Value (based on runtime.RawExtension) to string, used at the following configs:

• SubjectAccessReview authorization
• External HTTP requests (metadata, external Rego policies, etc)
• SpiceDB check permissions

This bug was introduced in v0.18.0, when the AuthConfig controller was updated to work with v1beta2 type (previously (v1beta1), thus activating the option to set static values to JSON/YAML types other than strings. The conversion functions used in the features listed above were overlooked in the process and remain naively treating static values (stored in Golang interface{} variables) as if they were always strings.

Verification steps

make local-setup FF=1

kubectl port-forward deployment/envoy 8000:8000 2>&1 >/dev/null &

kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api.127.0.0.1.nip.io
  authorization:
    "sar":
      kubernetesSubjectAccessReview:
        user:
          value: john
        resourceAttributes:
          resource:
            value: secrets
          name:
            selector: request.path.@extract:{"sep":"/","pos":1}
          verb:
            expression: request.method
  response:
    unauthorized:
      message:
        value: Access denied by the Kubernetes RBAC
EOF

curl http://talker-api.127.0.0.1.nip.io:8000/my-secret -i

Check the Authorino logs. You should spot en entry like the following:

{
  "level": "debug",
  "ts": "2024-11-04T11:06:44Z",
  "logger": "authorino.service.auth.authpipeline.authorization.kubernetesauthz",
  "msg": "calling kubernetes subject access review api",
  "request id": "b6471552-9154-41fb-87e4-843f5db9255a",
  "subjectaccessreview": {
    "metadata": {
      "creationTimestamp": null
    },
    "spec": {
      "resourceAttributes": {
        "verb": "GET",
        "resource": "secrets",
        "name": "my-secret"
      },
      "user": "john"
    },
    "status": {
      "allowed": false
    }
  }
}

Before the fix, attributes such as subjectaccessreview.spec.user were being stringified as "{\"john\" <nil>}", which is the direct print out of the RawExtension type in Golang as string.

[alexsnaps]

Tho... unsure about the last one, as per my comment...

[alexsnaps]

Is that always true tho? Couldn't this be "just a plain string" ever?

[guicassolato]

Unfortunately the types we used internally won't contribute much to make this clear, but, in short, yes, that is always true.

First thing to keep in mind is that the type that matters the most here is not of the source, but the one of where the value is used. The value of a header must always be a string, regardless of the source.

A second point – and the one less clear from the types used internally – regards to what json.StringifyJSON does with the multiple possible outputs from ResolveFor for the different types ultimately supported as input, i.e.:

• value → runtime.RawExtension (the one whose conversion this PR fixes)
• selector → a JSON-compatible Golang type as implemented by gjson (not affected by adding now another step of JSON marshalling and gjson parsing)
• expression → a CEL-resolved value (recently reduced back into the previous case anyway)

You can try this out if you want using this other example:

apiVersion: authorino.kuadrant.io/v1beta3
kind: AuthConfig
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api.127.0.0.1.nip.io
  metadata:
    "http-request":
      http:
        url: http://ip-api.com/json
        headers:
          "Accept":
            value: application/json
          "x-object-static":
            value:
              foo: bar
          "x-object-selector":
            selector: request.headers
          "x-object-expression":
            expression: request.headers
  response:
    success:
      headers:
        "geo-info":
          plain:
            expression: auth.metadata["http-request"]

You should see in the logs something similar to:

{
  "level": "debug",
  "ts": "2024-11-04T12:09:13Z",
  "logger": "authorino.service.auth.authpipeline.metadata.http",
  "msg": "sending request",
  "request id": "fce0cae9-a3af-4491-beab-076178c78261",
  "config": "http-request",
  "method": "GET",
  "url": "http://ip-api.com/json",
  "headers": {
    "Accept": [
      "application/json"
    ],
    "Content-Type": [
      "text/plain"
    ],
    "X-Object-Expression": [
      "{\":authority\":\"talker-api.127.0.0.1.nip.io:8000\",\":method\":\"GET\",\":path\":\"/my-secret\",\":scheme\":\"http\",\"accept\":\"*/*\",\"user-agent\":\"curl/8.7.1\",\"x-envoy-internal\":\"true\",\"x-forwarded-for\":\"10.244.0.10\",\"x-forwarded-proto\":\"http\",\"x-request-id\":\"fce0cae9-a3af-4491-beab-076178c78261\"}"
    ],
    "X-Object-Selector": [
      "{\":authority\":\"talker-api.127.0.0.1.nip.io:8000\",\":method\":\"GET\",\":path\":\"/my-secret\",\":scheme\":\"http\",\"accept\":\"*/*\",\"user-agent\":\"curl/8.7.1\",\"x-envoy-internal\":\"true\",\"x-forwarded-for\":\"10.244.0.10\",\"x-forwarded-proto\":\"http\",\"x-request-id\":\"fce0cae9-a3af-4491-beab-076178c78261\"}"
    ],
    "X-Object-Static": [
      "{\"foo\":\"bar\"}"
    ]
  }
}

[guicassolato]

One thing we can do to mitigate the impact of this PR over the other affected sources for the conversion is to make json.StringifyJSON return earlier if the interface{} input is a string.

[guicassolato]

Just pushed a commit: 113717d

[alexsnaps]

Ok, so it was not always the case? And now a "plain string" remains a plain string... do I get this right? In anycase... I think looking into something like what's proposed in #500 wrt to Value.ResolveFor's signature would be a good refactor for ... future us. Today's interface{} (and I know that's sourced in golang's type system) is just too broad I think. Further more the asymmetry of the function wrt its input & output is definitively a code smell to me.

[alexsnaps]

It was the case, I played with these functions a bit... I get what's going on now

[KevFan]

Looks good to me!

[alexsnaps]

🚀