Authentication example is presently harmful

[ejona86]

The example is currently equivalent to the "header" example. So there's no possible value-add it could contribute at the moment.

But unfortunately it is recommending making a ClientInterceptor for sending JWT to the server. That's not what we recommend. We would recommend using MoreCallCredentials.from(Credential) along with ServiceAccountJwtAccessCredentials (which is Google-centric, but users can potentially use the fromPkcs8() construction method) or to implement CallCredentials directly. CallCredentials has the advantage that the credential 1) can verify its security requirements, like how all JWTs should be sent on an encrypted connection, and 2) will be fresher, as it is called just before the RPC is sent (a waitForReady RPC could be delayed for days).

The example also needs to incorporate how to propagate the authenticated user to the application via Context. Ideally it would at least note how to fail the RPC if the user is unauthenticated. Examples of how to do this is available on SO.

I'm considering deleting the example for the upcoming v1.21.0 release.

[ejona86]

It also is using the Metadata Key name "jwt" for the JWT. However, it seems JWT typically uses the Authentication header with the Bearer prefix.

[sanjaypujare]

@ejona86 io.grpc.ClientInterceptor has this comment:

Implementers use this mechanism to add cross-cutting behavior to Channel and stub implementations. Common examples of such behavior include:
...
Adding credentials to header metadata

Whether you call it a recommendation or not, it documents a use case for client-interceptors especially the cross-cutting aspect, so the example was used to illustrate that aspect more than how to properly and securely implement authentication. Granted the example is simplistic and functionally somewhat similar to the "header" example (although that example shows the server sending a header in the response to the client).

And the other example io.grpc.examples.googleAuth.GoogleAuthClient shows using GoogleCredentials for Google APIs so I am not sure if we need another example using MoreCallCredentials.from(Credential) along with ServiceAccountJwtAccessCredentials.

Let me know what you think. Also curious about the recommendation bit you mentioned - where it is and when it was changed.

[ejona86]

io.grpc.ClientInterceptor has this comment:

Ah, I see. That line should be removed. That comment is very old, from 2015: 46dd47f

CallCredentials were introduced pre-1.0 in 2016 in 432cec7.

Granted the example is simplistic and functionally somewhat similar to the "header" example (although that example shows the server sending a header in the response to the client).

The header example shows both directions: the client sends a header and the server prints it out, and then the server sends a header and the client prints it out. I will grant you that the authentication example is a subset of the header example.

And the other example io.grpc.examples.googleAuth.GoogleAuthClient shows using GoogleCredentials for Google APIs so I am not sure if we need another example using MoreCallCredentials.from(Credential) along with ServiceAccountJwtAccessCredentials.

I'm okay with that. But I don't want any example using an interceptor for sending authentication credentials.

Note that the other example does not use ServiceAccountJwtAccessCredentials, so people may not be aware it exists (it is used behind-the-scenes).

If you are going to use JWT thought, I'd sort of expect to actually use JWT since we already have APIs for that. If you don't want to use ServiceAccountJwtAccessCredentials because "it is already documented" then I'd suggest not mention JWT at all in the example. Name it "custom auth" or something else. If you don't want to use ServiceAccountJwtAccessCredentials because you find that it can't be used in this circumstance, that is fine, but I'd hope to know what part of it is insufficient.

[sanjaypujare]

I'm okay with that. But I don't want any example using an interceptor for sending authentication credentials.

Sounds good.

Note that the other example does not use ServiceAccountJwtAccessCredentials, so people may not be aware it exists (it is used behind-the-scenes).

If you are going to use JWT thought, I'd sort of expect to actually use JWT since we already have APIs for that. If you don't want to use ServiceAccountJwtAccessCredentials because "it is already documented" then I'd suggest not mention JWT at all in the example.

I understand the problem with the example using just a string as the token value. However instead of using ServiceAccountJwtAccessCredentials (which is mainly for Google cloud APIs) can we use something like JsonWebToken which can be demonstrated on both client and server ends?

Name it "custom auth" or something else. If you don't want to use ServiceAccountJwtAccessCredentials because you find that it can't be used in this circumstance, that is fine, but I'd hope to know what part of it is insufficient.

As mentioned above, it will be good to show the server side as well where the token is received and processed by a the example code.

So the enhancements to the current example in my opinion could be:

• use JsonWebToken instead of a dummy string

• use Authorization header with Bearer prefix (if applicable)

• replace interceptor with a non-interceptor implementation (on the server side as well? what's the recommendation there?)

• also include error case of authentication failing with grpc error code of Status.UNAUTHENTICATED.

Does that sound okay for this example to be of value?

[ejona86]

JsonWebToken itself doesn't look useful, but JsonWebSignature looks more useful.

Bare minimum:

1. replace ClientInterceptor with a ClientCredential
2. include error case of authentication failing with grpc error code of Status.UNAUTHENTICATED
3. propagate authenticated user to application (via Context)
4. either use JWT or name it something else

Nice to haves:

1. Actual JWT, with Bearer prefix
2. Actual verification on server-side, potentially with JsonWebSignature

I'm worried about creating anything on client-side, since we really shouldn't recommend signing a JWT per-RPC. That implies a cache and thread-safety, yada yada. But that would be fine.

I expect doing "real JWT" to take a while, so I would prefer this to be done in stages. But given this is Friday and the branch cut is on Monday/Tuesday, I expect I will delete the current example for 1.21.0.

[sanjaypujare]

...

Sounds good.

I'm worried about creating anything on client-side, since we really shouldn't recommend signing a JWT per-RPC. That implies a cache and thread-safety, yada yada. But that would be fine.

In our TLS example, we have instructions to create keys/certs (in pem files) before running the example. In this case too we can have instructions to manually create a signed JWT which can then be used by the example for the channel.

[ejona86]

In our TLS example, we have instructions to create keys/certs (in pem files) before running the example. In this case too we can have instructions to manually create a signed JWT which can then be used by the example for the channel.

The TLS example creates test certs for their environment. But they need certs for any environment.

For JWT, it is not normal to manually create a JWT. Instead, you provide the private key to the client and it will sign things on-demand.

The pre-existing JWT client-side code we have seems it would be functional and "good enough" for an example. Yes, the user may want to do "something more" with JWT, but they can still do that. The example would have all the pieces together and they can tweak each in turn for their own environment.

[anarsultanov]

@ejona86 @sanjaypujare is anyone working on this? I am writing an article on this topic and could create a pool request with an example.

[sanjaypujare]

@anarsultanov I was going to but didn't get around to doing it. If you have your example ready, I don't see an issue with a PR.

[anarsultanov]

@sanjaypujare well, the example is ready, I just need to add documentation. I will do this as soon as possible and will create a PR.