feat(gateway): IPNS record response format (IPIP-351)

[hacdias]

What

Implementation of IPIP-351: IPNS Signed Records Response Format on HTTP Gateways (ipfs/specs#351)

Checklist

• IPIP Ready for Review: IPIP-351: IPNS Signed Records Response Format on HTTP Gateways  specs#351
• fix: honour --ttl flag in 'ipfs name publish' #9471
  • Approved & merged
  • Rebase this PR
• feat: add RoutingAPI to CoreAPI interface-go-ipfs-core#91
  • Approved, merged & released
  • Bump version here
• Caching
  • Improvements for the entire /ipns namespace is tracked on Gateway: Cache-Control, max-age=ttl, ETag for /ipns requests boxo#329. I can possibly take over afterwards.
  • Cache IPNS records according to their TTL (fix: honour --ttl flag in 'ipfs name publish' #9471 required for this).
• Sharness tests for request headers and key validation

Important Notes

• The sharness test put round trips (#3124) was failing. I understand that it expected some output, but the test doesn't validate the output so it's not clear to me what it was. When refactoring the Routing API to the CoreAPI interface, I used the direct calls to the internal routing value store to get or put a value, instead of listening to events on the context field.

BREAKING RPC CHANGE

This changes output of selected RPC commands (https://docs.ipfs.tech/reference/kubo/rpc/):

• /api/v0/dht/get and /api/v0/routing/get
• /api/v0/dht/put and /api/v0/routing/put

-{
-  "Extra": "<string>",
-  "ID": "<peer-id>",
-  "Responses": [
-    {
-      "Addrs": [
-        "<multiaddr-string>"
-      ],
-      "ID": "peer-id"
-    }
-  ],
-  "Type": "<int>"
-}
+"<base64-string>"

and removes --verbose flag from all of them.

I'm not sure how critical this is. It would be great to get some feedback on why it was the way it was.

(lidel): This makes us lose the ability to see what's happening internally with the routing API for get/put with the verbose option, but since there were no tests for this we assume it is acceptable to clean this up. I suspect our DHT monitoring uses custom code anyway, and worst case, will be acceptable to refactor.

Other Notes

• ipfs routing get <name> fetchs the signed IPNS key in protobuf: https://github.com/ipfs/go-ipns/blob/master/pb/ipns.proto. The output, in combination with the key, can be validated with the code below.
• Routing API is not part of the CoreAPI, therefore not exposed to the gateway handlers. It is, however, part of the RPC client. Still not sure how to make it available on the gateway but shouldn't be extremely hard.
• Code for validating IPNS signed records in Go and JS: https://gist.github.com/hacdias/f8935decee0c6a57513f2221c5a2fd7c

[hacdias]

@lidel while thinking about caching of the IPNS records, I found this: ipfs/boxo#329. Now I see the following options going forward:

1. Keep the PR as-is, do not work on IPNS TTLs now.
2. Change the PR to incorporate the namesys interface update. Here I see two ways:
   a. Add new function ResolveWithTTL which returns everything Resolve does + TTL. Use that for caching, and the introduced Routing API for the key.
   b. Add new function ResolvePlus (with a better name) which returns everything Resolve does + TTL + IPNS Key. This way we wouldn't need to add a new Routing API. After all, when resolving the name, we have to get the key somewhere, right?

[hacdias]

PSA: I added a small CLI command ipfs name validate <ipns key> <record> where we can validate if record is a valid record for ipns key. It is useful for testing. Not sure if it's the best place to put it though.

[lidel]

incorporate the namesys interface update. [...]
Add new function ResolveWithTTL which returns everything Resolve does + TTL.

@hacdias This sounds like the best course of action. Please add it to Resolver interface.

Light clients will resolve IPNS via gateway, and that means implicit assumption the HTTP response respects the TTL set in the signed IPNS record. Reverse proxies will cache responses, so this TTL needs to be translated to HTTP Cache control headers – without this we will have flaky behavior and bad UX/DX.

If we can also fix cache-control headers for regular /ipns/ responses, that would be nice win, but I suspect gettting correct TTL for DNSLink will be more involved. To avoid time sink it is ok to descope it and focus this PR on correct TTL in application/vnd.ipfs.ipns-record responses.

ps. Unsure what you mean by "Resolve does + TTL + IPNS Key" – the key is either included in IPNS record (RSA) or inlined in the libp2p-key CID (ED25519), no need for additional lookup.

[hacdias]

ps. Unsure what you mean by "Resolve does + TTL + IPNS Key" – the key is either included in IPNS record (RSA) or inlined in the libp2p-key CID (ED25519), no need for additional lookup.

What I meant is that: ResolveWithTTL (or just Resolve) already has to lookup for the keys in order to resolve the address. If we first do namesys.Resolve and then Routing.Get, we are performing the lookup twice, no? My other idea (2. b.) was to have a Resolve function in namesys that also returns the key. However, that could get hairy fast as there would be too many functions doing too many different things and additional return values. Wdyt?

[hacdias]

Note to self: tests failing in ./t0170-legacy-dht.sh and ./t0170-routing-dht.sh for ipfs routing put. It seems ipfs dag put uses ipfs routing put.

• put round trips failing. Did not understand why, will investigate later.

[lidel]

• let's keep API changes minimal – do not change namesys APIs beyond adding ability to read TTL
• if you want to return key with gateway response, you can manually add it to the record under Public Key field in protobuf – no need to change the underlying namesys API. but i dont think we need to do this – it is already present in RSA case, and is not needed for ED25519. so I'd do nothing here.
• refactoring gateway code and routing commands to be DRY (use coreapi) – this code is usually old, revisited once 3-4 years, so we may need to clean things up if needed 🙈

[hacdias]

@lidel started working on adding the TTL to the Namesys package (ipfs/go-namesys#34). However, updating it on Kubo its being slightly harder than I expected, as we don't directly call namesys.Resolve in the HTTP handler. So I probably have to add another set of functions in Kubo in order to bubble up the TTL to the gateway. I'm not sure if doing that in this PR is a great idea as I wanted to make the scope the smallest as possible.

What we can do in this PR is:

1. Handle the cache of IPNS records separately because we already have the record. We can just parse it and use the TTL to manually set the headers.
2. Do nothing and keep it cached as the rest of the IPNS namespace.

And then solve the IPNS TTL issue in a separate PR to avoid making this PR too large and unscoped.

[lidel]

(1) sounds like a safer path 👍

[hacdias]

Implementation ready for review with proper cache control. Note that this PR needs #9471 to be merged first.

[lidel]

Thank you, IPIP-351 things look good 👍

I've rebased.

This PR changes the wire format of some rarely used low level commands (details below),
but since all regression tests for put and get pass I assume they are useful mostly on the CLI, and the JSON response change does not break anyone (otherwise, there would be a regression test).

I agree with your decision to switch to CoreAPI and avoid having duplicated code.
Just to be safe, I've marked them as experimental and removed unused -v parameter.

We are missing tracing span, but that can be added after we are in go-libipfs.
Since we want to unblock gateway extraction, I'm merging this as-is.

[lidel]

💭 This changes both /api/v0/dht/get and /api/v0/routing/get (https://docs.ipfs.tech/reference/kubo/rpc/):

-{
-  "Extra": "<string>",
-  "ID": "<peer-id>",
-  "Responses": [
-    {
-      "Addrs": [
-        "<multiaddr-string>"
-      ],
-      "ID": "peer-id"
-    }
-  ],
-  "Type": "<int>"
-}
+"<base64-string>"

and breaks --verbose flag in both.

I've removed flag and marked routing/get as Experimental (dht/get was already deprecated)

[lidel]

💭 This changes both /api/v0/dht/put and /api/v0/routing/put (https://docs.ipfs.tech/reference/kubo/rpc/):

-{
-  "Extra": "<string>",
-  "ID": "<peer-id>",
-  "Responses": [
-    {
-      "Addrs": [
-        "<multiaddr-string>"
-      ],
-      "ID": "peer-id"
-    }
-  ],
-  "Type": "<int>"
-}
+"<base64-string>"

and breaks --verbose flag in both.

I've removed flag and marked routing/put as Experimental (dht/put was already deprecated)

[Winterhuman]

@lidel Just to ask, is this feature set to arrive for Kubo v0.19 or something like v0.18.1? Not sure what the timeline for this feature is

[lidel]

all IPNS improvements will land in 0.19 (we want to go over proper Release Candidate first)