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

feat(dgraph): enabling TLS config in http zero #6691

Merged
merged 12 commits into from
Oct 22, 2020

Conversation

aman-bansal
Copy link
Contributor

@aman-bansal aman-bansal commented Oct 9, 2020

Enabling TLS based encryption in Dgraph zero.
Fixes https://dgraph.atlassian.net/browse/DGRAPH-1366. Based on discussion here https://discuss.dgraph.io/t/enabling-tls-configuration-for-internal-ports-making-dgraph-more-secure/10860/2
The same variables are used which needs to be defined for alpha tls configuration

Things to consider:

  1. With TLS enabled, tls_enabled_route has to be defined.
  2. If defined all routes will be accessible with TLS. But ones mentioned in --tls_enabled_route will not be accessible via HTTP.

This change is Reviewable

Docs Preview: Dgraph Preview

@aman-bansal aman-bansal force-pushed the aman/enable_http_tls_zero branch from 4945380 to 5bc0812 Compare October 13, 2020 15:21
Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 15 rules errored during the review.

@aman-bansal aman-bansal marked this pull request as ready for review October 14, 2020 14:28
@aman-bansal aman-bansal force-pushed the aman/enable_http_tls_zero branch from 31437dc to 080704a Compare October 14, 2020 14:33
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 10 files reviewed, 2 unresolved discussions (waiting on @aman-bansal, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/zero/http.go, line 246 at r1 (raw file):

}

func (st *state) serveHTTP(l net.Listener) {

should be renamed as it serves both HTTP and HTTPS


dgraph/cmd/zero/http.go, line 278 at r1 (raw file):

func startServers(m cmux.CMux) {
	// if tls enabled http rule will check that the requested route should not be part of tls_enabled_routes

The naming needs to be fixed here.

@parasssh
Copy link
Contributor

-all tests for the combination of tls config (on/off) and enabled routes (0 , all, some).

  • bulk loader system test with and without TLS.
  • System test with alpha, zero and using Ratel. Full TLS enabled.

@aman-bansal aman-bansal force-pushed the aman/enable_http_tls_zero branch from 1544682 to 8e467c3 Compare October 19, 2020 12:38
@aman-bansal
Copy link
Contributor Author

aman-bansal commented Oct 20, 2020

Both bulk live or other clients connect with GRPC port of zero. This change doesn't affect any of those. We already have tests running to check those features.

For ratel, I have tested this locally. But couldn't find anything that tests end to end integration of zero, alpha, ratel.

@aman-bansal aman-bansal force-pushed the aman/enable_http_tls_zero branch from 8e467c3 to 93b2fd3 Compare October 20, 2020 15:42
@aman-bansal aman-bansal force-pushed the aman/enable_http_tls_zero branch from 93b2fd3 to a3398f4 Compare October 21, 2020 06:02
@aman-bansal
Copy link
Contributor Author

I changed the enable flag to disable because there are many endpoints like /debug which isn't initiated directly. We can introduce some side effects with negation logic. Also this align more with manish comment

Another thing to do would be to have a flag, which can tell the HTTP endpoints which can be plain-text. For e.g., “/health, /debug”, etc. That way, a very secure usage can keep them empty. But, selectively open some to easier `debugging.

@aman-bansal aman-bansal force-pushed the aman/enable_http_tls_zero branch from 5c3348d to a228342 Compare October 21, 2020 13:42
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 10 files at r1, 4 of 6 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aman-bansal, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/zero/run.go, line 97 at r3 (raw file):

	flag.Bool("tls_use_system_ca", true, "Include System CA into CA Certs.")
	flag.String("tls_client_auth", "VERIFYIFGIVEN", "Enable TLS client authentication")
	flag.String("tls_disabled_route", "", "comma separated zero endpoint which will disabled from TLS encryption."+

which will be ....


tlstest/zero_https/no_tls/no_tls_test.go, line 58 at r3 (raw file):

	defer func() { _ = do.Body.Close() }()
	body, err := ioutil.ReadAll(do.Body)
	if err != nil {

require.NoError(t, err)

@aman-bansal aman-bansal merged commit 5482c60 into master Oct 22, 2020
@aman-bansal aman-bansal deleted the aman/enable_http_tls_zero branch November 9, 2020 08:28
aman-bansal added a commit that referenced this pull request Nov 9, 2020
* enabling TLS config in http zero

* making zero https configured

* changing behaviour of cmux + adding test cases

* fixing zero address in test

* fixing docker files

* adding alpha in docker compose

* fixing test generate cert pool

* renaming functions based on review

* making zero https more vigilant with more checks

* changing the enabled to disabled flag

* fixing test case

* fixing zero cmd flag desc and refactoring test cases
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.

3 participants