[M] Change in openvpn[master]: multipeer: introduce asymmetric peer-id

Attention is currently required from: flichtenheld, its_Giaan, ordex, plaisthos.

cron2 has posted comments on this change by its_Giaan. ( http://gerrit.openvpn.net/c/openvpn/+/1089?usp=email )

Change subject: multipeer: introduce asymmetric peer-id
......................................................................


Patch Set 28: Code-Review-1

(5 comments)

Patchset:

PS28: 
reviewing "as if I had never seen it before", found a few more things.  Getting there.  Waiting for @[email protected] for a full protocol correctness review.


File src/openvpn/init.c:

http://gerrit.openvpn.net/c/openvpn/+/1089/comment/c19d8bf5_ec2feaad?usp=email :
PS28, Line 3483:         if (c->c2.tls_multi->rx_peer_id != MAX_PEER_ID)
should that not check `tx_peer_id`?  We control "what we send" with `use_peer_id`, and that would be "tx", no?  (In most cases the distinction is probably moot, as you'd have "something that is not MAX_PEER_ID" in both ways, or none at all, but I find it confusing still.


File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/1089/comment/da19c571_23390c09?usp=email :
PS28, Line 2082:     }
this is still ugly.  We do not decide whether to write the "out" string to "buf" by whether or not it has content, but by relying on a complex condition that nobody reading this function will understand.

What about

```
/* write peer info string if there is anything in it, empty string otherwise */
if ( blenz(&out) > 0 )
{
    if (!write_string(...))
      {...}
}
else
{
    if (!write_empty_string(..))
       {...}
}
```

moving this out from the `else {}` branch, and also removing the `write_string()` from the end of the previous `if ()` clause?


http://gerrit.openvpn.net/c/openvpn/+/1089/comment/ba74cf4c_9a52cb50?usp=email :
PS28, Line 2340:                 multi->tx_peer_id = multi->rx_peer_id;
that comment is confusing.  "We don't need it, but we store it", what is that good for?  Maybe make that `/* DCO currently can only handle symmetric peer IDs */`?


http://gerrit.openvpn.net/c/openvpn/+/1089/comment/11a198a6_27b5988b?usp=email :
PS28, Line 2352:         }
Looks like a repeat of the previous code block...?  Can the POINT_TO_POINT case not be included in the `MODE_SERVER` case?

```
if ( session->opt->mode == MODE_SERVER || session->opt_mode == MODE_POINT_TO_POINT )
{
    peer_id = ..
    if ( peer_id != MAX_PEER_ID && !session->opt_dco_enabled )
    {
        /* asymmetric */
    }
    else
    {
         /* peer has no support for asymmetric peer-id, DCO has no support for it */
         multi->tx_peer_id = multi->rx_peer_id;
    }       
}
```



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1089?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I0a13ee90b6706acf20eabcee3bab3f2dff639bf9
Gerrit-Change-Number: 1089
Gerrit-PatchSet: 28
Gerrit-Owner: its_Giaan <[email protected]>
Gerrit-Reviewer: cron2 <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-CC: ordex <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: its_Giaan <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
Gerrit-Attention: ordex <[email protected]>
Gerrit-Comment-Date: Mon, 27 Jul 2026 15:18:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel