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

feat: validate peer address using proof-of-identity transaction hash #1655

Merged
merged 12 commits into from
May 13, 2021

Conversation

anatollupacescu
Copy link
Contributor

@anatollupacescu anatollupacescu commented May 4, 2021

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 Reviewable

Copy link
Contributor

@Eknir Eknir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start!

pkg/p2p/libp2p/internal/handshake/handshake.go Outdated Show resolved Hide resolved
pkg/p2p/libp2p/internal/handshake/handshake.go Outdated Show resolved Hide resolved
@anatollupacescu anatollupacescu requested review from acud and zelig May 5, 2021 15:18
@anatollupacescu anatollupacescu marked this pull request as ready for review May 5, 2021 15:21
@anatollupacescu anatollupacescu added the ready for review The PR is ready to be reviewed label May 5, 2021
@anatollupacescu anatollupacescu changed the title Draft: validate address against spoofing Validate peer address against spoofing May 5, 2021
@anatollupacescu anatollupacescu marked this pull request as draft May 5, 2021 15:22
@anatollupacescu anatollupacescu removed the ready for review The PR is ready to be reviewed label May 5, 2021
cmd/bee/cmd/cmd.go Outdated Show resolved Hide resolved
pkg/node/node.go Outdated Show resolved Hide resolved
pkg/p2p/libp2p/internal/handshake/handshake.go Outdated Show resolved Hide resolved
pkg/p2p/libp2p/internal/handshake/handshake.go Outdated Show resolved Hide resolved
@anatollupacescu anatollupacescu changed the title Validate peer address against spoofing feat: validate peer address against spoofing May 7, 2021
@anatollupacescu anatollupacescu marked this pull request as ready for review May 10, 2021 07:23
@anatollupacescu anatollupacescu added the ready for review The PR is ready to be reviewed label May 10, 2021
@tfalencar
Copy link

tfalencar commented May 10, 2021

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
Some links for reference: ethereum/devp2p#109
ethereum/devp2p#161
https://hal.inria.fr/inria-00490509/file/HotP2P10-KAD_DHT_attack_mitigation-cholez.pdf
http://www.ipdps.org/ipdps2010/ipdps2010-slides/HOTP2P/HotP2P_DHT_attack_mitigation_Cholez.pdf

Copy link
Member

@acud acud left a 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

Copy link
Member

@zelig zelig left a 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

Copy link
Member

@zelig zelig left a 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

@acud
Copy link
Member

acud commented May 11, 2021

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
Some links for reference: ethereum/devp2p#109
ethereum/devp2p#161
https://hal.inria.fr/inria-00490509/file/HotP2P10-KAD_DHT_attack_mitigation-cholez.pdf

Thanks for the resources @tfalencar ! I'll have a look into this. Maybe @Eknir @zelig @anatollupacescu should too

@Eknir Eknir mentioned this pull request May 11, 2021
Copy link
Contributor Author

@anatollupacescu anatollupacescu left a 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.

@anatollupacescu anatollupacescu requested a review from acud May 11, 2021 09:26
Copy link
Contributor Author

@anatollupacescu anatollupacescu left a 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.

@anatollupacescu anatollupacescu changed the title feat: validate peer address against spoofing feat: validate peer address using proof-of-identity transaction hash May 11, 2021
Copy link
Member

@acud acud left a 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

Copy link
Contributor Author

@anatollupacescu anatollupacescu left a 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.

@anatollupacescu anatollupacescu merged commit 8925190 into master May 13, 2021
@anatollupacescu anatollupacescu deleted the spoofless-overlay branch May 13, 2021 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants