feat: Add support for tls #885

fredcarle · 2022-10-07T21:16:01Z

Relevant issue(s)

Resolves #418

Description

This PR adds the support for TLS configuration of the HTTP API. It offers both self signed certificate and auto certificate support.

Note while in draft: I need to add unit tests for the new functions in the api/http package. I don't think I will be adding integration test as it would be cumbersome to test the self sign certificates and almost impossible to test the auto certificate feature.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Deployment to remote server with a valid domain name.

Specify the platform(s) on which this was tested:

MacOS

codecov · 2022-10-07T21:23:56Z

Codecov Report

Merging #885 (f944818) into develop (42b7c0e) will increase coverage by 0.27%.
The diff coverage is 85.71%.

@@             Coverage Diff             @@
##           develop     #885      +/-   ##
===========================================
+ Coverage    59.80%   60.08%   +0.27%     
===========================================
  Files          159      159              
  Lines        17335    17523     +188     
===========================================
+ Hits         10368    10528     +160     
- Misses        6041     6061      +20     
- Partials       926      934       +8

Impacted Files	Coverage Δ
config/configfile.go	`75.00% <ø> (ø)`
cli/start.go	`44.40% <52.72%> (+1.99%)`	⬆️
config/config.go	`79.31% <78.57%> (-0.37%)`	⬇️
api/http/handler.go	`100.00% <100.00%> (ø)`
api/http/server.go	`100.00% <100.00%> (ø)`
node/node.go	`66.38% <100.00%> (+0.28%)`	⬆️

AndrewSisley

LGTM, couple of small comments but nothing major. Is mostly config (including the in go code), and I am not terribly knowledgeable here, so am largely trusting that that aspect is all good and sensible (might be good to wait on one of the others to approve too for this reason, as I probably wont be able to spot anything silly here)

api/http/server.go

AndrewSisley

LGTM, thanks - but still maybe get an approval from someone else (can do post merge IMO if this is blocking other tasks)

fredcarle · 2022-10-12T18:19:30Z

LGTM, thanks - but still maybe get an approval from someone else (can do post merge IMO if this is blocking other tasks)

I'll wait for @jsimnz to review as he seemed to have opinions about the PR.

api/http/server.go

orpheuslummis · 2022-10-17T16:30:20Z

suggestion: open a new issue for the usage of existing certificates, i.e. not self-signed or auto

orpheuslummis · 2022-10-17T16:31:45Z

api/http/server.go

+
+func newHTTPRedirServer(m *autocert.Manager) *Server {
+	srv := &Server{
+		Server: http.Server{


suggestion: add comment describing these magic constants

I've put random numbers in a hat and pulled 3 different ones.

JK! https://datatracker.ietf.org/doc/id/draft-thomson-hybi-http-timeout-00.html#rfc.section.1.1

They are durations that I've seen used with other API's but are in now way final. Meaning that they should evolve over time as we figure out what kind of durations are appropriate for our use case. The goal of these is to limit abuses.

config/configfile.go

jsimnz

Some minor changes, but overall looks great!

One additional suggestion that didnt make sense to put anywhere inline in the code: Add a short blurb/section in the readme on how to setup a TLS enabled version of the server. Both local cert gen, and autocert :)

api/http/server.go

jsimnz · 2022-10-18T12:11:40Z

api/http/server.go

+	// ID of the server node.
+	peerID string
+	// a non nil value means the server should run with TLS
+	tls *tlsOptions


suggestion(non-blocking): Not a huge fan of pointers for optional config structs, but I do like that the options are bundles under a struct. Maybe use an enabled bool field on the tlsOptions struct, which default value will be false.

A non blocking suggestion like this leads me to believe that this would be a personal preference. Can you elaborate as to why you prefer not to use a pointer?

lol...

Learning from my peers ;)

You could always use the client.Option struct. Stuff like this is a large part of it's reason to be

I don't understand the "unnecessary mutability" here since it's a private field in an private struct. The Optional type wouldn't improve anything in this situation.

I consider both internal and external developers when I think about shooting one's self in the foot

Sure. But how does the optional type help with mutability?

the tlsoptions field being a pointer, means it has the option of being mutable, just be the sheer fact that its a pointer. A developer making changes (internal) to the codebase, may accidentally misunderstand / violate the expected immutable properties that a config option should have.

I like using pointers in Go when were doing it on a type that is already expected to be mutable. But configs should really only be set once as much as possible, and be very cognisant when its being mutated, because there can be misunderstandings on the behavior of mutating configs.

I just dont want to add further confusion to that behavior/relation by having a pointer in a config struct.

I have a slight preference for client.Option mostly due to the non-mutability "guarantee".

jsimnz · 2022-10-18T12:12:36Z

api/http/server.go

+
+func newHTTPRedirServer(m *autocert.Manager) *Server {
+	srv := &Server{
+		Server: http.Server{


api/http/server.go

config/config.go

jsimnz · 2022-10-21T06:55:05Z

Gave it my approval, everything else is subjective if you want to change or not 👍

shahzadlone · 2022-10-25T05:59:37Z

api/http/server.go

-			s.options.tls = newTLS()
-		}
-		s.options.tls.port = fmt.Sprintf(":%d", port)
+		tlsOpt := s.options.tls.Value()


question: Do we want to also check if port == 0, if it is then assign the port to be a random port? or is that handled elsewhere?

if port == 0 it will automatically be assigned a random port :)

orpheuslummis · 2022-10-20T11:47:03Z

README.md

+
+The keys can be generated with your generator of choice or with `make tls-certs`.
+
+Note that the keys should be saved in `~/.defradb/certs`


suggestion: specify / at end of path to indicate it's a directory, i.e. ~/.defradb/certs/

orpheuslummis · 2022-10-20T11:47:33Z

Makefile

@@ -122,6 +122,17 @@ clean:
 clean\:test:
 	go clean -testcache

+.PHONY: tls-certs
+tls-certs:
+ifeq ($(path),)


idea: by default, generate in ~/.defradb/certs/

alternatively, in the README, indicate to use make tls-certs ~/.defradb

I think the second option is better otherwise we start getting into OS specific directory structure.

Relevant issue(s) Resolves sourcenetwork#418 Description This PR adds the support for TLS configuration of the HTTP API. It offers both self signed certificate and auto certificate support. Note while in draft: I need to add unit tests for the new functions in the api/http package. I don't think I will be adding integration test as it would be cumbersome to test the self sign certificates and almost impossible to test the auto certificate feature.

fredcarle added feature New feature or request area/api Related to the external API component labels Oct 7, 2022

fredcarle added this to the DefraDB v0.4 milestone Oct 7, 2022

fredcarle requested a review from a team October 7, 2022 21:16

fredcarle self-assigned this Oct 7, 2022

fredcarle changed the title ~~feat: Add support for TLS~~ feat: Add support for tls Oct 7, 2022

fredcarle force-pushed the fredcarle/feat/I418-tls-support branch from c6e0cf4 to b89be50 Compare October 7, 2022 21:19

fredcarle force-pushed the fredcarle/feat/I418-tls-support branch from 7fdc164 to a2d8692 Compare October 12, 2022 15:23

fredcarle marked this pull request as ready for review October 12, 2022 15:23

AndrewSisley requested changes Oct 12, 2022

View reviewed changes

api/http/server.go Outdated Show resolved Hide resolved

api/http/server.go Outdated Show resolved Hide resolved

api/http/server.go Outdated Show resolved Hide resolved

fredcarle added the action/no-benchmark Skips the action that runs the benchmark. label Oct 12, 2022

sourcenetwork deleted a comment from source-devs Oct 12, 2022

AndrewSisley approved these changes Oct 12, 2022

View reviewed changes

orpheuslummis reviewed Oct 17, 2022

View reviewed changes

api/http/server.go Outdated Show resolved Hide resolved

orpheuslummis reviewed Oct 17, 2022

View reviewed changes

config/configfile.go Outdated Show resolved Hide resolved

jsimnz requested changes Oct 18, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/feat/I418-tls-support branch 3 times, most recently from 77e327d to 175b642 Compare October 19, 2022 18:37

jsimnz approved these changes Oct 21, 2022

View reviewed changes

shahzadlone approved these changes Oct 21, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/feat/I418-tls-support branch from 175b642 to 99fe2f2 Compare October 24, 2022 02:09

shahzadlone reviewed Oct 25, 2022

View reviewed changes

orpheuslummis reviewed Oct 28, 2022

View reviewed changes

fredcarle added 17 commits October 28, 2022 23:02

add tls support

17329f8

improve tls config

49c4e46

add unit tests for tls

1580f5d

fix lint error

44ad9ad

fix restricted port error in test

53a470a

change to random port assignment

8bf2ade

apply feedback

476ce87

apply feedback

9558862

fix possible conflict of port for net related tests

f87baf6

fix bad merge

9c8c781

fix lint error

f4c5cd0

remove deadcode

2e4be50

add address flag

b22bf2c

add readme section on tls

6a9dc20

fix rebase conflict

3652379

change tls to optional type

94194dc

apply feedback to readme file

f944818

fredcarle force-pushed the fredcarle/feat/I418-tls-support branch from 99fe2f2 to f944818 Compare October 29, 2022 03:02

fredcarle merged commit b847465 into develop Oct 29, 2022

fredcarle deleted the fredcarle/feat/I418-tls-support branch October 29, 2022 03:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for tls #885

feat: Add support for tls #885

fredcarle commented Oct 7, 2022

codecov bot commented Oct 7, 2022 •

edited

Loading

AndrewSisley left a comment

AndrewSisley left a comment

fredcarle commented Oct 12, 2022

orpheuslummis commented Oct 17, 2022

orpheuslummis Oct 17, 2022

jsimnz Oct 18, 2022

fredcarle Oct 18, 2022

jsimnz left a comment

jsimnz Oct 18, 2022

fredcarle Oct 18, 2022

AndrewSisley Oct 18, 2022

fredcarle Oct 18, 2022

AndrewSisley Oct 18, 2022

fredcarle Oct 19, 2022

jsimnz Oct 19, 2022

fredcarle Oct 19, 2022

jsimnz Oct 21, 2022

shahzadlone Oct 21, 2022

jsimnz Oct 18, 2022

jsimnz commented Oct 21, 2022

shahzadlone Oct 25, 2022

fredcarle Oct 25, 2022

orpheuslummis Oct 20, 2022

fredcarle Oct 29, 2022

orpheuslummis Oct 20, 2022

orpheuslummis Oct 20, 2022

fredcarle Oct 29, 2022


		The keys can be generated with your generator of choice or with `make tls-certs`.

		Note that the keys should be saved in `~/.defradb/certs`

feat: Add support for tls #885

feat: Add support for tls #885

Conversation

fredcarle commented Oct 7, 2022

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Oct 7, 2022 • edited Loading

Codecov Report

AndrewSisley left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

fredcarle commented Oct 12, 2022

orpheuslummis commented Oct 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 7, 2022 •

edited

Loading