Use mktemp to create temporary files to hold ssh host keys and authorized keys #6732
Conversation
|
That's quite a security issue |
|
What is quite the security issue and why? |
|
Someone could monitor |
|
I'm happy to put the files into /root instead, but I still don't get the problem with using /tmp. These temporary files are only accessible by root both because of the umask 077. |
|
I would use |
|
Okay, I'm using /run to write temporary files. I think this is good, but I still don't understand domenkozar's security concerns. Granted it is all a little academic since any rogue process can download the private ssh keys right from the metadata server. |
| chmod 600 /root/.ssh/authorized_keys | ||
| echo "obtaining SSH key..." | ||
| mkdir -m 0700 -p /root/.ssh | ||
| AUTH_KEYS=$(${mktemp}) && { |
There was a problem hiding this comment.
What happens when mktemp fails? It seems like it would continue the script?
There was a problem hiding this comment.
Yes you are right. It will skip the block of code that depends on the result of mktemp, which is good, but in the end the service should fail if mktemp fails.
I'll try to redesign the error handling, but if you have a recommended pattern to use in this case please post it. Ideally I want the outer file created by mktemp to clean up even when the inner mktemp fails.
There was a problem hiding this comment.
Actually, how much do we care about handling the case when mktemp fails. We currently don't do anything when wget fails. What do you think? Would you like me to add error messages for all the failing possibilities?
There was a problem hiding this comment.
Service scripts run with the -e shell option, so execution should halt at the first unhandled non-zero return code. Replacing the mktemp && { ... } construct with a regular command sequence should give you the desired behaviour. The branch is pointless anyway: either mktemp succeeds and you're good to go or it fails and you might as well crash immediately. Cleanup seems unnecessary if you're writing to /run.
There was a problem hiding this comment.
I understand. I can also get rid of checking return value of wget as well since if wget does fail, it will never get to the next line. I will also change the check of the file existence to check that the file is non-zero in size instead, since mktemp makes sure the file exists.
There was a problem hiding this comment.
The difference is that /run is not world-writable, so you don't have to worry about malicious users.
There was a problem hiding this comment.
@edolstra ah, of course, thanks for clarifying that for me.
There was a problem hiding this comment.
Okay, I'm still working on replacing the mktemp && { ... } construct.
I wanted to clarify how does an adversary race temp files? Isn't the whole point of mktemp to prevent races?
There was a problem hiding this comment.
Yeah that's true, mktemp makes it hard/impossible to create the temp file before an adversary can create it.
There was a problem hiding this comment.
Okay, I've removed the && { ... }. It should be good now.
…ized keys when downloading them from the metadata server.
Scripts are run with -e so will abort when a command fails.
|
Okay, I've removed the && { ... }. It should be good now. |
Use mktemp to create temporary files to hold ssh host keys and authorized keys
|
Thanks! |
@oconnorr you can restrict access to the metadata service via |
|
@copumpkin that's a great idea! On Tue, Jun 2, 2015, 9:10 PM Daniel Peebles notifications@github.com
|
Currently I'm making the temporary files in $TMP which is /tmp. I could also put them in /root, but I couldn't think of any reason why I should do that. Have any preferences?