-
Notifications
You must be signed in to change notification settings - Fork 119
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
universe: add server info RPC, check we're not connecting to ourselves #364
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.
LGTM, pending the possible change to execute at startup.
@@ -185,5 +185,6 @@ var ( | |||
"/universerpc.Universe/AssetLeaves": {}, | |||
"/universerpc.Universe/QueryProof": {}, | |||
"/universerpc.Universe/InsertProof": {}, | |||
"/universerpc.Universe/Info": {}, |
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.
Hmm, do we actually want people to be able to hit this?
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.
If we want this simple check to work, then yes, we need users to be able to connect directly without a macaroon and query this info. That's why I chose not to include any sensitive information. Maybe we even need to remove the two counts we have as those issue DB queries. If that is a concern and we just want to use this for the ID, maybe we could rename the RPC to just ServerID
?
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.
Maybe we even need to remove the two counts we have as those issue DB queries.
In theory users can already obtain this information if the universe server is open, albeit with even more DB queries.
rpcserver.go
Outdated
"server %v: %w", server.HostStr(), err) | ||
} | ||
|
||
if info.RuntimeId == r.runtimeID { |
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.
Hmm, yeah seems simple enough, wondering if there're other alternatives we should consider 🤔
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.
Also fwiw, this isn't the call path that would lead to us adding ourselves, instead it's here:
taproot-assets/tapcfg/server.go
Lines 180 to 201 in 7ab8a57
federationMembers := cfg.Universe.FederationServers | |
switch cfg.ChainConf.Network { | |
case "testnet": | |
cfgLogger.Infof("Configuring %v as initial Universe "+ | |
"federation server", defaultTestnetFederationServer) | |
federationMembers = append( | |
federationMembers, defaultTestnetFederationServer, | |
) | |
} | |
universeFederation := universe.NewFederationEnvoy( | |
universe.FederationConfig{ | |
FederationDB: federationDB, | |
UniverseSyncer: universeSyncer, | |
LocalRegistrar: baseUni, | |
SyncInterval: cfg.Universe.SyncInterval, | |
NewRemoteRegistrar: tap.NewRpcUniverseRegistar, | |
StaticFederationMembers: federationMembers, | |
ErrChan: mainErrChan, | |
}, | |
) |
https://github.com/lightninglabs/taproot-assets/blob/main/universe/auto_syncer.go#L105-L118
This'll actually end up skipping this RPC path.
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.
wondering if there're other alternatives we should consider
I thought about it a bit and I think it's not trivial to find out if we are connecting to ourselves just from the URL/domain name. Because what we have configured as our RPC listener port might be completely different from the domain name/port we are given to connect to (due to load balancers and other things inbetween). So I thought this was the simplest approach.
But I'm open to other approaches.
Also fwiw, this isn't the call path that would lead to us adding ourselves, instead it's here:
Yeah, I somehow forgot about that path before. Fixed now.
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.
But I'm open to other approaches.
Hmm, good point about the ports, etc....not sure there's a good option other than this. Only other thing can think of is extending to also support using brontide
over gRPC, then we'd have the public key we can check against.
Also if we want a bit more assurance that we won't have to worry about collisions here, then we can sample a [32]byte
with the CSPRNG. I think we can run with this for now, then circle back to see if was have false positives in the wild (a bit of extra logging can help us detect that).
In terms of gaming, worst thing I can think of is someone sort of bans a server from connecting to them as they mimic their runtime ID, but then they can already to that by banning the IP itself.
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.
Wouldn't we also need to know the public key of the server when using brontide
? Or is there a variant of the Noise protocol where you learn the other side's long-term keys without also doing SPAKE?
I don't worry about collision too much. If there was a collision between my local daemon and the universe, I would just restart my daemon and the collision would be gone.
And I agree about banning, I don't think we introduce any new assumptions regarding that with this approach.
eb02379
to
7859756
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.
LGTM! No strong feeling about if we should leave the DB calls in.
7859756
to
3bd9bc3
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.
LGTM 🥬
Needs a rebase! |
3bd9bc3
to
aca9702
Compare
Fixes #350.