[S] Change in openvpn[master]: Introduce get_key_by_management_key_id helper function

"plaisthos \(Code Review\) via Openvpn-devel" <[email protected]>
Newsgroups gmane.network.openvpn.devel
Message-ID <571cd685b87db4f97b8d34da33ff8df1342c6cf8-EmailReviewComments-HTML@gerrit.openvpn.net>
Attention is currently required from: flichtenheld.

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

Change subject: Introduce get_key_by_management_key_id helper function
......................................................................


Patch Set 6:

(1 comment)

File src/openvpn/ssl_common.h:

http://gerrit.openvpn.net/c/openvpn/+/1856/comment/af028c42_ec4c2b57?usp=email :
PS6, Line 775:             return ks;
> The old code did not abort on first match but always checked all three key states. […]
So each key_state has its own unique mda_key_id so there is only one valid key_state with mda_key_id, so the code does the same. But valid does a bit of heavy lifting here:

If state is S_UNDEF, mda_key_id is 0 and 0 is also a valid key id for the very first client. So we need to check for S_UNDEF to determine if the client actually the one with key-id 0 or just not initialised. 

As alternative we can change the initialisation of context_2  or assignment of mda_key_id_counter to start with 1 instead of 0.

E.g.

session->opt->mda_context->mda_key_id_counter++;
    ks->mda_key_id = 

E.g. something like:

    index de9303b30..478e4d314 100644
    --- a/src/openvpn/manage.h
    +++ b/src/openvpn/manage.h
    @@ -69,6 +69,8 @@ struct man_def_auth_context
     #define DAF_INITIAL_AUTH           (1u << 2)
         unsigned int flags;
     
    +    /** Identify the key id in the management interface, always 1 or
    +     * or greater if valid */
         unsigned int mda_key_id_counter;
     };
     
    diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
    index 82d7daeb2..3495f9dd7 100644
    --- a/src/openvpn/ssl.c
    +++ b/src/openvpn/ssl.c
    @@ -870,7 +870,10 @@ key_state_init(struct tls_session *session, struct key_state *ks)
         ks->crypto_options.pid_persist = NULL;
     
     #ifdef ENABLE_MANAGEMENT
    -    ks->mda_key_id = session->opt->mda_context->mda_key_id_counter++;
    +    /* Increment key ID before assigning to always have a key id >= 1 to
    +     * make 0 signify an invalid key id */
    +    session->opt->mda_context->mda_key_id_counter++;
    +    ks->mda_key_id = session->opt->mda_context->mda_key_id_counter;
     #endif
     
         /*
    diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
    index 3b326a7c1..0270c23f2 100644
    --- a/src/openvpn/ssl_common.h
    +++ b/src/openvpn/ssl_common.h
    @@ -764,7 +764,7 @@ get_primary_key(const struct tls_multi *multi)
     
     #ifdef ENABLE_MANAGEMENT
     /**
    - * Gets the \c key_state  object that belong to the management key id or
    + * Gets the \c key_state object that belongs to the management key id or
      * return NULL if not found.
      */
     static inline struct key_state *
 
 But that is a much bigger/more invasive change but I change the commit to do that as well to avoid this check here.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1856?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: I42d8785959c24bf688190965e58b9b98251b8557
Gerrit-Change-Number: 1856
Gerrit-PatchSet: 6
Gerrit-Owner: plaisthos <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
Gerrit-Comment-Date: Wed, 26 Aug 2026 16:23:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: flichtenheld <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.