search

evgeny-gridasov / openvpn-otp

OpenVPN OTP token support plugin
GNU General Public License v3.0
279 stars 74 forks source link

Use of common name instead of username #44

Open DorianCoding opened 1 year ago

DorianCoding commented 1 year ago

Common_name is set before auth-pass-verify and I guess it should be used instead of username so it would allow to use CN of certificates as username and not to take the user input. Moreover, the username-as-common-name config can be used so username is used as common name. See : https://openvpn.net/community-resources/reference-manual-for-openvpn-2-4/ Note : We can also check if common name exists and if not, use username.

evgeny-gridasov commented 1 year ago

Is this patch going to break existing clients? I think it would be better to give priority to user_name if it is set and if not set, fall back to common_name. Prioritising common_name seems like a breaking change.

DorianCoding commented 1 year ago

Is this patch going to break existing clients? I think it would be better to give priority to user_name if it is set and if not set, fall back to common_name. Prioritising common_name seems like a breaking change.

I just did some test. Common_name is not set when certificate are not passed. So it falls back to username. However if a certificate is passed, common_name will take precedence. This seems logical as a username depends on user input but common_name depends on the leaf certificate, signed and therefore not alterable. Nonetheless, if someone wants a 3-part info, a certificate (common to everyone), a username and a password that only do the authentification, the option username-as-common-name should be used, which shifts the username to the variable common_name as well. If this option is not used then indeed it will break. And as this seems that auth-user-pass is required in 2-factor and 3-factor auth, username would always be set and therefore making username first would not do anything. I think checks should be made to ensure that only a limited number of devs has a incomplete config file that might break during this kind of update.

Note : I edited so you have in pull request the code I used.