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

Fix username validation for CommonName. #1

Closed
wants to merge 1 commit into from

Conversation

eiraelr
Copy link

@eiraelr eiraelr commented Mar 12, 2019

Fixes an issue with usernames containing numbers.

The IsValidFQDN code checks if the last part of its argument only contains ascii letters and fails otherwise.
As usernames only have one part any name containing any other characters will fail.

I should say that I haven't been able to test this fix.

Fixes an issue with usernames containing numbers.
The IsValidFQDN code checks if the last part of its argument only
contains ascii letters and fails otherwise.
As usernames only have one part any name containing any other
characters will fail.
@dantremblay dantremblay mentioned this pull request Mar 20, 2019
@ghost
Copy link

ghost commented Mar 20, 2019

the package go-utils/validation has not been updated to the latest one and since then there was a commit to fix the fqdn validation function.

I'm closing that PR as that's not the fix to your problem

@ghost ghost closed this Mar 20, 2019
@eiraelr
Copy link
Author

eiraelr commented Mar 20, 2019

The fix in go-utils/validation would still fail, if the check is supposed to check that the client name is not a fqdn. In the current state it only works because the fqdn check was not correct
However it fails if the username contains anything that is not a ASCII letter.
if err = validation.IsValidFQDN(csr.CR.Subject.CommonName); err != nil
With the current check a valid client CN might be "example.com" but not "user1", and with an updated go-utils/validation any client CN not containing a dot will be consider invalid, which if I understand the code/comment correctly is the wrong conclusion.

@ghost
Copy link

ghost commented Mar 20, 2019

yes with the new update of the function, a valid client CN like user.last will fail but user1 will not.

@ghost
Copy link

ghost commented Mar 20, 2019

I will use https://github.com/inhies/go-tld to check the last part of the name if the length is equal to 2.

@eiraelr
Copy link
Author

eiraelr commented Mar 20, 2019

If usernames with dots should work, then I think validation.IsValidUsername could be used instead.
With the current code and update of go-utils/validition will lead to user.name working, but not user1.

@ghost
Copy link

ghost commented Mar 20, 2019

ok, now I understand what you mean. I had in mind that if it's a valid fqdn then it should fail but the code says if it's not a valid fqdn then fail. The validation line is if err = validation.IsValidFQDN(csr.CR.Subject.CommonName); err != nil { and should be ; err == nil at the end of the line.

For now I prefer to keep validation.IsValidFQDN().

This pull request was closed.
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.

None yet

1 participant