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

multi: add CLI flag to enable public access to uni proof courier RPCs #499

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Sep 11, 2023

This PR removes the universe proof courier RPC endpoints (QueryProof and InsertProof) from the default macaroon whitelist and adds a CLI flag to enable them. Which is the same method used for the universe stats RPC endpoints.

This PR also removes a config param called AcceptRemoteUniverseProofs now that we shift access control to macaroons.

@ffranr ffranr self-assigned this Sep 12, 2023
@ffranr ffranr force-pushed the public-rpc-multiverse-proof-courier branch from ef592c1 to 13de95a Compare September 12, 2023 12:25
@Roasbeef
Copy link
Member

I think this might already be covered by this flag? https://github.com/lightninglabs/taproot-assets/blob/main/config.go#L84

Or is this where we wan to do things like: you can send issuance proofs, but not normal transfer proofs?

@ffranr
Copy link
Contributor Author

ffranr commented Sep 14, 2023

@Roasbeef I now notice that the universe RPCs are whitelisted by default. I missed that before.

	// For now, these are the Universe related read/write methods. We permit
	// InsertProof as a valid proof requires an on-chain transaction, so we
	// gain a layer of DoS defense.
	defaultMacaroonWhitelist = map[string]struct{}{
		"/universerpc.Universe/AssetRoots":      {},
		"/universerpc.Universe/QueryAssetRoots": {},
		"/universerpc.Universe/AssetLeafKeys":   {},
		"/universerpc.Universe/AssetLeaves":     {},
		"/universerpc.Universe/QueryProof":      {},
		"/universerpc.Universe/InsertProof":     {},
		"/universerpc.Universe/Info":            {},
	}

This PR was supposed to be about whitelisting them via a CLI flag. I think that would be better rather than default whitelisted. Smaller attack surface for most nodes.

Thanks for pointing out the AcceptRemoteUniverseProofs config field. I hadn't spotted that. I think we should remove that flag and use the macaroons permissions whitelist for access control.

I'll update the PR to what I think we should do. Please let me know if you have any thoughts on this.

@dstadulis
Copy link
Collaborator

For security context. Comment from guggero:
the RPC port is not accessible by the network by default:

if len(cfg.RpcConf.RawRPCListeners) == 0 {

@ffranr ffranr force-pushed the public-rpc-multiverse-proof-courier branch from 13de95a to efa6c4a Compare September 19, 2023 13:29
@ffranr ffranr marked this pull request as ready for review September 19, 2023 13:32
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@guggero
Copy link
Member

guggero commented Sep 19, 2023

Itest is failing because of the flag, probably need to set it to true in the tapd harness.

This commit removes the `QueryProof` and `InsertProof` RPC endpoints
from the default macaroon whitelist. It also adds a CLI flag for
whitelisting those endpoints. This macaroon whitelisting method mirrors
that used for the universe stats endpoints.
@ffranr ffranr force-pushed the public-rpc-multiverse-proof-courier branch from aff1e21 to 0d72a61 Compare September 19, 2023 15:48
@guggero
Copy link
Member

guggero commented Sep 19, 2023

Looks like the linter also found a now unused variable, then CI should be green ✔️

We can remove this parameter and use the macaroon whitelist to control
this behaviour.
@ffranr ffranr force-pushed the public-rpc-multiverse-proof-courier branch from 0d72a61 to 3fe7b88 Compare September 19, 2023 16:31
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 Roasbeef merged commit 4ee966a into main Sep 20, 2023
14 checks passed
@guggero guggero deleted the public-rpc-multiverse-proof-courier branch September 20, 2023 07:29
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.

4 participants