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

universe: add server info RPC, check we're not connecting to ourselves #364

Merged
merged 6 commits into from
Jun 22, 2023

Conversation

guggero
Copy link
Member

@guggero guggero commented Jun 15, 2023

Fixes #350.

Copy link
Collaborator

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

taprpc/universerpc/universe.proto Outdated Show resolved Hide resolved
rpcserver.go Show resolved Hide resolved
@@ -185,5 +185,6 @@ var (
"/universerpc.Universe/AssetLeaves": {},
"/universerpc.Universe/QueryProof": {},
"/universerpc.Universe/InsertProof": {},
"/universerpc.Universe/Info": {},
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

universe_rpc_registrar.go Show resolved Hide resolved
rpcserver.go Outdated
"server %v: %w", server.HostStr(), err)
}

if info.RuntimeId == r.runtimeID {
Copy link
Member

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 🤔

Copy link
Member

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:

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Collaborator

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

rpcserver.go Outdated Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🥬

@Roasbeef
Copy link
Member

Needs a rebase!

@jharveyb jharveyb added this pull request to the merge queue Jun 22, 2023
Merged via the queue into main with commit 2891eaf Jun 22, 2023
11 checks passed
@guggero guggero deleted the universe-server-info branch June 22, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[universes] add validation to prevent universe from adding federation connection with itself / loopback
4 participants