I log in to the Red Hat Bugzilla very rarely. So rarely, in fact, that EVERY TIME I log in, I have to reset my password. I suspect that this is the case for the majority of users in the community. Last time I had to do this, it almost just made me not want to file bugs or respond to questions anymore. Perhaps expiration should be limited to only @redhat.com accounts, or only people in significant security groups? I'd also like to point out (as I pointed out when this was being implemented) that password expiration isn't adding any security. (See bug 459513 comment 2 and bug 459513 comment 9.)
(In reply to comment #0) > I log in to the Red Hat Bugzilla very rarely. So rarely, in fact, that EVERY > TIME I log in, I have to reset my password. I suspect that this is the case for > the majority of users in the community. Last time I had to do this, it almost > just made me not want to file bugs or respond to questions anymore. Perhaps > expiration should be limited to only @redhat.com accounts, or only people in > significant security groups? > > I'd also like to point out (as I pointed out when this was being implemented) > that password expiration isn't adding any security. (See bug 459513 comment 2 > and bug 459513 comment 9.) Max I understand your comments and you do make sense in what you say but the change was passed down from management in crisis mode and my hands were tied. They wanted it quick and wanted it to span all Bugzilla accounts. The new expiration policy is in place and had to be consistent for all Red Hat accounts internally and externally which meant Bugzilla as well. We have expanded the expiration time several times since then so it should not expire near as much as it did. That being said, when we move over to 3.4 in the near future, I would like to integrate your locking code as well into our code base as I definitely feel that adds a better level of security than what we have now. So I am changing this bug to reflect the new feature for 3.4. Should I use the patch already attached to bug 355284 at bugzilla.mozilla.org or is there something better in your repository that is ready to go for 3.4? Thanks Dave
Okay, I understand that. Yeah, once bug 366283 passes review, you should be able to backport it to 3.4, so I'd recommend using it then. It has the most current code available.
The only bug I found in upstream bugzilla for the brute password issue was: https://bugzilla.mozilla.org/show_bug.cgi?id=355283 bug #355284 , and bug #366283 mentioned in the previous 2 comments of this bug both opened non related bugs to the brute password problems. So which one is the right upstream bug for this? Thanks, Noura
Noura upstream bug 355283 patch v7 is the correct patch although LpSolit made some suggested changes which maybe we should do as well. Once you are done we can submit your revised patch upstream as well. Bug 366283 was a typo error. Dave
I'll update the upstream patch.
Thanks Dave & Max, I applied the patch to our 3.4 code and currently testing it Noura
Created attachment 367609 [details] v1 for applying upstream brute password patch to 3.4 code branch Hey Dave, I have removed our previous patches that expires bugzilla passwords as in the bugs: https://bugzilla.redhat.com/show_bug.cgi?id=486197 https://bugzilla.redhat.com/show_bug.cgi?id=459513 and applied the upstream patch to our code, tested it and it looks good. it is now applied to bz-web1 for your testing. Please take a look and let me know what you think. Thanks, Noura
Comment on attachment 367609 [details] v1 for applying upstream brute password patch to 3.4 code branch Couple things: When I was testing on bz-web1, I was entering bad passwords for my account one after anohter. After the 2nd attempt I got the warning properly. When I hit the 5th bad password, instead of getting another error screen, I got a blank screen instead. Then the next time I tried to login, I got the normal screen telling me my account is locked. Do you get the blank screen as well? We do not yet have Bugzilla->cgi->remote_addr in our code so that may be the issue. Kevin and Schick are discussing with IS about whether we can remove the password expiration code or not. Schick thinks it is good to at least have a way to mass expire passwords if the need arises. So we may need to still make this work together. Stay tuned. Dave
Hey Dave, Yeah i get the blank screen as well when it is trying to retrieve the ip address to block it, so I guess as you mentioned it is because we don't have the Bugzilla->cgi->remote_addr in our code yet. I tried to look into upstream bugs for the code that has it and couldn't find it, is there a bug for it? also as we remove the old code that changes the password from bugzilla we will need to remove the columns: profiles.password_expire_days profiles.password_last_changed so shall we add $dbh->bz_drop_column($table, $column); for those columns to be executed when we run checksetup.pl for 3.4 upgrade? Thanks, Noura
also one more note , maybe we will need to apply the patch in the bug: https://bugzilla.mozilla.org/show_bug.cgi?id=527586 with the brute password patch Thanks, Noura
remote_addr is part of CGI.pm.
(In reply to comment #10) > also one more note , maybe we will need to apply the patch in the bug: > https://bugzilla.mozilla.org/show_bug.cgi?id=527586 > > with the brute password patch > > Thanks, > Noura Yes please apply the above patch to our code. We currently have a hack in Bugzilla.pm in init_page() that does similar that will need to be removed as well since we will now use the upstream method. But file that as a separate bug report and make this bug depend on it. Dave
(In reply to comment #9) > Hey Dave, > > Yeah i get the blank screen as well when it is trying to retrieve the ip > address to block it, so I guess as you mentioned it is because we don't have > the Bugzilla->cgi->remote_addr in our code yet. I tried to look into upstream > bugs for the code that has it and couldn't find it, is there a bug for it? > Using the patch from comment # 12 you would just use remote_ip() in Bugzilla::Util instead. > also as we remove the old code that changes the password from bugzilla we will > need to remove the columns: > > profiles.password_expire_days > profiles.password_last_changed > > so shall we add $dbh->bz_drop_column($table, $column); for those columns to be > executed when we run checksetup.pl for 3.4 upgrade? > Yes it would go in Bugzilla/Install/DB.pm since we didn't do this as a plugin originally. If it had have been does as a plugin then it would have gone in extensions/redhat_fields/code/install-update_db.pl > Thanks, > Noura
Please look into how much work would be needed to keep the password expiration in along with code but don't do it yet. I just want to give Kevin an idea on how much effort it would take. Dave
Hey Dave, I think it will take from 2 to 3 days of work to merge our password expiration code with upstream brute password patch. Cheers, Noura
Still some other leftover code: Can't locate object method "set_password_expire_days" via package "Bugzilla::User" at /var/www/html/bugzilla/editusers.cgi line 266.
Created attachment 369694 [details] v2 for applying upstream brute password patch to our 3.4 branch Hey Dave, This is a modified version of the previous patch that removes our password expiry code and replaces it with brute password upstream patch, this patch includes the following modifications: 1- the blank screen problem that we used to get at the user lockout time has been fixed, the problem was from missing to add the new template template/en/default/email/lockout.txt.tmpl in the patch 2- added code to remove the profiles table columns related to password expiry 3- removed still existing code of password expiry in editusers.cgi and in some documentation text 4- This patch must be applied after applying the patch in bug #537784, as $cgi->remote_addr has been replaced with remote_ip() I have tested the patch on my localhost and seems to be working perfectly, applied to bz-web1 at the moment but untested because bz-web1 is down. Cheers, Noura
Comment on attachment 369694 [details] v2 for applying upstream brute password patch to our 3.4 branch You missed a section referencing password_last_changed in token.cgi lines 223-231. Also I think we should make LOGIN_LOCKOUT_INTERVAL and MAX_LOGIN_ATTEMPTS configurable from editparams.cgi. Some people may want to have different shorter/longer time and more/less number of retries. Max, do you think we should email a token the locked out user allowing them to click on a link to be able to unlock right away? This would help legitimate users get running again sooner if they somehow messed up their passwords too many times? Malicious users would not see the email and continue to be locked out. Dave
(In reply to comment #18) > Max, do you think we should email a token the locked out user allowing them to > click on a link to be able to unlock right away? This would help legitimate > users get running again sooner if they somehow messed up their passwords too > many times? Malicious users would not see the email and continue to be locked > out. Original versions of the patch actually implemented this, yeah. Originally we removed it because it was too easy to spam somebody's account with lockout notices, but now that we restrict per-IP, it could be a reasonable thing to do, but it might also be very confusing to the user who didn't lock himself out (that is, it wasn't his IP that did the lockout). What do you think?
(In reply to comment #19) > (In reply to comment #18) > > Max, do you think we should email a token the locked out user allowing them to > > click on a link to be able to unlock right away? This would help legitimate > > users get running again sooner if they somehow messed up their passwords too > > many times? Malicious users would not see the email and continue to be locked > > out. > > Original versions of the patch actually implemented this, yeah. Originally we > removed it because it was too easy to spam somebody's account with lockout > notices, but now that we restrict per-IP, it could be a reasonable thing to do, > but it might also be very confusing to the user who didn't lock himself out > (that is, it wasn't his IP that did the lockout). What do you think? I agree that it would be confusing to the user, as we already got few users sending us emails asking why they got notifications about people trying to change their password and that they don't understand what is happening, I say as we are taking care of it for them then it should be okay, besides we "the bugzilla maintainer" get emails about all lockouts.
Created attachment 369863 [details] v3 brute password patch Please ignore this patch Dave
Created attachment 369865 [details] v4 of brute password patch Hey Dave, Thanks for your review, attached is a new patch with: 1- removed missing part in token.cgi related to password expiry code. 2- converted the constants LOGIN_LOCKOUT_INTERVAL, MAX_LOGIN_ATTEMPTS to parameters in editpatams.cgi , pointing to the constants as the default values. Please review and let me know what you think. Thanks, Noura
Comment on attachment 369863 [details] v3 brute password patch I am unable to get your patch to apply cleanly. Please create it from an updated svn tree if you can. patching file Bugzilla/Token.pm Hunk #1 FAILED at 126. Hunk #3 FAILED at 283. 2 out of 4 hunks FAILED -- saving rejects to file Bugzilla/Token.pm.rej patching file Bugzilla/Util.pm Hunk #1 FAILED at 35. Hunk #2 FAILED at 52. Hunk #3 succeeded at 315 with fuzz 1 (offset 10 lines). Hunk #5 succeeded at 471 (offset 10 lines). Hunk #7 succeeded at 991 (offset 10 lines). 2 out of 7 hunks FAILED -- saving rejects to file Bugzilla/Util.pm.rej
(In reply to comment #23) > (From update of attachment 369863 [details]) > I am unable to get your patch to apply cleanly. Please create it from an > updated svn tree if you can. > > patching file Bugzilla/Token.pm > Hunk #1 FAILED at 126. > Hunk #3 FAILED at 283. > 2 out of 4 hunks FAILED -- saving rejects to file Bugzilla/Token.pm.rej > patching file Bugzilla/Util.pm > Hunk #1 FAILED at 35. > Hunk #2 FAILED at 52. > Hunk #3 succeeded at 315 with fuzz 1 (offset 10 lines). > Hunk #5 succeeded at 471 (offset 10 lines). > Hunk #7 succeeded at 991 (offset 10 lines). > 2 out of 7 hunks FAILED -- saving rejects to file Bugzilla/Util.pm.rej Nevermind. Would help if I applied the right patch :) My fault.
Comment on attachment 369865 [details] v4 of brute password patch >Index: Bugzilla/Config/Auth.pm >+ default => Bugzilla::Constants::LOGIN_LOCKOUT_INTERVAL, >+ }, >+ >+ { >+ name => 'maxloginattempts', >+ type => 't', >+ default => Bugzilla::Constants::MAX_LOGIN_ATTEMPTS, >+ }); Nit: I prefer underscores in the names when possible. login_lockout_interval max_login_attempts >Index: template/en/default/admin/params/auth.html.tmpl >+ loginlockoutinterval => "This defines the time that the account with the maximum login " _ >+ "attempts will be locked for.", "This defines the time that the account with the maximum login " _ "attempts will be locked out for in minutes." >+ maxloginattempts => "This defines the maximum failed logins attempts to lock account using " _ >+ "its IP address.",} "This defines the maximum number of failed login attempts before locking account from " _ "it's IP address." Otherwise works well and looks fine. You can update the above before checkin. Dave
Thanks for the review Dave, I made the changes you suggested and committed to svn. Noura
If you get some free time please work on a version of this patch that applies cleanly to the upstream Bugzilla code. And then attach the new updated patch to the attached upstream bug report asking for review. We would like to get this accepted upstream as well. Thanks Dave
sure Dave, will do that.
Note that currently, upstream, we don't want additional parameters for this--we're trying our best to cut down on parameters.
(In reply to comment #29) > Note that currently, upstream, we don't want additional parameters for > this--we're trying our best to cut down on parameters. In this case, I don't really need to submit our patch upstream then, caze only thing extra that has is the adding of the parameters, and removal of our password expiry which upstream doesn't have.