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(web): Support TLS encryption #322

Merged
merged 19 commits into from
Apr 22, 2023
Merged

feat(web): Support TLS encryption #322

merged 19 commits into from
Apr 22, 2023

Conversation

chr1st1ank
Copy link
Contributor

@chr1st1ank chr1st1ank commented Aug 27, 2022

Summary

Implementing #253 (HTTPS/TLS support).

This is to run the server on HTTPS instead of plain HTTP.

Two considerations are worth mentioning:

  1. The options are just the minimum to have TLS. There are many more options and one could add a full section for configuration of TLS. (See e.g. here: https://github.com/PowerDNS/go-tlsconfig/blob/master/config.go). But for my purposes the simple setup is more than enough. If people want more options, they can also use a reverse proxy or load balancer.
  2. At first I wanted to add tests. But then I saw that the serving part of the controller is already untested now. I would have introduced a lot of additional complexity into the testing suite (create certificates, actually run the server, run queries against it) with possibly additional dependencies for creating the certificate. If we instead add hardcoded test certificates, most security scanners would remark that there are credentials in the repo. And all of this to test just a few calls into the standard library. So I'd like to ask first whether such a test should be added or rather be left out.

How to test it:

Create a certificate pair:

openssl ecparam -genkey -name secp384r1 -out server.key
openssl req -new -x509 -sha256 -key server.key -out server.crt -days 3650

Configure gatus to use the keypair in the config.yaml:

web:
  port: 4443
  cert_file: "server.crt"
  key_file: "server.key"
endpoints:
  - name: example
    url: https://example.org
    interval: 30s
    conditions:
      - "[STATUS] == 200"

Run gatus.

Checklist

  • Tested and/or added tests to validate that the changes work as intended, if applicable. (I tested it locally)
  • Added the documentation in README.md, if applicable.

@chr1st1ank
Copy link
Contributor Author

@TwiN Any chance of including this? I tried to make it as consistent as possible with the existing code

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
controller/controller.go Outdated Show resolved Hide resolved
config/web/web.go Outdated Show resolved Hide resolved
@TwiN TwiN added the feature New feature or request label Sep 6, 2022
@TwiN TwiN linked an issue Sep 6, 2022 that may be closed by this pull request
@TwiN TwiN changed the title Support for TLS encryption feat(web): Support TLS encryption Sep 6, 2022
Copy link
Owner

@TwiN TwiN left a comment

Choose a reason for hiding this comment

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

Could you add some tests?

README.md Outdated Show resolved Hide resolved
@chr1st1ank
Copy link
Contributor Author

chr1st1ank commented Sep 21, 2022

Could you add some tests?

It isn't straight-forward with the certificates. What way would you prefer:

  • Add hardcoded test certificates to the repository (which is simple, but linters ususally don't like it, e.g. see here: /test/test.key tornadoweb/tornado#3107)
  • Generate a self-signed certificate on the fly with Go (which might need additional dependencies?). Here is an example of such code: https://go.dev/src/crypto/tls/generate_cert.go
  • Generate a self-signed certificate locally on the dev environment & on github actions before running the test suite

@TwiN
Copy link
Owner

TwiN commented Sep 26, 2022

Generate a self-signed certificate on the fly with Go (which might need additional dependencies?). Here is an example of such code: https://go.dev/src/crypto/tls/generate_cert.go

I personally prefer this one.

Based on what I can see in the example you linked, the dependency is only used for building anyways:

//go:build ignore

@chr1st1ank
Copy link
Contributor Author

Alright. Now the code is almost fully tested with certificates which are generated on the fly.
It was actually not as complicated as I expected. I didn't know that go has all the necessary tools already in the standard library.

I'm looking forward to see the CI do its job. On my machine two tests in "client" and "core" are failing. But they don't seem to have any connection with my changes.

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2022

Codecov Report

Base: 82.80% // Head: 82.72% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (66d885d) compared to base (be88af5).
Patch coverage: 65.21% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #322      +/-   ##
==========================================
- Coverage   82.80%   82.72%   -0.09%     
==========================================
  Files          54       54              
  Lines        3902     3924      +22     
==========================================
+ Hits         3231     3246      +15     
- Misses        521      527       +6     
- Partials      150      151       +1     
Impacted Files Coverage Δ
controller/controller.go 82.75% <37.50%> (-12.70%) ⬇️
config/web/web.go 90.00% <80.00%> (-10.00%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -21,6 +22,21 @@ type Config struct {

// Port to listen on (default to 8080 specified by DefaultPort)
Port int `yaml:"port"`

// TLS configuration
Tls TlsConfig `yaml:"tls"`
Copy link
Owner

Choose a reason for hiding this comment

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

This should be TLS

tlsConfigError error
}

type TlsConfig struct {
Copy link
Owner

Choose a reason for hiding this comment

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

This should be TLSConfig.

Comment on lines 36 to 39
CertFile string `yaml:"certificate-file,omitempty"`

// Optional private key file for TLS in PEM format.
KeyFile string `yaml:"private-key-file,omitempty"`
Copy link
Owner

Choose a reason for hiding this comment

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

Can you rename these to CertificateFile and PrivateKeyFile respectively?

Tls TlsConfig `yaml:"tls"`

tlsConfig *tls.Config
tlsConfigError error
Copy link
Owner

Choose a reason for hiding this comment

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

Why are we persisting the error in the struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See below

config/web/web.go Outdated Show resolved Hide resolved
config/web/web.go Show resolved Hide resolved
@chr1st1ank
Copy link
Contributor Author

@TwiN I added tests and updated from master again.

@gaby
Copy link

gaby commented Apr 18, 2023

@chr1st1ank @TwiN Any updates on this? I would rather have TLS directly with gatus than adding NGINX

@gaby
Copy link

gaby commented Apr 18, 2023

@chr1st1ank I noticed there's no support for ClientCert auth, which is very useful for enterprise environments.

It basically requires adding a field for a CA, and adding a boolean option to verify that the incoming certs are signed by the same CA as the server.

This could probably be added later.

@chr1st1ank
Copy link
Contributor Author

I can resolve the conflicts to master and make it mergeable again, but only if there is an interest for this PR. @gaby the client certificates I'd really rather take separately

@TwiN
Copy link
Owner

TwiN commented Apr 19, 2023

@chr1st1ank If you can resolve the conflicts, I'll merge it.
Sorry for the trouble 😔

@chr1st1ank
Copy link
Contributor Author

@chr1st1ank If you can resolve the conflicts, I'll merge it. Sorry for the trouble pensive

Alright, no problem. I've merged master into my branch again and tested it locally. Looks ready to go unless you find any remaining flaws.

@chr1st1ank chr1st1ank requested a review from TwiN April 19, 2023 16:21
controller/controller.go Outdated Show resolved Hide resolved
@TwiN TwiN merged commit a05daed into TwiN:master Apr 22, 2023
@TwiN
Copy link
Owner

TwiN commented Apr 22, 2023

@chr1st1ank Thank you so much for your contribution (and patience 😂), I appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(web): HTTPS/TLS support
4 participants