Bug 202153

Summary: [PATCH] set ssh-keygen flags in rc.conf for rc.d/sshd
Product: Base System Reporter: Chad Jacob Milios <milios>
Component: confAssignee: freebsd-rc (Nobody) <rc>
Status: Open ---    
Severity: Affects Many People CC: 0mp, bdrewery, feld, jamie, milios, pi, vsasjason
Priority: --- Keywords: patch
Version: 10.2-BETA1   
Hardware: Any   
OS: Any   
See Also: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=202169
Attachments:
Description Flags
adds variables to sshd_keygen() that pass per key-type flags from rc.conf
none
adds variables to sshd_keygen() that pass per key-type flags from rc.conf and documents sshd related vars in defaults/rc.conf
none
adds variables to /etc/rc.d/sshd/sshd_keygen() and document existing and new variables none

Description Chad Jacob Milios 2015-08-07 08:03:40 UTC
Created attachment 159634 [details]
adds variables to sshd_keygen() that pass per key-type flags from rc.conf

/etc/rc.d/sshd has some rc.conf variables that are not documented in /etc/defaults/rc.conf:

sshd_rsa1_enable="YES"
sshd_rsa_enable="YES"
sshd_dsa_enable="YES"
sshd_ecdsa_enable="YES"
sshd_ed25519_enable="YES"

I propose adding more:

sshd_rsa1_flags=""
sshd_rsa_flags=""
sshd_dsa_flags=""
sshd_ecdsa_flags=""
sshd_ed25519_flags=""

My rc.conf.local for instance contains:

sshd_rsa1_enable="NO"
sshd_dsa_enable="NO"
sshd_rsa_flags="-b 4096"
sshd_ecdsa_flags="-b 521"
Comment 1 Chad Jacob Milios 2015-08-07 08:17:42 UTC
Created attachment 159635 [details]
adds variables to sshd_keygen() that pass per key-type flags from rc.conf and documents sshd related vars in defaults/rc.conf

Alternately to that tiny patch and undocumented variables, here's a slightly different patch I think is much better which clarifies the existing variable names and adds similar new variable names while documenting what they all do so the hasty admin has a fighting chance at realizing he cannot disable a key simply through rc.conf once the default action has been run through even just one time. Please inspect both and consider testing either for speedy inclusion into 10.2-RELEASE I hope.
Comment 2 Chad Jacob Milios 2015-08-07 08:32:57 UTC
The current variable names as they sit could be considered a security vulnerability since $sshd_rsa1_enable and $sshd_dsa_enable sure sound like they control use of RSA1 and DSA in sshd but actually they do not and setting any such variables to "NO" or "-b 4096" will not have the expected result if sshd was once ever run before.

I think it's important we deprecate those names in favor of clearer ones and add quality description to defaults/rc.conf. Heck, committer, maybe even go ahead and throw a blank line before and after that block of sshd_ lines please since it's now 13 lines instead of 3.

Thanks greatly for your time and consideration. Let me know if I should add a patch for man rc.conf(5) as well and I will go figure out how to work the mandoc or nroff or troff or whatever. I am hoping perhaps someone can lead me by copying my comments in this new defaults/rc.conf into man rc.conf(5) as well or tell me how that's done neatly.
Comment 3 Chad Jacob Milios 2015-08-07 15:09:55 UTC
Created attachment 159642 [details]
adds variables to /etc/rc.d/sshd/sshd_keygen() and document existing and new variables

Better said here I think.
Comment 4 Jamie Landeg-Jones 2015-08-07 22:11:56 UTC
I'm a nobody , but for what it's worth, I like it 6both the flags and the name change)
Comment 5 Jamie Landeg-Jones 2015-08-07 22:20:29 UTC
Incidentally, if you use openssh from ports, it uses openssh_xxxx, not sshd_xxxx based off file /usr//local/etc/rc.d/openssh so maybe you'd want to make the proposal there too?
Comment 6 Chad Jacob Milios 2015-08-08 02:41:03 UTC
Thanks Jamie! I created https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=202169 with a patch to implement identical functionality. If you use openssh-portable from ports could you test it out through a few configs please anyone?

I'll add it to openssh-portable-devel in a moment if I hear positive reports from somebody.
Comment 7 Mark Felder freebsd_committer freebsd_triage 2015-08-09 12:56:37 UTC
I like the idea of permitting this configuration, so you have my vote. The base sshd maintainer is des, so hopefully he can review this soon.
Comment 8 Mark Felder freebsd_committer freebsd_triage 2015-08-09 12:59:34 UTC
adding bdrewery as a CC as he maintains the openssh in ports and might have valuable input here too
Comment 9 Mark Linimon freebsd_committer freebsd_triage 2015-08-09 18:43:32 UTC
Although I'm not on re@, you should know that 10.2 is very, very, far along into the release process.
Comment 10 Chad Jacob Milios 2015-08-09 20:51:20 UTC
I know I'm super late here and don't necessarily expect mountains to move just for me to get this into 10.2. Believe me I'm feeling sorry it didn't occur to me to share this much sooner. I'm sure there are thousands more important considerations for re@ to deal with besides this trivial tweak.

Still, maybe it's just me but some of the things I've seen people do in the wild to try and accomplish the same thing though, just to get a few more bits of feel-good security... and in the process avoid (or unaware of how) to hack up rc scripts. Tell me you've seen it too: Either they create keys ahead of time as part of their deploy/install process, often wide open to vastly larger contexts of space/time/access/attack, or, they'll let the first keys auto-generate then delete some/all just to regenerate their "better" keys at a time when the system is basically starved for entropy already.

Such anti-solutions are brittle too. Imagine that maybe in the future FreeBSD wants to incorporate different behavior regarding the making/handling/storing of the keys, say it involves GELI or PEFS or something currently unforeseen like new hardware, whatever, who knows. Any possible features like that would be very hard to add if rc had no clue of the user's key preferences and how they had hacked their way into place. A tiny bit of simple groundwork laid early like these sshd_foo_keygen_flags would increase flexibility and allow people to retire their one-off hacks and workarounds, starting now.

I'm a huge advocate of ideas like sysrc and that universal config language project Allan Jude et al are working on and can't wait till we can drive the final nail in the coffin of mergemaster. :)

Hopefully this is small, straightforward and clear cut enough that even though it's not a huge deal or show-stopping bug, it'll benefit enough people to see it ship with 10.2 (especially in light of people lately being made more aware of security and wanting options, for technical and psychological reasons). C'mon re@ committers, give the people what they want, pretty please? :)

10.2 is the first 10.x a lot of the grumpy ol' legacy stalwarts like myself are finally considering putting into production and I bet I'll be seeing 10.2 systems for the better part of a decade to come. Surely during that time one of these key exchange algorithms is going to fall out of favor. Someone will want to sshd_keytype_enable="NO" and surely we can see that's a misleading variable name and deserves a quick mention in defaults/rc.conf.

If this just gets in HEAD some day though eventually, old or new variable names, then I'll be very happy I don't have to edit rc.d/sshd each deploy and hear mergemaster complain for many more years.

(Just realized maybe English is clearer than Yoda, haha:
"Generate RSA keys if missing from /etc/sshd when starting sshd." vs.
"Generate RSA keys when starting sshd if missing from /etc/sshd."
Feel free to swap as such or take any other liberties with my patch.)

Many thank yous so much again to all those working hard and tirelessly to keep making FreeBSD trusted to mean world-class stability security and features!