Scrub passwords from error output #81

mkenney · 2018-07-13T15:08:29Z

This is in reference to #79.

#79

mkenney · 2018-07-13T15:12:28Z

There's an s3 credentials error in the build trying to fetch the instantclient.

tgulacsi

A String method for ConnectionParams would be better.

tgulacsi

A String method on ConnectionParams would be better and more general.

mkenney · 2018-07-14T18:04:42Z

@tgulacsi, ConnectionParams does have a String method which is what's being used for the error output (which is the root of the issue).

I agree the two things should be separate, and I don't particularly like intercepting and sanitizing the flow this way, but it also smells bad to do to much more around scoping here as suggested by @veqryn without being more familiar with the consequences for downstream projects.

My only goal is to prevent unexpected or unintended output of sensitive data. I thought about maybe adding an error or even an Error method. Error feels weird, making ConnectionParams an error, but an unexported error or sanitizedString method might make more sense. Thoughts?

tgulacsi · 2018-07-18T06:56:47Z

I wanted to say that changing line 535 of drv.go from

User:   url.UserPassword(P.Username, P.Password),

to

User:   url.UserPassword(P.Username, "***"),

would be enough, wouldn't it?

tgulacsi · 2018-07-18T07:00:12Z

Oops, and everywhere where password is printed, logged, errored: lines 447, 458, 481, 492, 535 of drv.go.

mkenney · 2018-07-18T15:11:40Z

Ahh, I see. Ok, I'll take a closer look and push up a change. Thanks!

mkenney · 2018-07-18T16:02:11Z

So it looks like ConnectionParams.string is used for both logging and creating the actual connection string so changing 535 to User: url.UserPassword(P.Username, "***"), will break some things unless I'm missing something.

I'll find those cases (there might just be the one) and split them out to use another method or maybe add a param to the string method...

tgulacsi · 2018-07-18T16:23:00Z

Where is ConnectionParam.String (or string) used for the connection?

mkenney · 2018-07-18T16:42:41Z

It's in openConn.

I pushed up a change that I think will cover all the bases without breaking anything. I chose SECRET as the password replacement string instead of asterisks because url.UserPassword url encodes the string making the asterisks %2A which feels odd.

Please let me know what you think. The change makes the default behavior password sanitation, if you want the plaintext password there's a second (variadic) parameter you have to pass true to. I.E.

// Sanitized
P.string(true)
P.string(false)

// Plaintext
P.string(true, true)
P.string(false, true)

tgulacsi · 2018-07-18T17:39:15Z

drv.go

@@ -446,7 +444,7 @@ func (d *drv) openConn(P ConnectionParams) (*conn, error) {
 	if P.IsSysDBA || P.IsSysOper {
 		dc := C.malloc(C.sizeof_void)
 		if Log != nil {
-			Log("C", "dpiConn_create", "username", P.Username, "password", P.Password, "sid", P.SID, "common", commonCreateParams, "conn", connCreateParams)
+			Log("C", "dpiConn_create", "username", P.Username, "password", "SECRET", "sid", P.SID, "common", commonCreateParams, "conn", connCreateParams)


Just delete the printing of "password", and its value - this provides no information.

tgulacsi · 2018-07-18T17:39:41Z

drv.go

@@ -457,7 +455,7 @@ func (d *drv) openConn(P ConnectionParams) (*conn, error) {
 			&connCreateParams,
 			(**C.dpiConn)(unsafe.Pointer(&dc)),
 		) == C.DPI_FAILURE {
-			return nil, errors.Wrapf(d.getError(), "username=%q password=%q sid=%q params=%+v", P.Username, P.Password, P.SID, connCreateParams)
+			return nil, errors.Wrapf(d.getError(), "username=%q password=%q sid=%q params=%+v", P.Username, "SECRET", P.SID, connCreateParams)


Just leave it out.

tgulacsi · 2018-07-18T17:39:53Z

drv.go

@@ -480,7 +478,7 @@ func (d *drv) openConn(P ConnectionParams) (*conn, error) {

 	var dp *C.dpiPool
 	if Log != nil {
-		Log("C", "dpiPool_create", "username", P.Username, "password", P.Password, "sid", P.SID, "common", commonCreateParams, "pool", poolCreateParams)
+		Log("C", "dpiPool_create", "username", P.Username, "password", "SECRET", "sid", P.SID, "common", commonCreateParams, "pool", poolCreateParams)


Just leave it out.

tgulacsi · 2018-07-18T17:40:08Z

drv.go

@@ -492,7 +490,7 @@ func (d *drv) openConn(P ConnectionParams) (*conn, error) {
 		(**C.dpiPool)(unsafe.Pointer(&dp)),
 	) == C.DPI_FAILURE {
 		return nil, errors.Wrapf(d.getError(), "username=%q password=%q SID=%q minSessions=%d maxSessions=%d poolIncrement=%d extAuth=%d ",
-			P.Username, strings.Repeat("*", len(P.Password)), P.SID,
+			P.Username, "SECRET", P.SID,


Just leave it out.

tgulacsi · 2018-07-18T17:42:03Z

drv.go

@@ -364,12 +364,12 @@ func (d *drv) openConn(P ConnectionParams) (*conn, error) {
 	}

 	c := conn{drv: d, connParams: P}
-	connString := P.StringNoClass()
+	connString := P.string(false, true)


We don't need a new pool per-password, so we don't need the password in the connString, so we don't need the second parameter in P.string.

Ok, in that case I don't think the second param is needed at all and string can always leave out the password. I'll push an update shortly.

mkenney · 2018-07-18T18:25:45Z

Ok, I made those updates and connections look good in my test script. Let me know what you think, thanks!

tgulacsi · 2018-07-18T20:06:42Z

Thanks for your hard work!

mkenney · 2018-07-18T22:13:12Z

Awesome, thank you for your help!

mkenney added 2 commits July 12, 2018 21:11

https://github.com/go-goracle/goracle/issues/79

68aaaa1

Merge pull request #1 from ReturnPath/issue-79

a2f5bb9

#79

tgulacsi reviewed Jul 14, 2018

View reviewed changes

tgulacsi suggested changes Jul 14, 2018

View reviewed changes

Updates based on tgulacsi's feedback

4382678

tgulacsi suggested changes Jul 18, 2018

View reviewed changes

Updates based on tgulacsi's feedback

baa776e

revert one of the updates

8f46af4

tgulacsi approved these changes Jul 18, 2018

View reviewed changes

tgulacsi merged commit 65f0503 into go-goracle:master Jul 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scrub passwords from error output #81

Scrub passwords from error output #81

mkenney commented Jul 13, 2018

mkenney commented Jul 13, 2018

tgulacsi left a comment

tgulacsi left a comment

mkenney commented Jul 14, 2018 •

edited

Loading

tgulacsi commented Jul 18, 2018

tgulacsi commented Jul 18, 2018

mkenney commented Jul 18, 2018

mkenney commented Jul 18, 2018

tgulacsi commented Jul 18, 2018

mkenney commented Jul 18, 2018 •

edited

Loading

tgulacsi Jul 18, 2018

tgulacsi Jul 18, 2018

tgulacsi Jul 18, 2018

tgulacsi Jul 18, 2018

tgulacsi Jul 18, 2018

mkenney Jul 18, 2018

mkenney commented Jul 18, 2018

tgulacsi commented Jul 18, 2018

mkenney commented Jul 18, 2018

Scrub passwords from error output #81

Scrub passwords from error output #81

Conversation

mkenney commented Jul 13, 2018

mkenney commented Jul 13, 2018

tgulacsi left a comment

Choose a reason for hiding this comment

tgulacsi left a comment

Choose a reason for hiding this comment

mkenney commented Jul 14, 2018 • edited Loading

tgulacsi commented Jul 18, 2018

tgulacsi commented Jul 18, 2018

mkenney commented Jul 18, 2018

mkenney commented Jul 18, 2018

tgulacsi commented Jul 18, 2018

mkenney commented Jul 18, 2018 • edited Loading

tgulacsi Jul 18, 2018

Choose a reason for hiding this comment

tgulacsi Jul 18, 2018

Choose a reason for hiding this comment

tgulacsi Jul 18, 2018

Choose a reason for hiding this comment

tgulacsi Jul 18, 2018

Choose a reason for hiding this comment

tgulacsi Jul 18, 2018

Choose a reason for hiding this comment

mkenney Jul 18, 2018

Choose a reason for hiding this comment

mkenney commented Jul 18, 2018

tgulacsi commented Jul 18, 2018

mkenney commented Jul 18, 2018

mkenney commented Jul 14, 2018 •

edited

Loading

mkenney commented Jul 18, 2018 •

edited

Loading