Should a request triggering an error really be allowed through?

[alexsnaps]

Looks like this has always been the behavior, or at least it wasn't introduced with CEL support, yet on evaluating the top-level conditions, if an error occurs, we allow the request through...

I don't know whether that's a good thing or not really. In either case tho, I also wonder if it should be reflected to the user whether (sometimes?) the evaluation of the conditions fails... This came up with the behavior of CEL when no key is present in a Map, more specifically in this case ... request.headers. As now one would need to test for the presence of the header before accessing it: 'foo' in request.headers ? request.headers['foo'] == 'bar' : false... which if expressed as request.headers['foo'] == 'bar' and the header is not present, this would err out with no such key: foo.

[alexsnaps]

Also, it is worth noting that the behavior would/might change for a AuthPolicy when the error happens within the wasm-shim

[jsmolar]

Authorino logs for the request that has no key in headers:

{"level":"info","ts":"2024-11-13T13:47:19Z","logger":"authorino.service.auth","msg":"incoming authorization request","request id":"040c1d94-bd6d-4bbc-9827-ec3abe7548e7","object":{"source":{"address":{"Address":{"SocketAddress":{"address":"*","PortSpecifier":{"PortValue":*}}}}},"destination":{"address":{"Address":{"SocketAddress":{"address":"*","PortSpecifier":{"PortValue":8080}}}}},"request":{"http":{"id":"040c1d94-bd6d-4bbc-9827-ec3abe7548e7","method":"GET","path":"/get","host":*","scheme":"http"}}}}
{"level":"debug","ts":"2024-11-13T13:47:19Z","logger":"authorino.service.auth","msg":"incoming authorization request","request id":"040c1d94-bd6d-4bbc-9827-ec3abe7548e7","object":{"source":{"address":{"Address":{"SocketAddress":{"address":"*","PortSpecifier":{"PortValue":*}}}}},"destination":{"address":{"Address":{"SocketAddress":{"address":"*","PortSpecifier":{"PortValue":8080}}}}},"request":{"time":{"seconds":1731505639,"nanos":405610000},"http":{"id":"040c1d94-bd6d-4bbc-9827-ec3abe7548e7","method":"GET","headers":{":authority":"*",":method":"GET",":path":"/get",":scheme":"http","accept":"*/*","forwarded":"for=*;host=*;proto=http","user-agent":"curl/8.10.1","x-envoy-external-address":"*","x-forwarded-for":*","x-forwarded-host":"*
{"level":"debug","ts":"2024-11-13T13:47:19Z","logger":"authorino.service.auth.authpipeline","msg":"skipping","request id":"040c1d94-bd6d-4bbc-9827-ec3abe7548e7","reason":"no such key: key"}
{"level":"info","ts":"2024-11-13T13:47:19Z","logger":"authorino.service.auth","msg":"outgoing authorization response","request id":"040c1d94-bd6d-4bbc-9827-ec3abe7548e7","authorized":true,"response":"OK"}
{"level":"debug","ts":"2024-11-13T13:47:19Z","logger":"authorino.service.auth","msg":"outgoing authorization response","request id":"040c1d94-bd6d-4bbc-9827-ec3abe7548e7","authorized":true,"response":"OK"}

[guicassolato]

Authorino used to be "resilient" to this kind of situation before, by hiding the "error" as a condition mismatch and thus skipping the AuthConfig altogether. Now with CEL, the user has a proper way to distinguish between one thing and the other, according to how it gets flagged in the logs, but effectively the service still falls back to the same behavior.

If in CEL we always have ways for preventing an error in the expression (e.g. 'foo' in request.headers ? request.headers['foo'] == 'bar' : false), then I see value in changing the behavior to returning an error instead of skipping the AuthConfig. The same for rule-level conditions – i.e. the rule would fail instead of skipped.

The problem I guess is that the decision has to be made now, otherwise it would become a breaking change.

[alexsnaps]

I think the issue was always there... e.g. typo in a gjson, right? children|@reverser|0 == "Jack" (from their doc) where it should be @reverse? That would be an actual error right?