Explore CallCredentials and SessionCredentials for GRPC

[tegefaulkes]

Specification

Check the robustness of the usage of call credentials and SessionCredentials.

Additional context

Reference https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/207#note_652476363 and https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/207#note_652440417

An MR has been created for this issue.
https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/210

Tasks

1. ...
2. ...
3. ...

[CMCDragonkai]

@DrFacepalm can you take over this issue this week/weekend?

[DrFacepalm]

Taking a look, im not too sure what we'd prefer here.

We have two options here, either something like:

// Option 1
const pCall = grpcClient.vaultsCreate(
    vaultMessage,                                                      // Message Type
    await client.session.createCallCredentials(),       // grpc.CallOptions
);

or

// Option 2
const pCall = grpcClient.vaultsCreate(
    vaultMessage,                                                      // Message Type
    meta,                                                                    // grpc.Metadata
    await client.session.createCallCredentials(),       // grpc.CallOptions
);

Option 2 seems more robust simply because it seems more supported by the grpc library. In the generated code:

public vaultsCreate(request: Client_pb.VaultMessage, callback: (error: grpc.ServiceError | null, response: Client_pb.VaultMessage) => void): grpc.ClientUnaryCall;
public vaultsCreate(request: Client_pb.VaultMessage, metadata: grpc.Metadata, callback: (error: grpc.ServiceError | null, response: Client_pb.VaultMessage) => void): grpc.ClientUnaryCall;
public vaultsCreate(request: Client_pb.VaultMessage, metadata: grpc.Metadata, options: Partial<grpc.CallOptions>, callback: (error: grpc.ServiceError | null, response: Client_pb.VaultMessage) => void): grpc.ClientUnaryCall;

The last overload shows that the function call takes: MessageType, grpc.Metadata, grpc.CallOptions, which matches Options 2. There does not seem to be a function call for vaultsCreate that matches Option 1.

@CMCDragonkai mentioned before that maybe, the only reason Option 1 had worked as because we had removed typings from the call by using ...args in our GRPCClientClient

The strange thing is that the exist function overloads that match Option 1 for the stream based grpc calls.

public vaultsList(request: Client_pb.EmptyMessage, options?: Partial<grpc.CallOptions>): grpc.ClientReadableStream<Client_pb.VaultListMessage>;
public vaultsList(request: Client_pb.EmptyMessage, metadata?: grpc.Metadata, options?: Partial<grpc.CallOptions>): grpc.ClientReadableStream<Client_pb.VaultListMessage>;

The first one takes a MessageType and a grpc.CallOptions, but it also has the MessageType, grpc.Metadata and grpc.CallOptions call.

With that, honestly it seems like the best approach is to use the calls with grpc.Metadata in them. @tegefaulkes @CMCDragonkai thoughts?

[CMCDragonkai]

Try doing option 2 for unary calls. And for streams leave it as is. I reckon we should matching the call signature even if it is weird.

[tegefaulkes]

Looking into the callCredentials that you can create when setting up a client connection. I think it's possible for the client to provide metadata for each call automatically.

For example, if we provide the JWT token metadata to the client as a callCredential it should provide that each time we make a call. so we don't need to provide it each time we make the call. the documentation on this is very light so I need to do some testing to confirm this,

I'll look deeper into this once I've completed #229

[CMCDragonkai]

Yep the grpc documentation is quite light. I had to delve into the source code to see how to make use of it. In VSCode this was easy enough by jumping between the type definitions. But I had to clone the grpc source code and match up the type signatures to the TS code internally as well.

…

On 8/30/21 1:08 PM, Brian Botha wrote: Looking into the |callCredentials| that you can create when setting up a client connection. I think it's possible for the client to provide metadata for each call automatically. For example, if we provide the JWT token metadata to the client as a |callCredential| it should provide that each time we make a call. so we don't need to provide it each time we make the call. the documentation on this is very light so I need to do some testing to confirm this, I'll look deeper into this once I've completed #229 <#229> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#230 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHJXTMQ434Z4BQVU3KLT7LY3DANCNFSM5CLFEFMQ>.

[tegefaulkes]

I've done some testing now. Looks like you can provide a metadataGenerator to the callCredentials. This will generate new metadata each time a call is done add that to the metadata of the call. Using this we can get the JWT token from the session manager and provide that for each call automatically.

This way we don't have to provide the session token metadata explicitly for each call. if the token gets updated in the session manager this should be reflected in future calls.

This only solves half of the problem here. The server side still needs to verify the metadata for each call. I don't see a way around that however. but it does give the freedom to decide if the token is required on a call per call basis.

[CMCDragonkai]

Just remember that `SessionManager` is used by the agent side not directly from the client side. The client uses the token in its `Session` state (provided by a lockfile). It then updates the session state based on return information from the agent which may have been supplied with info from `SessionManager`.

…

On 8/30/21 2:50 PM, Brian Botha wrote: I've done some testing now. Looks like you can provide a metadataGenerator to the callCredentials. This will generate new metadata each time a call is done add that to the metadata of the call. Using this we can get the JWT token from the session manager and provide that for each call automatically. This way we don't have to provide the session token metadata explicitly for each call. if the token gets updated in the session manager this should be reflected in future calls. This only solves half of the problem here. The server side still needs to verify the metadata for each call. I don't see a way around that however. but it does give the freedom to decide if the token is required on a call per call basis. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#230 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAE4OHOMDSVLFJKTYC7LBVDT7MEZVANCNFSM5CLFEFMQ>.

[tegefaulkes]

I have a functioning example of this working now. We wont need to provide the token for every call now.
I've made it optional by optionally passing the GRPCClientClient a Session object otherwise it defaults to the original behaviour.

If you do try to manually provide the token with this enabled then you'll get a error ErrorGRPCConnection: 13 INTERNAL: "authorization" metadata cannot have multiple values so keep that in mind.

I can proceed with updating the code to use this change.

[CMCDragonkai]

That's great. Are you sure you want to pass the entire Session object as opposed to just the token data itself? Principle of least power... Etc.

[tegefaulkes]

This way if we update the token inside the Session that is reflected inside the metadata. Ideally I could just pass the CallMetadataGenerator but that had an issue with scoping. Right now I'm creating the CallCredential with const callCredentials = grpc.credentials.createFromMetadataGenerator(session.sessionMetadatagenerator.bind(session));.
There might be a better way but this is just what I got working.

[tegefaulkes]

Fixed all the GRPC calls by removing the call Credentials and metadata where relevant. All tests are passing, linted and checked that it builds. Unless I'm missing anything this issue should be done.

Refer to this commit for the changes. 70bdcfb, d977f6a

This will be done when https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/210 is merged

[CMCDragonkai]

Awesome.

Although in the future, make sure to plan out the tasks before hand in the issue before starting on it.

[CMCDragonkai]

Done now.