| Message ID | 20201215164221.5995-2-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,1/2] Move context_auth from context_2 to tls_multi | expand |
Hi, On 15/12/2020 17:42, Arne Schwabe wrote: > For --nobind clients OpenVPN reuses the context and tls_multi structs > of the previous clients and does not rerun the connect scripts on > connect. But since it is a new client connection, the key_id is 0 and > we postpone the key generation but it will never happen. Can you explain how the --nobind on the client is related to the server behaviour? Are you saying that a client connecting from the same IP of another client will share its session and tls_multi object?
Am 25.03.21 um 23:37 schrieb Antonio Quartulli: > Hi, > > On 15/12/2020 17:42, Arne Schwabe wrote: >> For --nobind clients OpenVPN reuses the context and tls_multi structs >> of the previous clients and does not rerun the connect scripts on >> connect. But since it is a new client connection, the key_id is 0 and >> we postpone the key generation but it will never happen. > > Can you explain how the --nobind on the client is related to the server > behaviour? > > Are you saying that a client connecting from the same IP of another > client will share its session and tls_multi object? (I will also copy that explanation to a v2 of the patch ) When OpenVPN sees a new (SSL) connection via HARD or SOFT_RESET with the same port/ip as an existing session, it will give it the slot of the renegotiation session (TM_UNTRUSTED). And when the authentication succeeds it will replace the current session. Since we already have gone through connect stages and set context_auth to CAS_SUCCEEDED, we don't call all the connect stages again, and therefore also never call multi_client_generate_tls_keys for this session. Arne
Hi, On 26/03/2021 14:10, Arne Schwabe wrote: > Am 25.03.21 um 23:37 schrieb Antonio Quartulli: >> Hi, >> >> On 15/12/2020 17:42, Arne Schwabe wrote: >>> For --nobind clients OpenVPN reuses the context and tls_multi structs >>> of the previous clients and does not rerun the connect scripts on >>> connect. But since it is a new client connection, the key_id is 0 and >>> we postpone the key generation but it will never happen. >> >> Can you explain how the --nobind on the client is related to the server >> behaviour? >> >> Are you saying that a client connecting from the same IP of another >> client will share its session and tls_multi object? > (I will also copy that explanation to a v2 of the patch ) > > When OpenVPN sees a new (SSL) connection via HARD or SOFT_RESET with the > same port/ip as an existing session, it will give it the slot of the > renegotiation session (TM_UNTRUSTED). And when the authentication > succeeds it will replace the current session. Since we already have gone > through connect stages and set context_auth to CAS_SUCCEEDED, we don't > call all the connect stages again, and therefore also never call > multi_client_generate_tls_keys for this session. Thanks for explaining this further. I agree that adding some more text to the commit message would be beneficial. Other than that the patch looks logically correct to me. Basically context_auth is set CAS_SUCCEEDED upon first connection cycle, therefore a new connection that re-uses the existing instance will already have context_auth set to CAS_SUCCEEDED and a new session key will be generated. I wonder if the connection can really be fully working if the server believes that the client was already setup and does not performs the usual connection steps. But this is another story, unrelated to this patch... This said, the new if condition does not look being properly aligned. Cheers,
> > Basically context_auth is set CAS_SUCCEEDED upon first connection cycle, > therefore a new connection that re-uses the existing instance will > already have context_auth set to CAS_SUCCEEDED and a new session key > will be generated. > > I wonder if the connection can really be fully working if the server > believes that the client was already setup and does not performs the > usual connection steps. But this is another story, unrelated to this > patch... It works. From the server perspective this new connection is almost the same as a renegotiation. On a PUSH_REQUEST the server will just send the same configuration as before. Basically if the new client (which is more than often the same client) can do everything the previous client did, everything is well. Arne
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index efbf688f..5d322598 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -2351,7 +2351,7 @@ error: * to the TLS control channel (cleartext). */ static bool -key_method_2_write(struct buffer *buf, struct tls_session *session) +key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct tls_session *session) { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ @@ -2442,12 +2442,17 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) goto error; } - /* Generate tunnel keys if we're a TLS server. - * If we're a p2mp server and IV_NCP >= 2 is negotiated, the first key - * generation is postponed until after the pull/push, so we can process pushed - * cipher directives. + /* + * Generate tunnel keys if we're a TLS server. + * + * If we're a p2mp server to allow NCP, the first key + * generation is postponed until after the connect script finished and the + * NCP options can be processed. Since that always happens at after connect + * script options are available the CAS_SUCCEEDED status is identical to + * NCP options are processed and we have no extra state for NCP finished. */ - if (session->opt->server && !(session->opt->mode == MODE_SERVER && ks->key_id <= 0)) + if (session->opt->server && (session->opt->mode != MODE_SERVER + || multi->context_auth == CAS_SUCCEEDED)) { if (ks->authenticated > KS_AUTH_FALSE) { @@ -2936,7 +2941,7 @@ tls_process(struct tls_multi *multi, if (!buf->len && ((ks->state == S_START && !session->opt->server) || (ks->state == S_GOT_KEY && session->opt->server))) { - if (!key_method_2_write(buf, session)) + if (!key_method_2_write(buf, multi, session)) { goto error; }
For --nobind clients OpenVPN reuses the context and tls_multi structs of the previous clients and does not rerun the connect scripts on connect. But since it is a new client connection, the key_id is 0 and we postpone the key generation but it will never happen. This commit changes postponing the key generation to the right condition of NCP done for this session. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/ssl.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)