-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Do not publish public keys extractable from ID #4020
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JustinDrake these changes look good to me, and all the required deps have now been updated (in #4059). Last thing I'd like to see before merging this is a test or two showing that using an ed25519 key doesnt publish to the dht.
(you'll want to rebase on latest master to make it all work) |
@whyrusleeping I've rebased. How do you suggest that the test be written? There are no tests for |
@vyzo could you help @JustinDrake out with some tests here? |
@JustinDrake we can add a publisher_test.go file. We can start with a simple test which directly tests the behaviour of The side-effect of |
Note that you can mock the network and even the value store. |
@vyzo Thank you for the suggestions of using the value store, and of testing both the RSA and Ed25519 cases. @whyrusleeping I now have a test that shows that an Ed25519 key does not publish to the DHT, and (bonus) a test that shows that an RSA key does publish to the DHT. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small things that would be nice to fix, but in general this LGTM.
namesys/publisher.go
Outdated
errs := make(chan error, 2) // At most two errors (IPNS, and public key) | ||
|
||
// Attempt to extract the public key from the ID | ||
var extractedPublicKey = id.ExtractPublicKey() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: extractedPublicKey := ...
instead of using var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 👍
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for completeness sake, it might be good to check in the dstore to make sure theres no key being put there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed 👍
License: MIT Signed-off-by: Justin Drake <drakefjustin@gmail.com>
Is this good to merge? :) |
Thanks @JustinDrake! |
License: MIT
Signed-off-by: Justin Drake drakefjustin@gmail.com