Skip to content

Added IP whitelist for admin/export #2233

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

Merged
merged 4 commits into from
Apr 15, 2018

Conversation

armcburney
Copy link
Contributor

@armcburney armcburney commented Mar 15, 2018

#1924 Added an IP whitelist for admin/export.

Background

This is my first time contributing to dgraph. I'm new to Go, but I'm interested in distributed systems, keen to learn, and would like to make more contributions in the future.

Usage

Configuration

# Start a zero
dgraph zero --port_offset 2000

# Start a server, passing in a list of whitelist ranges
dgraph server --memory_mb 1048 \
              --zero localhost:7080 \
              --my localhost:7085 \
              --port_offset 5 \
              --whitelist 144.142.126.222:144.142.126.600,97.116.72.68:97.116.72.99

Example

# Curl some remote IP, on some port
curl 144.142.126.222:8085/admin/export

Testing

I'm new to the project, so any advice on how to best test my new changes would be greatly appreciated. Thank you! 🙂


This change is Reviewable

@manishrjain
Copy link
Contributor

Thanks for your contribution, @AndrewMcBurney! I'd let @pawanrawal have a look at your change, and get back to you.

Just as a general observation, it might make sense to allow ranges of IPs for whitelisting.

@pawanrawal
Copy link
Contributor

Look like a change well-done @andrewmcburney! I have some comments. I will have another look when they are resolved.

Will this work for IPv6?
Well, you can test it using Docker. IIRC, Docker assigns container IPs from a range.


Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.


dgraph/cmd/server/admin.go, line 125 at r2 (raw file):

func ipInIPWhitelistRanges(ipString string) bool {
	ip := net.ParseIP(ipString)

Check for ip == nil.


dgraph/cmd/server/run.go, line 83 at r2 (raw file):

	flag.String("whitelist", defaults.WhitelistedIPs,
		"A comma separated list of ips ranges you wish to whitelist "+

...to whitelist for performing admin actions


edgraph/config.go, line 136 at r2 (raw file):

	worker.Config.ExpandEdge = Config.ExpandEdge

	ips, _ := parseIPsFromString(Config.WhitelistedIPs)

Can we handle the error here?


edgraph/config.go, line 191 at r2 (raw file):

			)
		} else if bytes.Compare(lowerBoundIP, upperBoundIP) > 0 {
			// Assert that both lower bound is less than the upper bound

both lower bound?


Comments from Reviewable

…ges`.

Updated `--whitelist` flag documentation.

Handle error in `parseIPsFromString` when setting worker configuration values.

Fixed documentation grammar.
@pawanrawal
Copy link
Contributor

@andrewmcburney are you still working/testing this or is it ready to be merged?


Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions.


Comments from Reviewable

@armcburney armcburney changed the title [WIP] Added IP whitelist for admin/export Added IP whitelist for admin/export Mar 19, 2018
@pawanrawal
Copy link
Contributor

Ping @AndrewMcBurney. Do you need any help to get this in?

@armcburney
Copy link
Contributor Author

Hi @pawanrawal, my apologies -I've been busy with a few things on my end. I'll resolve the conflicts, and let you know once it's good for a review. Thank you for your patience!

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2018

CLA assistant check
All committers have signed the CLA.

@pawanrawal
Copy link
Contributor

Merging this. I will quickly test this before doing the next release.

@pawanrawal pawanrawal merged commit 4cd30cb into hypermodeinc:master Apr 15, 2018
@armcburney armcburney deleted the whitelist branch April 15, 2018 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants