-
Notifications
You must be signed in to change notification settings - Fork 9
[merged] cloud-init: Fix SSH authentication #74
Conversation
Forgot to add root's own public key to /root/.ssh/authorized_keys.
contrib/cloud-init/part-handler.py
Outdated
| with open(keyfile + '.pub') as inpf: | ||
| with open(authorized_keys, 'a') as outf: | ||
| outf.writelines(inpf.readlines()) | ||
| os.chmod(authorized_keys, stat.S_IRUSR | stat.S_IWUSR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should set the mode as part of the open call so it's atomic. Unfortunately AFAIK Python makes this annoying, you have to drop to os.open I think.
Funny enough I fixed this in Homu recently in servo/homu#18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Will fix in the morning.
Use os.open() so we can specify permissions if creating the output file.
|
⬆️ fixup |
contrib/cloud-init/part-handler.py
Outdated
| authorized_keys = '/root/.ssh/authorized_keys' | ||
| with open(keyfile + '.pub') as inpf: | ||
| with open(authorized_keys, 'a') as outf: | ||
| with os.open(authorized_keys, os.O_APPEND, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe os.open(...) will end up giving you an int as a response and won't work with the with statement by itself. Maybe wrap this with os.fdopen()?
In [11]: type(os.open('/tmp/testing', os.O_APPEND))
Out[11]: int
In [12]: type(open('/tmp/testing', 'a'))
Out[12]: _io.TextIOWrapper
In [13]: with os.fdopen(os.open('/tmp/testing', os.O_APPEND)) as f:
....: print(type(f))
....:
<class '_io.TextIOWrapper'>There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right. Must've botched something in my testing to not catch that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I forgot to disconnect/reconnect the ISO image with that change. D'oh!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's just being human 😆
|
⬆️ Third time's a charm? |
| '-t', 'rsa', '-f', keyfile]) | ||
| except FileNotFoundError: | ||
| print('Missing /usr/bin/ssh-keygen', file=sys.stderr) | ||
| raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Not worth fixing in this PR but I'm OCD about raise providing a class and at least one arg à la:
raise Exception('Missing /usr/bin/ssh-keygen')There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to re-raise the current exception, but maybe I'm doing it wrong?
But it's really just to signal to cloud-init that the script failed so cloud-init can swallow all the error details and log something generic and not very helpful. 😡
|
LGTM @cgwalters-bot try |
Forgot to add root's own public key to /root/.ssh/authorized_keys. Closes: #74 Approved by: <try>
|
☀️ Test successful - travis |
|
📌 Commit 147d29d has been approved by |
This is part of the problem I was having with my cloud-init testing.
Forgot to add root's own public key to
/root/.ssh/authorized_keys.