| Message ID | 20220917231030.22565-1-a@unstable.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] implement --session-timeout | expand |
HI, On Sun, Sep 18, 2022 at 01:10:30AM +0200, Antonio Quartulli wrote: > From: Dmitry Zelenkovsky <Dmitry.Zelenkovskiy@nokia.com> > > Disconnect clients after session-timeout expires. > session-timeout can be defined in ccd files in order to limit > per-user connection time. I find this implementation needlessly complicated. > +/* > + * Should we exit due to session timeout? > + */ > +static void > +check_session_timeout(struct context *c) > +{ > + if (c->options.session_timeout > + && event_timeout_trigger(&c->c2.session_interval, &c->c2.timeval, > + ETT_DEFAULT)) > + { > + msg(M_INFO, "Session timeout, exiting"); > + register_signal(c, SIGTERM, "session-timeout"); > + } > +} Why are we working with event triggers here, if all we *want* to do is a single-shot if ( now > $somectx->session_must_end_at_this_time ) { /* kick out this user now */ ... } > + else if (streq(p[0], "session-timeout") && p[1] && !p[2]) > + { > + VERIFY_PERMISSION(OPT_P_TIMER); OPT_P_INSTANCE? gert
On 18/09/2022 12:37, Gert Doering wrote: > HI, > > On Sun, Sep 18, 2022 at 01:10:30AM +0200, Antonio Quartulli wrote: >> From: Dmitry Zelenkovsky <Dmitry.Zelenkovskiy@nokia.com> >> >> Disconnect clients after session-timeout expires. >> session-timeout can be defined in ccd files in order to limit >> per-user connection time. > > I find this implementation needlessly complicated. > >> +/* >> + * Should we exit due to session timeout? >> + */ >> +static void >> +check_session_timeout(struct context *c) >> +{ >> + if (c->options.session_timeout >> + && event_timeout_trigger(&c->c2.session_interval, &c->c2.timeval, >> + ETT_DEFAULT)) >> + { >> + msg(M_INFO, "Session timeout, exiting"); >> + register_signal(c, SIGTERM, "session-timeout"); >> + } >> +} > > Why are we working with event triggers here, if all we *want* to do is > a single-shot > > if ( now > $somectx->session_must_end_at_this_time ) > { > /* kick out this user now */ > ... > } > we don't get here at all, if we have no event object that is timing out. So, although one shot, we still need to setup a timer object that will trigger the machinery upon timeout. >> + else if (streq(p[0], "session-timeout") && p[1] && !p[2]) >> + { >> + VERIFY_PERMISSION(OPT_P_TIMER); > > OPT_P_INSTANCE? makes sense to add OPT_P_INSTANCE, although I wonder why other activity/timeout knobs are not marks as such, i.e. --inactivity) Cheers, > > gert > > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Hi, On Mon, Sep 19, 2022 at 12:29:20AM +0200, Antonio Quartulli wrote: > On 18/09/2022 12:37, Gert Doering wrote: > > On Sun, Sep 18, 2022 at 01:10:30AM +0200, Antonio Quartulli wrote: > >> From: Dmitry Zelenkovsky <Dmitry.Zelenkovskiy@nokia.com> > >> > >> Disconnect clients after session-timeout expires. > >> session-timeout can be defined in ccd files in order to limit > >> per-user connection time. > > > > I find this implementation needlessly complicated. > > > >> +/* > >> + * Should we exit due to session timeout? > >> + */ > >> +static void > >> +check_session_timeout(struct context *c) > >> +{ > >> + if (c->options.session_timeout > >> + && event_timeout_trigger(&c->c2.session_interval, &c->c2.timeval, > >> + ETT_DEFAULT)) > >> + { > >> + msg(M_INFO, "Session timeout, exiting"); > >> + register_signal(c, SIGTERM, "session-timeout"); > >> + } > >> +} > > > > Why are we working with event triggers here, if all we *want* to do is > > a single-shot > > > > if ( now > $somectx->session_must_end_at_this_time ) > > { > > /* kick out this user now */ > > ... > > } > > > > we don't get here at all, if we have no event object that is timing out. process_coarse_timers() is run once per second, and does not need additional timers to be called. This new check is added to process_coarse_timers(). Why do we need an extra event object, again? I can see the need for event objects for recurring things, but for a single-shot "terminate client instance at this time, done", I'm not convinced. > So, although one shot, we still need to setup a timer object that will > trigger the machinery upon timeout. > >> + else if (streq(p[0], "session-timeout") && p[1] && !p[2]) > >> + { > >> + VERIFY_PERMISSION(OPT_P_TIMER); > > > > OPT_P_INSTANCE? > > makes sense to add OPT_P_INSTANCE, although I wonder why other > activity/timeout knobs are not marks as such, i.e. --inactivity) Not "add" OPT_P_INSTANCE, but "just" OPT_P_INSTANCE. This is not something you want in the client config file. gert
Hi, On Mon, Sep 19, 2022 at 08:36:21AM +0200, Gert Doering wrote: > > we don't get here at all, if we have no event object that is timing out. > > process_coarse_timers() is run once per second, and does not need > additional timers to be called. > > This new check is added to process_coarse_timers(). > > Why do we need an extra event object, again? Mmmmh. I think we actually might need one, to ensure that this fires only *once*, and we can then do what is needed (like, explicit-exit-notify, reliable TLS processing, etc.) without raising more SIGTERM on every future visit here. (Of course we could avoid this by resetting the session end time, but then it might be easier to understand if we use the existing mechanisms, even if a bit heavyweight) gert
Hi, On 19/09/2022 09:11, Gert Doering wrote: > Hi, > > On Mon, Sep 19, 2022 at 08:36:21AM +0200, Gert Doering wrote: >>> we don't get here at all, if we have no event object that is timing out. >> >> process_coarse_timers() is run once per second, and does not need >> additional timers to be called. >> >> This new check is added to process_coarse_timers(). >> >> Why do we need an extra event object, again? > > Mmmmh. I think we actually might need one, to ensure that this fires > only *once*, and we can then do what is needed (like, explicit-exit-notify, > reliable TLS processing, etc.) without raising more SIGTERM on every > future visit here. > > (Of course we could avoid this by resetting the session end time, but > then it might be easier to understand if we use the existing mechanisms, > even if a bit heavyweight) I went down the multi-rabbit-hole and managed to get back alive. The coarse timer is set to the lowest needed value across all the active timers (through event_timer_trigger()) or required TLS action. However we don't have specific code to handle a bunch of "one-shot" events. For this reason the simplest approach is to set an event object and simply not rearm it after it is triggered. The rearm happens via _reset(), which we don't call. So this patch looks good to me as is. Cheers, > > gert > > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 3526dbf6..56ab5662 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -626,6 +626,21 @@ encrypt_sign(struct context *c, bool comp_frag) buffer_turnover(orig_buf, &c->c2.to_link, &c->c2.buf, &b->read_tun_buf); } +/* + * Should we exit due to session timeout? + */ +static void +check_session_timeout(struct context *c) +{ + if (c->options.session_timeout + && event_timeout_trigger(&c->c2.session_interval, &c->c2.timeval, + ETT_DEFAULT)) + { + msg(M_INFO, "Session timeout, exiting"); + register_signal(c, SIGTERM, "session-timeout"); + } +} + /* * Coarse timers work to 1 second resolution. */ @@ -677,6 +692,13 @@ process_coarse_timers(struct context *c) return; } + /* kill session if time is over */ + check_session_timeout(c); + if (c->sig->signal_received) + { + return; + } + /* restart if ping not received */ check_ping_restart(c); if (c->sig->signal_received) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index f2db8dd9..7b817612 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1322,6 +1322,13 @@ do_init_timers(struct context *c, bool deferred) event_timeout_init(&c->c2.inactivity_interval, c->options.inactivity_timeout, now); } + /* initialize inactivity timeout */ + if (c->options.session_timeout) + { + event_timeout_init(&c->c2.session_interval, c->options.session_timeout, + now); + } + /* initialize pings */ if (dco_enabled(&c->options)) { diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h index 00cd652f..f74125aa 100644 --- a/src/openvpn/openvpn.h +++ b/src/openvpn/openvpn.h @@ -288,6 +288,8 @@ struct context_2 struct event_timeout inactivity_interval; int64_t inactivity_bytes; + struct event_timeout session_interval; + /* the option strings must match across peers */ char *options_string_local; char *options_string_remote; diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 3d48c2d9..76c09a0a 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -261,6 +261,7 @@ static const char usage_message[] = " for m seconds.\n" "--inactive n [bytes] : Exit after n seconds of activity on tun/tap device\n" " produces a combined in/out byte count < bytes.\n" + "--session-timeout n: Limit connection time to n seconds.\n" "--ping-exit n : Exit if n seconds pass without reception of remote ping.\n" "--ping-restart n: Restart if n seconds pass without reception of remote ping.\n" "--ping-timer-rem: Run the --ping-exit/--ping-restart timer only if we have a\n" @@ -1818,6 +1819,7 @@ show_settings(const struct options *o) SHOW_INT(keepalive_ping); SHOW_INT(keepalive_timeout); SHOW_INT(inactivity_timeout); + SHOW_INT(session_timeout); SHOW_INT64(inactivity_minimum_bytes); SHOW_INT(ping_send_timeout); SHOW_INT(ping_rec_timeout); @@ -6583,6 +6585,11 @@ add_option(struct options *options, } } } + else if (streq(p[0], "session-timeout") && p[1] && !p[2]) + { + VERIFY_PERMISSION(OPT_P_TIMER); + options->session_timeout = positive_atoi(p[1]); + } else if (streq(p[0], "proto") && p[1] && !p[2]) { int proto; diff --git a/src/openvpn/options.h b/src/openvpn/options.h index c9144154..a674a0a6 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -317,6 +317,8 @@ struct options int inactivity_timeout; /* --inactive */ int64_t inactivity_minimum_bytes; + int session_timeout; /* Kill session after n seconds, regardless activity */ + int ping_send_timeout; /* Send a TCP/UDP ping to remote every n seconds */ int ping_rec_timeout; /* Expect a TCP/UDP ping from remote at least once every n seconds */ bool ping_timer_remote; /* Run ping timer only if we have a remote address */