[PATCH] Please review this rc.d/sshd tiny yet ripe low hanging fruit for me.

[Chad J. Milios]

it’s no cannon but i did find a foot-aimed .22 in rc.d/ssh i think deserves some red paint right away. and while in the neighborhood i’d like to share an improvement i’ve enjoyed for a while. i apologize for the list-bombing, if i may have a moment of your time:

TLDR:
https://bugs.freebsd.org/bugzilla/attachment.cgi?id=159642&action=diff

Quick Background:
there’s two unrelated issues going on here but i ran across them in close physical, mental and temporal proximity so let’s just knock them out together because the solutions are simple, small, and don’t modify today's default behavior.

My Concerns:
ONE is adding functionality allowing an admin to tweak the key generation sshd makes upon its first run using variables in rc.conf instead of the current day requirement of essentially manually generating those keys, hopefully the same way, putting them hopefully in the right place. (not hard for most of us, i know.) TWO, then, is adding some sort of red paint to a foot-aimed gun i came across when considering the variable names in rc.d/sshd and lack of mention in defaults/rc.conf or man 5 rc.conf.

My Plea:
please put this into head and 10/stable and please 10.2-RC as well. i know it’s really late for that but seriously if anyone can find anything wrong with this i’ll send them $10 now just for chiming in or $20 now for improving/fixing it. hopefully you’ll agree it’s pretty simple and solid as-is and you'll participate and say so for the warm feeling. :)

Full Story:
i’ve been running the following tweak to my /etc/rc.d/sshd for four years on dozens of machines and apparently applying it on most installations i do, 10 seconds of my time per, has thus far been less pain than writing this PR and email, LOL (most times i don’t even copy it from anywhere besides my temporal lobe, it’s such a tiny mod):

--- /usr/src/etc/rc.d/sshd	2014-11-15 00:04:00.000000000 +0000
+++ /etc/rc.d/sshd	2015-07-14 23:24:11.426005726 +0000
@@ -56,8 +56,9 @@
 	if [ -f "${keyfile}" ] ; then
 		info "$ALG host key exists."
 	else
+		eval keygen_flags=\$sshd_${alg}_flags
 		echo "Generating $ALG host key."
-		/usr/bin/ssh-keygen -q -t $alg -f "$keyfile" -N ""
+		/usr/bin/ssh-keygen -q -t $alg -f "$keyfile" $keygen_flags -N ""
 		/usr/bin/ssh-keygen -l -f "$keyfile.pub"
 	fi
 }

and then that lets me add this to rc.conf.local, for which i hope my purpose is apparent and goes without saying:

sshd_rsa_flags="-b 4096"
sshd_ecdsa_flags="-b 521”

and thanks to already existing functionality i also use the following in rc.conf.local in every single installation as well:

sshd_rsa1_enable="NO"
sshd_dsa_enable="NO"

so that’s it! that’s my whole functionality improvement i want included upstream, LOL. However, it rubs me as not adequate, not ready for general consumption. There’s something wrong and i couldn’t quite put my finger on it until lately.

Been doing that patch for years and never thought much of it. Always was aware i needed to precede sshd’s first-run with these settings and didn’t give it a second thought after including it in our in-house rollout scripts as well as many one-offs for family/friends. Then, a tech friend/partner/client picked up on those lines disabling rsa1 and dsa with a little bit of monkey-see-monkey-do but never said anything to me about it before applying just those on his 9.3 servers with the stock rc.d/sshd. So i happen to be poking around his hobby rig the other day and i saw the two *_enable="NO" lines in his rc.conf.local BUT all five few-month-old keys nonetheless sitting there in his /etc/sshd/ (!) and, of course, sshd was dutifully responding to hails on the frequencies he intended to have banned.

So he clearly was tripped up on some surprising disappointment of his expectation that sshd_dsa_enable=NO might do as named, as i see quickly skimming the top of rc.d/ssh might lead one to believe, so, i propose changing it to sshd_dsa_keygen_enable, if that name even helps at all, and, more importantly, explaining a bit in the defaults list and rc.conf(5) that: to effectively change any of these [now] ten settings one must then also go delete key files that may already exist. I don’t even know if i’ve accomplished a sufficient explanation in defaults/rc.conf and wonder if someone familiar with man pages might improve upon my terse lines in defaults/rc.conf with detail into rc.conf(5) for us, just documenting sshd_*_keygen_enable and sshd_*_keygen_flags to make it very clear they are put into effect at a particular moment in time and to effect a change one must manually invoke conditions leading again to that particular circumstance (delete the key so it can regenerate).

I know that might seem obvious to most here but it’s not currently intuitive to those learning. This guy’s a bright protege and i had to feel bad after i thwacked him in the head with my bo-staff for having his RSA1 and DSA zippers open because the more i thought about it the more i realized that as it sits it’s sort of counter-intuitive.

Conclusion:
so i’ve improved that years-old tiny patch with slightly better variable names for the existing vars, plus my own added vars renamed to follow form, as well as added hopefully meaningful if terse description to defaults/rc.conf while mapping the old existing names in rc.d/sshd to the better names along with issuing a warning about them if they’re set. any of those decisions/details are optional, i just want the original functionality of my original tiny patch (aforementioned, inlined) in head at least, if not my full patch (link, top of message) into 10.2-RC asap.

Thanks everyone so much for your efforts on FreeBSD and hopefully I haven’t taken up too much of your time pontificating on the color of the little shed for my poodle’s mini-tricycle and we can quickly agree this is a good morsel of low hanging fruit to improve the security, consistency and usability of FreeBSD at the same time and fast-track it into 10.2, if i’m not being too arrogant and bold in proposing so.

-Chad J. Milios