-
Notifications
You must be signed in to change notification settings - Fork 40
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: Add support for tls #885
Conversation
c6e0cf4
to
b89be50
Compare
Codecov Report
@@ 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
|
7fdc164
to
a2d8692
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
suggestion: open a new issue for the usage of existing certificates, i.e. not self-signed or auto |
|
||
func newHTTPRedirServer(m *autocert.Manager) *Server { | ||
srv := &Server{ | ||
Server: http.Server{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: add comment describing these magic constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
// ID of the server node. | ||
peerID string | ||
// a non nil value means the server should run with TLS | ||
tls *tlsOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol...
Learning from my peers ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could always use the client.Option
struct. Stuff like this is a large part of it's reason to be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider both internal and external developers when I think about shooting one's self in the foot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. But how does the optional type help with mutability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a slight preference for client.Option
mostly due to the non-mutability "guarantee".
|
||
func newHTTPRedirServer(m *autocert.Manager) *Server { | ||
srv := &Server{ | ||
Server: http.Server{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
77e327d
to
175b642
Compare
Gave it my approval, everything else is subjective if you want to change or not 👍 |
175b642
to
99fe2f2
Compare
s.options.tls = newTLS() | ||
} | ||
s.options.tls.port = fmt.Sprintf(":%d", port) | ||
tlsOpt := s.options.tls.Value() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if port == 0
it will automatically be assigned a random port :)
README.md
Outdated
|
||
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` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: specify /
at end of path to indicate it's a directory, i.e. ~/.defradb/certs/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
@@ -122,6 +122,17 @@ clean: | |||
clean\:test: | |||
go clean -testcache | |||
|
|||
.PHONY: tls-certs | |||
tls-certs: | |||
ifeq ($(path),) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idea: by default, generate in ~/.defradb/certs/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alternatively, in the README, indicate to use make tls-certs ~/.defradb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second option is better otherwise we start getting into OS specific directory structure.
99fe2f2
to
f944818
Compare
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.
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
How has this been tested?
Deployment to remote server with a valid domain name.
Specify the platform(s) on which this was tested: