Add key revocation checks, for credentials creation

[vlad-tim]

Overview

Add key revocation check when creating a credential, expose key revocation endpoint.

Description

• Expose key revocation REST endpoint.
• Add key revocation check for credential creation.
• Add tests.

How Has This Been Tested?

Manual test:

1. Create a new DID
2. Create a new key, make controller be ID of the DID.
3. Create a new credential using DID id and key ID from the previous steps. It succeeds.
4. Revoke the key.
5. Repeat the step 3. Now it fails with "error": "could not create credential: execute: failed to execute after retrying: signing credential: cannot use revoked key<MyKeyID>"

The above steps has been automated within pkg/server/router/credential_test.go file.

Also, a unit test added to pkg/service/keystore/service_test.go.

Checklist

Before submitting this PR, please make sure:

• I have read the CONTRIBUTING document.
• My code is consistent with the rest of the project
• I have tagged the relevant reviewers and/or interested parties
• I have updated the READMEs and other documentation of affected packages

References

Related to #451 .

[codecov-commenter]

Codecov Report

Merging #546 (2e74105) into main (0e4d0fc) will increase coverage by 0.21%.
The diff coverage is 100.00%.

❗ Current head 2e74105 differs from pull request most recent head d7b70d6. Consider uploading reports for the commit d7b70d6 to get more accurate results

@@            Coverage Diff             @@
##             main     #546      +/-   ##
==========================================
+ Coverage   23.98%   24.19%   +0.21%     
==========================================
  Files          46       46              
  Lines        5119     5120       +1     
==========================================
+ Hits         1228     1239      +11     
+ Misses       3663     3650      -13     
- Partials      228      231       +3     

Impacted Files	Coverage Δ
pkg/service/keystore/service.go	41.21% <ø> (+3.37%)	⬆️
pkg/server/server.go	58.96% <100.00%> (+0.16%)	⬆️

... and 1 file with indirect coverage changes

[vlad-tim]

At this point I wonder if the proposed approach (using credential creation as example) is good enough. If yes, I will implement the check in other similar places, e.g. schema creation.

A potential alternative is to move the revocation check inside keyStore.GetKey (or something like that). Such approach may help not to forget making this check where necessary in the future.

[andresuribe87]

This is an improvement, but I'm torn on whether we should support a generic "state" vs a specific revoked field, which is a boolean.

After seeing the APIs for a couple of KMS providers, I noticed that's the approach they took.

[andresuribe87]

I think it makes sense to test the value here as well. You can do so by injecting a clock instance to the service.

[vlad-tim]

I have tried to do it, but stopped after discovering that the keystore service does not use Clock, it uses time.Now() instead. I noticed that all other services also use time.Now() with the only exception being the manifest service that uses Clock. However, the Clock module is unmaintained. Are you sure?

[andresuribe87]

Thanks for pointing out that Clock is now archived. I wasn't aware of that! That said, I think it's still a good approach so that times are tested.

I'm make a ticket to clean up other places that use time.Now().

[vlad-tim]

Could you elaborate on how the times should be tested within this PR specifically?

[andresuribe87]

Following the same pattern as manifest service.

1. Create a field of type Clock in the Service struct.
2. Set that instance in the constructor by calling clock.New()
3. In this test, override the field by setting up a mock clock keyStore.Clock = mockClock.
4. Assert that the time in the mock clock is the same as the RevokedAt time.

This will need a little bit of refactoring of the Storage.RevokeKey method, but that's ok. The code will look much better.

[vlad-tim]

Thanks, updated.

[andresuribe87]

Thank you!

[vlad-tim]

I believe that all necessary changes have been addressed. Please let me know if there's anything else I need to do before this can be merged. Next, I will implement the revocation check in other places.

[andresuribe87]

@decentralgabe PTAL