From a4ac00b378c34868180acb54670d188c7b768d23 Mon Sep 17 00:00:00 2001 From: Karolis Rusenas Date: Tue, 16 Apr 2019 21:46:47 +0100 Subject: [PATCH 1/7] moving auth into a wrapper --- trigger/http/approvals_endpoint.go | 36 ----------------------- trigger/http/http.go | 47 +++++++++++++++++++++++------- 2 files changed, 37 insertions(+), 46 deletions(-) diff --git a/trigger/http/approvals_endpoint.go b/trigger/http/approvals_endpoint.go index 2b0e7fb7..4db81766 100644 --- a/trigger/http/approvals_endpoint.go +++ b/trigger/http/approvals_endpoint.go @@ -4,9 +4,7 @@ import ( "encoding/json" "fmt" "net/http" - "os" - "github.com/keel-hq/keel/constants" "github.com/keel-hq/keel/cache" "github.com/keel-hq/keel/types" ) @@ -26,23 +24,6 @@ const ( func (s *TriggerServer) approvalsHandler(resp http.ResponseWriter, req *http.Request) { - // basic auth - if os.Getenv(constants.EnvBasicAuthUser) != "" && os.Getenv(constants.EnvBasicAuthPassword) != "" { - - resp.Header().Set("WWW-Authenticate", `Basic realm="Restricted"`) - - username, password, authOK := req.BasicAuth() - if authOK == false { - http.Error(resp, "Not authorized", 401) - return - } - - if username != os.Getenv(constants.EnvBasicAuthUser) || password != os.Getenv(constants.EnvBasicAuthPassword) { - http.Error(resp, "Not authorized", 401) - return - } - } - // unknown lists all approvals, err := s.approvalsManager.List() if err != nil { @@ -67,23 +48,6 @@ func (s *TriggerServer) approvalsHandler(resp http.ResponseWriter, req *http.Req func (s *TriggerServer) approvalApproveHandler(resp http.ResponseWriter, req *http.Request) { - // basic auth - if os.Getenv(constants.EnvBasicAuthUser) != "" && os.Getenv(constants.EnvBasicAuthPassword) != "" { - - resp.Header().Set("WWW-Authenticate", `Basic realm="Restricted"`) - - username, password, authOK := req.BasicAuth() - if authOK == false { - http.Error(resp, "Not authorized", 401) - return - } - - if username != os.Getenv(constants.EnvBasicAuthUser) || password != os.Getenv(constants.EnvBasicAuthPassword) { - http.Error(resp, "Not authorized", 401) - return - } - } - var ar approveRequest dec := json.NewDecoder(req.Body) defer req.Body.Close() diff --git a/trigger/http/http.go b/trigger/http/http.go index d9411fd5..a62725c3 100644 --- a/trigger/http/http.go +++ b/trigger/http/http.go @@ -28,6 +28,10 @@ type Opts struct { Providers provider.Providers ApprovalManager approvals.Manager + + // Username and password are used for basic auth + Username string + Password string } // TriggerServer - webhook trigger & healthcheck server @@ -37,6 +41,10 @@ type TriggerServer struct { port int server *http.Server router *mux.Router + + // basic auth + username string + password string } // NewTriggerServer - create new HTTP trigger based server @@ -46,6 +54,8 @@ func NewTriggerServer(opts *Opts) *TriggerServer { providers: opts.Providers, approvalsManager: opts.ApprovalManager, router: mux.NewRouter(), + username: opts.Username, + password: opts.Password, } } @@ -53,6 +63,7 @@ func NewTriggerServer(opts *Opts) *TriggerServer { func (s *TriggerServer) Start() error { s.registerRoutes(s.router) + s.registerWebhookRoutes(s.router) n := negroni.New(negroni.NewRecovery()) n.UseHandler(s.router) @@ -91,27 +102,23 @@ func (s *TriggerServer) registerRoutes(mux *mux.Router) { mux.HandleFunc("/version", s.versionHandler).Methods("GET", "OPTIONS") // approvals - mux.HandleFunc("/v1/approvals", s.approvalsHandler).Methods("GET", "OPTIONS") + mux.HandleFunc("/v1/approvals", s.requireAdminAuthorization(s.approvalsHandler)).Methods("GET", "OPTIONS") // approving - mux.HandleFunc("/v1/approvals", s.approvalApproveHandler).Methods("POST", "OPTIONS") + mux.HandleFunc("/v1/approvals", s.requireAdminAuthorization(s.approvalApproveHandler)).Methods("POST", "OPTIONS") - // native webhooks handler + mux.Handle("/metrics", promhttp.Handler()) +} + +func (s *TriggerServer) registerWebhookRoutes(mux *mux.Router) { mux.HandleFunc("/v1/webhooks/native", s.nativeHandler).Methods("POST", "OPTIONS") - - // dockerhub webhooks handler mux.HandleFunc("/v1/webhooks/dockerhub", s.dockerHubHandler).Methods("POST", "OPTIONS") - - // quay webhooks handler mux.HandleFunc("/v1/webhooks/quay", s.quayHandler).Methods("POST", "OPTIONS") - mux.HandleFunc("/v1/webhooks/azure", s.azureHandler).Methods("POST", "OPTIONS") // Docker registry notifications, used by Docker, Gitlab, Harbor // https://docs.docker.com/registry/notifications/ //https://docs.gitlab.com/ee/administration/container_registry.html#configure-container-registry-notifications mux.HandleFunc("/v1/webhooks/registry", s.registryNotificationHandler).Methods("POST", "OPTIONS") - - mux.Handle("/metrics", promhttp.Handler()) } func (s *TriggerServer) healthHandler(resp http.ResponseWriter, req *http.Request) { @@ -135,3 +142,23 @@ func (s *TriggerServer) versionHandler(resp http.ResponseWriter, req *http.Reque func (s *TriggerServer) trigger(event types.Event) error { return s.providers.Submit(event) } + +func (s *TriggerServer) requireAdminAuthorization(next http.HandlerFunc) http.HandlerFunc { + return func(rw http.ResponseWriter, r *http.Request) { + + if s.username == "" && s.password == "" { + next(rw, r) + return + } + + rw.Header().Set("WWW-Authenticate", `Basic realm="Restricted"`) + + username, password, ok := r.BasicAuth() + if ok && username == s.username && password == s.password { + next(rw, r) + return + } + + http.Error(rw, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized) + } +} From b36789522f8b7fe7b8f66c8a79d504e8eaf05630 Mon Sep 17 00:00:00 2001 From: Karolis Rusenas Date: Tue, 16 Apr 2019 21:46:55 +0100 Subject: [PATCH 2/7] initializing basic auth --- cmd/keel/main.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/keel/main.go b/cmd/keel/main.go index d6f5c07a..ef429a48 100644 --- a/cmd/keel/main.go +++ b/cmd/keel/main.go @@ -268,6 +268,8 @@ func setupTriggers(ctx context.Context, providers provider.Providers, approvalsM Port: types.KeelDefaultPort, Providers: providers, ApprovalManager: approvalsManager, + Username: os.Getenv(constants.EnvBasicAuthUser), + Password: os.Getenv(constants.EnvBasicAuthPassword), }) go func() { From 5684d30598b8ba0d128a89cdaab03c38d58b68b9 Mon Sep 17 00:00:00 2001 From: Karolis Rusenas Date: Tue, 16 Apr 2019 21:54:51 +0100 Subject: [PATCH 3/7] tests --- tests/acceptance_test.go | 165 ++++++++++++++++++++++++++++++++++++++- tests/helpers.go | 6 +- 2 files changed, 169 insertions(+), 2 deletions(-) diff --git a/tests/acceptance_test.go b/tests/acceptance_test.go index e3e80a04..cd7328f5 100644 --- a/tests/acceptance_test.go +++ b/tests/acceptance_test.go @@ -4,14 +4,17 @@ import ( "bytes" "context" "encoding/json" + "fmt" "net/http" "testing" "time" + "github.com/keel-hq/keel/constants" + "github.com/keel-hq/keel/types" apps_v1 "k8s.io/api/apps/v1" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" log "github.com/sirupsen/logrus" @@ -405,3 +408,163 @@ func TestApprovals(t *testing.T) { } }) } + +func TestApprovalsWithAuthentication(t *testing.T) { + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + username := "foobar" + password := "barfood" + + // go startKeel(ctx) + keel := &KeelCmd{ + env: []string{ + fmt.Sprintf("%s=%s", constants.EnvBasicAuthUser, username), + fmt.Sprintf("%s=%s", constants.EnvBasicAuthPassword, password), + }, + } + go func() { + err := keel.Start(ctx) + if err != nil { + log.WithFields(log.Fields{ + "error": err, + }).Error("failed to start Keel process") + } + }() + + defer func() { + err := keel.Stop() + if err != nil { + log.WithFields(log.Fields{ + "error": err, + }).Error("failed to stop Keel process") + } + }() + + _, kcs := getKubernetesClient() + + t.Run("CreateDeploymentWithApprovals", func(t *testing.T) { + + testNamespace := createNamespaceForTest() + defer deleteTestNamespace(testNamespace) + + dep := &apps_v1.Deployment{ + meta_v1.TypeMeta{}, + meta_v1.ObjectMeta{ + Name: "deployment-1", + Namespace: testNamespace, + Labels: map[string]string{ + types.KeelPolicyLabel: "all", + types.KeelMinimumApprovalsLabel: "1", + types.KeelApprovalDeadlineLabel: "5", + }, + Annotations: map[string]string{}, + }, + apps_v1.DeploymentSpec{ + Selector: &meta_v1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "wd-1", + }, + }, + Template: v1.PodTemplateSpec{ + ObjectMeta: meta_v1.ObjectMeta{ + Labels: map[string]string{ + "app": "wd-1", + "release": "1", + }, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + v1.Container{ + Name: "wd-1", + Image: "karolisr/webhook-demo:0.0.14", + }, + }, + }, + }, + }, + apps_v1.DeploymentStatus{}, + } + + _, err := kcs.AppsV1().Deployments(testNamespace).Create(dep) + if err != nil { + t.Fatalf("failed to create deployment: %s", err) + } + // giving some time to get started + // TODO: replace with a readiness check function to wait for 1/1 READY + time.Sleep(2 * time.Second) + + // sending webhook + client := http.DefaultClient + buf := bytes.NewBufferString(dockerHub0150Webhook) + req, err := http.NewRequest("POST", "http://localhost:9300/v1/webhooks/dockerhub", buf) + if err != nil { + t.Fatalf("failed to create req: %s", err) + } + resp, err := client.Do(req) + if err != nil { + t.Errorf("failed to make a webhook request to keel: %s", err) + } + + if resp.StatusCode != 200 { + t.Errorf("unexpected webhook response from keel: %d", resp.StatusCode) + } + + time.Sleep(2 * time.Second) + + // req2, err := http.NewRequest("GET", "http://localhost:9300/v1/approvals", nil) + + reqNoAuth, err := http.NewRequest("GET", "http://localhost:9300/v1/approvals", nil) + if err != nil { + t.Fatalf("failed to create req: %s", err) + } + respNoAuth, err := client.Do(reqNoAuth) + if err != nil { + t.Logf("failed to make req: %s", err) + } + defer respNoAuth.Body.Close() + if respNoAuth.StatusCode != http.StatusUnauthorized { + t.Errorf("expected 401, got: %d", respNoAuth.StatusCode) + } + + // doing it again with authentication + reqAuth, err := http.NewRequest("GET", "http://localhost:9300/v1/approvals", nil) + if err != nil { + t.Fatalf("failed to create req: %s", err) + } + reqAuth.SetBasicAuth(username, password) + resp, err = client.Do(reqAuth) + if err != nil { + t.Errorf("failed to make req: %s", err) + } + + var approvals []*types.Approval + dec := json.NewDecoder(resp.Body) + defer resp.Body.Close() + err = dec.Decode(&approvals) + if err != nil { + t.Fatalf("failed to decode approvals resp: %s", err) + } + + if len(approvals) != 1 { + t.Errorf("expected to find 1 approval, got: %d", len(approvals)) + } else { + if approvals[0].VotesRequired != 1 { + t.Errorf("expected 1 required vote, got: %d", approvals[0].VotesRequired) + } + log.Infof("approvals deadline: %s, time since: %v", approvals[0].Deadline, time.Since(approvals[0].Deadline)) + if time.Since(approvals[0].Deadline) > -4*time.Hour && time.Since(approvals[0].Deadline) < -5*time.Hour { + t.Errorf("deadline is for: %s", approvals[0].Deadline) + } + } + + ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) + defer cancel() + + err = waitFor(ctx, kcs, testNamespace, dep.ObjectMeta.Name, "karolisr/webhook-demo:0.0.14") + if err != nil { + t.Errorf("update failed: %s", err) + } + }) +} diff --git a/tests/helpers.go b/tests/helpers.go index f0b5e0f4..528d7bb4 100644 --- a/tests/helpers.go +++ b/tests/helpers.go @@ -10,7 +10,7 @@ import ( "strings" "time" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" @@ -75,6 +75,8 @@ func deleteTestNamespace(namespace string) error { type KeelCmd struct { cmd *exec.Cmd + + env []string } func (kc *KeelCmd) Start(ctx context.Context) error { @@ -87,6 +89,8 @@ func (kc *KeelCmd) Start(ctx context.Context) error { c.Env = []string{ "DEBUG=true", } + c.Env = append(c.Env, kc.env...) + c.Stdout = os.Stdout c.Stderr = os.Stderr From a699cd4c13d81ee9b1c570e2d5ee8039071d822a Mon Sep 17 00:00:00 2001 From: Karolis Rusenas Date: Tue, 16 Apr 2019 21:59:37 +0100 Subject: [PATCH 4/7] register routes --- trigger/http/http.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/trigger/http/http.go b/trigger/http/http.go index a62725c3..9a5d86da 100644 --- a/trigger/http/http.go +++ b/trigger/http/http.go @@ -63,7 +63,6 @@ func NewTriggerServer(opts *Opts) *TriggerServer { func (s *TriggerServer) Start() error { s.registerRoutes(s.router) - s.registerWebhookRoutes(s.router) n := negroni.New(negroni.NewRecovery()) n.UseHandler(s.router) @@ -107,6 +106,8 @@ func (s *TriggerServer) registerRoutes(mux *mux.Router) { mux.HandleFunc("/v1/approvals", s.requireAdminAuthorization(s.approvalApproveHandler)).Methods("POST", "OPTIONS") mux.Handle("/metrics", promhttp.Handler()) + + s.registerWebhookRoutes(mux) } func (s *TriggerServer) registerWebhookRoutes(mux *mux.Router) { From 98378d54af9feda25925ce05ac4f36020b0a4632 Mon Sep 17 00:00:00 2001 From: Karolis Rusenas Date: Tue, 16 Apr 2019 22:12:10 +0100 Subject: [PATCH 5/7] approvals test --- trigger/http/approvals_endpoint_test.go | 105 ++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/trigger/http/approvals_endpoint_test.go b/trigger/http/approvals_endpoint_test.go index 1665ea72..88d028f3 100644 --- a/trigger/http/approvals_endpoint_test.go +++ b/trigger/http/approvals_endpoint_test.go @@ -352,3 +352,108 @@ func TestReject(t *testing.T) { } } + +func TestAuthListApprovalsA(t *testing.T) { + + fp := &fakeProvider{} + mem := memory.NewMemoryCache() + am := approvals.New(mem) + providers := provider.New([]provider.Provider{fp}, am) + srv := NewTriggerServer(&Opts{ + Providers: providers, + ApprovalManager: am, + Username: "user-1", + Password: "secret", + }) + srv.registerRoutes(srv.router) + + err := am.Create(&types.Approval{ + Identifier: "123", + VotesRequired: 5, + NewVersion: "2.0.0", + CurrentVersion: "1.0.0", + }) + + if err != nil { + t.Fatalf("failed to create approval: %s", err) + } + + // listing + req, err := http.NewRequest("GET", "/v1/approvals", nil) + if err != nil { + t.Fatalf("failed to create req: %s", err) + } + + rec := httptest.NewRecorder() + + srv.router.ServeHTTP(rec, req) + if rec.Code != 401 { + t.Errorf("expected 401 status code, got: %d", rec.Code) + + t.Log(rec.Body.String()) + } +} + +func TestAuthListApprovalsB(t *testing.T) { + + fp := &fakeProvider{} + mem := memory.NewMemoryCache() + am := approvals.New(mem) + providers := provider.New([]provider.Provider{fp}, am) + srv := NewTriggerServer(&Opts{ + Providers: providers, + ApprovalManager: am, + Username: "user-1", + Password: "secret", + }) + srv.registerRoutes(srv.router) + + err := am.Create(&types.Approval{ + Identifier: "123", + VotesRequired: 5, + NewVersion: "2.0.0", + CurrentVersion: "1.0.0", + }) + + if err != nil { + t.Fatalf("failed to create approval: %s", err) + } + + // listing + req, err := http.NewRequest("GET", "/v1/approvals", nil) + if err != nil { + t.Fatalf("failed to create req: %s", err) + } + + req.SetBasicAuth("user-1", "secret") + + rec := httptest.NewRecorder() + + srv.router.ServeHTTP(rec, req) + if rec.Code != 200 { + t.Errorf("expected 200 status code, got: %d", rec.Code) + + t.Log(rec.Body.String()) + } + + var approvals []*types.Approval + + err = json.Unmarshal(rec.Body.Bytes(), &approvals) + if err != nil { + t.Fatalf("failed to unmarshal response into approvals: %s", err) + } + + if len(approvals) != 1 { + t.Fatalf("expected to find 1 approval but found: %d", len(approvals)) + } + + if approvals[0].VotesRequired != 5 { + t.Errorf("unexpected votes required") + } + if approvals[0].NewVersion != "2.0.0" { + t.Errorf("unexpected new version: %s", approvals[0].NewVersion) + } + if approvals[0].CurrentVersion != "1.0.0" { + t.Errorf("unexpected current version: %s", approvals[0].CurrentVersion) + } +} From 6ce45cdf57712225f32df447bfedbd31a86accde Mon Sep 17 00:00:00 2001 From: Karolis Rusenas Date: Tue, 16 Apr 2019 22:18:37 +0100 Subject: [PATCH 6/7] ensuring non zero approvals --- tests/acceptance_test.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/tests/acceptance_test.go b/tests/acceptance_test.go index cd7328f5..a87c4d75 100644 --- a/tests/acceptance_test.go +++ b/tests/acceptance_test.go @@ -452,7 +452,7 @@ func TestApprovalsWithAuthentication(t *testing.T) { dep := &apps_v1.Deployment{ meta_v1.TypeMeta{}, meta_v1.ObjectMeta{ - Name: "deployment-1", + Name: "dep-1-auth-test", Namespace: testNamespace, Labels: map[string]string{ types.KeelPolicyLabel: "all", @@ -513,8 +513,6 @@ func TestApprovalsWithAuthentication(t *testing.T) { time.Sleep(2 * time.Second) - // req2, err := http.NewRequest("GET", "http://localhost:9300/v1/approvals", nil) - reqNoAuth, err := http.NewRequest("GET", "http://localhost:9300/v1/approvals", nil) if err != nil { t.Fatalf("failed to create req: %s", err) @@ -547,16 +545,8 @@ func TestApprovalsWithAuthentication(t *testing.T) { t.Fatalf("failed to decode approvals resp: %s", err) } - if len(approvals) != 1 { - t.Errorf("expected to find 1 approval, got: %d", len(approvals)) - } else { - if approvals[0].VotesRequired != 1 { - t.Errorf("expected 1 required vote, got: %d", approvals[0].VotesRequired) - } - log.Infof("approvals deadline: %s, time since: %v", approvals[0].Deadline, time.Since(approvals[0].Deadline)) - if time.Since(approvals[0].Deadline) > -4*time.Hour && time.Since(approvals[0].Deadline) < -5*time.Hour { - t.Errorf("deadline is for: %s", approvals[0].Deadline) - } + if len(approvals) == 0 { + t.Errorf("no approvals found") } ctx, cancel := context.WithTimeout(context.Background(), time.Second*10) From cf6f442106c86ced093ef9617ee68564f0f0c659 Mon Sep 17 00:00:00 2001 From: Karolis Rusenas Date: Tue, 16 Apr 2019 22:51:27 +0100 Subject: [PATCH 7/7] trim prefix if supplied --- bot/slack/slack.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bot/slack/slack.go b/bot/slack/slack.go index 7ee3cc6a..9401bc9b 100644 --- a/bot/slack/slack.go +++ b/bot/slack/slack.go @@ -63,7 +63,7 @@ func (b *Bot) Configure(approvalsRespCh chan *bot.ApprovalResponse, botMessagesC b.approvalsChannel = "general" if channel := os.Getenv(constants.EnvSlackApprovalsChannel); channel != "" { - b.approvalsChannel = channel + b.approvalsChannel = strings.TrimPrefix(channel, "#") } b.slackClient = client