Gitlw/acl over tls

[gitlw]

• Refactoring TLS code to allow a client to connect to the server over TLS without providing a certificate (one direction authentication). Now the logic for server side TLS set up and client side TLS set up are separated.
• Removed the logic that supports automatic reloading of server cert files (since it seems to complicate the code a lot, and the feature is probably not very important)

---

This change is 

[srfrog]

Reviewed 20 of 21 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

[codexnull]

Reviewed 18 of 21 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gitlw and @manishrjain)

---

tlstest/acl/acl_over_tls_test.go, line 100 at r3 (raw file):

	conf.Set("tls_cacert", "./tls/ca.crt")
	conf.Set("tls_server_name", "node")
	conf.Set("tls_server_name", "node")

Is this line supposed to be exactly like the one before it?

---

tlstest/acl/docker-compose.yml, line 40 at r3 (raw file):

      target: /gobin
      read_only: true
    command: /gobin/dgraph zero -o 0 --idx=1 --my=zero1:5080 --replicas=3 --logtostderr

Does the --replicas=3 do anything here since there's only one alpha?

[manishrjain]

@martinmr can proxy me as the reviewer. Once @codexnull tests this out and @martinmr reviews it -- feel free to merge.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gitlw and @manishrjain)

[martinmr]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @gitlw)

---

tlstest/acl/acl_over_tls_test.go, line 1 at r3 (raw file):

package acl

This file name is suffixed with _test but there are not any tests here. Maybe name it acl_over_tls_example.go?

---

tlstest/acl/acl_over_tls_test.go, line 35 at r3 (raw file):

		}
		if !pool.AppendCertsFromPEM(caFile) {
			return nil, fmt.Errorf("Error reading CA file '%s'.\n%s", certPath, err)

Don't add new lines in error messages. Also, if we reach this point err should be nil so why include it in the message?

---

tlstest/acl/acl_over_tls_test.go, line 110 at r3 (raw file):

	}

	// Output:

what's this comment doing here?

---

tlstest/acl/docker-compose.yml, line 42 at r3 (raw file):

    command: /gobin/dgraph zero -o 0 --idx=1 --my=zero1:5080 --replicas=3 --logtostderr
      -v=2 --bindall
volumes: {}

Is this line needed at all?

Same comment for all the other docker-compose files.

---

x/tls_helper.go, line 83 at r3 (raw file):

	// plaintext. However the client cert files are optional, depending on whether the server is
	// requiring a client certificate.

s/is requiring/requires

[codexnull]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @gitlw and @martinmr)

---

tlstest/acl/docker-compose.yml, line 42 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Is this line needed at all?

Same comment for all the other docker-compose files.

It's not needed but if these were generated with the compose tool, there is no way to tell yaml.Marshal() to omit empty lists or maps.

[gitlw]

Reviewable status: 6 of 23 files reviewed, 7 unresolved discussions (waiting on @codexnull, @martinmr, and @srfrog)

---

tlstest/acl/docker-compose.yml, line 40 at r3 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

Does the --replicas=3 do anything here since there's only one alpha?

Done.

---

tlstest/acl/docker-compose.yml, line 42 at r3 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

It's not needed but if these were generated with the compose tool, there is no way to tell yaml.Marshal() to omit empty lists or maps.

Done. Per Javier's comment.

---

x/tls_helper.go, line 83 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	// plaintext. However the client cert files are optional, depending on whether the server is
	// requiring a client certificate.

s/is requiring/requires

Done.

---

tlstest/acl/acl_over_tls_test.go, line 1 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

This file name is suffixed with _test but there are not any tests here. Maybe name it acl_over_tls_example.go?

Done.

---

tlstest/acl/acl_over_tls_test.go, line 35 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Don't add new lines in error messages. Also, if we reach this point err should be nil so why include it in the message?

Done.

---

tlstest/acl/acl_over_tls_test.go, line 100 at r3 (raw file):

Previously, codexnull (Javier Alvarado) wrote…

Is this line supposed to be exactly like the one before it?

Done. Removed the duplicate line.

---

tlstest/acl/acl_over_tls_test.go, line 110 at r3 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

what's this comment doing here?

Without this comment, the example code won't run with go test

[gitlw]

Reviewable status: 6 of 23 files reviewed, 7 unresolved discussions (waiting on @codexnull, @martinmr, and @srfrog)

---

tlstest/acl/acl_over_tls_test.go, line 1 at r3 (raw file):

Previously, gitlw (Lucas Wang) wrote…

Done.

I changed the file name back to *_test.go because otherwise go test won't be able to run the test.

[codexnull]

Reviewed 1 of 1 files at r2, 17 of 23 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @martinmr)