Re: [PATCH v2 7/7] OpenVPN: Moved TLS auth to advanced encryption section

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 7930 bytes --]

Hi,

> On 14 Dec 2020, at 16:12, ummeegge <ummeegge(a)ipfire.org> wrote:
> 
> Hi Michael,
> thanks for looking into this.
> 
> Am Montag, dem 14.12.2020 um 14:43 +0100 schrieb Michael Tremer:
>> Hello Erik,
>> 
>> Yes, I am very much interested in this.
>> 
>> And I have spent pretty much an hour to make sense out of all of
>> this, and unfortunately have to report that I failed.
> that´s not good to hear. OK, let me explain... I tried to come up with
> a solution where we already have spoken more times about in the past, i
> tried to follow up your suggestions, the latest about that topic was
> here -->
> https://lists.ipfire.org/pipermail/development/2020-October/008441.html
> in Oktober to bring up "a select box where multiple algorithms can be
> chosen (like in IPsec)". Since --cipher is deprectaed and will be
> replaced by --data-ciphers which is a >=2.5.0 option we need another
> selection field for the --data-ciphers-fallback which is a replacement
> for --cipher and the server uses this directive to talk also to client
> which are <2.5.0 .

Yes, the ciphers. But as far as I understand this, you are offering to select the whole cipher suite. That is something different.

And you are offering different options for TLSv1.3 and lower, which we do not do in IPsec either. You have one selection for both IKEv1 and IKEv2.

> OpenVPN presses to use cipher negotiation which we have avoid since the
> release of 2.4 with --ncp-disable, this option is no also deprecated so
> we need to take action on this.

I will skip my moan about introducing something and then deprecating it shortly after…

I agree that we need to act.

> We need to have then two new options, two seletion boxes which are odd
> to see in the global section. I tried it in the advanced server section
> too and it looks even more confusing with all the other already
> existing options, nothing i wanted to do. So i thought your above
> linked suggestions are pretty straight forward and the best solution to
> build an own crypto section which do have already defaults, nobody
> needs to do there anyting, the global section has been cleaned up and
> it should be even easier for everone to overview.

We have touched this topic multiple times. And I remember the last conclusion as follows:

The average user does not have to touch the advanced options. They have to put in their subnet, select a port and protocol maybe and that is about it. They do not need to think about what cipher to use, because we would have selected a reasonable default. If they want to change something (because of hardware requirements, etc.) they can do that on the advanced page.

I agree that this is all not idea, but adding a third page to this all makes it rather more confusing than less.

> It made for me no sense to leave parts of the crypto (TLS-auth and
> HMACs) in the global section, especially because there are also even
> more algorithms, but also new options for the TLS authentication which
> i have all integrated so i moved them also into that place (patch 6/7
> and 7/7).

I remember that we have agreed to move the existing crypto options to the advanced page.

> The control-channel cipher selection (patch 5/7) is new and should be
> really a part for the advanced ones, therefor no defaults has been set,
> it simply won´t appear if nothing has been configured.

What is the benefit of choosing a different cipher for the control channel?

> I have the problem that I cannot get on top of these things. You are
> submitting *massive* patchsets, in this case I even believe that the
> whole things has been submitted a second time, although I do not know
> if there are any changes.
> Yes there are. Old and deprecated ciphers will now be detected and a
> warning comes up in the global section to change them as soon as
> possible (patch 3/7), also all languages has been translated and has
> been included.
> Also i wanted to split it in more specific topics for better overview
> but it seems that i have been failed.

I do not think that you have failed. I just think that it is not clear what the patch intended to do. So I cannot really look at it and verify if the code actually does what you want it to do.

Could it have been split into even smaller changes? Yes, would that have helped? Maybe. But I do not think that we should waste too much time to reformat the patches. I want the changes to be ready to be merged as soon as possible.

> I could not even find out *what* you are actually trying to do. There
> are more and more buttons being added and I have not the slightest idea
> what I am supposed to do with them. I have given my thoughts about the
> cipher selection and I felt that I have not been heard.
> One intention was that you do not need anything to do except you want
> to do some advanced crypto stuff. The rest should have already been
> made...
> 
> 
> I fear that this is all way too complicated. I cannot review what I do
> not even understand what the desired outcome is. And failing to
> understand that either means that it is either way too complicated or I
> have not read enough anywhere about all of this.
> Please check out the above linked topic where you have wrote
> suggestions that i simply tried to achieve but again, it seems that i
> was not successfull with this...

So, maybe help me to understand why the user would want to use different ciphers for the control channel and for TLSv1.3/2.

I consider my ciphers as follows:

* Which algorithms do I trust? If I do not trust AES for example, I would unselect it for everything, regardless if that is the control or data channel).

* What does my hardware support? I would want to choose AES if my hardware has acceleration for it.

* Then I would sort them by most secure first (e.g. AES-256, then AES-192 and so on…)

I can replicate that with only one multiple-select box that simply lists:

* AES-256-GCM
* AES-256-CBC
* AES-128-GCM
* AES-128-CBC
* ChaCha20-Poly1305
* Blowfish
* ...

And a second one that has

* SHA512
* SHA384
* SHA256
* …

The code will then have to convert this into the fancy names for the OpenVPN configuration file. But I suppose that should be easy to do.

That way, the user can be certain that they have chosen the right thing for everything in one go.

> So, could you please summarise for me what you want to achieve here?
> Hope i have it done above.

Thank you for that.

I really appreciate all this time that you have invested in this, and the code looks clean. I just think we have been on different pages about what we want it to look and feel like for the user.

> Is this supposed to make OpenVPN more compatible, secure, or anything
> else?
> Yes both but also easier since the user do not need to do anything but
> he should become more security under the hood, the backward and the
> forward compatibility should be in a better standing than before and if
> someone wants to go into the depth, this is even also possible.

I think that you have implemented the “pro” version here. It has all the buttons you need and uses all features of OpenVPN. But it is complicated. So maybe lets hide it from the user that there is a control and data channel. Does the user need to know about this? I do not think so. It will work either way.

> Why do we not talk about this before things are being implemented, or
> did I just miss the discussion?
> Yes i think you missed some of that, hopefully i could remind you on
> some points.

This is good progress :)

-Michael

> 
> -Michael
> 
> Best,
> 
> Erik
> 
> 
>> On 14 Dec 2020, at 14:03, ummeegge <ummeegge(a)ipfire.org> wrote:
>> 
>> Hi all,
>> just wanted to ask if there is still interest in this patch series ?
>> 
>> Best,
>> 
>> Erik
>> 
> 
> 
>