diff mbox series

[Openvpn-devel,29/28] Add workaround for Softether server dropping P_ACK_V1 with >= 5 acks

Message ID 20220831134140.913337-2-arne@rfc2549.org
State Accepted
Delegated to: Gert Doering
Headers
Series Stateless three-way handshake and control channel improvements |

Commit Message

Arne Schwabe Aug. 31, 2022, 3:41 a.m. UTC 
  Softether had the number of ACKs in ANY OpenVPN packet limited to 4 and
dropped packets with more than 4 ACKs. This leads to Softether dropping
P_ACK_V1 packets with more than 4 ACKs as invalid. As the recent change
of always acking as many packets as possible, this leads to Softether
server not being able to successfully establish a connection anymore as
it never registers the ACKs.

This behaviour has been fixed on the Softether side with commit 37aa1ba5
but in order to allow clients to connect to older Softether servers, this
commit implements a workaround for the case that the peer might be a
Softether server (no tls-auth/tls-crypt and no other advanced protocol
feature) and limits ACKs to 4 in this case.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl_pkt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Gert Doering Aug. 31, 2022, 3:54 a.m. UTC | #1 
Hi,

On Wed, Aug 31, 2022 at 03:41:40PM +0200, Arne Schwabe wrote:
> This behaviour has been fixed on the Softether side with commit 37aa1ba5
[..]
> +     * in a packet (see commit a14d812dcbc in Softether) */

The code seems to do what the commit says, but seeing two different commit
IDs here confuses me.

gert
Gert Doering Nov. 5, 2022, 11:25 p.m. UTC | #2 
Acked-by: Gert Doering <gert@greenie.muc.de>

This is straightforward.  The detection "what makes a softether server?"
is a bit annoying... and this code will be with us forever...

I have not specifically tested this ("no softether server around") but
ran the normal server/client tests.

Your patch has been applied to the master branch.

commit 036517d5e06c1254233d51893b3a196a3cd37492
Author: Arne Schwabe
Date:   Wed Aug 31 15:41:40 2022 +0200

     Add workaround for Softether server dropping P_ACK_V1 with >= 5 acks

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20220831134140.913337-2-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25142.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c
index e86fbc1b8..9b180cbf4 100644
--- a/src/openvpn/ssl_pkt.c
+++ b/src/openvpn/ssl_pkt.c
@@ -177,6 +177,15 @@  write_control_auth(struct tls_session *session,
 {
     uint8_t header = ks->key_id | (opcode << P_OPCODE_SHIFT);
 
+    /* Workaround for Softether servers. Softether has a bug that it only
+     * allows 4 ACks in packets and drops packets if more ACKs are contained
+     * in a packet (see commit a14d812dcbc in Softether) */
+    if (session->tls_wrap.mode == TLS_WRAP_NONE && !session->opt->server
+        && !(session->opt->crypto_flags & CO_USE_TLS_KEY_MATERIAL_EXPORT))
+    {
+        max_ack = min_int(max_ack, 4);
+    }
+
     ASSERT(link_socket_actual_defined(&ks->remote_addr));
     ASSERT(reliable_ack_write
                (ks->rec_ack, ks->lru_acks, buf, &ks->session_id_remote,