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

issues with ipfs routing get | put #9716

Closed
3 tasks done
laurentsenta opened this issue Mar 10, 2023 · 6 comments
Closed
3 tasks done

issues with ipfs routing get | put #9716

laurentsenta opened this issue Mar 10, 2023 · 6 comments
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization

Comments

@laurentsenta
Copy link
Contributor

Checklist

Installation method

built from source

Version

Kubo version: 0.20.0-dev-0d94bac30
Repo version: 13
System version: arm64/darwin
Golang version: go1.19.1

(current master)

Config

(default)

Description

My work on ipfs/gateway-conformance#2 got me into #9654, and now #9667 is blocked with a few issues.

I run the following code:

CID=$(echo "hello" | ipfs add --cid-version 1 --inline -Q)
KEY_NAME=test_key_rsa_$RANDOM
RSA_KEY=$(ipfs key gen --ipns-base=b58mh --type=rsa --size=2048 ${KEY_NAME} | head -n1 | tr -d "\n")

# publish a record valid for a 100 years
ipfs name publish --key ${KEY_NAME} --allow-offline -Q  --ttl=876600h "/ipfs/$CID"
ipfs routing get /ipns/${RSA_KEY} > ${RSA_KEY}.ipns-record

echo RSA_KEY=${RSA_KEY}

KEY_NAME=test_key_ed25519_$RANDOM
ED25519_KEY=$(ipfs key gen --ipns-base=b58mh --type=ed25519 ${KEY_NAME} | head -n1 | tr -d "\n")

# publish a record valid for a 100 years
ipfs name publish --key ${KEY_NAME} --allow-offline -Q --ttl=876600h "/ipfs/$CID"
ipfs routing get /ipns/${ED25519_KEY} > ${ED25519_KEY}.ipns-record

echo ED25519_KEY=${ED25519_KEY}

it generates ${RSA_KEY}.ipns-record, ${ED25519_KEY}.ipns-record.

Then I rm my .ipfs folder to start from a clean slate.

I have the following issues:

1. Wrong Key

ipfs routing put /ipns/${ED25519_KEY} ./${RSA_KEY}.ipns-record succeeds

expected: failure, we're using the wrong ipns name.

touch empty && ipfs routing put /ipns/${ED25519_KEY} ./empty succeeds

expect: failure, we're using an empty file

2. PUT(K, V) && GET(K) fails on GET

ipfs routing put /ipns/${ED25519_KEY} ./${ED25519_KEY}.ipns-record && ipfs routing get /ipns/${ED25519_KEY} fails

expected:

  • either the PUT fails
  • or the PUT and the GET succeed.

This is failing in PR on #9667.
See sharness result: https://tf-aws-gh-runner.s3.amazonaws.com/kubo/ipfs/kubo/4382756151/3/sharness.html#t0114-gateway-subdomains

3. ipfs routing .... commands print the wrong key

ipfs routing put /ipns/${ED25519_KEY} ./${ED25519_KEY}.ipns-record prints the self key.

Expected: print the key that was used; which should be ${ED25519_KEY}

4. ipfs routing put | get does not support --allow-offline

I prepared a change to support allow-offline in the following branches:

We seem to have the rest of the API ready, but I'm not sure this should be prioritized.

@laurentsenta laurentsenta added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Mar 10, 2023
@hacdias
Copy link
Member

hacdias commented Mar 10, 2023

cc @lidel

@laurentsenta
Copy link
Contributor Author

I'm getting closer, we're putting in the self key apparently, the mechanism is not clear to me:

  ipfs dag import ../t0114-gateway-subdomains/fixtures.car &&
  WRONG_KEY=`ipfs routing put /ipns/${RSA_KEY} ../t0114-gateway-subdomains/${RSA_KEY}.ipns-record` &&
  echo "Wrong key: $WRONG_KEY" && ## this is the SELF key here, not RSA_KEY
  echo &&
  ipfs routing get /ipns/${WRONG_KEY} && ## succeeds
  echo &&
  echo "RSA_KEY: $RSA_KEY" && 
  ipfs routing get /ipns/${RSA_KEY} ## routing not found

@laurentsenta
Copy link
Contributor Author

https://docs.ipfs.tech/reference/kubo/cli/#ipfs-routing-put

The value must be a valid value for the given key type. For example, if the key
is /ipns/QmFoo, the value must be IPNS record (protobuf) signed with the key
identified by QmFoo.

@lidel
Copy link
Member

lidel commented Mar 23, 2023

Thank you for digging into this and preparing fixes @laurentsenta
Thin on time to look deeper into this, but some thoughts below, hopefully will unblock you

  • (4) fixes around --allow-offline sound sensible but we are in the middle of feat: Über Migration (and Boxo rename) boxo#220 which does two things:

    1. rename go-libipfs→boxo
    2. moving related packages
      • github.com/ipfs/interface-go-ipfs-core => boxo/coreiface
      • github.com/ipfs/go-namesys => ./namesys
      • github.com/ipfs/go-ipns => ./ipns

    This means more work: after mentioned PR lands, you need to re-apply fixes onto boxo/coreiface

  • (3) always printing self key it is not a bug. It looks weird in the context of IPNS, but you could be storing arbitrary data. What you get in response is not IPNS key, but the PeerID of a node that accepted the DHT PUT (see code here). It happens to be the same as self key because we kave a single keystore for both libp2p and ipns.

  • (2) PUT(K, V) && GET(K) fails on GET

    • If you do ipfs name resolve instead of ipfs routing get, does it fail too? I had no time to reproduce, but assume yes.
    • I suspect there is a bug and doing the sane thing (ipfs dht put /ipns/mbase-string-encoded-multihash) does not work because IPNS implementations (spec are based on them) expect different key: /ipns/{raw-bytes-of-peerid-key} (without mbase envelope).
    • Some pointers that made me thing this is the source of the problem:
      • we turn raw bytes to string here (without multibase)
      • then /ipns/{binary} key is used for DHT query here
    • This is legacy which we need to deal with for now, will be able to fix it while we switch to double-hashed DHT (cc @guillaumemichel @aschmahmann for visibility)
    • As a quick fix, I suggest detecting routing put where key starts with /ipns/ and then try to parse the remainder as a string representation of peerid, and if it is successful, use that (raw bytes) instead, and prefix them with /ipfs/ string.

@laurentsenta
Copy link
Contributor Author

Thanks for taking the time to share advice and clarifying some of my misunderstandings @lidel

I'll focus on (2) since that's the most blocking one:

We do normalize the key before sending it to the routing API,

}
dhtKey, err := normalizeKey(key)
if err != nil {
return err
}
return r.routing.PutValue(ctx, dhtKey, value)
}
func normalizeKey(s string) (string, error) {
parts := path.SplitList(s)
if len(parts) != 3 ||
parts[0] != "" ||
!(parts[1] == "ipns" || parts[1] == "pk") {
return "", errors.New("invalid key")
}
k, err := peer.Decode(parts[2])
if err != nil {
return "", err
}
return path.Join(append(parts[:2], string(k))), nil
}

I realized getting an output with ipfs get /ipns/WRONG_KEY might be expected, it's an empty dir by default apparently.

I'll look into the routing implementation.

@laurentsenta
Copy link
Contributor Author

laurentsenta commented May 12, 2023

Closing as solved!

  • --allow-offline is now supported and it solves my issues with GET & PUTs
  • the weird self key output makes sense now and is not a bug.

Thanks for the inputs and help @hacdias @lidel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

3 participants