From patchwork Thu May 6 04:12:59 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arne Schwabe X-Patchwork-Id: 1795 X-Patchwork-Delegate: a@unstable.cc Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director10.mail.ord1d.rsapps.net ([172.30.191.6]) by backend30.mail.ord1d.rsapps.net with LMTP id SNW5D8n5k2DxaQAAIUCqbw (envelope-from ) for ; Thu, 06 May 2021 10:14:33 -0400 Received: from proxy1.mail.ord1d.rsapps.net ([172.30.191.6]) by director10.mail.ord1d.rsapps.net with LMTP id aJF+D8n5k2BUCgAApN4f7A (envelope-from ) for ; Thu, 06 May 2021 10:14:33 -0400 Received: from smtp37.gate.ord1c ([172.30.191.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by proxy1.mail.ord1d.rsapps.net with LMTPS id cBEkD8n5k2AuKAAAasrz9Q (envelope-from ) for ; Thu, 06 May 2021 10:14:33 -0400 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO X-Virus-Scanned: OK X-Orig-To: openvpnslackdevel@openvpn.net X-Originating-Ip: [216.105.38.7] Authentication-Results: smtp37.gate.ord1c.rsapps.net; iprev=pass policy.iprev="216.105.38.7"; spf=pass smtp.mailfrom="openvpn-devel-bounces@lists.sourceforge.net" smtp.helo="lists.sourceforge.net"; dkim=fail (signature verification failed) header.d=sourceforge.net; dkim=fail (signature verification failed) header.d=sf.net; dmarc=none (p=nil; dis=none) header.from=rfc2549.org X-Suspicious-Flag: YES X-Classification-ID: 608f167a-ae75-11eb-a957-525400e8d833-1-1 Received: from [216.105.38.7] ([216.105.38.7:41914] helo=lists.sourceforge.net) by smtp37.gate.ord1c.rsapps.net (envelope-from ) (ecelerity 4.2.38.62370 r(:)) with ESMTPS (cipher=DHE-RSA-AES256-GCM-SHA384) id EB/4C-11941-8C9F3906; Thu, 06 May 2021 10:14:32 -0400 Received: from [127.0.0.1] (helo=sfs-ml-2.v29.lw.sourceforge.com) by sfs-ml-2.v29.lw.sourceforge.com with esmtp (Exim 4.92.3) (envelope-from ) id 1leeki-0004VS-Mc; Thu, 06 May 2021 14:13:20 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-2.v29.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92.3) (envelope-from ) id 1leekh-0004VK-50 for openvpn-devel@lists.sourceforge.net; Thu, 06 May 2021 14:13:19 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Transfer-Encoding:MIME-Version:References: In-Reply-To:Message-Id:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=yO+ZCxMDHIIZWBgpT3BjTBzQ3cqBt3eeXdmX/dtvPmI=; b=M88AtkC/wC2LooHFWS313CUTeT i6vDR8+sRY//golY7k9N9nB9uHr7UaU7Mu/RqvVuX2k76IHhjogFZIiSHaLL2c5W87g4dwZEF2CBz hGAq7radzzQt13B7NqFO1IWXtREj8T9F9Hh2GbZCuLdXFW8h365+G5nTE4Ys3KTLzN0E=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To:Message-Id: Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=yO+ZCxMDHIIZWBgpT3BjTBzQ3cqBt3eeXdmX/dtvPmI=; b=c/q6RY02Q1ZZvfRMQs9X7j51S4 h8HJQDWKq35DJcV0d9qHus8z8k4B0JwmroybUrQ+9TRc/+D6VynqEtmZyLRSL6I9l4KbKjseJUOz7 J8P8zeqm31auIkGl4eMUSRZ8gXZrwa3Wtya+sFhIK19SVJIoXMYz/7UVtEBY3ay6/i2I=; Received: from mail.blinkt.de ([192.26.174.232]) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92.3) id 1leekY-0000ru-Lp for openvpn-devel@lists.sourceforge.net; Thu, 06 May 2021 14:13:19 +0000 Received: from kamera.blinkt.de ([2001:638:502:390:20c:29ff:fec8:535c]) by mail.blinkt.de with smtp (Exim 4.94.2 (FreeBSD)) (envelope-from ) id 1leekN-000LRj-MF for openvpn-devel@lists.sourceforge.net; Thu, 06 May 2021 16:12:59 +0200 Received: (nullmailer pid 309787 invoked by uid 10006); Thu, 06 May 2021 14:12:59 -0000 From: Arne Schwabe To: openvpn-devel@lists.sourceforge.net Date: Thu, 6 May 2021 16:12:59 +0200 Message-Id: <20210506141259.309741-1-arne@rfc2549.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20210422151724.2132573-1-arne@rfc2549.org> References: <20210422151724.2132573-1-arne@rfc2549.org> MIME-Version: 1.0 X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. 0.0 URIBL_BLOCKED ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [URIs: rfc2549.org] 0.2 HEADER_FROM_DIFFERENT_DOMAINS From and EnvelopeFrom 2nd level mail domains are different 0.0 SPF_NONE SPF: sender does not publish an SPF Record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record X-Headers-End: 1leekY-0000ru-Lp Subject: [Openvpn-devel] [PATCH v3] Return cached result in tls_authentication_status X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox tls_authentication_status does caching to avoid file I/O more than every TLS_MULTI_AUTH_STATUS_INTERVAL (10s) per connection. But counter-intuitively it does not return the cached result but rather TLS_AUTHENTICATION_UNDEFINED if the cache is not refreshed by the call. This is workarounded by forcing a refresh in some areas of the code (latency = 0). This patch changes the behaviour by always returning the last known status and only updating the file status when the i/o timeout for the caches is reached. The old logic in send_auth_failed is fragile in the sense that if it is called again while an exit is scheduled it will reset the timer to 5s again. Since we now always report the status from tls_authentication_status() instead only every 10s, this caused OpenVPN to infinitively reset the timer. Fix this by only setting the status if no exit is scheduled. The function is still called multiple times but since it is with coarse timer frequency, the 4 extra calls (1 per second) are better than to add more extra code to avoid these calls. The patch also changes the DEFINE enum into a real enum. Patch v2: only update tas_cache_last_udpate when actually updating the cache. Patch v3: avoid rearming timer Signed-off-by: Arne Schwabe Acked-by: Antonio Quartulli --- src/openvpn/multi.c | 2 +- src/openvpn/push.c | 11 ++++++++- src/openvpn/ssl_common.h | 16 +++++++++--- src/openvpn/ssl_verify.c | 53 ++++++++++++++++++++++------------------ src/openvpn/ssl_verify.h | 3 +-- 5 files changed, 54 insertions(+), 31 deletions(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 666456da9..ab2270a58 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2596,7 +2596,7 @@ static const multi_client_connect_handler client_connect_handlers[] = { static void multi_connection_established(struct multi_context *m, struct multi_instance *mi) { - if (tls_authentication_status(mi->context.c2.tls_multi, 0) + if (tls_authentication_status(mi->context.c2.tls_multi, TLS_MULTI_AUTH_STATUS_INTERVAL) != TLS_AUTHENTICATION_SUCCEEDED) { return; diff --git a/src/openvpn/push.c b/src/openvpn/push.c index fcafc5003..671220c3f 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -335,10 +335,18 @@ __attribute__ ((format(__printf__, 4, 5))) /* * Send auth failed message from server to client. + * + * Does nothing if an exit is already scheduled */ void send_auth_failed(struct context *c, const char *client_reason) { + if (event_timeout_defined(&c->c2.scheduled_exit)) + { + msg(D_TLS_DEBUG, "exit already scheduled for context"); + return; + } + struct gc_arena gc = gc_new(); static const char auth_failed[] = "AUTH_FAILED"; size_t len; @@ -855,7 +863,8 @@ process_incoming_push_request(struct context *c) { int ret = PUSH_MSG_ERROR; - if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED + + if (tls_authentication_status(c->c2.tls_multi, TLS_MULTI_AUTH_STATUS_INTERVAL) == TLS_AUTHENTICATION_FAILED || c->c2.tls_multi->multi_state == CAS_FAILED) { const char *client_reason = tls_client_reason(c->c2.tls_multi); diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 9c923f2a6..52488a163 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -152,6 +152,15 @@ struct auth_deferred_status unsigned int auth_control_status; }; +/* key_state_test_auth_control_file return values, these specify the + * current status of a deferred authentication */ +enum auth_deferred_result { + ACF_PENDING, /**< deferred auth still pending */ + ACF_SUCCEEDED, /**< deferred auth has suceeded */ + ACF_DISABLED, /**< deferred auth is not used */ + ACF_FAILED /**< deferred auth has failed */ +}; + /** * Security parameter state of one TLS and data channel %key session. * @ingroup control_processor @@ -219,7 +228,7 @@ struct key_state #ifdef ENABLE_MANAGEMENT unsigned int mda_key_id; - unsigned int mda_status; + enum auth_deferred_result mda_status; #endif time_t acf_last_mod; @@ -561,8 +570,9 @@ struct tls_multi char *locked_username; struct cert_hash_set *locked_cert_hash_set; - /* Time of last call to tls_authentication_status */ - time_t tas_last; + /* Time of last when we updated the cached state of + * tls_authentication_status deferred files */ + time_t tas_cache_last_update; /* * An error message to send to client on AUTH_FAILED diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 34ee19caf..b1b01f777 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -845,13 +845,6 @@ cleanup: * user/password authentication. *************************************************************************** */ -/* key_state_test_auth_control_file return values, - * NOTE: acf_merge indexing depends on these values */ -#define ACF_UNDEFINED 0 -#define ACF_SUCCEEDED 1 -#define ACF_DISABLED 2 -#define ACF_FAILED 3 - void auth_set_client_reason(struct tls_multi *multi, const char *client_reason) { @@ -866,7 +859,7 @@ auth_set_client_reason(struct tls_multi *multi, const char *client_reason) #ifdef ENABLE_MANAGEMENT -static inline unsigned int +static inline enum auth_deferred_result man_def_auth_test(const struct key_state *ks) { if (management_enable_def_auth(management)) @@ -1041,13 +1034,23 @@ key_state_gen_auth_control_files(struct auth_deferred_status *ads, return (acf && apf); } -static unsigned int -key_state_test_auth_control_file(struct auth_deferred_status *ads) +/** + * Checks the control status from a file. The function will try to read + * and update the cached status if the status is still pending and the paramter + * cached is false. The function returns the + * + * + * @param ads deferred status control structure + * @param cached Return only cached status + * @return + */ +static enum auth_deferred_result +key_state_test_auth_control_file(struct auth_deferred_status *ads, bool cached) { if (ads->auth_control_file) { unsigned int ret = ads->auth_control_status; - if (ret == ACF_UNDEFINED) + if (ret == ACF_PENDING && !cached) { FILE *fp = fopen(ads->auth_control_file, "r"); if (fp) @@ -1084,11 +1087,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency) /* at least one key already failed authentication */ bool failed_auth = false; - if (latency && multi->tas_last + latency >= now) - { - return TLS_AUTHENTICATION_UNDEFINED; - } - multi->tas_last = now; + bool cached = multi->tas_cache_last_update + latency >= now; for (int i = 0; i < KEY_SCAN_SIZE; ++i) { @@ -1102,11 +1101,11 @@ tls_authentication_status(struct tls_multi *multi, const int latency) } else { - unsigned int auth_plugin = ACF_DISABLED; - unsigned int auth_script = ACF_DISABLED; - unsigned int auth_man = ACF_DISABLED; - auth_plugin = key_state_test_auth_control_file(&ks->plugin_auth); - auth_script = key_state_test_auth_control_file(&ks->script_auth); + enum auth_deferred_result auth_plugin = ACF_DISABLED; + enum auth_deferred_result auth_script = ACF_DISABLED; + enum auth_deferred_result auth_man = ACF_DISABLED; + auth_plugin = key_state_test_auth_control_file(&ks->plugin_auth, cached); + auth_script = key_state_test_auth_control_file(&ks->script_auth, cached); #ifdef ENABLE_MANAGEMENT auth_man = man_def_auth_test(ks); #endif @@ -1118,9 +1117,9 @@ tls_authentication_status(struct tls_multi *multi, const int latency) ks->authenticated = KS_AUTH_FALSE; failed_auth = true; } - else if (auth_plugin == ACF_UNDEFINED - || auth_script == ACF_UNDEFINED - || auth_man == ACF_UNDEFINED) + else if (auth_plugin == ACF_PENDING + || auth_script == ACF_PENDING + || auth_man == ACF_PENDING) { if (now < ks->auth_deferred_expire) { @@ -1140,6 +1139,12 @@ tls_authentication_status(struct tls_multi *multi, const int latency) } } + /* we did not rely on a cached result, remember the cache update time */ + if (!cached) + { + multi->tas_cache_last_update = now; + } + #if 0 dmsg(D_TLS_ERRORS, "TAS: a=%d s=%d d=%d f=%d", active, success, deferred, failed_auth); #endif diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h index 8358fb986..06b88b568 100644 --- a/src/openvpn/ssl_verify.h +++ b/src/openvpn/ssl_verify.h @@ -69,8 +69,7 @@ enum tls_auth_status { TLS_AUTHENTICATION_SUCCEEDED=0, TLS_AUTHENTICATION_FAILED=1, - TLS_AUTHENTICATION_DEFERRED=2, - TLS_AUTHENTICATION_UNDEFINED=3 + TLS_AUTHENTICATION_DEFERRED=2 }; /**