-
Notifications
You must be signed in to change notification settings - Fork 145
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
Normalize the authentication ID #3283
Normalize the authentication ID #3283
Conversation
By normalize, it is intended that; 1) Authentication IDs all can be lowercased for more accurate comparison without being volatile to, say, user error, and 2) Any leading or trailing blank space can be stripped
@@ -346,6 +346,8 @@ EXPORTED int cyrus_init(const char *alt_config, const char *ident, unsigned flag | |||
config_getswitch(IMAPOPT_UNIX_GROUP_ENABLE)); | |||
libcyrus_config_setswitch(CYRUSOPT_USERNAME_TOLOWER, | |||
config_getswitch(IMAPOPT_USERNAME_TOLOWER)); | |||
libcyrus_config_setswitch(CYRUSOPT_NORMALIZEUID, | |||
config_getswitch(CYRUSOPT_NORMALIZEUID)); |
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.
Bad indentation.
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.
The bad indentation can be fixed easily enough with tools/remove-tabs.pl...
The bigger problem here is calling config_getswitch()
with a CYRUSOPT_ option rather than an IMAPOPT_ one. It should probably be config_getswitch(IMAPOPT_NORMALIZEUID)
like the other lines around it.
I think this is the source of one of the warnings @anatoli26 noticed.
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.
Upon deeper examination, I don't think this patch is useful, and Debian should just drop it (eventually).
I guess you'll probably need some sort of transition plan for users who are using it to change their configuration to use username_tolower: yes
instead -- though it's on by default, so maybe it just works already and you can drop it immediately with just a note in the changes file.
@@ -346,6 +346,8 @@ EXPORTED int cyrus_init(const char *alt_config, const char *ident, unsigned flag | |||
config_getswitch(IMAPOPT_UNIX_GROUP_ENABLE)); | |||
libcyrus_config_setswitch(CYRUSOPT_USERNAME_TOLOWER, | |||
config_getswitch(IMAPOPT_USERNAME_TOLOWER)); | |||
libcyrus_config_setswitch(CYRUSOPT_NORMALIZEUID, | |||
config_getswitch(CYRUSOPT_NORMALIZEUID)); |
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.
The bad indentation can be fixed easily enough with tools/remove-tabs.pl...
The bigger problem here is calling config_getswitch()
with a CYRUSOPT_ option rather than an IMAPOPT_ one. It should probably be config_getswitch(IMAPOPT_NORMALIZEUID)
like the other lines around it.
I think this is the source of one of the warnings @anatoli26 noticed.
{ "normalizeuid", 0, SWITCH } | ||
/* Lowercase uid and strip leading and trailing blanks. It is recommended | ||
to set this to yes, especially if OpenLDAP is used as authentication | ||
source. */ | ||
|
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.
This "it is recommended" advice directly contradicts the comment around line 165 of auth_unix.c, where it explains that we used to do something naive like this, but then did it better instead...
Also, we already have a standard option that does the same thing, and is enabled by default:
username_tolower: 1 Convert usernames to all lowercase before login/authentication. This is useful with authentication backends which ignore case during username lookups (such as LDAP).
It doesn't trim leading and trailing whitespace... but if this is a real problem, we should apply a real fix (with tests).
Description: Normalize the authentication ID By normalize, it is intended that; 1) Authentication IDs all can be lowercased for more accurate comparison without being volatile to, say, user error, and 2) Any leading or trailing blank space can be stripped N.B.: upstream advises to just drop this patch, since it's broken (or rather, that they had something like this in the past, but it turned out that many sites break under these assumptions). Author: "Jeroen van Meeuwen (Kolab Systems)" <vanmeeuwen@kolabsys.com> Forwarded: cyrusimap/cyrus-imapd#3283 Reviewed-By: Xavier Guimard <yadd@debian.org Last-Update: 2020-02-10
Description: Normalize the authentication ID By normalize, it is intended that; 1) Authentication IDs all can be lowercased for more accurate comparison without being volatile to, say, user error, and 2) Any leading or trailing blank space can be stripped N.B.: upstream advises to just drop this patch, since it's broken (or rather, that they had something like this in the past, but it turned out that many sites break under these assumptions). Author: "Jeroen van Meeuwen (Kolab Systems)" <vanmeeuwen@kolabsys.com> Forwarded: cyrusimap/cyrus-imapd#3283 Reviewed-By: Xavier Guimard <yadd@debian.org Last-Update: 2020-02-10
Description: Normalize the authentication ID By normalize, it is intended that; 1) Authentication IDs all can be lowercased for more accurate comparison without being volatile to, say, user error, and 2) Any leading or trailing blank space can be stripped N.B.: upstream advises to just drop this patch, since it's broken (or rather, that they had something like this in the past, but it turned out that many sites break under these assumptions). Author: "Jeroen van Meeuwen (Kolab Systems)" <vanmeeuwen@kolabsys.com> Forwarded: cyrusimap/cyrus-imapd#3283 Reviewed-By: Xavier Guimard <yadd@debian.org Last-Update: 2020-02-10
Description: Normalize the authentication ID By normalize, it is intended that; 1) Authentication IDs all can be lowercased for more accurate comparison without being volatile to, say, user error, and 2) Any leading or trailing blank space can be stripped N.B.: upstream advises to just drop this patch, since it's broken (or rather, that they had something like this in the past, but it turned out that many sites break under these assumptions). Author: "Jeroen van Meeuwen (Kolab Systems)" <vanmeeuwen@kolabsys.com> Forwarded: cyrusimap/cyrus-imapd#3283 Reviewed-By: Xavier Guimard <yadd@debian.org Last-Update: 2020-02-10
Description: Normalize the authentication ID By normalize, it is intended that; 1) Authentication IDs all can be lowercased for more accurate comparison without being volatile to, say, user error, and 2) Any leading or trailing blank space can be stripped N.B.: upstream advises to just drop this patch, since it's broken (or rather, that they had something like this in the past, but it turned out that many sites break under these assumptions). Author: "Jeroen van Meeuwen (Kolab Systems)" <vanmeeuwen@kolabsys.com> Forwarded: cyrusimap/cyrus-imapd#3283 Reviewed-By: Xavier Guimard <yadd@debian.org Last-Update: 2020-02-10
Description: Normalize the authentication ID By normalize, it is intended that; 1) Authentication IDs all can be lowercased for more accurate comparison without being volatile to, say, user error, and 2) Any leading or trailing blank space can be stripped N.B.: upstream advises to just drop this patch, since it's broken (or rather, that they had something like this in the past, but it turned out that many sites break under these assumptions). Author: "Jeroen van Meeuwen (Kolab Systems)" <vanmeeuwen@kolabsys.com> Forwarded: cyrusimap/cyrus-imapd#3283 Reviewed-By: Xavier Guimard <yadd@debian.org Last-Update: 2020-02-10
Description: Normalize the authentication ID By normalize, it is intended that; 1) Authentication IDs all can be lowercased for more accurate comparison without being volatile to, say, user error, and 2) Any leading or trailing blank space can be stripped N.B.: upstream advises to just drop this patch, since it's broken (or rather, that they had something like this in the past, but it turned out that many sites break under these assumptions). Author: "Jeroen van Meeuwen (Kolab Systems)" <vanmeeuwen@kolabsys.com> Forwarded: cyrusimap/cyrus-imapd#3283 Reviewed-By: Xavier Guimard <yadd@debian.org Last-Update: 2020-02-10
Closing this for the reasons outlined in #3283 (comment) |
Debian author comment: