Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Revoking key didn't actually revoke it #2795

Closed
sidvishnoi opened this issue Jul 5, 2024 · 8 comments
Closed

[BUG] Revoking key didn't actually revoke it #2795

sidvishnoi opened this issue Jul 5, 2024 · 8 comments
Assignees
Labels
type: bug Something isn't working

Comments

@sidvishnoi
Copy link
Member

Bug Report

Describe the bug

Revoking key from https://rafiki.money was no longer causing API calls to fail. Following were working fine after key being revoked:

  • payments from existing grant going through,
  • token rotations too working fine,
  • could even revoke a grant and get new grant

To Reproduce

This was resolved for me after I changed key-pair (but can occur again). As you can see in screenshots below, jwks.json still has the keys, but UI (and API request in website) don't have any keys.

  1. I tested via Web Monetization extension (it's not a bug there)
  2. Setup extension, connect wallet.
  3. Let some payments go through (go to https://radu.sh to test payments working)
  4. Revoke the key from https://rafiki.money/settings/developer-keys
  5. API calls work as if key not revoked. Previously, they used to fail with "invalid_client", and "Signature validation error: could not find key in list of client keys" errors.

Expected behavior

All API calls relying on key should fail with above errors.

Desktop (please complete the following information):

Screenshots

image
image

@sidvishnoi sidvishnoi added the type: bug Something isn't working label Jul 5, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Rafiki Jul 5, 2024
@golobitch
Copy link
Collaborator

I can confirm that rafiki.money is running on version 1.0.0-alpha.14.

I tried reproducing this on rafiki.money, but did not test with web monetization extension. I created few developer keys and checked jwks.json. For me, UI showed exactly the same thing that jwks.json showed.

@sidvishnoi
Copy link
Member Author

sidvishnoi commented Jul 8, 2024

Are above keys showing up in database for mentioned wallet address? They're not showing up in UI for me (UI part could be a caching issue, I'll check again).

I couldn't reproduce it again either as I mentioned, but there's a chance it can happen again.

Update: confirmed jwks.json returns keys that are not shown in the rafiki.money UI (even after clearing cache). Screenshots as shown in issue description.

@sabineschaller sabineschaller self-assigned this Jul 25, 2024
@sabineschaller sabineschaller moved this from Backlog to In Progress in Rafiki Jul 25, 2024
@sabineschaller
Copy link
Member

sabineschaller commented Jul 25, 2024

I could reproduce that behavior with the extension and a payment pointer from test wallet (rafiki.money).

I then fired up the Rafiki localenv (alpha 15, EDIT: same with alpha 14),

  • checked the keys for a random account (https://cloud-nine-wallet-backend/accounts/gfranklin), there was one
  • revoked that key using the Admin API endpoint
  • fetched the keys for https://cloud-nine-wallet-backend/accounts/gfranklin, there was none.

I wonder if something fails during key revocation in test wallet, i.e. whether the Admin API call to Rafiki is done correctly. @Tymmmy @rico191013 @dragosp1011

@Tymmmy
Copy link
Contributor

Tymmmy commented Jul 28, 2024

We will check this out @sabineschaller

@raducristianpopa
Copy link
Member

raducristianpopa commented Jul 30, 2024

I could reproduce that behavior with the extension and a payment pointer from test wallet (rafiki.money).

I then fired up the Rafiki localenv (alpha 15, EDIT: same with alpha 14),

checked the keys for a random account (https://cloud-nine-wallet-backend/accounts/gfranklin), there was one

revoked that key using the Admin API endpoint

fetched the keys for https://cloud-nine-wallet-backend/accounts/gfranklin, there was none.

I wonder if something fails during key revocation in test wallet, i.e. whether the Admin API call to Rafiki is done correctly.

Looking at the screenshot with the response from jwks.json, it looks like we have two public keys that are duplicated. I know that multiple addresses can share a public key, but should we allow uploading the same key to a wallet address?

@sabineschaller

@sabineschaller
Copy link
Member

sabineschaller commented Jul 31, 2024

Very well spotted @raducristianpopa. You are right. I can add a check to make sure they are unique.

But I don't think that solves the issue unless the extension creates new keys every time. Because otherwise, a key is revoked but still existing in Rafiki and it won't let you add it again.

EDIT: Also, something still seems to be wrong with the revoking because the keys should just not show up in the jwks.json once revoked.

@raducristianpopa
Copy link
Member

Very well spotted @raducristianpopa. You are right. I can add a check to make sure they are unique.

But I don't think that solves the issue unless the extension creates new keys every time. Because otherwise, a key is revoked but still existing in Rafiki and it won't let you add it again.

EDIT: Also, something still seems to be wrong with the revoking because the keys should just not show up in the jwks.json once revoked.

Found the issue on the test wallet with @rico191013 yesterday: whenever an user tries to upload the same public key to the same or to another wallet address, they will see an error - This key already exists, but the admin API is still getting called. Here is a screen recording:

Screen.Recording.2024-07-31.at.12.37.43.PM.mov

@sabineschaller
Copy link
Member

This is being fixed here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
Status: Done
Development

No branches or pull requests

6 participants