-
Notifications
You must be signed in to change notification settings - Fork 247
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
util: Improve re_expression defaults #6646
Conversation
src/util/util.h
Outdated
@@ -250,11 +250,11 @@ struct sss_names_ctx { | |||
sss_regexp_t *re; | |||
}; | |||
|
|||
#define SSS_DEFAULT_RE "(?P<name>[^@]+)@?(?P<domain>[^@]*$)" | |||
#define SSS_DEFAULT_RE "^((?P<name>.+)@(?P<domain>[^@]*)|(?P<name>[^@]+))$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this regex accepts the expression name@
as valid and considers the domain is an empty string. Shouldn't we have a +
instead of the *
in the domain part (?P<domain>[^@]*
) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aplopez - yes you are right but the old expression allowed the same thing.
It looks like we also have tests for that trailing/leading @
and they should fail (but they do not)
sssd/src/tests/cmocka/test_fqnames.c
Line 507 in 9e9d582
sss_parse_name_check(test_ctx, "@"NAME, ERR_REGEX_NOMATCH, NULL, NULL); |
Thanks for pointing that out. I will fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aplopez - the tests were using only IPA/AD regexp in this negative test. I extended them to have this tested also for default regular expression.
The *
is now replaced by +
as you suggested. Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
@@ -3554,12 +3568,6 @@ pam_gssapi_indicators_map = sudo:pkinit, sudo-i:pkinit | |||
default the third one is introduced to allow easy | |||
integration of users from Windows domains. | |||
</para> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I think it might be worth to spell out the character restrictions explicitly here as well. E.g, with respect to group names with an @
in the name. To make this work a fully-qualified name has to be used since we do not allow @
in short names. If a user wants to use short name with an @
character they have to create their own expression.
bye,
Sumit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Idea. Man page updated.
The original defaults of re_expressions did not use "^" so they may skip/ignore some leading character (@ and \). The new defaults uses ^ and $ to be sure that all characters are used. Resolves: SSSD#6635
Update ifp and ssh responders to use regular expression defined centrally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
thanks for the update, ACK.
bye,
Sumit
The original defaults of re_expressions did not use "^" so they
may skip/ignore some leading character (@ and ).
The new defaults uses ^ and $ to be sure that all characters
are used.
Resolves: #6635