search

interledger / rafiki

An open-source, comprehensive Interledger service for wallet providers, enabling them to provide Interledger functionality to their users.
https://rafiki.dev/
Apache License 2.0
230 stars 80 forks source link

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

Closed sidvishnoi closed 3 weeks ago

sidvishnoi commented 1 month ago

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:

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

golobitch commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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),

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 commented 3 weeks ago

We will check this out @sabineschaller

raducristianpopa commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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:

https://github.com/user-attachments/assets/5798e2b9-5ccb-434b-b89f-580ab254c045

sabineschaller commented 3 weeks ago

This is being fixed here