embed public keys inside ipns records, use for validation #5079
Conversation
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
ghost
assigned
namesys/validator.go
Outdated
| if entry.PubKey != nil { | ||
| pk, err := ic.UnmarshalPublicKey(entry.PubKey) | ||
| if err != nil { | ||
| // TODO: i think this counts as a 'malformed record' and should be discarded |
There was a problem hiding this comment.
@Stebalien WDYT? I think enforcing that the data in that field is valid and that it matches the expected peerID is the right thing to do.
There was a problem hiding this comment.
I agree. If the user specifies the public key, it had better damn well be correct.
namesys/validator.go
Outdated
| log.Debugf("public key in ipns record failed to parse: ", err) | ||
| return nil, err | ||
| } | ||
| expPid, err := peer.IDFromPublicKey(pk) |
There was a problem hiding this comment.
This could fail if the user manually created the peerID differently than this function does (for example, made their key the hash of a public key that this function would choose to embed). I'm not sure how much of a problem is
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
There was a problem hiding this comment.
I don't know much about IPNS (nothing really) but the logic sounds good to me, +1 on adding the comments/explanations which made the PR much easier to understand and review. Newbie question: this won't be necessary with elliptic keys because of the MaxInlineKeyLength restriction? Smaller keys will always be inlined (identity hash)?
Correct, smaller keys will be inlined, and we won't need to embed them in the records anymore. |
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT Signed-off-by: Jeromy <jeromyj@gmail.com>
Currently, publishing ipns records to 0.4.{14,15} nodes using a key other than your peers key is unlikely to succeed, as those peers will throw away the record upon receipt if they don't have the corresponding public key. In tests, this isnt noticed so much as the set of peers we push the public key to overlaps heavily with the set of peers we push the ipns record to. In the larger network, that is not the case.
In any case, publishing the public key separately is just a bit weird. We really should have been doing this the whole time. It's not that many extra bytes, and it saves us a whole separate lookup in many cases. Plus, when embeddable ed25519 keys become widely used (support for which was just merged) we won't even need this code :)
Looking for some review while I work on some tests for this.
License: MIT
Signed-off-by: Jeromy jeromyj@gmail.com