From 9eee1c158aa21afc3838bbaae4decf07ad44a329 Mon Sep 17 00:00:00 2001 From: Don Date: Wed, 16 Nov 2016 17:08:25 -0800 Subject: [PATCH 1/3] Revert "Implement secrets concealer in build logs" This reverts commit 5377c628449ffe827206aed6157d99efc8820da6. --- agent/agent.go | 38 +++++++++++--------------------------- agent/agent_test.go | 23 ----------------------- drone/agent/agent.go | 18 ++++++------------ drone/agent/exec.go | 30 ++++++++++++++---------------- drone/exec.go | 28 +++++++++++----------------- 5 files changed, 42 insertions(+), 95 deletions(-) delete mode 100644 agent/agent_test.go diff --git a/agent/agent.go b/agent/agent.go index 1132cdd9..02aac61d 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -22,18 +22,17 @@ type Logger interface { } type Agent struct { - Update UpdateFunc - Logger LoggerFunc - Engine build.Engine - Timeout time.Duration - Platform string - Namespace string - Disable []string - Escalate []string - Netrc []string - Local string - Pull bool - ConcealSecrets bool + Update UpdateFunc + Logger LoggerFunc + Engine build.Engine + Timeout time.Duration + Platform string + Namespace string + Disable []string + Escalate []string + Netrc []string + Local string + Pull bool } func (a *Agent) Poll() error { @@ -189,7 +188,6 @@ func (a *Agent) exec(spec *yaml.Config, payload *model.Work, cancel <-chan bool) return err } - secretsReplacer := newSecretsReplacer(payload.Secrets) timeout := time.After(time.Duration(payload.Repo.Timeout) * time.Minute) for { @@ -229,25 +227,11 @@ func (a *Agent) exec(spec *yaml.Config, payload *model.Work, cancel <-chan bool) pipeline.Exec() } case line := <-pipeline.Pipe(): - // FIXME(vaijab): avoid checking a.ConcealSecrets is true everytime new line is received - if a.ConcealSecrets { - line.Out = secretsReplacer.Replace(line.Out) - } a.Logger(line) } } } -// newSecretsReplacer takes []*model.Secret as secrets and returns a list of -// secret value, "*****" pairs. -func newSecretsReplacer(secrets []*model.Secret) *strings.Replacer { - var r []string - for _, s := range secrets { - r = append(r, s.Value, "*****") - } - return strings.NewReplacer(r...) -} - func toEnv(w *model.Work) map[string]string { envs := map[string]string{ "CI": "drone", diff --git a/agent/agent_test.go b/agent/agent_test.go deleted file mode 100644 index 5857e878..00000000 --- a/agent/agent_test.go +++ /dev/null @@ -1,23 +0,0 @@ -package agent - -import "testing" -import "github.com/drone/drone/model" - -func Test_newSecretsReplacer(t *testing.T) { - secrets := []*model.Secret{ - {Name: "SECRET", - Value: "secret_value", - Images: []string{"*"}, - Events: []string{"*"}, - }, - } - - text := "This is SECRET: secret_value" - expected := "This is SECRET: *****" - secretsReplacer := newSecretsReplacer(secrets) - result := secretsReplacer.Replace(text) - - if result != expected { - t.Errorf("Wanted %q, got %q.", expected, result) - } -} diff --git a/drone/agent/agent.go b/drone/agent/agent.go index 99a010cd..09417ce2 100644 --- a/drone/agent/agent.go +++ b/drone/agent/agent.go @@ -75,11 +75,6 @@ var AgentCmd = cli.Command{ Name: "drone-secret", Usage: "drone agent secret", }, - cli.BoolFlag{ - Name: "conceal-secrets", - Usage: "conceal secrets from build logs", - EnvVar: "DRONE_CONCEAL_SECRETS", - }, cli.DurationFlag{ EnvVar: "DRONE_BACKOFF", Name: "backoff", @@ -191,13 +186,12 @@ func start(c *cli.Context) { drone: client, docker: docker, config: config{ - platform: c.String("docker-os") + "/" + c.String("docker-arch"), - timeout: c.Duration("timeout"), - namespace: c.String("namespace"), - privileged: c.StringSlice("privileged"), - pull: c.BoolT("pull"), - logs: int64(c.Int("max-log-size")) * 1000000, - concealSecrets: c.Bool("conceal-secrets"), + platform: c.String("docker-os") + "/" + c.String("docker-arch"), + timeout: c.Duration("timeout"), + namespace: c.String("namespace"), + privileged: c.StringSlice("privileged"), + pull: c.BoolT("pull"), + logs: int64(c.Int("max-log-size")) * 1000000, }, } diff --git a/drone/agent/exec.go b/drone/agent/exec.go index 79a9de21..7ad0b458 100644 --- a/drone/agent/exec.go +++ b/drone/agent/exec.go @@ -13,13 +13,12 @@ import ( ) type config struct { - platform string - namespace string - privileged []string - pull bool - logs int64 - timeout time.Duration - concealSecrets bool + platform string + namespace string + privileged []string + pull bool + logs int64 + timeout time.Duration } type pipeline struct { @@ -41,15 +40,14 @@ func (r *pipeline) run(w *model.Work) { engine := docker.NewClient(r.docker) a := agent.Agent{ - Update: agent.NewClientUpdater(r.drone), - Logger: agent.NewClientLogger(r.drone, w.Job.ID, r.config.logs), - Engine: engine, - Timeout: r.config.timeout, - Platform: r.config.platform, - Namespace: r.config.namespace, - Escalate: r.config.privileged, - Pull: r.config.pull, - ConcealSecrets: r.config.concealSecrets, + Update: agent.NewClientUpdater(r.drone), + Logger: agent.NewClientLogger(r.drone, w.Job.ID, r.config.logs), + Engine: engine, + Timeout: r.config.timeout, + Platform: r.config.platform, + Namespace: r.config.namespace, + Escalate: r.config.privileged, + Pull: r.config.pull, } cancelFunc := func(m *stomp.Message) { diff --git a/drone/exec.go b/drone/exec.go index 17111c48..f061f1bc 100644 --- a/drone/exec.go +++ b/drone/exec.go @@ -48,11 +48,6 @@ var execCmd = cli.Command{ Usage: "build secrets file in KEY=VALUE format", EnvVar: "DRONE_SECRETS_FILE", }, - cli.BoolFlag{ - Name: "conceal-secrets", - Usage: "conceal secrets from build logs", - EnvVar: "DRONE_CONCEAL_SECRETS", - }, cli.StringSliceFlag{ Name: "matrix", Usage: "build matrix in KEY=VALUE format", @@ -331,18 +326,17 @@ func exec(c *cli.Context) error { } a := agent.Agent{ - Update: agent.NoopUpdateFunc, - Logger: agent.TermLoggerFunc, - Engine: engine, - Timeout: c.Duration("timeout.inactivity"), - Platform: "linux/amd64", - Namespace: c.String("namespace"), - Disable: c.StringSlice("plugin"), - Escalate: c.StringSlice("privileged"), - Netrc: []string{}, - Local: dir, - Pull: c.Bool("pull"), - ConcealSecrets: c.Bool("conceal-secrets"), + Update: agent.NoopUpdateFunc, + Logger: agent.TermLoggerFunc, + Engine: engine, + Timeout: c.Duration("timeout.inactivity"), + Platform: "linux/amd64", + Namespace: c.String("namespace"), + Disable: c.StringSlice("plugin"), + Escalate: c.StringSlice("privileged"), + Netrc: []string{}, + Local: dir, + Pull: c.Bool("pull"), } payload := &model.Work{ From 9781e160a4c4591d7fa8eec33054bf127a994616 Mon Sep 17 00:00:00 2001 From: Don Date: Wed, 16 Nov 2016 18:33:48 -0800 Subject: [PATCH 2/3] Hide secrets --- agent/agent.go | 2 ++ agent/secret.go | 50 ++++++++++++++++++++++++++++++++++++++++++++ agent/secret_test.go | 39 ++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+) create mode 100644 agent/secret.go create mode 100644 agent/secret_test.go diff --git a/agent/agent.go b/agent/agent.go index 02aac61d..72e79351 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -188,6 +188,7 @@ func (a *Agent) exec(spec *yaml.Config, payload *model.Work, cancel <-chan bool) return err } + replacer := NewSecretReplacer(payload.Secrets) timeout := time.After(time.Duration(payload.Repo.Timeout) * time.Minute) for { @@ -227,6 +228,7 @@ func (a *Agent) exec(spec *yaml.Config, payload *model.Work, cancel <-chan bool) pipeline.Exec() } case line := <-pipeline.Pipe(): + line.Out = replacer.Replace(line.Out) a.Logger(line) } } diff --git a/agent/secret.go b/agent/secret.go new file mode 100644 index 00000000..4f6e4575 --- /dev/null +++ b/agent/secret.go @@ -0,0 +1,50 @@ +package agent + +import ( + "strings" + + "github.com/drone/drone/model" +) + +// SecretReplacer hides secrets from being exposed by the build output. +type SecretReplacer interface { + // Replace conceals instances of secrets found in s. + Replace(s string) string +} + +// NewSecretReplacer creates a SecretReplacer based on whether any value in +// secrets requests it be hidden. +func NewSecretReplacer(secrets []*model.Secret) SecretReplacer { + var r []string + for _, s := range secrets { + if s.Conceal { + r = append(r, s.Value, "*****") + } + } + + var replacer SecretReplacer + + if len(r) > 0 { + replacer = &secretReplacer{ + replacer: strings.NewReplacer(r...), + } + } else { + replacer = &noopReplacer{} + } + + return replacer +} + +type noopReplacer struct{} + +func (*noopReplacer) Replace(s string) string { + return s +} + +type secretReplacer struct { + replacer *strings.Replacer +} + +func (r *secretReplacer) Replace(s string) string { + return r.replacer.Replace(s) +} diff --git a/agent/secret_test.go b/agent/secret_test.go new file mode 100644 index 00000000..a88b523c --- /dev/null +++ b/agent/secret_test.go @@ -0,0 +1,39 @@ +package agent + +import ( + "testing" + + "github.com/drone/drone/model" + "github.com/franela/goblin" +) + +const testString = "This is SECRET: secret_value" + +func TestSecret(t *testing.T) { + g := goblin.Goblin(t) + g.Describe("SecretReplacer", func() { + g.It("Should conceal secret", func() { + secrets := []*model.Secret{ + { + Name: "SECRET", + Value: "secret_value", + Conceal: true, + }, + } + r := NewSecretReplacer(secrets) + g.Assert(r.Replace(testString)).Equal("This is SECRET: *****") + }) + + g.It("Should not conceal secret", func() { + secrets := []*model.Secret{ + { + Name: "SECRET", + Value: "secret_value", + Conceal: false, + }, + } + r := NewSecretReplacer(secrets) + g.Assert(r.Replace(testString)).Equal(testString) + }) + }) +} From 36e528e3658a1c850e3587aff4f8fa032b5e2f40 Mon Sep 17 00:00:00 2001 From: Don Date: Thu, 17 Nov 2016 10:09:58 -0800 Subject: [PATCH 3/3] Modify return to follow Golang conventions --- agent/secret.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/agent/secret.go b/agent/secret.go index 4f6e4575..531963bd 100644 --- a/agent/secret.go +++ b/agent/secret.go @@ -22,17 +22,13 @@ func NewSecretReplacer(secrets []*model.Secret) SecretReplacer { } } - var replacer SecretReplacer - - if len(r) > 0 { - replacer = &secretReplacer{ - replacer: strings.NewReplacer(r...), - } - } else { - replacer = &noopReplacer{} + if len(r) == 0 { + return &noopReplacer{} } - return replacer + return &secretReplacer{ + replacer: strings.NewReplacer(r...), + } } type noopReplacer struct{}