Investigate adding ListClients and DeleteClient operations

[paulhowardarm]

Summary

There is a use case for Parsec to be able to eliminate and forget all keys/resources associated with a particular client application identity. This can be achieved by introducing new admin-level operations to enumerate and delete clients. This issue is a request to break down the work into individual issues in all required repos.

Requirements

• There is a dependency on admin-level operations. See Investigate admin role and admin-level operations #292
• A ListClients Parsec opcode will be introduced. This opcode will be an admin-level operation, and must fail with an appropriate permission denied error if any call is attempted from a non-admin client, subject to the design for admin-level operations. This opcode will take no inputs. It will return as its output an unordered set of strings that are application identities according to the endpoint's currently-configured auth/identity mechanism. Each identity returned should be one for which at least one key (or other object/resource) has been stored within the Parsec service,
• A DeleteClient Parsec opcode will be introduced. This opcode will be an admin-level operation, and must fail with an appropriate permission denied error if any call is attempted from a non-admin client, subject to the design for admin-level operations. This opcode will take a single string as its input. The string must be an application identity according to the endpoint's currently-configured auth/identity mechanism. The string should be a string that was previously returned by a call to ListClients, since the intention is that callers would call ListClients first to determine the set of clients for which keys exist, and would then call DeleteClient one or more times to delete the resources of some subset of those clients. The Parsec service must be robust against the possibility that the string is either invalid or corresponds to a client identity for which no keys or other resources currently exist. It should return a suitable status code and take no other action for such cases.
• The DeleteClient operation should be absolute in terms of all keys or other resources that might be stored within Parsec. When Parsec is expanded to include things such as secure blob storage, then DeleteClient would apply to these as well, not just keys that were provisioned through PSA Crypto operations.
• There is no atomicity requirement in Parsec. Race conditions can exist if clients are being deleted while their resources are being used, or if the client creates new resources during the deletion process. It is the responsibility of the integrated system to ensure that DeleteClient is called at times when the system is appropriately quiesced.
• The ListClients and DeleteClient operations must be accessible at least through the Rust client library and the parsec-tool. Access via other client libraries is optional, and can be supported later on an as-needed basis.

Definition of Done

There should be an open PR into parsec-book indicating the ListClients and DeleteClient opcodes and their contracts. There should also be GitHub issues for the individual work items needed to implement and plumb these through all of the layers, including e2e tests and parsec-tool integration.

[ionut-arm]

Some questions:

1. Should the DeleteClient operation be valid for the admin deleting itself as well?
2. Are the operations supposed to be directed to the Core provider? I remember a discussion around it, but I can't recall whether it should be core provider-only, or if all providers should implement it, for more granular control.
3. Would we want, in the ListClients operation, to also return a list of providers per client as well, indicating where the client has a "presence"? My thought was that we shouldn't necessarily tie this down to DeleteClient and to what we need for that operation

[hug-dev]

My answers to your questions

1. I would say yes, similarly that it is possible on Unix to delete privileged files if you have the privilege.

2. I would say that those should be core operations as well. Very similar to ListKeys, which also takes authentication.

3. Agree that we want to make sure if it's needed to know which client is using which provider. The ListClients operations could return a map of list of application names per provider ID or something similar. But I am not sure if that would not make this operation too "heavy". Maybe it's possible to get that information with another new operation, in the future if it is needed. Maybe another ListClients with a provider ID parameter or put that in this one? I guess that's what you meant with "granular-control" in your question 2.

One more question

If we have at some point in the future the possibility of multiple authenticators being supported in parallel (see #271), we might not be able to select a client in the DeleteClient operation with just an application name as multiple authentication methods can yield the same client name. For example, it might be a tuple (AuthenticationType, ApplicationName) to only select a client in one authentication method. It depends to what we want to do in the future.
If we just use a string corresponding to the application name, we might have problem to scale that to a Parsec service that supports multiple authenticators.

The design indicates:

It will return as its output an unordered set of strings that are application identities according to the endpoint's currently-configured auth/identity mechanism.

"according to the endpoint's currently-configured auth/identity mechanism" works nicely if only one authentication method is currently used at the endpoint. If multiple are used, we can precise the design by saying that ListClients will only return application names "created" with the same authentication type that is specified in the auth type field of the ListClients request. This is similar than what ListKeys is currently doing.

[ionut-arm]

If multiple are used, we can precise the design by saying that ListClients will only return application names "created" with the same authentication type that is specified in the auth type field of the ListClients request.

Would this rely on the further partitioning that we'll do within the providers?

This sounds like a neat idea, it would allow different admins to manage different authentication mechanisms or endpoints (actually yeah, we have to think about service endpoints too 😬 )

[hug-dev]

Would this rely on the further partitioning that we'll do within the providers?

I think that it would. If we support only one authentication method per endpoint and multiple endpoint then that's not needed. Maybe it does not harm anyway to check that the auth type in the ListClients operation is the same as the current authentication method used by the service for now.

[hug-dev]

About atomicity, since our key info managers are accessed in parallel, they must be guarded by locking mechanisms and hence are protected against data races. I believe that ListClients will ultimately lock the KIM for reading and DeleteClient for writing and it would prevent creating or deleting keys while that happens.

[ionut-arm]

I'm not so sure, unless you design it like that - I was thinking the core provider would just issue a series of DestroyKey operations for DeleteClient, so it could overlap with other operations from other clients.

[hug-dev]

Design proposal of operations

They would both be core operations, adressed to provider ID 0.

ListClients

Lists all clients currently having keys in the service. Opcode: 27 (0x001B)

Parameters

No parameters are needed for this operation.

Results

Name	Type	Description
clients	Vector of String	List of clients

Specific response status codes

No specific response status codes returned.

Description

This operation lists all clients that are currently storing keys in the Parsec service. The clients field contain a vector of the application names used by clients.

This operation necessitates admin privilege.

Only the clients using the same authentication method as this request will be listed. It has no impact currently as only one authentication method in the service is supported but might do if the service supports multiple.

Contract

No contract yet.

DeleteClient

Delete all keys a client has in the service. Opcode: 28 (0x001C)

Parameters

Name	Type	Description
client	String	Client to delete

Results

No values are returned by this operation.

Specific response status codes

No specific response status codes returned.

Description

This operation deletes all keys a client owns in all Parsec providers. The client parameter string must be one of the clients returned by the ListClients operation.

This operation necessitates admin privilege.

Only the clients using the same authentication method as this request will be deleted. It has no impact currently as only one authentication method in the service is supported but might do if the service supports multiple.

Contract

No contract yet.

Design proposal of implementation

They would need an implementation in the interface as well, which would be straightforward as other operations.

We can let the filtering per provider to a future operation if that's needed, to keep those one simple.

In the service

Implemented similarly as other operations (before the provider level), but they would require admin privilege.

Provide trait method:

    fn list_clients(
        &self,
        op: list_clients::Operation,
    ) -> Result<list_clients::Result> {

In the future this would also contain as parameter the authentication type to only return clients of that type.

The operation would be implemented in the core provider.

The core Provider structure would contain a vector of all Key Info Manager (that it would share concurrently with the providers), which is passed during service build-up. The avantage of that instead of using the providers directly is for the RWLock that would have to be taken: a key would not be able to be deleted while ListClients is being called and keys won't be able to be read/write while DeleteClient is called.

For ListClients, I believe the get_all method on all provider IDs can be called and then we can return a set of application names.
For DeleteClient, we would do a get_all and then as many remove as needed.

In the Rust client

Admin operations can only be executed by the admins. But clients can not know if they are admin or not 😔 I guess that would be a reason why adding a new response status is needed so that clients can return the appropriate error and the user can know if either their auth token is wrong or if they are not admin?
To list/delete clients on other auth methods, it should be possible for the clients to change its current authentication method as well. I guess they can already use the set_auth method to change it.

The rest of the implementation is straightforward.

Testing

We can test this in the all-providers multitenancy tests. Making sure that the admin can see all clients and that he can delete all keys of the other one 💯

Documentation

We need to add in the book the new operations. As far as I know, no more threat model updates are needed than the ones already done in the admin feature PR.

Question

This relies on the fact that "only clients created with the same auth method as the request will be listed/destroyed". Is this good? Should we already think about the multiple authenticators scenario and make the ListClients operations return clients created with all authentication method or is this too complicated for now?

[ionut-arm]

The core Provider structure would contain a vector of all Key Info Manager (that it would share concurrently with the providers), which is passed during service build-up. The avantage of that instead of using the providers directly is for the RWLock that would have to be taken: a key would not be able to be deleted while ListClients is being called and keys won't be able to be read/write while DeleteClient is called.

I don't think you can do that. For TPM it works out because we store the keys themselves in the KIM, but for Mbed Crypto and PKCS11 it means the keys would never be deleted.

[hug-dev]

I don't think you can do that. For TPM it works out because we store the keys themselves in the KIM, but for Mbed Crypto and PKCS11 it means the keys would never be deleted.

Ah yes you are totally right, forgot about that 🤕

In that case I can propose the following:

For ListClients to do something similar than ListKeys: have the list_clients method be implemented by all providers to use the key info manager implementation they are using to return a list of client. The Core provider would call list_clients on all of its Provide implementations (which it already has) and merge them.
As far as I know, this would not prevent ListClients to be called on a specific provider rather than the core one only? But then it seems that we have the same behaviour with ListKeys? Is that a bug or a feature 😋?

For DeleteClient, we can either do something similar by making all providers implement this method or we can use a mix of existing methods: list_keys on the application name given as parameter and then psa_destroy_key on each one of them. The first one is consitent with the other similar operations as it would allow to be called on independent provider, the second one would prevent that. The first option would lock the key info manager for a longer time leaving less option for concurrent calls.

This will break the atomicity of these operations but they will still be safe.

[ionut-arm]

Is that a bug or a feature 😋?

I'd rather leave that question to @paulhowardarm 😁

I'm not sure about how to implement DeleteClient - on one hand, it feels safer/better to not have it on every provider (though it might just be paranoia). On the other hand, implementing it in the provider would probably be more efficient and simpler...

[hug-dev]

Ah check this

Although we need to be sure somewhere that ListKeys can not be called directly on the crypto providers. Probably somewhere higher on the stack.

Here as well:

The check I was talking in the other comment could be here but could also be in the is_capable method where we would check if the request's opcode is a Core or a Crypto operation and based on that check if the provider can do it or not.

Seems like I forgot to add that check somewhere! We should probably add it with a test.

[hug-dev]

Issues created:

• Implement ListClients and DeleteClient in the core provider #311
• ListKeys should only be callable on the Core provider #310
• Add ListClients and DeleteClient operations parsec-book#79
• Add support for ListClients and DeleteClient parsec-client-rust#66
• Add ListClients and DeleteClient structures parsec-interface-rs#94
• Add support for ListClients and DeleteClient parsec-tool#22
• Add ListClients and DeleteClients contracts parsec-operations#26