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

[plaisthos (C. Review) <ge...@op...>]

Attention is currently required from: cron2, flichtenheld, its_Giaan.

plaisthos 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 4: Code-Review-2

(8 comments)

Patchset:

PS4: 
The part that picks the "peer-id" pushed and parsed options.c that sets peer-id on receiving peer-id

    if (found & OPT_P_PEER_ID)
    {
        msg(D_PUSH_DEBUG, "OPTIONS IMPORT: peer-id set");
        c->c2.tls_multi->use_peer_id = true;
        c->c2.tls_multi->tx_peer_id = c->options.peer_id;
    }
    

should probably also be adjusted to set both rx and tx as peer-id as pushed option should set both.


File src/openvpn/ssl.c:

http://gerrit.openvpn.net/c/openvpn/+/1089/comment/7b89f526_1248648b?usp=email :
PS4, Line 1179:     ret->rx_peer_id = MAX_PEER_ID;
Add comment here that we also use the rx peer id to identify DCO clients as this has become now a important distinction.


http://gerrit.openvpn.net/c/openvpn/+/1089/comment/18e30fd6_0e66350f?usp=email :
PS4, Line 1982:         }
This is still not guarded by DCO capability. With the current version we still always indicate to the peer that we are always asymmetric peer ID capable even if the underlying DCO module is not able to use a different peer ID for TX.


http://gerrit.openvpn.net/c/openvpn/+/1089/comment/3fb67b50_b42dfeaf?usp=email :
PS4, Line 2165:     if (multi->rx_peer_id == MAX_PEER_ID && session->opt->mode != MODE_SERVER)
This feel be a very hacky place to set the multi rx peer id. I think there is a better place to do that.


File src/openvpn/ssl_ncp.c:

http://gerrit.openvpn.net/c/openvpn/+/1089/comment/c5df3a60_53fab78f?usp=email :
PS4, Line 425:     if (tx_peer_id)
This also need to take DCO capability into account.


http://gerrit.openvpn.net/c/openvpn/+/1089/comment/1a14b321_0b6cf1e1?usp=email :
PS4, Line 450:         if (multi->use_peer_id)
I think this parts needs to be skipped if we are using/negotiated asymmetric peer-id as it would overwrite both rx and tx ids with the EKM generated ones. Probably move the if (tx_peer_id) above and have this as else path with a comment that asymmetric peer id trumps EKM


File src/openvpn/ssl_util.h:

http://gerrit.openvpn.net/c/openvpn/+/1089/comment/17f8e5bb_aeb5d80c?usp=email :
PS4, Line 56: uint32_t extract_asymmetric_peer_id(const char *peer_info);
Add doxygen please


File src/openvpn/ssl_util.c:

http://gerrit.openvpn.net/c/openvpn/+/1089/comment/aa5446c4_405e7c5e?usp=email :
PS4, Line 90:     return 0;
0 is a valid peer id. So I would rather have -1 (and int32_t as return type) or MAX_PEER_ID, MAX_UINT value or similar as not defined.

In fact the first client that typically connects to a p2mp server is assigned value 0.



-- 
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: 4
Gerrit-Owner: its_Giaan <gia...@ma...>
Gerrit-Reviewer: cron2 <ge...@gr...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-Reviewer: plaisthos <arn...@rf...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: its_Giaan <gia...@ma...>
Gerrit-Attention: cron2 <ge...@gr...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-Comment-Date: Tue, 07 Oct 2025 15:50:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes