Add a valid_user type #184

XaF · 2018-08-07T19:14:11Z

In some cases, we need to be able to check a username received as
parameter, this allows it by providing a new valid_user_name function
that checks that the username received as parameter is a valid one,
i.e. comprised only of alphanumeric characters, plus a few
special ones: .+:-

hpcugentbot · 2018-08-07T19:14:13Z

Can one of the admins verify this patch?

stdweird · 2018-08-07T20:25:52Z

quattor/functions/validation.pan

@@ -139,3 +139,15 @@ function valid_absolute_file_paths = {
    };
    true;
 };
+
+@documentation{
+    desc = Function to validate whether the parameter received is a valid username or not; a valid username is considered to only be comprised of alpha numeric characters, plus a few special ones which are: .+:-


line too long

How do you split desc lines ?
Most of the examples I've seen don't split the documentation lines...

usual \ followed by newline works.
ideally newline and eg some indentation should work too; i opened quattor/pan#210

Ok! Will update. Should I use indentations or not ? (i.e. \ then writing from the first char of the next line, or using indentations to align such as in the example you give in your opened issue ?)

use the indentations, we'll fix the generated docs rather than make the annotation itslef unreadable/ugly

stdweird · 2018-08-07T20:27:22Z

quattor/functions/validation.pan

+}
+function valid_user_name = {
+    user = to_string(ARGV[0]);
+    if(!match(user, "^[A-Za-z0-9._:-]+$")) {


and the usernames can start with the funky stuff too?

True, it shouldn't. I actually converted a regex that Morgan Stanley was using internally to a function. Not sure why characters such as . or : are authorized. Will check. Might change that to the more standard regex for usernames: ^[A-Za-z_]([A-Za-z0-9_-]{0,31}|[A-Za-z0-9_-]{0,30}\$)$

usernames can start with digits (i think posix allows usernames to be digits only, but lets not go there ;)

If we want to be entirely accurate, we might as well support actually all usernames that can be accepted, from http://manpages.ubuntu.com/manpages/bionic/man8/useradd.8.html (and other resources):

It is usually recommended to only use usernames that begin with a lower case letter or an underscore, followed by lower case letters, digits, underscores, or dashes. They can end with a dollar sign. In regular expression terms: [a-z_][a-z0-9_-]*[$]? On Debian, the only constraints are that usernames must neither start with a dash ('-') nor plus ('+') nor tilde ('~') nor contain a colon (':'), a comma (','), or a whitespace (space: ' ', end of line: '\n', tabulation: '\t', etc.). Note that using a slash ('/') may break the default algorithm for the definition of the user's home directory. Usernames may only be up to 32 characters long.

The lack of limitation is linked to the fact that PAM allows pretty much anything to be used, while the old passwd was more limitating. Maybe @ned21 would have a better idea as to why this regex is currently used at Morgan Stanley, and what would be acceptable not to break anything for us nor for others ?

hmm.. I probably don't know the real history, git blame on the internal repository might yield some light but I doubt it.

I would go with what is well documented for e.g. ubuntu (can we check RH too?) The one caveat is that I know . is definitely required because in some places we ended up using firstname.lastname as a userid. I am guessing that also exceeds the 32 character limit that Debian/Ubuntu imposes.

I would randomly guess that : might have been considered valid because in various configuration components, passing user:group was a way to set the user and group of a file. In fact, @XaF you will probably find that the there is template code internally that says a field is a "user" but actually accepts user:group syntax to allow for the automatic provisioning of directories based on which users may login to a host. (Sorry I forgot about this when discussing the code internally, it was only when comparing the two regexes that I spotted the potential significance of them.)

From what I understand of the useradd man page, the only real "official" limitation is for the first char (Debian/Ubuntu) and for the username length of 32 characters. The rest is only a recommendation. Moreover, as I was saying before, the generalisation of using PAM for authentication made those limitations even blurrier, and we now only have recommendations that apply. Pretty much every system can have a different setup and different rules for usernames (from starting and ending characters to username length).

It seems to be a bigger issue than one that could be tackled in a general way. I feel that if we are very specific about that internally, we might want to keep that type for ourselves and not burden the community with something that will only be useful to us. Thoughts?

If this validation result in allowing usernames that other sites would consider invalid then I agree: let's defer it to each site to define their own. It would be nice to document that decision somewhere to prevent a re-run of this conversation in the future but I'm not sure where would make sense. This would be a clear use case for being able to override or add to types in PAN: we could then define type valid_username with a very basic string type/minimal regex, and comment that this is designed to be overridden by site-specific policies.

I feel that's a good idea, this would allow to standardize the existence of that variable if we need it in other generic templates, while still keeping it generic enough from the beginning, and site-specific as we recommend to override it.

What would you feel would be a good default value for that? I think that going for the recommended regex defined in the useradd man page (that I quoted earlier) might be interesting here. Or would you prefer a regex that allows everything? ^. *$. Thoughts @ned21 @stdweird @jrha ?

stdweird · 2018-08-07T20:28:08Z

quattor/functions/validation.pan

+    user = to_string(ARGV[0]);
+    if(!match(user, "^[A-Za-z0-9._:-]+$")) {
+        error(user + " is not a valid username!");
+        return(false);


this will never be reached. if you want the function to be a proper boolean function, remove the error (replace by debug)

Will the true/false answer be sufficient for using types ?
i.e. type valid_user = string with valid_user_name;

yes.
do you actually need the function at all? why not make a type to start with, like

type valid_username = string with match(SELF, someregex);

not sure about the error message, if quattor/pan#201 gets merged, the error will contain the failed value

I don't mind not having it as a function. I'm just starting doing stuff in pan, so most of what I do might be optimized! Once we find a common ground for the regex itself, I'll probably move to using that directly as you suggest. I suggested to merge the type in quattor/configuration-modules-core#1312

since 10.7 you can use a type directly to test a value
before that release, people used to make a separate function if they wanted to test for composite types and such. that's why the current template library has some many type functions

The first occurrence of that code was actually a if (!match(...)) repeated at multiple places, that I replaced by the above function, that then was suggested to me to use to check the type directly instead of having it in the if. Did not catch at that point that I could do the match directly... héhé.

Regarding the type itself, do you know if there is a better place for me to submit it, or the place I did is the best place? The split of all the templates in multiple repositories is quite confusing for now.

@jrha any thougths where to put the type? here or in account component?

I guess that user names are used for more than just ncm-accounts, so here is ok.

stdweird · 2018-08-07T20:29:30Z

i'm not too happy with this, given that the special characters make this an invalid username for us...

jrha · 2018-08-08T09:40:44Z

@XaF whose definition of valid user is this?

XaF · 2018-08-08T10:13:45Z

@jrha tagged your id in the ongoing conversation about the regex itself. Not sure why github is considering that part of the code outdated.

TLDR; this is Morgan Stanley's regex, but we seem to be moving to using a generic regex and recommending it to be overridden by each site.

XaF · 2018-09-25T22:16:27Z

Updated to set up as type, and added the comment about it being a "default value" meant to be overriden for site-specific uses.

jrha · 2018-10-05T13:15:06Z

@stdweird what is your feeling about this now?

If necessary, it seems like it might be easier to override if the pattern were defined as a variable, e.g.

variable VALID_USER_PATTERN ?= '^[a-zA-Z_][a-zA-Z0-9_-]{0,30}([a-zA-Z0-9_-]|\$)?$';
type valid_user = string with match(SELF, VALID_USER_PATTERN);

XaF · 2018-10-05T13:52:17Z

Happy to have it like you suggest @jrha. Just tell me if you want to have it with the variable and I'll update the commit.

jrha · 2018-10-10T16:19:53Z

@hpcugentbot please test this please

stdweird · 2018-10-29T11:03:59Z

@jrha i'm prefer your variable solution, esp if you make it final

jrha · 2018-10-29T11:05:06Z

Discussed at workshop, consensus was to use a variable to allow sites to completely redefine the regex.

stdweird · 2018-10-29T11:05:20Z

quattor/functions/validation.pan

@@ -139,3 +139,11 @@ function valid_absolute_file_paths = {
    };
    true;
 };
+
+@documentation{
+    desc = This types is meant to be used to check if strings provided are \


remove the desc = , and you can also get rid of the \ at the end of each line

Just

@documentation{ foo bar baz }

?

Did it like that. Please comment if I'm mistaken :)

quattor/functions/validation.pan

This types is meant to be used to check if strings provided are valid user names. This is however designed to be overriden by site-specific policies. The regex used as default for this type is the one suggested by the useradd man page.

jrha · 2018-11-21T17:07:15Z

@hpcugentbot test this please

jrha · 2018-11-21T17:08:47Z

Travis failure caused by pre-existing lint elsewhere, doesn't need fixing in this PR.

Problem addressed with overridable global variable.

XaF mentioned this pull request Aug 7, 2018

ncm-accounts: Add a valid_user type to validate user names quattor/configuration-modules-core#1312

Closed

stdweird reviewed Aug 7, 2018

View reviewed changes

XaF force-pushed the patch-1 branch from 1163814 to 93baa7b Compare August 7, 2018 20:59

XaF force-pushed the patch-1 branch from 93baa7b to da6152f Compare September 25, 2018 22:15

XaF changed the title ~~Add a valid_user_name function~~ Add a valid_user type Sep 25, 2018

XaF mentioned this pull request Sep 25, 2018

types: aquilon: add new structure_entitlements in the system parameters #189

Merged

XaF force-pushed the patch-1 branch from da6152f to b6082dd Compare September 25, 2018 22:43

jrha added this to the 18.9 milestone Sep 27, 2018

jrha added the enhancement label Sep 27, 2018

jrha approved these changes Oct 5, 2018

View reviewed changes

jrha added the discuss at workshop label Oct 25, 2018

stdweird reviewed Oct 29, 2018

View reviewed changes

jrha removed the discuss at workshop label Oct 29, 2018

ned21 previously requested changes Oct 29, 2018

View reviewed changes

quattor/functions/validation.pan Outdated Show resolved Hide resolved

XaF force-pushed the patch-1 branch from b6082dd to 87f373b Compare October 30, 2018 00:22

stdweird reviewed Oct 30, 2018

View reviewed changes

quattor/functions/validation.pan Show resolved Hide resolved

Add a valid_user type

30a8727

This types is meant to be used to check if strings provided are valid user names. This is however designed to be overriden by site-specific policies. The regex used as default for this type is the one suggested by the useradd man page.

jrha force-pushed the patch-1 branch from 87f373b to 30a8727 Compare November 21, 2018 16:03

jrha merged commit a45c395 into quattor:master Nov 22, 2018

Add a valid_user type #184

Add a valid_user type #184

Conversation

XaF commented Aug 7, 2018

hpcugentbot commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ned21 Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stdweird commented Aug 7, 2018

jrha commented Aug 8, 2018

XaF commented Aug 8, 2018 • edited Loading

XaF commented Sep 25, 2018

jrha commented Oct 5, 2018

XaF commented Oct 5, 2018

jrha commented Oct 10, 2018

stdweird commented Oct 29, 2018

jrha commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrha commented Nov 21, 2018

jrha commented Nov 21, 2018

ned21 Aug 7, 2018 •

edited

Loading

XaF commented Aug 8, 2018 •

edited

Loading