Skip to content
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

fix broken zone cloning due to OpenSSH DSA deprecation #503

Merged
merged 2 commits into from
Dec 4, 2024

Conversation

snltd
Copy link

@snltd snltd commented Nov 24, 2024

Trying to clone a lipkg zone (and, I assume, any other brand) currently fails with

ERROR: Zone unconfiguration failed

This is because the system-unconfigure script attempts to use ssh-keygen to generate a DSA key. DSA keys have been removed from OpenSSH, so the script errors.

This change creates an ECDSA key instead. Perhaps DSA was only ever in there for backward compatibility, so I leave it to the more knowledgeable reviewer to decide if a more correct fix would be to simply remove the DSA lines altogether.

I ran the test suite and had a few permissions-based failures (I don't have a proper OmniOS build env at the moment) but nothing which looked related to this change.

oetiker
oetiker previously approved these changes Nov 25, 2024
Copy link
Member

@oetiker oetiker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding ecdsa instead seems like a sensible move

Copy link
Member

@citrus-it citrus-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this and submitting the change. It's been a while since I've looked at this file and so I have a couple of suggestions to future proof things in here.

If you prefer, we can take your change as it is, as it certainly improves the situation, and then follow up with the rest.

Comment on lines 140 to 145
/usr/bin/ssh-keygen -q -t rsa -b 2048 -N '' -C root@unknown \
-f $ALTROOT/etc/ssh/ssh_host_rsa_key \
|| bomb "Failed to create new $ALTROOT/etc/ssh/ssh_host_rsa_key"
/usr/bin/ssh-keygen -q -t dsa -N '' -C root@unknown \
-f $ALTROOT/etc/ssh/ssh_host_dsa_key \
/usr/bin/ssh-keygen -q -t ecdsa -b 521 -N '' -C root@unknown \
-f $ALTROOT/etc/ssh/ssh_host_ecdsa_key \
|| bomb "Failed to create new $ALTROOT/etc/ssh/ssh_host_dsa_key"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point I think we should replace all of these with

/usr/bin/ssh-keygen -A -f $ALTROOT

That way we'll end up with the same set of keys as a freshly installed system, and generated with the same options.

We should also do something more generic with the old key renaming as it shouldn't need to know what the valid set of key types is, and that's even harder across upgrades.
This is already missing ed25519 for example.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as above. Here's a zone cloned with the new script:

root@serv-cron:~# ls -l /etc/*ssh*
total 161
-rw-r--r--   1 root     bin       541716 Dec  1 22:36 moduli
-rw-r--r--   1 root     bin         1526 Dec  1 22:36 ssh_config
-rw-------   1 root     root         505 Dec  1 23:11 ssh_host_ecdsa_key
-rw-r--r--   1 root     root         176 Dec  1 23:11 ssh_host_ecdsa_key.pub
-rw-------   1 root     root         411 Dec  1 23:11 ssh_host_ed25519_key
-rw-r--r--   1 root     root          96 Dec  1 23:11 ssh_host_ed25519_key.pub
-rw-------   1 root     root        2602 Dec  1 23:11 ssh_host_rsa_key
-rw-r--r--   1 root     root         568 Dec  1 23:11 ssh_host_rsa_key.pub
-rw-r--r--   1 root     bin         3259 Dec  1 23:11 sshd_config

@citrus-it citrus-it merged commit 0b0f979 into omniosorg:master Dec 4, 2024
1 check passed
@citrus-it
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants