remove this

Done.

we may require token exchange service to keep origin header of jwt, so that filter could restore original jwt payload (for back compatibility in case app process original jwt payload); this could be done later. /cc @diemtvu

Is the goal making AP token "transparent" to the service admin? I don't think this is right way of handling ap token. The service/namespace admin should be well aware that he/she is handling AP token, which is different from an external token.

Agree with Limin. This should be configured explicitly. There could be use cases that admin want to keep the aptoken credential, or it can pack original payload differently (which we may need more than just a boolean flag).

I would suggest adding a flag to JWT configuration, and calling this function base on that value.

Regarding sending the original payload to backend, it's up to our decision on the feature. If we want user can use IAP without any change on there application, then it would be a nice option to have, though I think it would be more complicated. We can ignore that for now.

Based on the discussion today, the code has been revised.

when have time, could you pls add a short readme on how this fake jwt is generated ? thanks !

When I have time, I can create a separate PR for the readme of how this JWT is generated.

Change to "https://example.token_service.com".

Done.

How do you decide when to extract original claims instead of normal claims? From the code, it seems that as long as "original_payload" claim exists, you think it is an AP token and extract from original payload? I don't think this is the right way to handle it. Admin should either specifically mention that this is AP token, or "iss" matches configured AP token issuer.

If a token is an APToken, the original claims are extracted. Otherwise, normal claims are extracted. What token is considered an APToken is based on Quanjie's suggestion: if a token has an original_payload claim, and there is an issuer claim in the original_payload claim, and the issuer in the original_payload claim is different from the issuer of the token, the token is considered an APToken.
The rationale of such code is that a token exchange makes the issuers of the APToken and the original JWT to be different (if the two issuers are the same, there is no need for the token exchange).The code to check that the issuers are different are on the lines 125~129.

Will the extracted original_payload be verified? If not, it seems there will be a security hole that the current request could impersonate any issuer and claims by using this original_payload field even if APToken is not enabled.

It might be better to only extract the original_payload if the outter jwt is issued by google.com. This prevents user/attacker to abuse the original_payload field without the need to do another verification.

/cc @quanjielin @diemtvu

The extracted original_payload will not be verified by a recipient of the APToken: the APToken issuer verified the original JWT and the recipient of the APToken verified the APToken. If an APToken is valid, the recipient regards the payload is valid.

@yangminzhu already gave an example why just checking "original_payload" (and issuers are different) is not enough to prove the token is indeed an AP token. My preferred way is to introduce APTokenConfig where users can specify their specific AP token properties (e.g., iss). If you want to work something out faster, an alternative is to standardize AP token header (e.g., x-ingress-authorization (TBD)). We can say that the token in the standardized header must be AP token.

While agree with not doing this implicitly, I don't totally agree with Yangmin example. The original_payload (and all other claims in the outer jwt) are still verified by the outer jwt issuer, right

Based on the discussion today, the code has been revised.

@diemtvu
Sorry for missing the background, I was trying to say what should we do when the outter issuer is compromised. In this case, the request could explicitly include the original_claims field, like the following one:

{
  "aud": "abc",
  "exp": 4698361508,
  "iat": 1544761508,
  "iss": "xyz.com",
  "original_claims": {
    "iss": "google.com",
    "sub": "userABC"
  }
}

The issuer xyz.com is compromised so the above JWT will pass the verification. However it contains the original_claims with google.com as the issuer and userABC as subject. This is not a problem for now because even if the xyz.com gets compromised, it still cannot impersonate the google.com.

@liminw
Could the attacker set the x-ingress-authorization header explicitly in their request to trick us to believe it's a JWT (APToken) returned by the token exchange service, right?

@yangminzhu "xyz.com" is listed by the operator as a trusted issuer, and we also do signature verification to make sure the token is signed by "xyz.com". So whatever contained in the token, we can assume that it is trusted.

"ingress-authorization" header is the default AP token location. The token put in there we can assume it is AP token. We will validate the token's signature to ensure that it is trusted. So I don't have concern on its security aspect.

This logic sounds like a hack. It basically says "if original_payload exists, and original issuer is not the same as new issuer, it is AP token". This is not how AP token is defined.

Based on the discussion today, the code has been revised.

Do we still need to compare issuer?

I recommend comparing issuers because if an token exchange occurs, the issuer of the exchanged token should be different from the issuer of the original token. @quanjielin @liminw, what is your thought on comparing issuers?

I don't think it is a hard requirement that the issuers are different. We can probably drop the check here.

Done: the compare/check issuers have been removed from the code.

Is the goal making AP token "transparent" to the service admin? I don't think this is right way of handling ap token. The service/namespace admin should be well aware that he/she is handling AP token, which is different from an external token.

Agree with Limin. This should be configured explicitly. There could be use cases that admin want to keep the aptoken credential, or it can pack original payload differently (which we may need more than just a boolean flag).

I would suggest adding a flag to JWT configuration, and calling this function base on that value.

Regarding sending the original payload to backend, it's up to our decision on the feature. If we want user can use IAP without any change on there application, then it would be a nice option to have, though I think it would be more complicated. We can ignore that for now.

Do we have this agreement on APToken format?

Based on the discussion today, original_payload has been changed to original_claims.

What are these key/cert file for. Please add a README if they are used to generate test data, and/or how to replace this file if need to.

These key/cert files have been removed. I have added a guide of sending an example exchanged token to the jwt-authn filter and the Istio authn filter, and observing that the example backend echoes back the request when the authentication succeeds.

Does this file used in unit test? I don't see any code using this.

This file is used in a guide I added. I have added a guide of sending an example exchanged token to the jwt-authn filter and the Istio authn filter, and observing that the example backend echoes back the request when the authentication succeeds.

It's better to write a test. You can add them to test/integration. There is already few test setup examples.

A test has been added.

While agree with not doing this implicitly, I don't totally agree with Yangmin example. The original_payload (and all other claims in the outer jwt) are still verified by the outer jwt issuer, right

It's better to write a test. You can add them to test/integration. There is already few test setup examples.

For readability, make a helper function (say IsAPTokenIssuer) for this.

Done.

If it is APToken (found), but cannot extract original payload, then probably should return false.

Limin mentioned that an APToken may not necessarily contain the original payload (i.e., original_claims). If an APToken must contain the original payload, we can use it as a way to tell whether a token is an APToken. Hence, no false is returned here.

Is that so? Again, I feel wrong to while making assumption of the location, but not on the token format and it's behavior. Anyway, please add comment to explain the rationale.

Done: added comments to explain the rationale.

I think for the first version, it is ok to assume that AP token always carries original_claims. This may not be true going forward, but let's focus on this JWT token exchange use case for now.

Code has been revised to require an APToken to always carry original_claims: if it is APToken, but cannot extract original payload from original_claims, the code will return false.

You should have new unit tests for this change.

Test added.

use "ingress-authorization".

Done.

Instead of doing string copy, you can directly put "payload_to_process" in the function.

AuthnUtils::ExtractOriginalPayload(jwt_payload, &payload_to_process)

If put "payload_to_process" in ExtractOriginalPayload(), ExtractOriginalPayload() may change "payload_to_process" even when ExtractOriginalPayload() returns false. Therefore, "payload_to_process" should not be put in ExtractOriginalPayload().

@yangminzhu "xyz.com" is listed by the operator as a trusted issuer, and we also do signature verification to make sure the token is signed by "xyz.com". So whatever contained in the token, we can assume that it is trusted.

"ingress-authorization" header is the default AP token location. The token put in there we can assume it is AP token. We will validate the token's signature to ensure that it is trusted. So I don't have concern on its security aspect.

If we make assumption on the location value, should we also make assumption/requirement that the Jwt spec should declare only one header location. It seems weird (and could be wrong) to define more than one for aptoken.

• @liminw

yes, sounds reasonable. We can require that there is only one header specified.

Please change this per agreement. You can add check for jwt_headers() size and compare the only one in there.

Is that so? Again, I feel wrong to while making assumption of the location, but not on the token format and it's behavior. Anyway, please add comment to explain the rationale.

Please add a test for the case when original payload was not there. Any may be another test with original payload, but jwt was not in the intended location.

Done: added two more tests.

Do we still need to compare issuer?

nit Parse -> Parses, Return -> Returns

Done.

Please add integration test to cover this setup, rather than giving sample.

aptoken-envoy.conf and the user guide added in this PR are for a developer that is interested in going through the APToken authentication flow on Istio proxy. The code change in this PR is on validateJwt(). Three new tests on validateJwt() have been added to test the code change. An integration test will duplicate what these three new tests have tested with respect to the code change on validateJwt().

The tests you added already simulate the payload input to authN filter. It doesn't cover the full interaction between jwt filter and authn filter (and even mixer) as you can have with 'integration' test (I admit, the name is a bit misleading; it's still unit test, but testing few filters chained together instead of one).

Having sample config add a little benefit to the maintenance in the long run (config might change, overhead of setting up runtime etc). Given the way to set up the test envoy exactly likes the one specified by this config is fairly simple, I would suggest convert this into a test.

The Istio authn integration tests that cover jwt filter, authn filter and mixer are in a different repository istio/istio under the path https://github.com/istio/istio/mixer/test/client. To add an integration test for authenticating an exchange token, this PR should be merged first, then the SHA in the istio.deps of the istio/istio proxy should be updated to the new SHA of istio-proxy, and finally an integration test for an exchanged token can be added under https://github.com/istio/istio/mixer/test/client. I have created an issue istio/istio#10514 to add an integration test in the istio/istio repository and assigned it to myself.