From 7cf2a7c576dfda1f51536406b0d70cecbdb6b77c Mon Sep 17 00:00:00 2001 From: Julien Duchesne Date: Tue, 18 Apr 2023 10:38:39 -0400 Subject: [PATCH] feat: Disallow forks from getting secrets - Adds a global `DRONE_DISALLOW_FORKS` env variable to disallow forks from getting secrets - Adds a secret-level override (`X-Drone-Disallow-Forks`) to allow or disallow a specific secret --- main.go | 9 +++- plugin/plugin.go | 38 ++++++++++--- plugin/plugin_test.go | 121 +++++++++++++++++++++++++++++++++++++++--- plugin/util.go | 17 +++++- 4 files changed, 169 insertions(+), 16 deletions(-) diff --git a/main.go b/main.go index da280ff..ab5c514 100644 --- a/main.go +++ b/main.go @@ -12,8 +12,8 @@ import ( "github.com/drone/drone-go/plugin/secret" "github.com/drone/drone-vault/plugin" "github.com/drone/drone-vault/plugin/token" - "github.com/drone/drone-vault/plugin/token/kubernetes" "github.com/drone/drone-vault/plugin/token/approle" + "github.com/drone/drone-vault/plugin/token/kubernetes" "github.com/hashicorp/vault/api" "github.com/kelseyhightower/envconfig" @@ -40,6 +40,7 @@ type config struct { Address string `envconfig:"DRONE_BIND"` Debug bool `envconfig:"DRONE_DEBUG"` Secret string `envconfig:"DRONE_SECRET"` + DisallowForks bool `envconfig:"DRONE_DISALLOW_FORKS"` VaultAddr string `envconfig:"VAULT_ADDR"` VaultRenew time.Duration `envconfig:"VAULT_TOKEN_RENEWAL"` VaultTTL time.Duration `envconfig:"VAULT_TOKEN_TTL"` @@ -80,9 +81,13 @@ func main() { // global context ctx := context.Background() + if spec.DisallowForks { + logrus.Info("globally disallowing secrets in forks") + } + http.Handle("/", secret.Handler( spec.Secret, - plugin.New(client), + plugin.New(client, spec.DisallowForks), logrus.StandardLogger(), )) diff --git a/plugin/plugin.go b/plugin/plugin.go index 509d95c..7bb7ad8 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -17,22 +17,33 @@ import ( // New returns a new secret plugin that sources secrets // from the AWS secrets manager. -func New(client *api.Client) secret.Plugin { +func New(client *api.Client, disallowForks bool) secret.Plugin { return &plugin{ - client: client, + client: client, + disallowForks: disallowForks, } } type plugin struct { - client *api.Client + client *api.Client + disallowForks bool } func (p *plugin) Find(ctx context.Context, req *secret.Request) (*drone.Secret, error) { + // The Fork attribute will be empty on a branch build (e.g. master). + // Branch builds cannot be from a fork. + isFork := req.Build.Fork != "" && req.Build.Fork != req.Repo.Slug + forkRepo := "" + if isFork { + forkRepo = req.Build.Fork + } + logEvent := logrus.WithFields(logrus.Fields{ "event": req.Build.Event, "repo": req.Repo.Slug, "ref": req.Build.Ref, "secret": req.Path, + "fork": forkRepo, }) path := req.Path @@ -52,7 +63,7 @@ func (p *plugin) Find(ctx context.Context, req *secret.Request) (*drone.Secret, return nil, errors.New("secret key not found") } - // the user can filter out requets based on event type + // the user can filter out requests based on event type // using the X-Drone-Events secret key. Check for this // user-defined filter logic. events := extractEvents(params) @@ -62,7 +73,7 @@ func (p *plugin) Find(ctx context.Context, req *secret.Request) (*drone.Secret, return nil, errors.New(msg) } - // the user can filter out requets based on repository + // the user can filter out requests based on repository // using the X-Drone-Repos secret key. Check for this // user-defined filter logic. repos := extractRepos(params) @@ -72,7 +83,7 @@ func (p *plugin) Find(ctx context.Context, req *secret.Request) (*drone.Secret, return nil, errors.New(msg) } - // the user can filter out requets based on repository + // the user can filter out requests based on repository // branch using the X-Drone-Branches secret key. Check // for this user-defined filter logic. branches := extractBranches(params) @@ -82,13 +93,26 @@ func (p *plugin) Find(ctx context.Context, req *secret.Request) (*drone.Secret, return nil, errors.New(msg) } + // the user can disallow fork builds using the + // X-Drone-Disallow-Forks secret key. Check for this + // user-defined filter logic. + disallowForks := p.disallowForks + if secretSetting := extractDisallowForks(params); secretSetting != nil { + disallowForks = *secretSetting + } + if disallowForks && isFork { + msg := "access denied: forks are not allowed" + logEvent.WithField("disallow_forks", disallowForks).Debug(msg) + return nil, errors.New(msg) + } + logEvent.Debug("secret matched and returned") return &drone.Secret{ Name: name, Data: value, Pull: true, // always true. use X-Drone-Events to prevent pull requests. - Fork: true, // always true. use X-Drone-Events to prevent pull requests. + Fork: true, // always true. use X-Drone-Disallow-Forks to prevent secrets from forks. }, nil } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index d53a3db..f4f7bc1 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -6,6 +6,7 @@ package plugin import ( "context" + "encoding/json" "io/ioutil" "net/http" "net/http/httptest" @@ -50,7 +51,7 @@ func TestPlugin(t *testing.T) { Slug: "octocat/hello-world", }, } - plugin := New(client) + plugin := New(client, false) got, err := plugin.Find(noContext, req) if err != nil { t.Error(err) @@ -92,7 +93,7 @@ func TestPlugin_FilterBranches(t *testing.T) { Slug: "octocat/hello-world", }, } - plugin := New(client) + plugin := New(client, false) _, err := plugin.Find(noContext, req) if err == nil { t.Errorf("Expect error") @@ -126,7 +127,7 @@ func TestPlugin_FilterRepo(t *testing.T) { Slug: "spaceghost/hello-world", }, } - plugin := New(client) + plugin := New(client, false) _, err := plugin.Find(noContext, req) if err == nil { t.Errorf("Expect error") @@ -160,7 +161,7 @@ func TestPlugin_FilterEvent(t *testing.T) { Slug: "octocat/hello-world", }, } - plugin := New(client) + plugin := New(client, false) _, err := plugin.Find(noContext, req) if err == nil { t.Errorf("Expect error") @@ -171,6 +172,114 @@ func TestPlugin_FilterEvent(t *testing.T) { } } +func TestPlugin_FilterForks(t *testing.T) { + cases := []struct { + name string + disallowForksSecretSetting string + disallowForksGlobalSetting bool + expectedError string + isFork bool + }{ + { + name: "disallow forks, secret setting is true", + disallowForksSecretSetting: "true", + disallowForksGlobalSetting: false, + expectedError: "access denied: forks are not allowed", + isFork: true, + }, + { + name: "disallow forks, global setting is true", + disallowForksSecretSetting: "", + disallowForksGlobalSetting: true, + expectedError: "access denied: forks are not allowed", + isFork: true, + }, + { + name: "disallow forks, secret setting is false", + disallowForksSecretSetting: "false", + disallowForksGlobalSetting: false, + expectedError: "", + isFork: true, + }, + { + name: "disallow forks, secret setting is not set", + disallowForksSecretSetting: "", + disallowForksGlobalSetting: false, + expectedError: "", + isFork: true, + }, + { + name: "disallow forks, secret setting enabled but not a fork", + disallowForksSecretSetting: "true", + disallowForksGlobalSetting: false, + expectedError: "", + isFork: false, + }, + { + name: "disallow forks, global setting enabled but not a fork", + disallowForksSecretSetting: "", + disallowForksGlobalSetting: true, + expectedError: "", + isFork: false, + }, + { + name: "allow forks, secret setting is false (override global setting)", + disallowForksSecretSetting: "false", + disallowForksGlobalSetting: true, + expectedError: "", + isFork: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + payloadBefore, _ := ioutil.ReadFile("testdata/secret.json") + payload := make(map[string]interface{}) + json.Unmarshal(payloadBefore, &payload) + if c.disallowForksSecretSetting != "" { + data := payload["data"].(map[string]interface{}) + data["X-Drone-Disallow-Forks"] = c.disallowForksSecretSetting + payload["data"] = data + } + payloadAfter, _ := json.Marshal(payload) + w.Write(payloadAfter) + })) + defer ts.Close() + + client, _ := api.NewClient(&api.Config{ + Address: ts.URL, + MaxRetries: 1, + }) + + req := &secret.Request{ + Path: "secret/docker", + Name: "username", + Build: drone.Build{ + Event: "push", + Target: "master", + }, + Repo: drone.Repo{ + Slug: "octocat/hello-world", + }, + } + if c.isFork { + req.Build.Fork = "spaceghost/hello-world" + } + + plugin := New(client, c.disallowForksGlobalSetting) + gotErr := "" + if _, err := plugin.Find(noContext, req); err != nil { + gotErr = err.Error() + } + + if gotErr != c.expectedError { + t.Errorf("Want error %q, got %q", c.expectedError, gotErr) + } + }) + } +} + func TestPlugin_NotFound(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { out, _ := ioutil.ReadFile("testdata/not_found.json") @@ -195,7 +304,7 @@ func TestPlugin_NotFound(t *testing.T) { Slug: "octocat/hello-world", }, } - plugin := New(client) + plugin := New(client, false) _, err := plugin.Find(noContext, req) if err == nil { t.Errorf("Expect error") @@ -231,7 +340,7 @@ func TestPlugin_KeyNotFound(t *testing.T) { Slug: "octocat/hello-world", }, } - plugin := New(client) + plugin := New(client, false) _, err := plugin.Find(noContext, req) if err == nil { t.Errorf("Expect error") diff --git a/plugin/util.go b/plugin/util.go index e5ecef3..461c2d3 100644 --- a/plugin/util.go +++ b/plugin/util.go @@ -4,7 +4,10 @@ package plugin -import "strings" +import ( + "strconv" + "strings" +) // helper function extracts the branch filters from the // secret payload in key value format. @@ -39,6 +42,18 @@ func extractEvents(params map[string]string) []string { return nil } +// helper function extracts the fork filter from the +// secret payload in key value format. +func extractDisallowForks(params map[string]string) *bool { + for key, value := range params { + if strings.EqualFold(key, "X-Drone-Disallow-Forks") { + v, _ := strconv.ParseBool(value) + return &v // Allow non-truthy or non-falsey values are false + } + } + return nil +} + func parseCommaSeparated(s string) []string { parts := strings.Split(s, ",") if len(parts) == 1 && parts[0] == "" {