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

Whitelist feature for wallet #135

Closed
wants to merge 2 commits into from

Conversation

WhoSoup
Copy link
Member

@WhoSoup WhoSoup commented Jan 28, 2020

This implements a simple whitelist feature for wsapi. It's enabled by starting the webserver with RPCConfig.WalletWhiteListEnable = true. IPs to whitelist can be added with wsapi.WhiteListIP(string).

Localhost (127.0.0.1, ::1) will always be able to connect. If a connection is attempted from a remote address not on the white list, the attempt is logged in the console and a 401 error is returned.

@WhoSoup WhoSoup added this to In progress in go.mod release (v0.4) via automation Aug 13, 2020
@WhoSoup WhoSoup mentioned this pull request Aug 13, 2020
@PaulBernier
Copy link
Contributor

Should the wallet API be concerned with networking security? I'd rather see that handled at another layer, which are much better suited for the task : firewalls, network ACLs/hardware, reverse proxy etc. which will be much more powerful and configurable. I am not convinced the library should be concerned by those things and would advocate for separation of concern here.

@WhoSoup
Copy link
Member Author

WhoSoup commented Aug 17, 2020

Should the wallet API be concerned with networking security?

That's a fair point. The motivation for this was the knowledge of how many people are running unsecured wallets and the general technical level of people who run wallets. Yes, real security would be preferable but unfortunately this won't be the case in many situations.

This was a feature combination with FactomProject/factom-walletd#84 to disallow remote connections by default and then enable specific ones if needed.

FactomdRPCPassword string
FactomdServer string
FactomdTimeout time.Duration
WalletWhiteListEnable bool
Copy link
Contributor

Choose a reason for hiding this comment

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

whitelist is a single word and should be camel cased Whitelist (like you did in other places actually)

@PaulBernier
Copy link
Contributor

Should the wallet API be concerned with networking security?

That's a fair point. The motivation for this was the knowledge of how many people are running unsecured wallets and the general technical level of people who run wallets. Yes, real security would be preferable but unfortunately this won't be the case in many situations.

This was a feature combination with FactomProject/factom-walletd#84 to disallow remote connections by default and then enable specific ones if needed.

Right, open wallets in the wild has been a plague... ok we can do it I guess, but we will need to document that properly to avoid people questions about why they can't call their wallet.

@PaulBernier PaulBernier changed the base branch from master to develop August 19, 2020 03:40
@PaulBernier PaulBernier moved this from In progress to In Review in go.mod release (v0.4) Aug 19, 2020
@PaulBernier
Copy link
Contributor

@WhoSoup does this PR still makes sense with the wallet stuff gone?

@WhoSoup
Copy link
Member Author

WhoSoup commented Aug 20, 2020

@WhoSoup does this PR still makes sense with the wallet stuff gone?

Nope, this would have to be re-done at the new location. I'll close this and make a note in the issue.

@WhoSoup WhoSoup closed this Aug 20, 2020
go.mod release (v0.4) automation moved this from In Review to Done Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants