Merge lp:~jwharshaw/juju-core/fixlogbuild into lp:~go-bot/juju-core/trunk

[James Wilson Harshaw IV]

Removes to-be logged passwords.

https://codereview.appspot.com/77500045/

[James Wilson Harshaw IV (jwharshaw)]

Reviewers: mp+211655_code.launchpad.net,

Message:
Please take a look.

Description:
Removes to-be logged passwords.

https://code.launchpad.net/~jwharshaw/juju-core/fixlogbuild/+merge/211655

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/77500045/

Affected files (+12, -2 lines):
   A [revision details]
   M state/apiserver/apiserver.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140318185725-9jyi4x944ag57vik
+New revision: <email address hidden>

Index: state/apiserver/apiserver.go
=== modified file 'state/apiserver/apiserver.go'
--- state/apiserver/apiserver.go 2014-03-13 22:47:05 +0000
+++ state/apiserver/apiserver.go 2014-03-19 01:06:53 +0000
@@ -7,6 +7,7 @@
   "crypto/tls"
   "net"
   "net/http"
+ "regexp"
   "sync"
   "sync/atomic"
   "time"
@@ -117,14 +118,21 @@
    return
   }
   // TODO(rog) 2013-10-11 remove secrets from some requests.
- logger.Debugf("<- [%X] %s %s", n.id, n.tag(), jsoncodec.DumpRequest(hdr,
body))
+ r := regexp.MustCompile(`"Password":"(.*?)"`)
+ jsonString := string(jsoncodec.DumpRequest(hdr, body))
+ jsonString = r.ReplaceAllLiteralString(jsonString, "\"Password\":\"\"")
+ logger.Debugf("<- [%X] %s %s", n.id, n.tag(), jsonString)
  }

  func (n *requestNotifier) ServerReply(req rpc.Request, hdr *rpc.Header,
body interface{}, timeSpent time.Duration) {
   if req.Type == "Pinger" && req.Action == "Ping" {
    return
   }
- logger.Debugf("-> [%X] %s %s %s %s[%q].%s", n.id, n.tag(), timeSpent,
jsoncodec.DumpRequest(hdr, body), req.Type, req.Id, req.Action)
+ r := regexp.MustCompile(`"Password":"(.*?)"`)
+ jsonString := string(jsoncodec.DumpRequest(hdr, body))
+ jsonString = r.ReplaceAllLiteralString(jsonString, "\"Password\":\"\"")
+
+ logger.Debugf("-> [%X] %s %s %s %s[%q].%s", n.id, n.tag(), timeSpent,
jsonString, req.Type, req.Id, req.Action)
  }

  func (n *requestNotifier) join(req *http.Request) {

[Roger Peppe (rogpeppe)]

Thanks very much for working on this. It's much appreciated, and
something that really needs to be done. I have a suggestion for an
alternative approach below. Also, this should really have a test - I
suggest a test in apiserver that makes some of the relevant calls and
uses the LogMatches checker to check that the messages look appropriate.

https://codereview.appspot.com/77500045/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/77500045/diff/1/state/apiserver/apiserver.go#newcode121
state/apiserver/apiserver.go:121: r :=
regexp.MustCompile(`"Password":"(.*?)"`)
I'm not that keen on this approach - since this happens on every rpc
request and reply (even if it does happen in debugging mode only), I
think it is worth being a bit more efficient here.

Also, this won't work correctly if the password contains a quote
character, we don't need to compile the regexp on every request, and
some secrets aren't named "Password".

I *think* we should be able to do a better job by type-switching on
body. It will be some more code, but we can potentially do a better job.

Something like this, perhaps?

func (n *requestNotifier) ServerRequest(hdr *rpc.Header, body
interface{}) {
     if hdr.Request.Type == "Pinger" && hdr.Request.Action == "Ping" {
         return
     }
     body = elideSecrets(body)
     logger.Debugf("<- [%X] %s %s", n.id, n.tag(),
jsoncodec.DumpRequest(hdr, body))
}

func (n *requestNotifier) ServerReply(req rpc.Request, hdr *rpc.Header,
body interface{}, timeSpent time.Duration) {
     if hdr.Request.Type == "Pinger" && hdr.Request.Action == "Ping" {
         return
     }
     body = elideSecrets(body)
     logger.Debugf("-> [%X] %s %s %s %s[%q].%s", n.id, n.tag(),
timeSpent, jsoncodec.DumpRequest(hdr, body), req.Type, req.Id,
req.Action)
}

func (n *requestNotifier) elideSecrets(body interface{}) interface{} {
     switch body := body.(type) {
     case params.EntityPasswords:
         newBody := params.EntityPasswords{}
         newBody.Changes = append(newBody.Changes, body.Changes)
         for i := range newBody.Changes {
              newBody.Changes[i].Password = "<elided>"
         }
         return newBody
     case params.EnvironmentSet:
         newBody := params.EnvironmentSet{
              Config: make(map[string]interface{})
         }
         // TODO change environs.Environ so we can find secret
         // attributes without needing a full environment
         // configuration. In the meantime, just hide
         // everything.
         for attr := range body.Config {
             newBody.Config[attr] = "<elided>"
         }
      case params.EnvironmentGetResults:
         newBody := params.EnvironmentGetResults{
              Config: make(map[string]interface{})
         }
         // TODO as above
         for attr := range body.Config {
             newBody.Config[attr] = "<elided>"
         }
         cfg, err := config.New(body.Config)
         if err != nil {
             return body
         }
     case params.Creds:
         body.Password = "<elided>"
         return body
     }
     return body
}

https://codereview.appspot.com/77500045/

[James Wilson Harshaw IV (jwharshaw)]

I completely agree. My approach was a very quick approach to solving the
problem, but as you have pointed out, does have some issues. I do like the
approach you suggested, and I will get right on to implementing it. Thanks
for your feedback!
On Mar 19, 2014 5:34 AM, <email address hidden> wrote:

> Thanks very much for working on this. It's much appreciated, and
> something that really needs to be done. I have a suggestion for an
> alternative approach below. Also, this should really have a test - I
> suggest a test in apiserver that makes some of the relevant calls and
> uses the LogMatches checker to check that the messages look appropriate.
>
>
>
> https://codereview.appspot.com/77500045/diff/1/state/
> apiserver/apiserver.go
> File state/apiserver/apiserver.go (right):
>
> https://codereview.appspot.com/77500045/diff/1/state/
> apiserver/apiserver.go#newcode121
> state/apiserver/apiserver.go:121: r :=
> regexp.MustCompile(`"Password":"(.*?)"`)
> I'm not that keen on this approach - since this happens on every rpc
> request and reply (even if it does happen in debugging mode only), I
> think it is worth being a bit more efficient here.
>
> Also, this won't work correctly if the password contains a quote
> character, we don't need to compile the regexp on every request, and
> some secrets aren't named "Password".
>
> I *think* we should be able to do a better job by type-switching on
> body. It will be some more code, but we can potentially do a better job.
>
> Something like this, perhaps?
>
> func (n *requestNotifier) ServerRequest(hdr *rpc.Header, body
> interface{}) {
> if hdr.Request.Type == "Pinger" && hdr.Request.Action == "Ping" {
> return
> }
> body = elideSecrets(body)
> logger.Debugf("<- [%X] %s %s", n.id, n.tag(),
> jsoncodec.DumpRequest(hdr, body))
> }
>
> func (n *requestNotifier) ServerReply(req rpc.Request, hdr *rpc.Header,
> body interface{}, timeSpent time.Duration) {
> if hdr.Request.Type == "Pinger" && hdr.Request.Action == "Ping" {
> return
> }
> body = elideSecrets(body)
> logger.Debugf("-> [%X] %s %s %s %s[%q].%s", n.id, n.tag(),
> timeSpent, jsoncodec.DumpRequest(hdr, body), req.Type, req.Id,
> req.Action)
> }
>
> func (n *requestNotifier) elideSecrets(body interface{}) interface{} {
> switch body := body.(type) {
> case params.EntityPasswords:
> newBody := params.EntityPasswords{}
> newBody.Changes = append(newBody.Changes, body.Changes)
> for i := range newBody.Changes {
> newBody.Changes[i].Password = "<elided>"
> }
> return newBody
> case params.EnvironmentSet:
> newBody := params.EnvironmentSet{
> Config: make(map[string]interface{})
> }
> // TODO change environs.Environ so we can find secret
> // attributes without needing a full environment
> // configuration. In the meantime, just hide
> // everything.
> for attr := range body.Config {
> newBody.Config[attr] = "<elided>"
> }
> case params.EnvironmentGetResults:
> newBody := params.EnvironmentGetResults{
> Config: make(map[string]interfac...

[lp:~jwharshaw/juju-core/fixlogbuild]

2443. By James Wilson Harshaw IV on 2014-03-19

Add function elideSecrets to remove secrets in place of regex.

2444. By James Wilson Harshaw IV on 2014-03-19

formatted again.

[James Wilson Harshaw IV (jwharshaw)]

Please take a look.

https://codereview.appspot.com/77500045/

[lp:~jwharshaw/juju-core/fixlogbuild]

2445. By James Wilson Harshaw IV on 2014-03-22

Didn't see the point of append.

[James Wilson Harshaw IV (jwharshaw)]

Please take a look.

https://codereview.appspot.com/77500045/

[William Reade (fwereade)]

We will need tests for this functionality. It's probably best to do them
at a relatively high level: ie call the affected APIs, and scan the log
for secrets; local testing of just ServerRequest is unlikely to stay a
good test as things evolve.

https://codereview.appspot.com/77500045/diff/40001/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/77500045/diff/40001/state/apiserver/apiserver.go#newcode132
state/apiserver/apiserver.go:132: // everything.
Please register a bug for this and reference it in the comments.

https://codereview.appspot.com/77500045/diff/40001/state/apiserver/apiserver.go#newcode165
state/apiserver/apiserver.go:165:
Should we be eliding secrets here as well?

https://codereview.appspot.com/77500045/

[John A Meinel (jameinel)]

There is a little bit of whack-a-mole here, where we have to realize we are adding a new request that exposes a secret and then go filter it out.
I think we can live with it, and it is certainly better than what we have today. I was just wondering if we could have test infrastructure in place that could help us realize when we add something new. (For example, by default the LoggingSuite reads every log message and makes sure that the contents of known passwords are never present in any of the messages.)

Not something you have to change here, just thinking about ways to be more proactive about helping us to not leak secrets.

[John A Meinel (jameinel)]

James, can you give an update as to whether you're able to address the feedback here? (It looks like William is asking for tests, which is probably the only specific requirements we're making.)

Unmerged revisions

2445. By James Wilson Harshaw IV on 2014-03-22

Didn't see the point of append.

2444. By James Wilson Harshaw IV on 2014-03-19

formatted again.

2443. By James Wilson Harshaw IV on 2014-03-19

Add function elideSecrets to remove secrets in place of regex.

2442. By James Wilson Harshaw IV on 2014-03-19

Formatted

2441. By James Wilson Harshaw IV on 2014-03-19

Parsed out "Password" value of apiserver log with regexp.

2440. By James Wilson Harshaw IV on 2014-03-18

Removed json Request dump

=== modified file 'state/apiserver/apiserver.go'
--- state/apiserver/apiserver.go	2014-03-13 22:47:05 +0000
+++ state/apiserver/apiserver.go	2014-03-22 01:11:34 +0000
@@ -15,9 +15,11 @@
 	"github.com/juju/loggo"
 	"launchpad.net/tomb"
 
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/rpc"
 	"launchpad.net/juju-core/rpc/jsoncodec"
 	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/apiserver/common"
 )
 
@@ -112,11 +114,47 @@
 	return
 }
 
+func elideSecrets(body interface{}) interface{} {
+	switch body := body.(type) {
+	case params.EntityPasswords:
+		newBody := params.EntityPasswords{}
+		newBody.Changes = body.Changes
+		for i := range newBody.Changes {
+			newBody.Changes[i].Password = "<elided>"
+		}
+		return newBody
+	case params.EnvironmentSet:
+		newBody := params.EnvironmentSet{
+			Config: make(map[string]interface{})}
+		// TODO change environs.Environ so we can find secret
+		// attributes without needing a full environment
+		// configuration. In the meantime, just hide
+		// everything.
+		for attr := range body.Config {
+			newBody.Config[attr] = "<elided>"
+		}
+	case params.EnvironmentGetResults:
+		newBody := params.EnvironmentGetResults{
+			Config: make(map[string]interface{})}
+		// TODO as above
+		for attr := range body.Config {
+			newBody.Config[attr] = "<elided>"
+		}
+		_, err := config.New(config.NoDefaults, body.Config)
+		if err != nil {
+			return body
+		}
+	case params.Creds:
+		body.Password = "<elided>"
+		return body
+	}
+	return body
+}
 func (n *requestNotifier) ServerRequest(hdr *rpc.Header, body interface{}) {
 	if hdr.Request.Type == "Pinger" && hdr.Request.Action == "Ping" {
 		return
 	}
-	// TODO(rog) 2013-10-11 remove secrets from some requests.
+	body = elideSecrets(body)
 	logger.Debugf("<- [%X] %s %s", n.id, n.tag(), jsoncodec.DumpRequest(hdr, body))
 }
 
@@ -124,6 +162,7 @@
 	if req.Type == "Pinger" && req.Action == "Ping" {
 		return
 	}
+
 	logger.Debugf("-> [%X] %s %s %s %s[%q].%s", n.id, n.tag(), timeSpent, jsoncodec.DumpRequest(hdr, body), req.Type, req.Id, req.Action)
 }