Garry Zacheiss
2002-May-29 04:29 UTC
[PATCH] Add config option disabling drop_connection() behavior
The patch below (against openssh 3.2.3p1) adds a
CheckMaxStartups option, defaulting to yes, to determine whether sshd
calls drop_connection().
The motivation behind this is twofold. In our environment, our
timesharing machines get enough incoming connections that will trigger
spuriously with the default value (10 forked unauthenticated
connections) as well as some significantly higher values, and I'd rather
disable this feature than just configure it to some ridiculously high
value.
A secondary motivation is that this code is sometimes triggered
when the machine's AFS client has gotten into a broken state (forked
sshd tries to touch AFS for user homedir, loses), and I've already had
at least one coworker get dragged down the wrong debugging path and "try
to debug why sshd is accepting new connections and immediately dropping
them" when the real problem the machine is experiencing is different.
It didn't seem like being able to selectively disable this
feature would be a bad thing, so please consider this patch for
inclusion in a future version of OpenSSH.
I'm not currently subscribed to this list, so please cc me on
any replies. Thanks in advance for your consideration.
Garry
--- servconf.h 2002/05/29 03:50:01 1.1
+++ servconf.h 2002/05/29 03:50:53
@@ -112,6 +112,7 @@
char *subsystem_name[MAX_SUBSYSTEMS];
char *subsystem_command[MAX_SUBSYSTEMS];
+ int check_max_startups;
int max_startups_begin;
int max_startups_rate;
int max_startups;
--- servconf.c 2002/05/29 03:49:54 1.1
+++ servconf.c 2002/05/29 03:54:09
@@ -112,6 +112,7 @@
options->protocol = SSH_PROTO_UNKNOWN;
options->gateway_ports = -1;
options->num_subsystems = 0;
+ options->check_max_startups = -1;
options->max_startups_begin = -1;
options->max_startups_rate = -1;
options->max_startups = -1;
@@ -228,6 +229,8 @@
options->allow_tcp_forwarding = 1;
if (options->gateway_ports == -1)
options->gateway_ports = 0;
+ if (options->check_max_startups == -1)
+ options->check_max_startups = 1;
if (options->max_startups == -1)
options->max_startups = 10;
if (options->max_startups_rate == -1)
@@ -281,7 +284,8 @@
sUseLogin, sAllowTcpForwarding,
sAllowUsers, sDenyUsers, sAllowGroups, sDenyGroups,
sIgnoreUserKnownHosts, sCiphers, sMacs, sProtocol, sPidFile,
- sGatewayPorts, sPubkeyAuthentication, sXAuthLocation, sSubsystem,
sMaxStartups,
+ sGatewayPorts, sPubkeyAuthentication, sXAuthLocation, sSubsystem,
+ sCheckMaxStartups, sMaxStartups,
sBanner, sVerifyReverseMapping, sHostbasedAuthentication,
sHostbasedUsesNameFromPacketOnly, sClientAliveInterval,
sClientAliveCountMax, sAuthorizedKeysFile, sAuthorizedKeysFile2,
@@ -353,6 +357,7 @@
{ "protocol", sProtocol },
{ "gatewayports", sGatewayPorts },
{ "subsystem", sSubsystem },
+ { "checkmaxstartups", sCheckMaxStartups },
{ "maxstartups", sMaxStartups },
{ "banner", sBanner },
{ "verifyreversemapping", sVerifyReverseMapping },
@@ -835,6 +840,10 @@
options->num_subsystems++;
break;
+ case sCheckMaxStartups:
+ intptr = &options->check_max_startups;
+ goto_parse_flag;
+
case sMaxStartups:
arg = strdelim(&cp);
if (!arg || *arg == '\0')
--- sshd.8 2002/05/29 03:50:10 1.1
+++ sshd.8 2002/05/29 03:54:38
@@ -656,6 +656,11 @@
Multiple algorithms must be comma-separated.
The default is
.Dq hmac-md5,hmac-sha1,hmac-ripemd160,hmac-sha1-96,hmac-md5-96 .
+.It Cm CheckMaxStartups
+Specifies whether the server should check the number of concurrent
+unauthenticated connections to the daemon, and drop new incoming
+connections if this number exceeds some threshold. See the
+"MaxStartups" configuration option for more information.
.It Cm MaxStartups
Specifies the maximum number of concurrent unauthenticated connections to the
.Nm
--- sshd.c 2002/05/29 03:50:13 1.1
+++ sshd.c 2002/05/29 03:55:59
@@ -1243,7 +1243,8 @@
close(newsock);
continue;
}
- if (drop_connection(startups) == 1) {
+ if (options.check_max_startups &&
+ drop_connection(startups) == 1) {
debug("drop connection #%d", startups);
close(newsock);
continue;
Ben Lindstrom
2002-May-29 05:10 UTC
[PATCH] Add config option disabling drop_connection() behavior
I'd rather see the following applied before yours. Mainly because
I don't want 'Yet another Fine Option' floating arounding. Plus
it touchs less code and acts the way 90% of what people expect.
The more options you provide the more chances someone will fuck up.
Besides this follows the KISS concept. =)
- Ben
[Against -current portable]
Index: sshd.c
==================================================================RCS file:
/var/cvs/openssh/sshd.c,v
retrieving revision 1.207
diff -u -r1.207 sshd.c
--- sshd.c 21 May 2002 17:59:13 -0000 1.207
+++ sshd.c 29 May 2002 05:12:07 -0000
@@ -721,6 +721,10 @@
{
double p, r;
+ /* If Max Startup is zero, then the feature is disabled */
+ if (options->max_startups == 0)
+ return 0;
+
if (startups < options.max_startups_begin)
return 0;
if (startups >= options.max_startups)
On Wed, 29 May 2002, Garry Zacheiss wrote:
> The patch below (against openssh 3.2.3p1) adds a
> CheckMaxStartups option, defaulting to yes, to determine whether sshd
> calls drop_connection().
>
> The motivation behind this is twofold. In our environment, our
> timesharing machines get enough incoming connections that will trigger
> spuriously with the default value (10 forked unauthenticated
> connections) as well as some significantly higher values, and I'd
rather
> disable this feature than just configure it to some ridiculously high
> value.
>
> A secondary motivation is that this code is sometimes triggered
> when the machine's AFS client has gotten into a broken state (forked
> sshd tries to touch AFS for user homedir, loses), and I've already had
> at least one coworker get dragged down the wrong debugging path and
"try
> to debug why sshd is accepting new connections and immediately dropping
> them" when the real problem the machine is experiencing is different.
>
> It didn't seem like being able to selectively disable this
> feature would be a bad thing, so please consider this patch for
> inclusion in a future version of OpenSSH.
>
> I'm not currently subscribed to this list, so please cc me on
> any replies. Thanks in advance for your consideration.
>
> Garry
>
> --- servconf.h 2002/05/29 03:50:01 1.1
> +++ servconf.h 2002/05/29 03:50:53
> @@ -112,6 +112,7 @@
> char *subsystem_name[MAX_SUBSYSTEMS];
> char *subsystem_command[MAX_SUBSYSTEMS];
>
> + int check_max_startups;
> int max_startups_begin;
> int max_startups_rate;
> int max_startups;
> --- servconf.c 2002/05/29 03:49:54 1.1
> +++ servconf.c 2002/05/29 03:54:09
> @@ -112,6 +112,7 @@
> options->protocol = SSH_PROTO_UNKNOWN;
> options->gateway_ports = -1;
> options->num_subsystems = 0;
> + options->check_max_startups = -1;
> options->max_startups_begin = -1;
> options->max_startups_rate = -1;
> options->max_startups = -1;
> @@ -228,6 +229,8 @@
> options->allow_tcp_forwarding = 1;
> if (options->gateway_ports == -1)
> options->gateway_ports = 0;
> + if (options->check_max_startups == -1)
> + options->check_max_startups = 1;
> if (options->max_startups == -1)
> options->max_startups = 10;
> if (options->max_startups_rate == -1)
> @@ -281,7 +284,8 @@
> sUseLogin, sAllowTcpForwarding,
> sAllowUsers, sDenyUsers, sAllowGroups, sDenyGroups,
> sIgnoreUserKnownHosts, sCiphers, sMacs, sProtocol, sPidFile,
> - sGatewayPorts, sPubkeyAuthentication, sXAuthLocation, sSubsystem,
sMaxStartups,
> + sGatewayPorts, sPubkeyAuthentication, sXAuthLocation, sSubsystem,
> + sCheckMaxStartups, sMaxStartups,
> sBanner, sVerifyReverseMapping, sHostbasedAuthentication,
> sHostbasedUsesNameFromPacketOnly, sClientAliveInterval,
> sClientAliveCountMax, sAuthorizedKeysFile, sAuthorizedKeysFile2,
> @@ -353,6 +357,7 @@
> { "protocol", sProtocol },
> { "gatewayports", sGatewayPorts },
> { "subsystem", sSubsystem },
> + { "checkmaxstartups", sCheckMaxStartups },
> { "maxstartups", sMaxStartups },
> { "banner", sBanner },
> { "verifyreversemapping", sVerifyReverseMapping },
> @@ -835,6 +840,10 @@
> options->num_subsystems++;
> break;
>
> + case sCheckMaxStartups:
> + intptr = &options->check_max_startups;
> + goto_parse_flag;
> +
> case sMaxStartups:
> arg = strdelim(&cp);
> if (!arg || *arg == '\0')
> --- sshd.8 2002/05/29 03:50:10 1.1
> +++ sshd.8 2002/05/29 03:54:38
> @@ -656,6 +656,11 @@
> Multiple algorithms must be comma-separated.
> The default is
> .Dq hmac-md5,hmac-sha1,hmac-ripemd160,hmac-sha1-96,hmac-md5-96 .
> +.It Cm CheckMaxStartups
> +Specifies whether the server should check the number of concurrent
> +unauthenticated connections to the daemon, and drop new incoming
> +connections if this number exceeds some threshold. See the
> +"MaxStartups" configuration option for more information.
> .It Cm MaxStartups
> Specifies the maximum number of concurrent unauthenticated connections to
the
> .Nm
> --- sshd.c 2002/05/29 03:50:13 1.1
> +++ sshd.c 2002/05/29 03:55:59
> @@ -1243,7 +1243,8 @@
> close(newsock);
> continue;
> }
> - if (drop_connection(startups) == 1) {
> + if (options.check_max_startups &&
> + drop_connection(startups) == 1) {
> debug("drop connection #%d", startups);
> close(newsock);
> continue;
> _______________________________________________
> openssh-unix-dev at mindrot.org mailing list
> http://www.mindrot.org/mailman/listinfo/openssh-unix-dev
>
Garry Zacheiss
2002-Jun-03 03:57 UTC
[PATCH] Add config option disabling drop_connection() behavior
>> I'd rather see the following applied before yours. Mainly because >> I don't want 'Yet another Fine Option' floating arounding. Plus >> it touchs less code and acts the way 90% of what people expect. >> >> The more options you provide the more chances someone will fuck up. >> Besides this follows the KISS concept. =) >> >> - BenThe patch you propose would be fine with me; I'm not particularly wedded to the way I implemented it as much as I want the functionality, and would prefer to have it in OpenSSH rather than keeping it as a local modification. Can we expect to see your proposed patch applied to the openssh tree? Garry