On 20/07/2020 15:22, Arne Schwabe wrote: > Am 20.07.20 um 15:16 schrieb David Sommerseth: >> On 17/07/2020 15:47, Arne Schwabe wrote: >>> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients. >>> >>> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling >>> v1 op codes and give a good warning message if we encounter >>> them in the legal op codes pre-check. >>> >>> Signed-off-by: Arne Schwabe <a...@rfc2549.org> >>> --- >>> doc/doxygen/doc_control_processor.h | 6 +- >>> doc/doxygen/doc_key_generation.h | 6 +- >>> doc/doxygen/doc_protocol_overview.h | 2 +- >>> src/openvpn/forward.c | 2 +- >>> src/openvpn/helper.c | 5 - >>> src/openvpn/init.c | 1 - >>> src/openvpn/options.c | 35 +---- >>> src/openvpn/options.h | 4 - >>> src/openvpn/ssl.c | 230 ++++------------------------ >>> src/openvpn/ssl.h | 19 +-- >>> src/openvpn/ssl_common.h | 1 - >>> 11 files changed, 42 insertions(+), 269 deletions(-) >> >> [...snip...] >> >>> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c >>> index 00b97352..4144217d 100644 >>> --- a/src/openvpn/ssl.c >>> +++ b/src/openvpn/ssl.c >> >> [...snip...] >> >>> @@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf) >>> return str; >>> } >>> >>> -/* >>> - * Handle the reading and writing of key data to and from >>> - * the TLS control channel (cleartext). >>> - */ >> >> Is it needed to remove this comment? Or move it after the push_peer_info() >> function? > > Yeah, we can move the comment.
Yes, please ... Since it's not a bad comment, I don't like loosing helpful
pointers in comments :)
>>
>>> -static bool
>>> -key_method_1_write(struct buffer *buf, struct tls_session *session)
>>> -{
>> [...snip...]
>>> -}
>>> -
>>> static bool
>>> push_peer_info(struct buffer *buf, struct tls_session *session)
>>> {
>>> @@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session
>>> *session)
>>> * push request, also signal that the client wants
>>> * to get push-reply messages without without requiring a round
>>> * trip for a push request message*/
>>> - if(session->opt->pull)
>>> + if (session->opt->pull)
>>> {
>>> iv_proto |= IV_PROTO_REQUEST_PUSH;
>>> }
>>> @@ -2394,7 +2328,6 @@ key_method_2_write(struct buffer *buf, struct
>>> tls_session *session)
>>
>> [...snip...]
>>
>>> @@ -3470,14 +3316,12 @@ tls_pre_decrypt(struct tls_multi *multi,
>>> }
>>>
>>> /* hard reset ? */
>>> - if (is_hard_reset(op, 0))
>>> + if (is_hard_reset_method2(op))
>>> {
>>> /* verify client -> server or server -> client connection
>>> */
>>> - if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
>>> - || op == P_CONTROL_HARD_RESET_CLIENT_V2
>>> + if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
>>> || op == P_CONTROL_HARD_RESET_CLIENT_V3) &&
>>> !multi->opt.server)
>>> - || ((op == P_CONTROL_HARD_RESET_SERVER_V1
>>> - || op == P_CONTROL_HARD_RESET_SERVER_V2) &&
>>> multi->opt.server))
>>> + || ((op == P_CONTROL_HARD_RESET_SERVER_V2) &&
>>> multi->opt.server))
>>> {
>>> msg(D_TLS_ERRORS,
>>> "TLS Error: client->client or server->server
>>> connection attempted from %s",
>>> @@ -3542,19 +3386,11 @@ tls_pre_decrypt(struct tls_multi *multi,
>>> * Initial packet received.
>>> */
>>>
>>> - if (i == TM_SIZE && is_hard_reset(op, 0))
>>> + if (i == TM_SIZE && is_hard_reset_method2(op))
>>> {
>>> struct tls_session *session = &multi->session[TM_ACTIVE];
>>> struct key_state *ks = &session->key[KS_PRIMARY];
>>>
>>> - if (!is_hard_reset(op, multi->opt.key_method))
>>> - {
>>> - msg(D_TLS_ERRORS, "TLS ERROR: initial packet
>>> local/remote key_method mismatch, local key_method=%d, op=%s",
>>> - multi->opt.key_method,
>>> - packet_opcode_name(op));
>>> - goto error;
>>> - }
>>> -
>>> /*
>>> * If we have no session currently in progress, the
>>> initial packet will
>>> * open a new session in TM_ACTIVE rather than
>>> TM_UNTRUSTED.
>>> @@ -3594,7 +3430,7 @@ tls_pre_decrypt(struct tls_multi *multi,
>>> }
>>> }
>>>
>>> - if (i == TM_SIZE && is_hard_reset(op, 0))
>>> + if (i == TM_SIZE && is_hard_reset_method2(op))
>>
>> Do we need this if() block? Considering it is exactly the same if()
>> statement
>> as the previous if() block. I would suggest merging them, but you might hit
>> a
>> challenge with reuse of struct tls_session *session, where one is referring
>> to
>> TM_ACTIVE vs TM_UNTRUSTED.
>
> That is something that might be possible but I would consider outside of
> patch. The first block actually modifies i (i = TM_ACTIVE;), so that
> needs a separate patch with an explaination why/how we simplify the
> logic here.
Alright, I can see the argument of not touching it now.
Could we then just add a comment to these two sections for now, indicating why
they have been kept separate despite the if() statement being identical?
Something like "This section processes {TM_ACTIVE, TM_UNTRUSTED} sessions" and
elaborate a little bit around that, which may help refactoring this code later
on.
--
kind regards,
David Sommerseth
OpenVPN Inc
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
