Removes to-be logged passwords.

Removes to-be logged passwords.



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

(do not edit description out of merge proposal)

[jwharshaw, 2014-03-19 01:13:28]

Please take a look.

[rog, 2014-03-19 09:34:07]

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#n...
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
}

[jwharshaw, 2014-03-19 17:06:33]

Please take a look.

[jwharshaw, 2014-03-22 01:11:26]

Please take a look.

[fwereade, 2014-03-24 09:33:05]

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....
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....
state/apiserver/apiserver.go:165: 
Should we be eliding secrets here as well?