-
Notifications
You must be signed in to change notification settings - Fork 880
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: early hostkey generation #5728
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,9 +8,12 @@ | |
|
||
import logging | ||
import os | ||
import pathlib | ||
import pwd | ||
import subprocess | ||
from contextlib import suppress | ||
from typing import List, Sequence, Tuple | ||
from multiprocessing import Process | ||
from typing import List, NamedTuple, Sequence, Tuple | ||
|
||
from cloudinit import lifecycle, subp, util | ||
|
||
|
@@ -64,6 +67,21 @@ | |
"exit " + str(_DISABLE_USER_SSH_EXIT) + '"' | ||
) | ||
|
||
GENERATE_KEY_NAMES = ["rsa", "ecdsa", "ed25519"] | ||
|
||
KEY_NAME_TPL = "ssh_host_%s_key" | ||
KEY_FILE_TPL = f"/etc/ssh/{KEY_NAME_TPL}" | ||
|
||
|
||
def get_early_host_key_dir(rundir: str): | ||
return pathlib.Path(rundir, "tmp_host_keys") | ||
|
||
|
||
class KeyPair(NamedTuple): | ||
key_type: str | ||
private_path: pathlib.Path | ||
public_path: pathlib.Path | ||
|
||
|
||
class AuthKeyLine: | ||
def __init__( | ||
|
@@ -683,3 +701,116 @@ def get_opensshd_upstream_version(): | |
return upstream_version | ||
except (ValueError, TypeError): | ||
LOG.warning("Could not parse sshd version: %s", upstream_version) | ||
|
||
|
||
def _get_early_key_fifo_path(rundir: str) -> pathlib.Path: | ||
return pathlib.Path(rundir, "ssh-keygen-finished") | ||
|
||
|
||
def _write_and_close(path: pathlib.Path, data: bytes) -> None: | ||
path.write_bytes(data) | ||
path.unlink() | ||
|
||
|
||
def _early_generate_host_keys_body( | ||
rundir: str, early_key_fifo_path: pathlib.Path | ||
) -> None: | ||
key_dir = get_early_host_key_dir(rundir) | ||
key_dir.mkdir(mode=0o600, exist_ok=False) | ||
|
||
for key_type in GENERATE_KEY_NAMES: | ||
path = key_dir / (KEY_NAME_TPL % key_type) | ||
stdout_path = path.with_suffix(".stdout") | ||
stderr_path = path.with_suffix(".stderr") | ||
processes = [] | ||
with open(stdout_path, "w") as stdout, open( | ||
stderr_path, "w" | ||
) as stderr: | ||
try: | ||
# Using subprocess.Popen instead of subp.subp to run | ||
# multiple ssh-keygen commands in parallel. | ||
p = subprocess.Popen( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why generate these in parallel? RSA keygen is time-consuming, but generating the other keys is not, so I'm not convinced that the extra complexity outweighs the benefit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't really seem any more complex to me, but serial is fine too. |
||
[ | ||
"ssh-keygen", | ||
"-t", | ||
key_type, | ||
"-N", | ||
"", | ||
"-f", | ||
path, | ||
], | ||
stdout=stdout, | ||
stderr=stderr, | ||
) | ||
processes.append(p) | ||
except Exception as e: | ||
LOG.warning("Failed to generate %s host key: %s", key_type, e) | ||
for process in processes: | ||
if process.wait() != 0: | ||
_write_and_close(early_key_fifo_path, b"failed") | ||
return | ||
_write_and_close(early_key_fifo_path, b"done") | ||
|
||
|
||
def _early_generate_host_keys( | ||
rundir: str, early_key_fifo_path: pathlib.Path | ||
) -> None: | ||
try: | ||
_early_generate_host_keys_body(rundir, early_key_fifo_path) | ||
except Exception as e: | ||
LOG.warning("Failed to generate host keys: %s", e) | ||
_write_and_close(early_key_fifo_path, b"failed") | ||
|
||
|
||
def start_early_generate_host_keys(rundir: str): | ||
if all( | ||
pathlib.Path(KEY_FILE_TPL % key).exists() for key in GENERATE_KEY_NAMES | ||
): | ||
LOG.debug( | ||
"Existing host keys present; skipping early host key generation" | ||
) | ||
return | ||
early_key_fifo_path = _get_early_key_fifo_path(rundir) | ||
early_key_fifo_path.parent.mkdir(mode=0o700, exist_ok=True) | ||
os.mkfifo(early_key_fifo_path) | ||
try: | ||
Process( | ||
target=_early_generate_host_keys, | ||
args=(rundir, early_key_fifo_path), | ||
daemon=True, | ||
).start() | ||
except Exception as e: | ||
LOG.warning("Failed to start early host key generation: %s", e) | ||
early_key_fifo_path.unlink() | ||
|
||
|
||
def wait_for_early_generated_keys(rundir: str) -> List[KeyPair]: | ||
early_key_fifo_path = _get_early_key_fifo_path(rundir) | ||
if not early_key_fifo_path.exists(): | ||
return [] | ||
if early_key_fifo_path.read_bytes() != b"done": | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If not complete, this will block. It would be good to know if this happens, such as by using |
||
LOG.warning("Failed to retrieve early generated host keys") | ||
return [] | ||
|
||
key_dir = get_early_host_key_dir(rundir) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: why not just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops, I forgot to update the name (shouldn't end with |
||
keys = [] | ||
for key_type in GENERATE_KEY_NAMES: | ||
private_path = key_dir / (KEY_NAME_TPL % key_type) | ||
public_path = private_path.with_suffix(".pub") | ||
if private_path.exists() and public_path.exists(): | ||
keys.append(KeyPair(key_type, private_path, public_path)) | ||
else: | ||
stdout = "" | ||
stderr = "" | ||
with suppress(FileNotFoundError): | ||
stdout = util.load_text_file(public_path / ".stdout") | ||
with suppress(FileNotFoundError): | ||
stderr = util.load_text_file(private_path / ".stderr") | ||
LOG.warning( | ||
"Failed to find generated host key pair for %s. " | ||
"Stdout: %s. Stderr: %s", | ||
key_type, | ||
stdout, | ||
stderr, | ||
) | ||
return keys |
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.
There is a problem with this that needs to be addressed.
Cloud-init's Azure and Openstack code has automatic entropy seeding, which bypasses cloud-config. This change highlights the fact that cloud-init implements datasource-specific code in configuration modules and in datasource modules. Simply checking the merged configuration is insufficient, because for some reason when this was implemented it was decided that this shouldn't just be transformed into vendor-data to be merged (overwritten by user-data).
I strongly suspect that automatic entropy seeding exists on these platforms for legacy reasons only[1][2], and is no longer needed. However, until this tech debt has been resolved, cloud-init should still respect the entropy provided by these platforms.
[1] torvalds/linux@f2580a9
[2] https://bugs.launchpad.net/nova/+bug/1789868
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.
Aside: I realized this should be
random_seed
rather thanseed_random
. Having the key opposite order from the module name is confusing.To your point, I was aware of datasources doing their own thing here. I saw
random_seed
in the datasources and thought it was using the same key. I didn't realize it's storing the seed under a separate metadata key. Thisif
statement can just be updated to also check ifrandom_seed
is incloud.datasource.metadata
, correct?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.
Yes, I believe so