Merge lp:~natefinch/juju-core/053-nolog-jenv into lp:~go-bot/juju-core/trunk

Proposed by Nate Finch on 2014-05-28
Status: Work in progress
Proposed branch: lp:~natefinch/juju-core/053-nolog-jenv
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 11 lines (+0/-1)
1 file modified
environs/open.go (+0/-1)
To merge this branch: bzr merge lp:~natefinch/juju-core/053-nolog-jenv
Reviewer Review Type Date Requested Status
Juju Engineering 2014-05-28 Pending
Review via email: mp+221288@code.launchpad.net

Description of the change

Stop logging jenv

This fixes bug https://bugs.launchpad.net/juju-core/+bug/1289038
We shouldn't log the jenv, because it often contains sensitive information.

https://codereview.appspot.com/98580048/

To post a comment you must log in.
Nate Finch (natefinch) wrote :

Reviewers: mp+221288_code.launchpad.net,

Message:
Please take a look.

Description:
Stop logging jenv

This fixes bug https://bugs.launchpad.net/juju-core/+bug/1289038
We shouldn't log the jenv, because it often contains sensitive
information.

https://code.launchpad.net/~natefinch/juju-core/053-nolog-jenv/+merge/221288

(do not edit description out of merge proposal)

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

Affected files (+2, -1 lines):
   A [revision details]
   M environs/open.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-20140509085708-l65txq9m00uwa1z3
+New revision: <email address hidden>

Index: environs/open.go
=== modified file 'environs/open.go'
--- environs/open.go 2014-04-14 12:36:13 +0000
+++ environs/open.go 2014-05-28 20:02:25 +0000
@@ -83,7 +83,6 @@
     if len(info.BootstrapConfig()) == 0 {
      return nil, ConfigFromNowhere, EmptyConfig{fmt.Errorf("environment has
no bootstrap configuration data")}
     }
- logger.Debugf("ConfigForName found bootstrap config %#v",
info.BootstrapConfig())
     cfg, err := config.New(config.NoDefaults, info.BootstrapConfig())
     return cfg, ConfigFromInfo, err
    }

Andrew Wilkins (axwalk) wrote :

On 2014/05/28 20:15:40, nate.finch wrote:
> Please take a look.

(Just echoing my reply to juju-dev)

I think it'd be better just to sanitise the output by using the
EnvironProvider.SecretAttrs method. Then we get to see the .jenv minus
secrets, which may be helpful in debugging.

Also, we log the bootstrap script, and that contains the full bootstrap
config. That needs to be sanitised (or suppressed) as well. See
RunConfigureScript, cloudinit/sshinit/configure.go; perhaps change that
to Tracef?

https://codereview.appspot.com/98580048/

Unmerged revisions

2715. By Nate Finch on 2014-05-28

remove logging of jenv, since it contains sensitive data

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/open.go'
2--- environs/open.go 2014-05-13 04:50:10 +0000
3+++ environs/open.go 2014-05-28 20:15:11 +0000
4@@ -82,7 +82,6 @@
5 if len(info.BootstrapConfig()) == 0 {
6 return nil, ConfigFromNowhere, EmptyConfig{fmt.Errorf("environment has no bootstrap configuration data")}
7 }
8- logger.Debugf("ConfigForName found bootstrap config %#v", info.BootstrapConfig())
9 cfg, err := config.New(config.NoDefaults, info.BootstrapConfig())
10 return cfg, ConfigFromInfo, err
11 }

Subscribers

People subscribed via source and target branches

to status/vote changes: