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

Use of common name instead of username #44

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DorianCoding
Copy link

@DorianCoding DorianCoding commented Feb 17, 2023

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
Copy link
Owner

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
Copy link
Author

DorianCoding commented Feb 18, 2023

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.

@DorianCoding DorianCoding reopened this Mar 1, 2023
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.

2 participants