Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TLS 1.3 for EAP-TLS, user search by certificate CN #1751

Merged
merged 4 commits into from
Feb 1, 2023

Conversation

Evengard
Copy link
Member

This fixes #1722, restricting the TLS 1.3 only for the EAP-TLS flow. Allowing TLS 1.3 requires more research (more info about that in #1722 (comment)), so a safe choice would be to let it be disabled - it is still treated as highly experimental pretty much everywhere.

As an added bonus, implemented searching a user account by it's certificate CommonName (CN). This is useful when the certificate CN doesn't match the user login in the VPN server, and as the Windows PPP client sends the certificate CN as EAP Identify packet, this fixes the inability to find the user by it's certificate.

Maybe this should also be backported to other protocols relying on cert auth (namely OpenVPN?), but I have no ideas how that works for them (maybe the actual username is still being sent).

@Evengard Evengard marked this pull request as draft January 23, 2023 01:36
@domosekai
Copy link
Contributor

Looking up user by certificate is not reliable. What if two users have the same certificate?
Windows SSTP client can actually specify a different username than the CN in EAP-TLS options. I have tested and it worked fine.

@Evengard
Copy link
Member Author

@domosekai
That would be weird, two users having the same cert (the whole certificate paradigm tells that an entity may have multiple certificates, not the other way around), but technically that is possible, although I'd say that's more of a server misconfiguration case. Either way, the user-by-certificate lookup without specifying the username is used pretty widely, from smartcard Windows authorization to authorization by client certificates on the web.

In any way, if two users have the same cert, then technically the user can and should login as either of theese users, and logging in as the user with the same username as the certificate CN is a valid default, but the expectation under Win is that no other configuration is required for logging in with a certificate...

The certificate lookup is implemented as a fallback mechanism only if the username wasn't found. The older mechanism of looking up by username still works, and if a username with the same name as a CN was found (or was overriden in configuration - actually overriding in config is the only way to customize another hub name) it'll use it.

@domosekai
Copy link
Contributor

the user-by-certificate lookup without specifying the username is used pretty widely, from smartcard Windows authorization to authorization by client certificates on the web.

I am not against the idea generally, but we need to pay attention to the implementation difference.
Here at SoftEther VPN, the authentication system is structured in the way that a username is evaluated before the actual authentication method. We do not match the certificate before user presenting the username.
I agree your argument that two users sharing the same certificate is a kind of misconfiguration. But we cannot "exploit" that misconfiguration by making it easy for a user who tries to log into other account unless he/she knows the exact username.

if two users have the same cert, then technically the user can and should login as either of theese users.

This is not an assumption in SoftEther and we should not expand our interpretation of the design.

A client certificate should bear the username in its CN or UPN. This is true in Windows domain environment and should be true in SoftEther as well unless there is a strong reason. Moreover, Windows client already offers a standard way to override the implied username. Therefore I don't think this function is useful.

@Evengard
Copy link
Member Author

One of the options would be making it behind a configuration flag (false by default, for example).

@domosekai
Copy link
Contributor

I tested on Windows 2022 RRAS.
If I present a valid user certificate with either a non-exist username or a username of others, the login fails in both cases.
I would consider anything that would allow someone login into an account that he/she does not even know the account name a security breach.
The usage of this function is that a client certificate was not generated in the right format in the first place. More specifically, it is very likely a certificate borrowed from elsewhere so that it does not bear the right CN. Or is there a real use case?

@Evengard
Copy link
Member Author

Well, on my previous job we used smartcards, and the CN contained the "full name" instead of the "account name" in it. The account name being in the username_un format, while the CN being "Username U. Name". This was used as a domain authentication certificate, and it matched the user just fine (apparently using the E in the format of [email protected]), so there's that.

@Evengard
Copy link
Member Author

Now EAP-TLS 1.3 works by default with Windows 11. EAP-TLS 1.2 works with Windowses which doesn't work with TLS 1.3. PPPD defaults are 1.2, it works with this EAP-TLS rework too (for PPPD 1.3 support refer ppp-project/ppp#388 - I consider it broken for now anyway). VPN Client Pro for Android tries using EAP-TLS 1.3, errors and then downgrades to 1.2, resulting in working authorization too. Honestly, I did probably all I could for supporting different clients. Requesting tests from other clients (eg I don't have any Apple devices to test).

@Evengard Evengard marked this pull request as ready for review January 23, 2023 17:06
@Evengard
Copy link
Member Author

About the certificate-user matching - I tried to create a certificate with the E field, and it still sends the CN to the server. So there are existings setups where the CN can differ from the actual account name, and the user is matched by certificate.

@Evengard
Copy link
Member Author

Found a way to support PPPD 1.3 without breaking Windows 1.3. The support works the same way as the support for VPN Client Pro was implemented - by iteratively downgrading the TLS specs - first we start in TLS 1.3 mode + Session Tickets, if we fail we downgrade to TLS 1.3 without Session Tickets, if we still fail we downgrade to TLS 1.2.
Thanks EAP-TLS spec defining the PPP_EAP_TLS_FLAG_SSLSTARTED flag which resets the SSL handshake, and clients abiding it.

@Evengard Evengard changed the title TLS 1.3 only for EAP-TLS, user search by certificate CN TLS 1.3 for EAP-TLS, user search by certificate CN Jan 23, 2023
* Reworking EAP-TLS flow
* Implementing iterative TLS downgrade supporting PPPD TLS 1.3+Tickets, Windows TLS 1.3 w/o Tickets, VPN Client Pro TLS 1.2.
@domosekai
Copy link
Contributor

Windows client allows to override the default username inferred from the certificate. It's the recommended way to tackle the problem where CN differs from the real username.
Again, in SoftEther VPN it's not reliable or secure to lookup user by its certificate, because unlike in AD, there is no CA function and certificates cannot represent users.

@chipitsine
Copy link
Member

chipitsine commented Jan 24, 2023 via email

@Evengard
Copy link
Member Author

So, what do you think about hiding it behind a false by default configuration parameter?

@chipitsine
Copy link
Member

chipitsine commented Jan 24, 2023 via email

@Evengard
Copy link
Member Author

I'll try to update the PR in the coming days. Admin options seems like a good place for that?

@Evengard
Copy link
Member Author

Now EAP-TLS by default does not match by certificate. This can be enabled by setting allow_eap_tls_match_user_by_cert Admin option of the hub to 1.

@Evengard
Copy link
Member Author

Reworked it to AllowEapMatchUserByCert Extended hub option instead of Admin option.

@Evengard
Copy link
Member Author

A review and a test on different clients (namely MacOS/iOS ones) would be appreciated.

@Evengard
Copy link
Member Author

Evengard commented Jan 31, 2023

Never knew, but there's actually a EAP-TLS 1.3 separate RFC! https://www.rfc-editor.org/rfc/rfc9190.html
And actually, because of privacy requirements, matching by certificate (but differently than done right now) may be still needed, as the RFC specifies that the identity info may be incomplete or scrambled for privacy reasons.
There's also some more details I missed (such as AppData 0x00 message which is mandated by this RFC), which may be the reason Windows didn't actually accepted the session ticket, while it should had. I'll try to rework this to account for that and retest.
Btw, the PPP project confirmed that their implementation is incorrect, but this one may be incorrect too (even though it works with major clients).

@chipitsine
Copy link
Member

chipitsine commented Jan 31, 2023 via email

@Evengard
Copy link
Member Author

@chipitsine wait for now please, as I said in the previous message I need to rework it for the RFC conformance.

@chipitsine
Copy link
Member

chipitsine commented Jan 31, 2023 via email

@Evengard Evengard marked this pull request as draft January 31, 2023 09:40
@Evengard Evengard force-pushed the eap-tls-fixups branch 2 times, most recently from 3d5baf8 to d1af75e Compare January 31, 2023 17:27
@shipitsin-ilia
Copy link

@Evengard . I have an idea. are there other implementations of RFC9190 ? maybe we can have a look and take some automated test cases from those projects ?

@Evengard
Copy link
Member Author

Evengard commented Jan 31, 2023

Not that I am aware of, if it could exist anywhere then probably in IEEE-802.1X, IEEE-802.11 or IKEv2 environments. The PPP project will apparently attempt to implement it as close to the RFC as possible, but that will be sometimes in the future...

Anyway, I think I managed to rework it.

As I thought, this AppData 0x00 message made the Windows client accept the session tickets. I still kept the downgrade attempt without session tickets just in case. We can remove it though and leave only the TLS 1.3 => TLS 1.2 downgrade instead.

The certificate matching was reworked too. It is still behind a virtual hub option, but now it matches not by CNs, but by the certificate itself. If enabled and when the server can't find the identity specified in the EAP Identity flow, it restricts the EAP auth flow to only use EAP-TLS. During the EAP-TLS it tries to find the user with the specified certificate, failing if it can't find it.

This certificate matching behaviour seems to be something which would work OK with the privacy shenanigans described by the RFC. Still, disabled by default, as requested.

@Evengard Evengard marked this pull request as ready for review January 31, 2023 17:43
@Evengard
Copy link
Member Author

Evengard commented Jan 31, 2023

Forgot about tested clients. The compability is the same, both Windows and PPPD clients works in both EAP-TLS 1.3 and 1.2 modes, and VPN Client Pro (Android) automatically downgrades to EAP-TLS 1.2 without problems.

I still don't have any MacOS/iOS devices.

@chipitsine
Copy link
Member

can you share steps how do you invoke pppd for testing ? I'll try to automate.

it is not a blocker, just side note

@chipitsine
Copy link
Member

same as myself, windows and android.

@Evengard
Copy link
Member Author

Evengard commented Jan 31, 2023

I'm using the SSTP client for that. Debian Testing have the needed package for that.
The PPPD options file is as follows:

require-eap
ipcp-accept-local
ipcp-accept-remote
noipdefault
cert  <path_to_user_cert>.cer
key   <path_to_user_private_key>.key
capath /etc/ssl/certs
name <the_user_name>
debug
logfile /tmp/pppd.log
pty "sstpc --log-level 4 --log-stderr --nolaunchpppd <server_IP_or_domain_name>"
max-tls-version 1.3
noauth

Invoke it like this (as root): pppd file <path_to_options_file>

As a side note, without --log-stderr the sstp client was segfaulting.

@Evengard
Copy link
Member Author

And yeah, the PR is ready for a review or merge I guess.

@chipitsine chipitsine merged commit 11828be into SoftEtherVPN:master Feb 1, 2023
domosekai added a commit to domosekai/SoftEtherVPN that referenced this pull request Feb 2, 2023
domosekai added a commit that referenced this pull request Feb 2, 2023
@Evengard Evengard deleted the eap-tls-fixups branch February 2, 2023 20:19
@shipitsin-ilia
Copy link

@Evengard , this PR is merged.

I tried to implement automatic pppd testing.

so far, I tried your script to check whether sstpc will crash or not without --log-stderr

mine did not crash, I tried 1.0.17-1 on Ubuntu 22.04 and I built master branch https://gitlab.com/eivnaes/sstp-client
neither crashed.

which version is shipped with debian (?) that crashes ?

hopefully, I'll be back with automatic testing soon :)

@shipitsin-ilia
Copy link

as for automated testing, it fails with "authentication fails", I'm investingating what I did wrong.

however, there's interesting message

EAP: Identity prompt "Welcome to the SoftEther VPN server!"
sent [EAP Response id=0x1 Identity <Name "test">]
rcvd [EAP Request id=0x2 TLS --S]
MTU = 1486
calling get_eaptls_secret
calling eaptls_init_ssl
Loading OpenSSL config file
EAP-TLS: Error in OpenSSL config file /etc/ppp/openssl.cnf at line 33

why it tries to find /etc/ppp/openssl.cnf ?

@Evengard
Copy link
Member Author

Evengard commented Feb 15, 2023

Surprisingly, I also did compile it from the source code AND both tried to use it from the Debian packages, but I did use it from within WSL2. That may add unexpected things. Maybe it crashed because it failed to find the syslog or smth.

As for the openssl.cnf, that is weird, I don't have such a file on my system and it did "Just Work" ©, although I did use the version from Debian repos, maybe they pointed it to the system default /etc/ssl/openssl.cnf.

The eap-tls README does mention it though: https://github.com/ppp-project/ppp/blob/master/README.eap-tls (3. Configuration).

Is your private key encrypted? I used a decrypted one, for the encrypted it seems to use the openssl.cnf for the decryption key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Developer Edition (DE) not work OpenVPN when using TLS 1.3
4 participants