-
Notifications
You must be signed in to change notification settings - Fork 108
Improve peerguard #863
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
base: master
Are you sure you want to change the base?
Improve peerguard #863
Changes from all commits
79ef50d
02753f1
16ccc04
3891a67
957f610
c0a9648
326a9e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
otp: | ||
dht: | ||
interval: 360 | ||
key: YVMwYeeoIJ9yGQeuRNHY3wigEhDEisAPcbt0L30vQ3q | ||
length: 43 | ||
crypto: | ||
interval: 360 | ||
key: 0Yu91JT1WPmWtmmrFD5tWrSzeyhLUVFWyiEXzIezcyz | ||
length: 43 | ||
room: k9xRlX6brHxKMAWhGgvsuoiwq4fcNyNtA7JQivZBYm7 | ||
rendezvous: tFMpGqtqtFHckVt62FCklh9xqjTi9upKWCmXNrNBCpL | ||
mdns: NG8LRHaJTRnR0tbOGgr73oQTZqvKEn0kllXQr5SfKDs | ||
max_message_size: 20971520 | ||
|
||
trusted_peer_ids: | ||
- 12D3KooWQi1XDFy1Ntv5WXLYJWbuFy1zbXM3F6jc4DUJKtaoZPpC | ||
protected_store_key: | ||
- trustzone | ||
- trustzoneAuth |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
otp: | ||
dht: | ||
interval: 360 | ||
key: YVMwYeeoIJ9yGQeuRNHY3wigEhDEisAPcbt0L30vQ3q | ||
length: 43 | ||
crypto: | ||
interval: 360 | ||
key: 0Yu91JT1WPmWtmmrFD5tWrSzeyhLUVFWyiEXzIezcyz | ||
length: 43 | ||
room: k9xRlX6brHxKMAWhGgvsuoiwq4fcNyNtA7JQivZBYm7 | ||
rendezvous: tFMpGqtqtFHckVt62FCklh9xqjTi9upKWCmXNrNBCpL | ||
mdns: NG8LRHaJTRnR0tbOGgr73oQTZqvKEn0kllXQr5SfKDs | ||
max_message_size: 20971520 | ||
|
||
trusted_peer_ids: | ||
- 12D3KooWQi1XDFy1Ntv5WXLYJWbuFy1zbXM3F6jc4DUJKtaoZPpC | ||
protected_store_key: | ||
- trustzone | ||
- trustzoneAuth |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,8 @@ import ( | |
"io" | ||
"io/ioutil" | ||
"log" | ||
"maps" | ||
"slices" | ||
"sync" | ||
"time" | ||
|
||
|
@@ -35,6 +37,10 @@ type Ledger struct { | |
blockchain Store | ||
|
||
channel io.Writer | ||
|
||
skipVerify bool | ||
trustedPeerIDS []string | ||
protectedStoreKeys []string | ||
} | ||
|
||
type Store interface { | ||
|
@@ -59,6 +65,16 @@ func (l *Ledger) newGenesis() { | |
l.blockchain.Add(genesisBlock) | ||
} | ||
|
||
func (l *Ledger) SkipVerify() { | ||
l.skipVerify = true | ||
} | ||
func (l *Ledger) SetTrustedPeerIDS(ids []string) { | ||
l.trustedPeerIDS = ids | ||
} | ||
func (l *Ledger) SetProtectedStoreKeys(keys []string) { | ||
l.protectedStoreKeys = keys | ||
} | ||
|
||
// Syncronizer starts a goroutine which | ||
// writes the blockchain to the periodically | ||
func (l *Ledger) Syncronizer(ctx context.Context, t time.Duration) { | ||
|
@@ -123,8 +139,17 @@ func (l *Ledger) Update(f *Ledger, h *hub.Message, c chan *hub.Message) (err err | |
return | ||
} | ||
|
||
if len(l.protectedStoreKeys) > 0 && !slices.Contains(l.trustedPeerIDS, h.SenderID) { | ||
for _, key := range l.protectedStoreKeys { | ||
if !maps.Equal(l.blockchain.Last().Storage[key], block.Storage[key]) { | ||
err = errors.Wrapf(err, "unauthorized attempt to write to protected bucket: %s", key) | ||
return | ||
} | ||
} | ||
} | ||
|
||
l.Lock() | ||
if block.Index > l.blockchain.Len() { | ||
if l.skipVerify || block.Index > l.blockchain.Len() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure - why do we need still skipVerify? otherwise changes looks good to me! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As i mentioned before, we need to have nodes as passive listeners, and in that mode they listen only from trusted peers
|
||
l.blockchain.Add(*block) | ||
} | ||
l.Unlock() | ||
|
@@ -350,12 +375,14 @@ func (l *Ledger) Index() int { | |
func (l *Ledger) writeData(s map[string]map[string]Data) { | ||
newBlock := l.blockchain.Last().NewBlock(s) | ||
|
||
if newBlock.IsValid(l.blockchain.Last()) { | ||
l.Lock() | ||
l.blockchain.Add(newBlock) | ||
l.Unlock() | ||
if !l.skipVerify && !newBlock.IsValid(l.blockchain.Last()) { | ||
return | ||
} | ||
|
||
l.Lock() | ||
l.blockchain.Add(newBlock) | ||
l.Unlock() | ||
|
||
bytes, err := json.Marshal(l.blockchain.Last()) | ||
if err != nil { | ||
log.Println(err) | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -20,6 +20,7 @@ import ( | |||||
"io" | ||||||
mrand "math/rand" | ||||||
"net" | ||||||
"slices" | ||||||
|
||||||
internalCrypto "github.com/mudler/edgevpn/pkg/crypto" | ||||||
|
||||||
|
@@ -253,6 +254,21 @@ func (e *Node) handleEvents(ctx context.Context, inputChannel chan *hub.Message, | |||||
continue | ||||||
} | ||||||
|
||||||
// If we have enabled trusted arbiter peers | ||||||
if len(e.config.TrustedPeerIDS) > 0 && e.host.ID().String() != m.SenderID { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it would make sense to be consistent here and instantiate the PeerGater with static trusted peer IDs coming from the config. For instance, the gater constructor could take as arguments a set of trustedPeerIDs coming from the config edgevpn/pkg/trustzone/peergater.go Line 35 in a11f392
and edgevpn/pkg/trustzone/peergater.go Line 62 in a11f392
could check both in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i have reviewed that now more deeply, as i have returned to edgevpn code while doing LocalAI PR init commit - the thing is, the configured initial trust IDs should work as a switch for authoritative/passive mode, where we either gating messages based on trustDB in authoritative mode, or gating all messages that came not from the initial trusted IDs in passive mode, disabling peergater further, as it don't have priority in that mode The peergater most likely remains the same, and stays in the scope of working only for blockchain trustDB There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, but this sounds like implementing a whitelisting mechanism which is very much similar to the PeerGater. To keep code concise and clean we could or either expand the PeerGater module to accept a static ids and act it as a whitelister, or have a separate component dedicated to whitelisting (which would be even more clean). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think i will expand peergater then, as for now whitelisting is effective only in pair with peergater There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yet, the ProtectedStoreKeys is relying at TrustedPeerIDs, so most likely the separated whitelisting component is needed |
||||||
// If we are not the trusted one | ||||||
if !slices.Contains(e.config.TrustedPeerIDS, e.host.ID().String()) { | ||||||
// If incoming message is not from trusted one | ||||||
if !slices.Contains(e.config.TrustedPeerIDS, m.SenderID) { | ||||||
e.config.Logger.Warnf("%s gated room message from %s - not present in trusted peer IDS", e.host.ID(), m.SenderID) | ||||||
continue | ||||||
} else { | ||||||
// If we a non-trusted peer, and we receive a meesage from the trusted one - disable peerGater | ||||||
peerGater = false | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
if peerGater { | ||||||
if e.config.PeerGater != nil && e.config.PeerGater.Gate(e, peer.ID(m.SenderID)) { | ||||||
e.config.Logger.Warnf("gated message from %s", m.SenderID) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ package node | |
import ( | ||
"context" | ||
"fmt" | ||
"slices" | ||
"sync" | ||
"time" | ||
|
||
|
@@ -123,6 +124,12 @@ func (e *Node) Start(ctx context.Context) error { | |
return err | ||
} | ||
|
||
if len(e.config.TrustedPeerIDS) > 0 && !slices.Contains(e.config.TrustedPeerIDS, e.host.ID().String()) { | ||
ledger.SkipVerify() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why skipping verification of blockchain blocks? this smells a bit off because from what I can see it would be used just to skip blockchain checks. These are more for integrity rather then security (it gates for example nodes to update blockchain versions which are older then what the current node has): by removing that check any node could post a older version of the blockchain, or a different blockchain coming from a different genesis block, and all the nodes skipping verification would accept that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first naive approach was the usage of genesis block for store authored peers from the start, as well, as the list of peer ids for protected keys So i came with the approach of storing that data in the config, which is encoded as string token later, which providing a secure way to have that data from the start Yet, later, if authored node is running through API mode, i noticed that healthcheck didn't start by default, so the later joined nodes always fails to sync, since the indexes are dramatically out of sync So if the authoritarian trusted nodes base is present from the start - they will guarantee the proper view at blockchain, and every regular client most likely need to accept the blocks from them without any checks (since they filter out any other, than trusted nodes messages at top level) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I would then remove these bits as they are replaced by having static nodes via config, no? The other way around I see it, the nodes having this set should periodically try to reconcile to the ledger the information if missing. Also: I think you found something missing there, in api mode only healthcheck should run regularly to keep everything update, that probably requires a fix on its own There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, either we need to have a reconciliation mechanism implemented, still having priority over trusted nodes vs. block validation We can still verify blocks for everyone, if we haven't defined the trusted nodes, so i would not remove that bits if i understand you correctly, because edgevpn still can be used as fully decentralized non-authoritarian VPN this way (yet, i see that later joined peers never go in sync that way, but for now that is needed only for healthchecks and other simple metadata) About healthchecks in API mode - they can be enabled through flag --enable-healthchecks, so that is looking more like on purpose for me |
||
} | ||
ledger.SetTrustedPeerIDS(e.config.TrustedPeerIDS) | ||
ledger.SetProtectedStoreKeys(e.config.ProtectedStoreKeys) | ||
|
||
// Send periodically messages to the channel with our blockchain content | ||
ledger.Syncronizer(ctx, e.config.LedgerSyncronizationTime) | ||
|
||
|
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.
I guess this is a leftover, right?
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.
yeah, there are also testing script, and other leftovers for demo purposes out of the box
that ofc will be cleaned if the main direction of that pr resolved, so further cleaning is upcoming