[Openvpn-devel] [PATCH v2 1/2] Introduce get_key_by_management_key_id helper function

2023-05-22 Thread Arne Schwabe
This function allows us to map from a management key id to a key structure
and also allows this function to be reused.

Patch v2: add message when key is not found.

Signed-off-by: Arne Schwabe 
---
 src/openvpn/ssl_common.h | 20 
 src/openvpn/ssl_verify.c | 23 +--
 2 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 27b029479..ebfd25432 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -722,4 +722,24 @@ get_primary_key(const struct tls_multi *multi)
 return >session[TM_ACTIVE].key[KS_PRIMARY];
 }
 
+#ifdef ENABLE_MANAGEMENT
+/**
+ * Gets the \c key_state  object that belong to the management key id or
+ * return NULL if not found.
+ */
+static inline struct key_state *
+get_key_by_management_key_id(struct tls_multi *multi, unsigned int mda_key_id)
+{
+for (int i = 0; i < KEY_SCAN_SIZE; ++i)
+{
+struct key_state *ks = get_key_scan(multi, i);
+if (ks->mda_key_id == mda_key_id)
+{
+return ks;
+}
+}
+return NULL;
+}
+#endif
+
 #endif /* SSL_COMMON_H_ */
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 1b589f1a6..d4d291098 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1268,22 +1268,25 @@ tls_authentication_status(struct tls_multi *multi)
 bool
 tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, 
const bool auth, const char *client_reason)
 {
-bool ret = false;
+struct key_state *ks = NULL;
 if (multi)
 {
-int i;
+
 auth_set_client_reason(multi, client_reason);
-for (i = 0; i < KEY_SCAN_SIZE; ++i)
+ks = get_key_by_management_key_id(multi, mda_key_id);
+
+if (ks)
 {
-struct key_state *ks = get_key_scan(multi, i);
-if (ks->mda_key_id == mda_key_id)
-{
-ks->mda_status = auth ? ACF_SUCCEEDED : ACF_FAILED;
-ret = true;
-}
+ks->mda_status = auth ? ACF_SUCCEEDED : ACF_FAILED;
 }
+else
+{
+msg(D_TLS_DEBUG_LOW, "%s: no key state found for management key id 
"
+"%d", __func__, mda_key_id);
+}
+
 }
-return ret;
+return (bool) ks;
 }
 #endif /* ifdef ENABLE_MANAGEMENT */
 
-- 
2.39.2 (Apple Git-143)



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


Re: [Openvpn-devel] [PATCH 2/2] Fix CR_RESPONSE mangaement message using wrong key_id

2023-05-22 Thread Arne Schwabe

Am 19.05.23 um 15:45 schrieb Selva Nair:

Hi,

While this bugfix should be merged, I'm a conflicted about the way these 
two patches are split up. It just makes reviewing harder than it should 
be. They actually form two independent changes but with one half 
intersecting with the other for no reason.


Yeah. I moved the logging to 1/2. It seems to fit much better in that patch.

Arne



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH v2 2/2] Fix CR_RESPONSE mangaement message using wrong key_id

2023-05-22 Thread Arne Schwabe
the management interface expects the management key id instead
of the openvpn key id. In the past they often were the same for low ids
which hid the bug quite well.

Also do not pick uninitialised keystates (management key_id is not valid
in these).

Patch v2: do not add logging

Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe
Signed-off-by: Arne Schwabe 
---
 src/openvpn/push.c   | 4 ++--
 src/openvpn/ssl_common.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 8e9627199..8f0a534ac 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -267,9 +267,9 @@ receive_cr_response(struct context *c, const struct buffer 
*buffer)
 struct tls_session *session = >c2.tls_multi->session[TM_ACTIVE];
 struct man_def_auth_context *mda = session->opt->mda_context;
 struct env_set *es = session->opt->es;
-int key_id = get_primary_key(c->c2.tls_multi)->key_id;
+unsigned int mda_key_id = get_primary_key(c->c2.tls_multi)->mda_key_id;
 
-management_notify_client_cr_response(key_id, mda, es, m);
+management_notify_client_cr_response(mda_key_id, mda, es, m);
 #endif
 #if ENABLE_PLUGIN
 verify_crresponse_plugin(c->c2.tls_multi, m);
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index ebfd25432..be0f18746 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -733,7 +733,7 @@ get_key_by_management_key_id(struct tls_multi *multi, 
unsigned int mda_key_id)
 for (int i = 0; i < KEY_SCAN_SIZE; ++i)
 {
 struct key_state *ks = get_key_scan(multi, i);
-if (ks->mda_key_id == mda_key_id)
+if (ks->mda_key_id == mda_key_id && ks->state > S_UNDEF)
 {
 return ks;
 }
-- 
2.39.2 (Apple Git-143)



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel


[Openvpn-devel] [PATCH] Print a more user-friendly error when tls-crypt-v2 client auth fails

2023-05-22 Thread Arne Schwabe
While it might be clear to people being (too?) well versed in
typical crypto applications that an authentication failure probably
mean wrong decryption key, this is not really obvious for the typical
user/server admin.

Change-Id: If0f0e7d53f915d39ab69c43dc73bb9c26ae9
Signed-off-by: Arne Schwabe 
---
 src/openvpn/tls_crypt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index 88b2d6d7c..73542368e 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -524,6 +524,8 @@ tls_crypt_v2_unwrap_client_key(struct key2 *client_key, 
struct buffer *metadata,
 dmsg(D_CRYPTO_DEBUG, "tag_check: %s",
  format_hex(tag_check, sizeof(tag_check), 0, ));
 CRYPT_ERROR("client key authentication error");
+msg(D_TLS_DEBUG_LOW, "This might be a client-key that was generated 
for "
+"a different tls-crypt-v2 server key)");
 }
 
 if (buf_len() < sizeof(client_key->keys))
-- 
2.39.2 (Apple Git-143)



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel