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

EVEREST-1180 | TLS support #888

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
dfaa2d2
wip: tls support
mayankshah1607 Nov 30, 2024
e980bc0
Merge branch 'main' into EVEREST-1180
mayankshah1607 Dec 3, 2024
f465bd7
Merge branch 'main' into EVEREST-1180
mayankshah1607 Dec 3, 2024
ecde075
Merge branch 'main' into EVEREST-1180
mayankshah1607 Dec 3, 2024
0caa254
Merge branch 'main' into EVEREST-1180
mayankshah1607 Dec 4, 2024
5ef5aee
Merge branch 'main' into EVEREST-1180
mayankshah1607 Dec 9, 2024
6ad2b4d
Merge branch 'main' into EVEREST-1180
mayankshah1607 Dec 18, 2024
46fbf85
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 3, 2025
0615010
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 9, 2025
75d3aca
update comment
mayankshah1607 Jan 9, 2025
8dcec89
add ability to refresh tls certs
mayankshah1607 Jan 9, 2025
c6411e3
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 9, 2025
385a8c2
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 10, 2025
d6cafb5
update go mod
mayankshah1607 Jan 10, 2025
c3172ed
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 12, 2025
eee377a
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 13, 2025
02a1661
add back HTTPPort, deprecate it
mayankshah1607 Jan 13, 2025
9783325
update condition
mayankshah1607 Jan 13, 2025
15e9e06
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 13, 2025
beea433
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 14, 2025
1dc7fe6
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 15, 2025
8d6001b
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 24, 2025
327a819
add licence header
mayankshah1607 Jan 24, 2025
5607d40
return copy of certs
mayankshah1607 Jan 24, 2025
506d8eb
add filepath cleaning
mayankshah1607 Jan 24, 2025
62a3f51
Merge branch 'main' into EVEREST-1180
mayankshah1607 Jan 24, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions cmd/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package config

import (
"crypto/aes"
"path/filepath"

"github.com/kelseyhightower/envconfig"
)
Expand All @@ -39,9 +40,11 @@ var (

// EverestConfig stores the configuration for the application.
type EverestConfig struct {
DSN string `default:"postgres://admin:[email protected]:5432/postgres?sslmode=disable" envconfig:"DSN"`
HTTPPort int `default:"8080" envconfig:"HTTP_PORT"`
Verbose bool `default:"false" envconfig:"VERBOSE"`
DSN string `default:"postgres://admin:[email protected]:5432/postgres?sslmode=disable" envconfig:"DSN"`
// DEPRECATED: Use ListenPort instead.
HTTPPort int `envconfig:"HTTP_PORT"`
ListenPort int `default:"8080" envconfig:"PORT"`
Verbose bool `default:"false" envconfig:"VERBOSE"`
// TelemetryURL Everest telemetry endpoint.
TelemetryURL string `envconfig:"TELEMETRY_URL"`
// TelemetryInterval Everest telemetry sending frequency.
Expand All @@ -54,6 +57,9 @@ type EverestConfig struct {
CreateSessionRateLimit int `default:"1" envconfig:"CREATE_SESSION_RATE_LIMIT"`
// VersionServiceURL contains the URL of the version service.
VersionServiceURL string `default:"https://check.percona.com" envconfig:"VERSION_SERVICE_URL"`
// TLSCertsPath contains the path to the directory with the TLS certificates.
// Setting this will enable HTTPS on ListenPort.
TLSCertsPath string `envconfig:"TLS_CERTS_PATH"`
}

// ParseConfig parses env vars and fills EverestConfig.
Expand All @@ -71,5 +77,9 @@ func ParseConfig() (*EverestConfig, error) {
c.TelemetryInterval = TelemetryInterval
}

if c.TLSCertsPath != "" {
c.TLSCertsPath = filepath.Clean(c.TLSCertsPath)
}

return c, nil
}
2 changes: 1 addition & 1 deletion cmd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func main() {
}

go func() {
err := server.Start()
err := server.Start(tCtx)
if err != nil && !errors.Is(err, http.ErrServerClosed) {
l.Fatal(err)
}
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ require (
github.com/cenkalti/backoff v2.2.1+incompatible
github.com/cenkalti/backoff/v4 v4.3.0
github.com/fatih/color v1.18.0
github.com/fsnotify/fsnotify v1.7.0
github.com/getkin/kin-openapi v0.128.0
github.com/go-logr/zapr v1.3.0
github.com/golang-jwt/jwt/v5 v5.2.1
Expand Down
38 changes: 36 additions & 2 deletions internal/server/everest.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ package server

import (
"context"
"crypto/tls"
"encoding/json"
"errors"
"fmt"
"io/fs"
"net/http"
"path"
"slices"

"github.com/getkin/kin-openapi/openapi3filter"
Expand All @@ -42,6 +44,7 @@ import (
k8shandler "github.com/percona/everest/internal/server/handlers/k8s"
rbachandler "github.com/percona/everest/internal/server/handlers/rbac"
valhandler "github.com/percona/everest/internal/server/handlers/validation"
"github.com/percona/everest/pkg/certwatcher"
"github.com/percona/everest/pkg/common"
"github.com/percona/everest/pkg/kubernetes"
"github.com/percona/everest/pkg/oidc"
Expand All @@ -67,6 +70,11 @@ func NewEverestServer(ctx context.Context, c *config.EverestConfig, l *zap.Sugar
return nil, errors.Join(err, errors.New("failed creating Kubernetes client"))
}

if c.HTTPPort != 0 {
l.Warn("HTTP_PORT is deprecated, use PORT instead")
c.ListenPort = c.HTTPPort
}

echoServer := echo.New()
echoServer.Use(echomiddleware.RateLimiter(echomiddleware.NewRateLimiterMemoryStore(rate.Limit(c.APIRequestsRateLimit))))
middleware, store := sessionRateLimiter(c.CreateSessionRateLimit)
Expand Down Expand Up @@ -293,8 +301,34 @@ func newSkipperFunc() (echomiddleware.Skipper, error) {
}

// Start starts everest server.
func (e *EverestServer) Start() error {
return e.echo.Start(fmt.Sprintf("0.0.0.0:%d", e.config.HTTPPort))
func (e *EverestServer) Start(ctx context.Context) error {
addr := fmt.Sprintf("0.0.0.0:%d", e.config.ListenPort)
if e.config.TLSCertsPath != "" {
return e.startHTTPS(ctx, addr)
}
return e.echo.Start(addr)
}

func (e *EverestServer) startHTTPS(ctx context.Context, addr string) error {
tlsKeyPath := path.Join(e.config.TLSCertsPath, "tls.key")
tlsCertPath := path.Join(e.config.TLSCertsPath, "tls.crt")

mayankshah1607 marked this conversation as resolved.
Show resolved Hide resolved
watcher, err := certwatcher.New(e.l, tlsCertPath, tlsKeyPath)
if err != nil {
return fmt.Errorf("failed to create cert watcher: %w", err)
}
if err := watcher.Start(ctx); err != nil {
return fmt.Errorf("failed to start cert watcher: %w", err)
}

e.echo.TLSServer = &http.Server{
Addr: addr,
TLSConfig: &tls.Config{
// server periodically calls GetCertificate and reloads the certificate.
GetCertificate: watcher.GetCertificate,
},
}
return e.echo.StartServer(e.echo.TLSServer)
}

// Shutdown gracefully stops the Everest server.
Expand Down
96 changes: 96 additions & 0 deletions pkg/certwatcher/watcher.go
mayankshah1607 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// everest
// Copyright (C) 2023 Percona LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package certwatcher

import (
"context"
"crypto/tls"
"sync"

"github.com/fsnotify/fsnotify"
"go.uber.org/zap"
)

type certWatcher struct {
cert *tls.Certificate
certFile, keyFile string
log *zap.SugaredLogger

mutex sync.RWMutex
}

// GetCertificate returns the certificate.
func (w *certWatcher) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error) {
mayankshah1607 marked this conversation as resolved.
Show resolved Hide resolved
w.mutex.RLock()
defer w.mutex.RUnlock()

certCopy := *w.cert
return &certCopy, nil
}

// New returns a new cert watcher.
func New(log *zap.SugaredLogger, certFile, keyFile string) (*certWatcher, error) {
w := &certWatcher{
certFile: certFile,
keyFile: keyFile,
log: log,
}
if err := w.loadCertificate(); err != nil {
return nil, err
}
return w, nil
}

func (w *certWatcher) loadCertificate() error {
w.mutex.Lock()
defer w.mutex.Unlock()

cert, err := tls.LoadX509KeyPair(w.certFile, w.keyFile)
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems w.mutex.Lock() should be only there, no need to lock it before loading certs

Copy link
Member Author

@mayankshah1607 mayankshah1607 Jan 24, 2025

Choose a reason for hiding this comment

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

should be only there

should be where?

no need to lock it before loading certs

Why not? If someone reads it while it is being written, they can potentially read corrupted or inconsistent data. By locking before loading the cert, we prevent readers from reading the cert until after the loading is complete. IMO this lock is needed here, otherwise it makes no sense to have just a read lock and not protect the writes.

Please note that the other lock we have (in GetCertificates) is a read lock - meaning that multiple reader goroutines can read without blocking each other. While we have only 1 writer routine, we still need the write lock here so that the readers don't read bad data. Let me know if I missed something

w.cert = &cert
return nil
}

// Start the certificate files watcher until the context is closed.
func (w *certWatcher) Start(ctx context.Context) error {
watcher, err := fsnotify.NewWatcher()
Copy link
Contributor

Choose a reason for hiding this comment

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

are we really sure that you will get there filesystem notify signal in case certs mounted to Everest pod are changed?

We have re-check this with K8S because I remember earlier not everywhere it worked.

I remember NGINX recommended to implement it in the following way:

  • use sidecar container
  • subscribe in K8S API to event "secret changed"
  • re-read and write new secret data to
  • send signal to the main container to re-read certs.

Copy link
Member Author

@mayankshah1607 mayankshah1607 Jan 24, 2025

Choose a reason for hiding this comment

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

are we really sure that you will get there filesystem notify signal in case certs mounted to Everest pod are changed?

Yes, please see: https://kubernetes.io/docs/concepts/configuration/secret/#using-secrets-as-files-from-a-pod

When a volume contains data from a Secret, and that Secret is updated, Kubernetes tracks this and updates the data in the volume, using an eventually-consistent approach.

.. and I also tested this on GKE and k3d before opening this PR.

earlier not everywhere it worked.

Yes, earlier, I don't think it worked very well. I have also had to use some sidecar watcher. But Kubernetes has fixed it to make this work more reliably, I can't remember from which version, but its probably 1.18.

Another issue is that getting this signal takes about a minute or so, its not instant. However, for TLS certs, its fine, because usually they are renewed much ahead of the actual expiry. I would not complicate this by introducing a sidecar, etc.

This fsnotify mechanism is also used almost everywhere in most popular Kubernetes projects, like controller-runtime, so I wouldn't worry

if err != nil {
return err
}
watcher.Add(w.certFile)
watcher.Add(w.keyFile)

w.log.Infow("Watching certificate files for changes", "certFile", w.certFile, "keyFile", w.keyFile)

go func() {
for {
select {
case <-ctx.Done():
if err := watcher.Close(); err != nil {
w.log.Errorw("Failed to close watcher", "error", err)
}
case <-watcher.Events:
w.log.Info("Certificate updated")
if err := w.loadCertificate(); err != nil {
w.log.Errorw("Failed to reload certificate", "error", err)
}
}
}
}()
return nil
}
Loading