| Message ID | 20220728193542.14801-1-a@unstable.cc |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Thu, Jul 28, 2022 at 09:35:42PM +0200, Antonio Quartulli wrote: > Data channel keys are periodically regenarated and installed in > ovpn-dco. > However, there is a certain moment when keys are rotated in order > to elect the new primary one. > > Check the key status in userspace so that kernelspace can be informed as > well when rotations happen. [...] Small musings about the code comments: > +void > +dco_update_keys(dco_context_t *dco, struct tls_multi *multi) > +{ > + msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->peer_id); > + > + /* this function checks if keys have to be swapped or erased, therefore it > + * can't do much if we don't have any key installed > + */ > + if (multi->dco_keys_installed == 0) > + { > + return; > + } > + > + struct key_state *primary = tls_select_encryption_key(multi); > + /* either we have no primary key at all or, if we do, it must have been > + * installed already (keys are installed upon generation in the TLS code) > + */ > + ASSERT(!primary || primary->dco_status != DCO_NOT_INSTALLED); > + > + /* no primary key available -> no usable key exists, therefore we should > + * tell DCO to simply wipe all keys > + */ > + if (!primary) > + { > + msg(D_DCO, "No encryption key found. Purging data channel keys"); > + > + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_PRIMARY); > + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); > + multi->dco_keys_installed = 0; > + return; > + } > + > + struct key_state *secondary = dco_get_secondary_key(multi, primary); > + /* same reason as the primary key above */ > + ASSERT(!secondary || secondary->dco_status != DCO_NOT_INSTALLED); > + > + /* the current primary key was installed as secondary in DCO, this means > + * that userspace has promoted it and we should tell DCO to swap keys I found the use of "userspace" here a bit confusing. This code is part of userspace, right? So wouldn't it be easier to say "we have promoted it"? > + */ > + if (primary->dco_status == DCO_INSTALLED_SECONDARY) > + { > + msg(D_DCO_DEBUG, "Swapping primary and secondary keys, now: id1=%d id2=%d", > + primary->key_id, secondary ? secondary->key_id : -1); > + > + dco_swap_keys(dco, multi->peer_id); > + primary->dco_status = DCO_INSTALLED_PRIMARY; > + if (secondary) > + { > + secondary->dco_status = DCO_INSTALLED_SECONDARY; > + } > + } > + > + /* if we have no secondary key anymore, inform DCO about it */ > + if (!secondary && multi->dco_keys_installed == 2) > + { > + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); > + multi->dco_keys_installed = 1; > + } > + > + /* all keys that are not installed are set to NOT installed */ > + for (int i = 0; i < KEY_SCAN_SIZE; ++i) > + { > + struct key_state *ks = get_key_scan(multi, i); > + if (ks != primary && ks != secondary) > + { > + ks->dco_status = DCO_NOT_INSTALLED; > + } > + } > +} > + > static bool > dco_check_option_conflict_platform(int msglevel, const struct options *o) > { > diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h > index 1692f5c3..b926e236 100644 > --- a/src/openvpn/dco.h > +++ b/src/openvpn/dco.h > @@ -132,6 +132,14 @@ int init_key_dco_bi(struct tls_multi *multi, struct key_state *ks, > const struct key2 *key2, int key_direction, > const char *ciphername, bool server); > > +/** > + * Possibly swap or wipe keys from DCO I would actually prefer to be a bit more explicit here: "Sync our key status to DCO, potentially swapping or wiping keys" > + * > + * @param dco DCO device context > + * @param multi TLS multi instance > + */ > +void dco_update_keys(dco_context_t *dco, struct tls_multi *multi); > + > #else /* if defined(ENABLE_DCO) */ > > typedef void *dco_context_t; [...] Regards,
Hi, On 29/07/2022 09:41, Frank Lichtenheld wrote: > On Thu, Jul 28, 2022 at 09:35:42PM +0200, Antonio Quartulli wrote: >> Data channel keys are periodically regenarated and installed in >> ovpn-dco. >> However, there is a certain moment when keys are rotated in order >> to elect the new primary one. >> >> Check the key status in userspace so that kernelspace can be informed as >> well when rotations happen. > [...] > > Small musings about the code comments: > >> +void >> +dco_update_keys(dco_context_t *dco, struct tls_multi *multi) >> +{ >> + msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->peer_id); >> + >> + /* this function checks if keys have to be swapped or erased, therefore it >> + * can't do much if we don't have any key installed >> + */ >> + if (multi->dco_keys_installed == 0) >> + { >> + return; >> + } >> + >> + struct key_state *primary = tls_select_encryption_key(multi); >> + /* either we have no primary key at all or, if we do, it must have been >> + * installed already (keys are installed upon generation in the TLS code) >> + */ >> + ASSERT(!primary || primary->dco_status != DCO_NOT_INSTALLED); >> + >> + /* no primary key available -> no usable key exists, therefore we should >> + * tell DCO to simply wipe all keys >> + */ >> + if (!primary) >> + { >> + msg(D_DCO, "No encryption key found. Purging data channel keys"); >> + >> + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_PRIMARY); >> + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); >> + multi->dco_keys_installed = 0; >> + return; >> + } >> + >> + struct key_state *secondary = dco_get_secondary_key(multi, primary); >> + /* same reason as the primary key above */ >> + ASSERT(!secondary || secondary->dco_status != DCO_NOT_INSTALLED); >> + >> + /* the current primary key was installed as secondary in DCO, this means >> + * that userspace has promoted it and we should tell DCO to swap keys > > I found the use of "userspace" here a bit confusing. This code is part of userspace, > right? So wouldn't it be easier to say "we have promoted it"? Sure - whatever makes it easier to understand. > >> + */ >> + if (primary->dco_status == DCO_INSTALLED_SECONDARY) >> + { >> + msg(D_DCO_DEBUG, "Swapping primary and secondary keys, now: id1=%d id2=%d", >> + primary->key_id, secondary ? secondary->key_id : -1); >> + >> + dco_swap_keys(dco, multi->peer_id); >> + primary->dco_status = DCO_INSTALLED_PRIMARY; >> + if (secondary) >> + { >> + secondary->dco_status = DCO_INSTALLED_SECONDARY; >> + } >> + } >> + >> + /* if we have no secondary key anymore, inform DCO about it */ >> + if (!secondary && multi->dco_keys_installed == 2) >> + { >> + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); >> + multi->dco_keys_installed = 1; >> + } >> + >> + /* all keys that are not installed are set to NOT installed */ >> + for (int i = 0; i < KEY_SCAN_SIZE; ++i) >> + { >> + struct key_state *ks = get_key_scan(multi, i); >> + if (ks != primary && ks != secondary) >> + { >> + ks->dco_status = DCO_NOT_INSTALLED; >> + } >> + } >> +} >> + >> static bool >> dco_check_option_conflict_platform(int msglevel, const struct options *o) >> { >> diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h >> index 1692f5c3..b926e236 100644 >> --- a/src/openvpn/dco.h >> +++ b/src/openvpn/dco.h >> @@ -132,6 +132,14 @@ int init_key_dco_bi(struct tls_multi *multi, struct key_state *ks, >> const struct key2 *key2, int key_direction, >> const char *ciphername, bool server); >> >> +/** >> + * Possibly swap or wipe keys from DCO > > I would actually prefer to be a bit more explicit here: > "Sync our key status to DCO, potentially swapping or wiping keys" Makes sense. Thanks > >> + * >> + * @param dco DCO device context >> + * @param multi TLS multi instance >> + */ >> +void dco_update_keys(dco_context_t *dco, struct tls_multi *multi); >> + >> #else /* if defined(ENABLE_DCO) */ >> >> typedef void *dco_context_t; > [...] > > > Regards,
On Thu, Jul 28, 2022 at 09:35:42PM +0200, Antonio Quartulli wrote: > Data channel keys are periodically regenarated and installed in > ovpn-dco. > However, there is a certain moment when keys are rotated in order > to elect the new primary one. > > Check the key status in userspace so that kernelspace can be informed as > well when rotations happen. Thinking about the actual code now. [...] > +void > +dco_update_keys(dco_context_t *dco, struct tls_multi *multi) > +{ > + msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->peer_id); > + > + /* this function checks if keys have to be swapped or erased, therefore it > + * can't do much if we don't have any key installed > + */ > + if (multi->dco_keys_installed == 0) > + { > + return; > + } > + > + struct key_state *primary = tls_select_encryption_key(multi); > + /* either we have no primary key at all or, if we do, it must have been > + * installed already (keys are installed upon generation in the TLS code) > + */ > + ASSERT(!primary || primary->dco_status != DCO_NOT_INSTALLED); > + > + /* no primary key available -> no usable key exists, therefore we should > + * tell DCO to simply wipe all keys > + */ > + if (!primary) > + { > + msg(D_DCO, "No encryption key found. Purging data channel keys"); > + > + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_PRIMARY); > + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); > + multi->dco_keys_installed = 0; > + return; > + } > + > + struct key_state *secondary = dco_get_secondary_key(multi, primary); > + /* same reason as the primary key above */ > + ASSERT(!secondary || secondary->dco_status != DCO_NOT_INSTALLED); > + > + /* the current primary key was installed as secondary in DCO, this means > + * that userspace has promoted it and we should tell DCO to swap keys > + */ > + if (primary->dco_status == DCO_INSTALLED_SECONDARY) > + { > + msg(D_DCO_DEBUG, "Swapping primary and secondary keys, now: id1=%d id2=%d", > + primary->key_id, secondary ? secondary->key_id : -1); > + > + dco_swap_keys(dco, multi->peer_id); > + primary->dco_status = DCO_INSTALLED_PRIMARY; > + if (secondary) > + { > + secondary->dco_status = DCO_INSTALLED_SECONDARY; > + } Why do we have no error handling? We just assume that DCO swapped the keys but if dco_swap_keys returns an error that is probably not true? Also can we really be sure that the kernel state is correct? I.e. we have determined we have swapped the keys, but can we be sure that the kernel actually has the two same keys and so a swap will replicate our state? > + } > + > + /* if we have no secondary key anymore, inform DCO about it */ > + if (!secondary && multi->dco_keys_installed == 2) > + { > + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); Would error handling make sense here? It is clearly not as critical as for dco_swap_keys because a delete is idempotent. > + multi->dco_keys_installed = 1; > + } > + > + /* all keys that are not installed are set to NOT installed */ > + for (int i = 0; i < KEY_SCAN_SIZE; ++i) > + { > + struct key_state *ks = get_key_scan(multi, i); > + if (ks != primary && ks != secondary) > + { > + ks->dco_status = DCO_NOT_INSTALLED; > + } > + } > +} > + > static bool > dco_check_option_conflict_platform(int msglevel, const struct options *o) > { Regards, Frank
Hi, On 01/08/2022 17:44, Frank Lichtenheld wrote: > On Thu, Jul 28, 2022 at 09:35:42PM +0200, Antonio Quartulli wrote: >> Data channel keys are periodically regenarated and installed in >> ovpn-dco. >> However, there is a certain moment when keys are rotated in order >> to elect the new primary one. >> >> Check the key status in userspace so that kernelspace can be informed as >> well when rotations happen. > > Thinking about the actual code now. > > [...] >> +void >> +dco_update_keys(dco_context_t *dco, struct tls_multi *multi) >> +{ >> + msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->peer_id); >> + >> + /* this function checks if keys have to be swapped or erased, therefore it >> + * can't do much if we don't have any key installed >> + */ >> + if (multi->dco_keys_installed == 0) >> + { >> + return; >> + } >> + >> + struct key_state *primary = tls_select_encryption_key(multi); >> + /* either we have no primary key at all or, if we do, it must have been >> + * installed already (keys are installed upon generation in the TLS code) >> + */ >> + ASSERT(!primary || primary->dco_status != DCO_NOT_INSTALLED); >> + >> + /* no primary key available -> no usable key exists, therefore we should >> + * tell DCO to simply wipe all keys >> + */ >> + if (!primary) >> + { >> + msg(D_DCO, "No encryption key found. Purging data channel keys"); >> + >> + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_PRIMARY); >> + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); >> + multi->dco_keys_installed = 0; >> + return; >> + } >> + >> + struct key_state *secondary = dco_get_secondary_key(multi, primary); >> + /* same reason as the primary key above */ >> + ASSERT(!secondary || secondary->dco_status != DCO_NOT_INSTALLED); >> + >> + /* the current primary key was installed as secondary in DCO, this means >> + * that userspace has promoted it and we should tell DCO to swap keys >> + */ >> + if (primary->dco_status == DCO_INSTALLED_SECONDARY) >> + { >> + msg(D_DCO_DEBUG, "Swapping primary and secondary keys, now: id1=%d id2=%d", >> + primary->key_id, secondary ? secondary->key_id : -1); >> + >> + dco_swap_keys(dco, multi->peer_id); >> + primary->dco_status = DCO_INSTALLED_PRIMARY; >> + if (secondary) >> + { >> + secondary->dco_status = DCO_INSTALLED_SECONDARY; >> + } > > Why do we have no error handling? We just assume that DCO swapped the keys > but if dco_swap_keys returns an error that is probably not true? Right - the call may fail for various reasons and in that case we should assume that no swap happened. However, I am not sure what OpenVPN should do at this point. Trigger SIGUSR1 for this instance? The other end won't notice anything anyway. > > Also can we really be sure that the kernel state is correct? I.e. we > have determined we have swapped the keys, but can we be sure that > the kernel actually has the two same keys and so a swap will replicate > our state? We have no way to confirm that, because we have no way to identify the keys that have been installed. We can only trust that we injected the right key at the right place. > >> + } >> + >> + /* if we have no secondary key anymore, inform DCO about it */ >> + if (!secondary && multi->dco_keys_installed == 2) >> + { >> + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); > > Would error handling make sense here? It is clearly not as critical > as for dco_swap_keys because a delete is idempotent. The question again is "what do we do?" Maybe in this case we simply print a message but we continue as if it was successful? > >> + multi->dco_keys_installed = 1; >> + } >> + >> + /* all keys that are not installed are set to NOT installed */ >> + for (int i = 0; i < KEY_SCAN_SIZE; ++i) >> + { >> + struct key_state *ks = get_key_scan(multi, i); >> + if (ks != primary && ks != secondary) >> + { >> + ks->dco_status = DCO_NOT_INSTALLED; >> + } >> + } >> +} >> + >> static bool >> dco_check_option_conflict_platform(int msglevel, const struct options *o) >> { > > > Regards, > Frank > > Thanks! > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel >
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index 0471e4d0..07c16de1 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -94,6 +94,107 @@ init_key_dco_bi(struct tls_multi *multi, struct key_state *ks, ciphername); } +/** + * Find a usable key that is not the primary (i.e. the secondary key) + * + * @param multi The TLS struct to retrieve keys from + * @param primary The primary key that should be skipped during the scan + * + * @return The secondary key or NULL if none could be found + */ +static struct key_state * +dco_get_secondary_key(struct tls_multi *multi, const struct key_state *primary) +{ + for (int i = 0; i < KEY_SCAN_SIZE; ++i) + { + struct key_state *ks = get_key_scan(multi, i); + struct key_ctx_bi *key = &ks->crypto_options.key_ctx_bi; + + if (ks == primary) + { + continue; + } + + if (ks->state >= S_GENERATED_KEYS && ks->authenticated == KS_AUTH_TRUE) + { + ASSERT(key->initialized); + return ks; + } + } + + return NULL; +} + +void +dco_update_keys(dco_context_t *dco, struct tls_multi *multi) +{ + msg(D_DCO_DEBUG, "%s: peer_id=%d", __func__, multi->peer_id); + + /* this function checks if keys have to be swapped or erased, therefore it + * can't do much if we don't have any key installed + */ + if (multi->dco_keys_installed == 0) + { + return; + } + + struct key_state *primary = tls_select_encryption_key(multi); + /* either we have no primary key at all or, if we do, it must have been + * installed already (keys are installed upon generation in the TLS code) + */ + ASSERT(!primary || primary->dco_status != DCO_NOT_INSTALLED); + + /* no primary key available -> no usable key exists, therefore we should + * tell DCO to simply wipe all keys + */ + if (!primary) + { + msg(D_DCO, "No encryption key found. Purging data channel keys"); + + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_PRIMARY); + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); + multi->dco_keys_installed = 0; + return; + } + + struct key_state *secondary = dco_get_secondary_key(multi, primary); + /* same reason as the primary key above */ + ASSERT(!secondary || secondary->dco_status != DCO_NOT_INSTALLED); + + /* the current primary key was installed as secondary in DCO, this means + * that userspace has promoted it and we should tell DCO to swap keys + */ + if (primary->dco_status == DCO_INSTALLED_SECONDARY) + { + msg(D_DCO_DEBUG, "Swapping primary and secondary keys, now: id1=%d id2=%d", + primary->key_id, secondary ? secondary->key_id : -1); + + dco_swap_keys(dco, multi->peer_id); + primary->dco_status = DCO_INSTALLED_PRIMARY; + if (secondary) + { + secondary->dco_status = DCO_INSTALLED_SECONDARY; + } + } + + /* if we have no secondary key anymore, inform DCO about it */ + if (!secondary && multi->dco_keys_installed == 2) + { + dco_del_key(dco, multi->peer_id, OVPN_KEY_SLOT_SECONDARY); + multi->dco_keys_installed = 1; + } + + /* all keys that are not installed are set to NOT installed */ + for (int i = 0; i < KEY_SCAN_SIZE; ++i) + { + struct key_state *ks = get_key_scan(multi, i); + if (ks != primary && ks != secondary) + { + ks->dco_status = DCO_NOT_INSTALLED; + } + } +} + static bool dco_check_option_conflict_platform(int msglevel, const struct options *o) { diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 1692f5c3..b926e236 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -132,6 +132,14 @@ int init_key_dco_bi(struct tls_multi *multi, struct key_state *ks, const struct key2 *key2, int key_direction, const char *ciphername, bool server); +/** + * Possibly swap or wipe keys from DCO + * + * @param dco DCO device context + * @param multi TLS multi instance + */ +void dco_update_keys(dco_context_t *dco, struct tls_multi *multi); + #else /* if defined(ENABLE_DCO) */ typedef void *dco_context_t; @@ -192,5 +200,11 @@ init_key_dco_bi(struct tls_multi *multi, struct key_state *ks, return 0; } +static inline void +dco_update_keys(dco_context_t *dco, struct tls_multi *multi) +{ + ASSERT(false); +} + #endif /* defined(ENABLE_DCO) */ #endif /* ifndef DCO_H */ diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 28f3c088..38d2683c 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -41,6 +41,7 @@ #include "dhcp.h" #include "common.h" #include "ssl_verify.h" +#include "dco.h" #include "memdbg.h" @@ -140,6 +141,18 @@ context_reschedule_sec(struct context *c, int sec) } } +void +check_dco_key_status(struct context *c) +{ + /* DCO context is not yet initialised or enabled */ + if (!dco_enabled(&c->options)) + { + return; + } + + dco_update_keys(&c->c1.tuntap->dco, c->c2.tls_multi); +} + /* * In TLS mode, let TLS level respond to any control-channel * packets which were received, or prepare any packets for @@ -182,6 +195,12 @@ check_tls(struct context *c) interval_schedule_wakeup(&c->c2.tmp_int, &wakeup); + /* Our current code has no good hooks in the TLS machinery to update + * DCO keys. So we check the key status after the whole TLS machinery + * has been completed and potentially update them + */ + check_dco_key_status(c); + if (wakeup) { context_reschedule_sec(c, wakeup);
Data channel keys are periodically regenarated and installed in ovpn-dco. However, there is a certain moment when keys are rotated in order to elect the new primary one. Check the key status in userspace so that kernelspace can be informed as well when rotations happen. Signed-off-by: Antonio Quartulli <a@unstable.cc> --- Changes from v1: * added comments to ASSERT() in dco_update_keys() --- src/openvpn/dco.c | 101 ++++++++++++++++++++++++++++++++++++++++++ src/openvpn/dco.h | 14 ++++++ src/openvpn/forward.c | 19 ++++++++ 3 files changed, 134 insertions(+)