-
Notifications
You must be signed in to change notification settings - Fork 340
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
feat: validate peer address using proof-of-identity transaction hash #1655
Conversation
1c3a2ce
to
f4d004d
Compare
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.
Good start!
022d8ee
to
263b76e
Compare
263b76e
to
f11136b
Compare
Not directly related to the code in this PR, but I just wanted to give a heads-up to swarm team, that (afaik) this topic has been research a bit by geth team, so if you haven't yet, it might be worth checking whats the best solution they arrived at, and compare with the one proposed here |
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.
Reviewed 2 of 3 files at r1, 1 of 7 files at r2, 2 of 6 files at r3, 4 of 6 files at r4, 3 of 3 files at r5.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @anatollupacescu, @Eknir, @ralph-pichler, and @zelig)
pkg/p2p/libp2p/internal/handshake/pb/handshake.proto, line 19 at r1 (raw file):
Previously, anatollupacescu (Anatolie Lupacescu) wrote…
I was looking into it but didn't find a solution.
This article says that's expected: https://www.aapelivuorinen.com/blog/2019/07/12/protobuf-arrays/
please use bytes
. also, this is a breaking change to the handshake protocol. i think that we already incremented the version once the storage incentives went in (or the variable pricing), but i'm not entirely sure
pkg/settlement/swap/transaction/sender_matcher_test.go, line 25 at r5 (raw file):
t.Fatal(err) } // WIP
?
pkg/settlement/swap/transaction/sender_matcher_test.go, line 28 at r5 (raw file):
} // type mockSigner struct {
remove commented code
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.
Reviewable status: 10 of 12 files reviewed, 12 unresolved discussions (waiting on @acud, @anatollupacescu, @Eknir, and @ralph-pichler)
pkg/settlement/swap/transaction/sender_matcher.go, line 22 at r6 (raw file):
ErrTransactionNotFound = errors.New("transaction not found") ErrTransactionPending = errors.New("transaction in pending status") ErrTransactionSender = errors.New("transaction sender")
you mean: invalid transaction sender
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.
Reviewable status: 10 of 12 files reviewed, 13 unresolved discussions (waiting on @acud, @anatollupacescu, @Eknir, and @ralph-pichler)
pkg/p2p/libp2p/internal/handshake/handshake.go, line 65 at r6 (raw file):
} type SenderMatcher interface {
i dont feel this is a good name
Thanks for the resources @tfalencar ! I'll have a look into this. Maybe @Eknir @zelig @anatollupacescu should too |
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.
Reviewable status: 1 of 12 files reviewed, 13 unresolved discussions (waiting on @acud, @anatollupacescu, @Eknir, @ralph-pichler, and @zelig)
cmd/bee/cmd/cmd.go, line 234 at r3 (raw file):
Previously, acud (acud) wrote…
proof-of-identity transaction hash?
fixed
pkg/node/node.go, line 279 at r3 (raw file):
Previously, ralph-pichler (Ralph Pichler) wrote…
there should be a better error here. if nothing was specified and there was no deployment tx either bee will terminate with a storage.NotFound which does not tell the user much.
Done.
pkg/p2p/libp2p/internal/handshake/handshake.go, line 65 at r6 (raw file):
Previously, zelig (Viktor Trón) wrote…
i dont feel this is a good name
It could be TransactionSenderMatcher
or OverlayMatcher
@ralph-pichler any suggestions?
pkg/p2p/libp2p/internal/handshake/pb/handshake.proto, line 19 at r1 (raw file):
Previously, acud (acud) wrote…
please use
bytes
. also, this is a breaking change to the handshake protocol. i think that we already incremented the version once the storage incentives went in (or the variable pricing), but i'm not entirely sure
updated to use bytes
last time the version was updated was:
metacertain, 2 months ago (March 24th, 2021 8:03pm)
Pricing update strategy (#1134)
pkg/settlement/swap/transaction/sender_matcher.go, line 22 at r6 (raw file):
Previously, zelig (Viktor Trón) wrote…
you mean: invalid transaction sender
fixed
pkg/settlement/swap/transaction/sender_matcher_test.go, line 25 at r5 (raw file):
Previously, acud (acud) wrote…
?
Done.
pkg/settlement/swap/transaction/sender_matcher_test.go, line 28 at r5 (raw file):
Previously, acud (acud) wrote…
remove commented code
Done.
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.
Reviewable status: 1 of 12 files reviewed, 13 unresolved discussions (waiting on @acud, @Eknir, @ralph-pichler, and @zelig)
pkg/p2p/libp2p/internal/handshake/handshake.go, line 72 at r1 (raw file):
Previously, Eknir (Eknir) wrote…
Perhaps it is time to reconsider this name, given that we use it now for settlements, postage stamps and (upcoming) spoofless overlay protection. @ralph-pichler , wdyt?
It's not related to this PR though
Done.
pkg/p2p/libp2p/internal/handshake/handshake.go, line 283 at r1 (raw file):
Previously, ralph-pichler (Ralph Pichler) wrote…
why not use the existing ctx here?
Done.
pkg/p2p/libp2p/internal/handshake/handshake.go, line 290 at r1 (raw file):
Previously, anatollupacescu (Anatolie Lupacescu) wrote…
good point, I'll update it
Done.
pkg/p2p/libp2p/internal/handshake/handshake.go, line 75 at r3 (raw file):
Previously, ralph-pichler (Ralph Pichler) wrote…
let's call this
chainBackend
here.
Done.
pkg/p2p/libp2p/internal/handshake/handshake.go, line 294 at r3 (raw file):
Previously, ralph-pichler (Ralph Pichler) wrote…
also check that txReceipt is not nil
Done.
pkg/p2p/libp2p/internal/handshake/handshake.go, line 296 at r3 (raw file):
Previously, ralph-pichler (Ralph Pichler) wrote…
wrong address used
Done.
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.
Reviewed 2 of 2 files at r6, 11 of 11 files at r7.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @Eknir, @ralph-pichler, and @zelig)
pkg/p2p/libp2p/internal/handshake/pb/handshake.proto, line 19 at r1 (raw file):
Previously, anatollupacescu (Anatolie Lupacescu) wrote…
updated to use bytes
last time the version was updated was:
metacertain, 2 months ago (March 24th, 2021 8:03pm) Pricing update strategy (#1134)
cool that's fine then, since this wasn't released yet
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.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @Eknir, @ralph-pichler, and @zelig)
pkg/p2p/libp2p/internal/handshake/pb/handshake.proto, line 19 at r1 (raw file):
Previously, acud (acud) wrote…
cool that's fine then, since this wasn't released yet
Done.
This PR implements protection against spoofed addresses.
It introduces a new command line argument that will provide a transaction hash.
If the hash is not provided it will be fetched from the state store.
The transaction hash value is added to the handshake, so that (during connection) the other party can check if this transaction is present on the blockchain and if the initiator of the transaction has the same identity as the one connecting.
If the overlays differ the connection will be reset.
This change is