set SNI for VPN connection

[astibal]

It is usually good practice to set SNI for the TLS connection.

While this is not a huge issue as SSLVPN is not requiring it, it's probably a good idea to set it:

• it may get required in future
• other middleboxes may require SNI to correctly handle the connection

I set the default to ON (don't think it should be OFF by default). Option --no-sni will indeed disable SNI to be set and sent.

[DimitriPapadopoulos]

@astibal Do we really want SNI to be optional? We already have option --trusted-cert in case of a certificate problem.

[DimitriPapadopoulos]

By the way, we must use standard C style for the source code; otherwise it won't pass the CI tests. For example: if( → if (

[astibal]

Hi Dimitri,

@astibal Do we really want SNI to be optional?

My pull request makes SNI enabled by default, disabled with --no-sni.

We already have option --trusted-cert in case of a certificate problem.

Yes, I am aware and using it too when needed.

This is however different problem (admittedly not a big one): SNI is not present in ClientHello.
Well behaving clients should add SNI.

By the way, we must use standard C style for the source code

I will reformat the code to look as required

Thanks, Ales

[astibal]

@DimitriPapadopoulos I have fixed if statements. Let me know should I change yet something else.

I followed set-routes config variable. I hope all places are handled correctly.
I will fix otherwise.

[DimitriPapadopoulos]

My question still stands, why make SNI optional? I mean, I understand you have made it the default, but why would end-users need option --no-sni?

[DimitriPapadopoulos]

A simple rebase will fix the Codespell CI job error.

[astibal]

My question still stands, why make SNI optional? I mean, I understand you have made it the default, but why would end-users need option --no-sni?

I see! I just considered it would be fair to add an option which would revert to original behavior.

[DimitriPapadopoulos]

I see! I just considered it would be fair to add an option which would revert to original behavior.

I just feel that it won't be needed in practice. The only possible reason for failure would be that the FortiGate chokes on the SNI request. However, this is not the case as far as I know.

[astibal]

I see! I just considered it would be fair to add an option which would revert to original behavior.

I just feel that it won't be needed in practice. The only possible reason for failure would be that the FortiGate chokes on the SNI request. However, this is not the case as far as I know.

Patch assumes gateway is a hostname. If someone uses IP, this IP will be sent in SNI in its string form.
That's another reason I added --no-sni. Just for the case.

At any rate I would be happy if you accept the patch in whichever form you like. With or without --no-sni option.

[DimitriPapadopoulos]

Perhaps it woiuld make sense to have an --sni=HOST option instead. So SNI itself wouldn't be an option, but the SNI hostname could be set to a different value than the hostname or IP address passed to openfortivpn.

[astibal]

Hmm, taking it from this angle is also ok.
What would be the approach?

I am happy with all versions:

• added SNI
• added SNI + --no-sni
• added SNI + --sni=<string>

It's indeed your call now. I can redo it once more, but want to know what is approach with a potential to be accepted :)
A.

[DimitriPapadopoulos]

I would most certainly accept the PR with --sni=<string> – and passing Style CI tests.

[DimitriPapadopoulos]

The rationale is that --sni could help bypass a broken or hostile DNS server.

[DimitriPapadopoulos]

@astibal I have rebased onto branch master and applied the suggested change in branch sni. Comments?

[astibal]

@astibal I have rebased onto branch master and applied the suggested change in branch sni. Comments?

It's great, thank you! I was busy with other things recently, sorry.

[DimitriPapadopoulos]

It would be great if you could test on your side.

@mrbaseman Your opinion would be valued, especially on the decision to always define the SNI during TLS handshake in branch sni.

[mrbaseman]

well, since my review has been requested: 91527f2 looks good to me ;)

[DimitriPapadopoulos]

@mrbaseman Thank you Martin. I couldn't find corner cases where SNI breaks the VPN negotiation, since the server name can be modified using option --sni where needed (for example when connecting directly to an IP address).