JWT metadata broken in Envoyfilter since 1.21.0 about istio HOT 19 OPEN

This is a pretty huge change for us to consume (in terms of # of affected AuthorizationPolicy files).

Just to confirm, is #49913 is contributing to the above? My interpretation of @kyessenov comment above is that requestPrincipal is parsing out issuer and subject and then comparing both to the values provided in the JWT. Once that issue is fixed (which was possibly in #50061), will there still be changes that need to be made to policies that are using a value other than * for the requestPrincipal field? Having to set a * in all cases seems like that field is broken.

For context, we're testing requestPrincipal in our policies today and our subject doesn't contain slashes -- actually we're using "*" for subject in the requestPrincipals (e.g. https://my.issuer/*), but don't specifically test [iss] or [sub] today. In 1.21.2 tests using a * for requestPrincipal and then specifically testing [iss] (set to https://myissuer in the when section) seems to result in success, but would like to avoid rolling that change if at all possible.

from istio.

This appears to be an issue in the proxy, rather than the control plane.
By using the sidecar.istio.io/proxyImage to force a 1.20.4 sidecar, it works again.

from istio.

I think the key under meta is now simply payload rather than the issuer, as it was in 1.20.4?
I can't see anything related to this in the release notes?

from istio.

https://istio.io/latest/news/releases/1.21.x/announcing-1.21/change-notes/#security talks about some changes to the dynamic metadata key

Improved request JWT authentication to use the upstream Envoy JWT filter instead of the custom Istio Proxy filter. Because the new upstream JWT filter capabilities are needed, the feature is gated for the proxies that support them. Note that a custom Envoy or Wasm filter that used istio_authn dynamic metadata key needs to be updated to use envoy.filters.http.jwt_authn dynamic metadata key..

So there's definitely been some jwt changes. However this doesn't explain the issue i'm having (we were using envoy.filters.http.jwt_authn before, and currently). The issue specifically seems to be that previously the claims were nested under envoy.filters.http.jwt_authn["issuer"] whereas now they're under envoy.filters.http.jwt_authn[payload]

from istio.

I can work around this by doing:

local meta = request_handle:streamInfo():dynamicMetadata():get("envoy.filters.http.jwt_authn")
local claimsOld = meta["https://app.platform-identity-provider.testing.k8.atcloud.io"]
local claimsNew = meta["payload"]
local claims = claimsNew or claimsOld

But would be good to know if this change is intentional? If so; it probably should be on the release notes somewhere.

from istio.

@kyessenov was looking into something similar I think

from istio.

#50061 (there's a backport to 1.21) fixes it and #47407 broke it

from istio.

@Stono I think it was not under envoy.filters.http.jwt_authn before but an Istio filter. That Istio filter duplicated the data and re-organized, but all the data is still there. The issuer is:

              path:
              - key: payload
              - key: iss

The reason for payload is that the decoded JWT header, not the payload, needs another top-level field.

from istio.

@kyessenov I'm not sure I understand you, we have a perfectly working envoyfilter on 1.20; which has the claims under request_handle:streamInfo():dynamicMetadata():get("envoy.filters.http.jwt_authn")["https://app.platform-identity-provider.testing.k8.atcloud.io"]; which no longer works, but request_handle:streamInfo():dynamicMetadata():get("envoy.filters.http.jwt_authn")["payload"] does.

from istio.

Ah, yes, you are right, the payload was placed under the hard-coded issuer key previously. Now the key is always "payload" since it simplifies the configuration.

from istio.

@keithmattix comment regarding the breakage only impacts you if you have an Authorization policy using JWT claims. If you do your own logic with Lua, the bug doesn't affect you. We can update the notes to mention the internal change to xDS, but we usually reserve the right to do small changes like this.

from istio.

Now the key is always "payload" since it simplifies the configuration.

Agree, it's simpler.

but we usually reserve the right to do small changes like this

That's why I suggested it might simply be a release note thing. I'm willing to bet I'm not the only user reading jwt claims in a lua envoyfilter?

from istio.

Oh I missed that this was Lua; my bad

from istio.

Does this also mean the requestPrincipal is no longer in the same format ($issuer/$sub) as it used to? We've run into issues (no match/RBAC denied), in a place where we used a JWT from GitHub actions and matched against the branch name (subject), like this:

apiVersion: security.istio.io/v1beta1
kind: AuthorizationPolicy
metadata:
  name: allow-ingress-infra-w-token
spec:
  action: ALLOW
  rules:
  - from:
    - source:
        namespaces:
        - istio-system
        principals:
        - cluster.local/ns/istio-system/sa/foo-ingressgateway-service-account
        requestPrincipals:
        # i RequestAuthentication URL (<ISS>/<SUB>)
        - 'https://token.actions.githubusercontent.com/repo:ORGNAME/REPONAME:ref:refs/heads/main'

Downgrading the proxy worked.

cc @larhauga

from istio.

@chlunde The format is the same. However, it's interpreted as two equalities "iss=A" and "sub=B" for a value "A/B". When you have slashes in the subject like in your cause, the slash is chosen at the wrong position, e.g. "sub=main" above because it becomes ambiguous where the issuer ends. JWT spec seems to allow arbitrary strings in the issuer:

StringOrURI
      A JSON string value, with the additional requirement that while
      arbitrary string values MAY be used, any value containing a ":"
      character MUST be a URI [[RFC3986](https://datatracker.ietf.org/doc/html/rfc3986)].  StringOrURI values are
      compared as case-sensitive strings with no transformations or
      canonicalizations applied.

I think for your case, it's probably better to explicitly specify "iss" and "sub" claim equality with conditions.

from istio.

Not sure if I fully understand, @kyessenov is this a regression? Should it be fixed? If not, would it be best to deprecate requestPrincipals and request.auth.principal or at least update the documentation, discouraging using anything but * and using request.auth.claims[iss] + request.auth.claims[sub]?

from istio.

Not sure if I fully understand, @kyessenov is this a regression? Should it be fixed? If not, would it be best to deprecate requestPrincipals and request.auth.principal or at least update the documentation, discouraging using anything but * and using request.auth.claims[iss] + request.auth.claims[sub]?

Yes, we should update the documentation, this is a regression or at the very least under-specified API.

from istio.

If someone stumbles upon this issue in the meantime, here's what we changed to:

    - from:
        - source:
            namespaces:
              - istio-system
            principals:
              - cluster.local/ns/istio-system/sa/FOO-ingressgateway-service-account
            requestPrincipals: ['*']
      when:
        - key: request.auth.claims[iss]
          values: [https://token.actions.githubusercontent.com]
        - key: request.auth.claims[sub]
          values:
            - repo:ORG/REPO:ref:refs/heads/main

from istio.

The issue @chlunde run into is that the subject contains a slash. If you do not have a slash, I think the matching should work as expected when it includes * as a suffix. A pattern A/* for the request principal gets translated to issuer = A and any subject.

from istio.