Multiple issues with special characters in passwords

[amhuber]

1. The following template entries are not shell escaping passwords:

https://github.com/cloudfoundry-incubator/diego-release/blob/develop/jobs/route_emitter/templates/route_emitter_ctl.erb#L42

https://github.com/cloudfoundry-incubator/diego-release/blob/develop/jobs/ssh_proxy/templates/ssh_proxy_ctl.erb#L62

2. In addition, the ssh_proxy secret would probably fail if there was a "@" in it from looking at the code, it may need to be URL encoded in the config as well. Automated tests need to be improved to actually test special characters in all passwords.

3. We also had a problem with cc_uploader not working when there is a special character in the staging_upload_password in the Cloud Controller config. When looking at the upload POST sent by DEA the password is URL encoded (before being Base64 encoded) but this is not being done by cc_uploader. My guess is that if URL encoding is added to this line it might fix the problem but testing would be needed:

https://github.com/cloudfoundry-incubator/cc-uploader/blob/master/ccclient/uploader.go#L44

In our test case a "$" in the password caused it to break during staging with an upload error, and when testing with an "@" and "!" in the password, part of the password was actually returned to the client in the error message:

Server error, status code: 500, error code: 170011, message: Stager error: bad component(expected user component): <passwordhere>!!(MISSING)!(MISSING)

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/111624030.

[emalm]

Thanks very much for the thorough report, @amhuber. I've added https://www.pivotaltracker.com/story/show/111846095 and https://www.pivotaltracker.com/story/show/111846711 and revised the existing https://www.pivotaltracker.com/story/show/109642196 to address these three specific issues you've discovered. I've also added https://www.pivotaltracker.com/story/show/111846405 for the team to review any other places where credentials may be mis-encoded in ctl scripts or in the Diego components themselves. We'll discuss these in our team IPM this coming Monday and will then prioritize them highly.

Thanks again,
Eric, CF Runtime Diego PM

[SocalNick]

@amhuber we are having a tough time reproducing the "part of the password was actually returned to the client in the error message". We changed our password to some-p@!ssword and looked through logs in the cloud_controller_ng (cf-release), cc_bridge(diego-release), and cell (diego-release) VMs and could not find that log message. Could you provide any more information for us to reproduce?

[amhuber]

I'll try to recreate that now assuming I can recall what password it was. The error message that included part of the password was returned by the CLI as part of the streaming log output on starting the app so it wasn't hard to find.

[amhuber]

I can't reproduce it now. I have since moved to a newer Diego release (0.1447.0) so it's possible something else changed or I just can't recall the specific sequence of characters that triggered it. In any case, the root issue is that the special characters weren't being escaped or encoded so as long as they work correctly then I'm confident that it won't happen again. On 1447 I'm still able to get it to fail when using special characters but the error message doesn't include the password:

 Staging complete
 Uploading droplet, build artifacts cache...
 Uploading build artifacts cache...
 Uploading droplet...
 Failed to upload build artifacts cache
 Failed to upload droplet
 Uploading failed

 FAILED
 StagingError

[emalm]

Thanks for the additional reproduction attempt, @amhuber. @SocalNick and @adowns01 would be able to comment in more detail, but we think the cc-uploader issue is really a problem with how CC encodes the staging credentials before it sends them to the backend. We've raised the issue with the CAPI team in https://www.pivotaltracker.com/story/show/112847097 and will work with them to get the fix prioritized.

Thanks again,
Eric

[dieucao]

@amhuber The fix for this should be a part of cf-release v231.

[emalm]

The ssh-proxy and route-emitter bug stories were previously accepted as fixed, so I'm closing this issue out. Thanks again, @amhuber!

Best,
Eric

[amhuber]

Thanks, I'll be testing everything thoroughly once v231 is officially released.

[emalm]

Looks like there are some continuing edge cases that fail: We've added https://www.pivotaltracker.com/story/show/114707747 to the CAPI tracker to address them. CC isn't even able to send its staging request to the Diego stager component, though, so this doesn't seem to involve the CC-Uploader any more.

Thanks,
Eric

[amhuber]

Ah, that was the error message I got that had the password included as part of the error message sent to the CLI so it looks like you found a repro for that. :-( Definitely need to make sure that whatever happens the password isn't being sent back to the client.

Aaron

[emalm]

Yeah, I've added context to that story to illustrate explicitly that the password is visible to end-users of the CLI.