GNOME Bugzilla – Bug 477881
GDM password entry character stuff rework
Last modified: 2008-03-02 05:48:04 UTC
Previously, * was the implicit default masking character in GTK. GDM had options for changing this to be either invisible or use a Black Circle (U+25CF). These options were presented as letting you choose between Asterisk, Circle and Invisible. Many downstreams (Ubuntu, Fedora, SuSE) have black circle as their default password character in GTK. As a result, this option becomes a choice between "Asterisk" (which is actually default, i.e. Circle), Circle and Invisible. Of course this seems broken, so we patched GDM recently to explicitly set it to asterisk if it was neither Circle nor Invisible, restoring sanity to the situation. However, this just seems wrong. Ignoring the fact that applications setting their invisible characters causes problems for consistency, the choice is between three hardcoded settings with no regard for the default. So, I'd like to change GDM to have 2 configuration options: "EntryUseCustomInvisibleChar" (boolean flag) and "EntryInvisibleChar" (integer Unicode codepoint). We can change the setup UI to choose between "Default", "Invisible" and "Custom" with a widget to enter a codepoint.
Created attachment 95762 [details] [review] Patch No change to gdmsetup UI yet. This doesn't seem to work, can anyone tell me why?
Created attachment 95763 [details] [review] Patch Whoops, forgot to delete old config macros.
I think this is a better way to configure things. However, GDM configuration is considered stable, so we shouldn't be deleting old configuration options. We should have a reasonable approach to supporting users who previously had the old configuration settings. I'd say if the custom configuration file contains UseInvisbleInEntry=true or UseCirclesInEntry=true, that this should override the new configuration setting. I'd be okay with dropping the older/deprecated configuration options from the gdm.conf.in file. Also, you need to update the docs in docs/C/gdm.xml. Also, you only modified gui/gdmlogin.c. Note there is similar code in gui/greeter/greeter.c that also needs to be changed.
Created attachment 95770 [details] [review] Patch I copied and pasted the code for now (as was the situation before) but added some FIXMEs to highlight this for future readers.
Created attachment 95771 [details] [review] Patch Whoops again, looks like I forgot what language I was coding in again. This actually compiles now.
So one thing that bothers me, is that gdm already has a lot of config options. Do we really want to bring up the number of config options dealing with /the password character/ to 4? I don't think there should have ever been a config option in the first place, personally, but it's too late to change that now.
I agree with you mostly, but I recognise that some people would like "invisible" password entry (i.e. no feedback at all) on the login screen for added security. If we want to drop customizability, we could just go back to UseInvisibleInEntry and drop the rest of the options, but then you can guarantee that someone will complain that we're dropping useful functionality in GNOME again. I hate complicating the code base as much as you do. I for one will be leaving it as default, and I imagine most downstreams and end-users will, too, once the default setting actually respects the default GTK setting!
Ray, it isn't too late to change that now ;) I have no plans on supporting this option in the gobject branch. We need to excise config options. http://mail.gnome.org/archives/usability/2006-February/msg00176.html
Any idea when GDM3 is ready? 2.24?
William, when you say that we need to excise config options, I'd like to understand more what you mean. I'm a bit confused after reviewing the latest D-Bus GDM code. It seems that this branch is missing configurability in general. Most of the previously configurable options now seem hardcoded. I assume this is because more work needs to be done to add configuration, or is your plan that the D-Bus branch of GDM will be configurable in a very different way than the current version? Personally, I think one of the strengths of GDM is that it is highly configurable. Many people want to do very different things with GDM, and it seems hard to support that without configuration. Or is your plan that the new GDM will only support a subset of users who currently use GDM? This seems problematic. We have talked a bit about whether the D-Bus GDM branch will support the existing configuration, and I seem to be getting different answers depending on when I ask. If our plan is to excise options, then I think we should be careful about which ones we get rid of. That said, there is a good bit of cruft in the configuration and it might make sense to drop certain options, and to reorganize how GDM manages configuration in general. However, I worry about trying to redo the configuration system along with all the other changes that we are doing. While it might be tempting to try to fix all the badness of GDM at once, I think trying to re-figure how to most sensibly support configuration in the new GDM would cause the project to fall into a rathole. If we drop configuration options, we will find all the users who depend on them screaming, which I'd prefer to avoid. Further, figuring out how to redesign the configuration system in GDM is a big project in and of itself. ---- In terms of this enhancement request: I don't think that this option is one that should go away. Users should be able to configure the degree of visual feedback that they see when they type their password. If people were to disagree, I would push that GDM should never provide any feedback since this is the most secure option and doesn't give away the length of the password (which *is* important to some users - and security should trump usability). In terms of this bug report, I think there is a need to support different feedback mechanisms. Some users want absolutely no feedback and other users want some character (circles or an asterisk) to appear, and perhaps some users want other options. I'd prefer to fix the configuration file so you can specify the character as in this patch and getting rid of the existing circles/invisible options. This just seems cleaner. It would be better to have one configuration option than two. I was simply suggesting that if the user's custom configuration file is using the older options, it isn't bad to continue to support them for backwards compatibility. If people feel that backwards compatibility isn't important, then I'd be agreeable to drop that - especially in the new D-Bus branch.
Brian the only problem with one option instead of two is that you have no way (at least that I can find) of specifying to use the "default" setting, unless you give a special meaning to, say, a codepoint of -1. What do you think of the current patch? I'm sorry if I screwed the indentation up a bit, there really seems to be some contradiction in the file, with a modeline at the top specifying indentation rules that clearly aren't being used!
Brian, the problem with putting this config option in GDM is that GDM isn't the only thing that asks for passwords. If the password characters matters for one password entry, then it should matter for most password entries. Configuring it on an app by app basis is wrong. It should be global setting, or a theme setting. It's what we already have though, I just think it was a mistake to add in the first place.
It depends if the application author wishes a password entry to be high-security or more convenient. I care less about my Facebook password than I do my server password.
It is a good point that it would make sense for there to be one single place where the password style is configured, so it is shared across GDM and gnome-screensaver, etc. That said, I think it is incorrect to assume that all users will be using a screensaver that is aware of this configuration option. What about users who use xscreensaver or other screensavers (e.g. KDE style). However, I think this patch will be put on hold a bit until we sort out the D-Bus GDM branch a bit more. I agree we shouldn't be adding new configuration options to GDM until we understand how they will impact the new D-Bus branch.
Brian, I have a patch waiting on Bugzilla to make the default invisible character a GtkSetting so that it affects GtkEntries everywhere. We currently have no way in GDM to use the default. I consider that pretty broken. Please don't let this fix fester. :(
I'm a bit confused. If the default invisible character is a GtkSetting, then why can't GDM just use the default setting. Then, perhaps, it would make sense to simply deprecate the existing settings and force GDM to use the default? Note that GDM does have the ability to specify a default GtkRC file, so if the setting is stored there, then it would just pick it up, no?
Maybe we could just remove the UseCirclesInEntry option, so that the choice is between default and invisible. People concerned with heightened security can use invisible. Please make a decision and I will code it up.
I would be agreeable to this. Another option would be to use invisible if UseInvisibleInEntry=true, use circles if UseCirclesInEntry is true, and fallback to the default if both are false? Then in the default GDM configuration we can set both to false. What do others think?
This was the exact behaviour up until a few months ago. See Comment #1 for why it was changed!
I thought Comment #1 was complaining that GDM hardcodes the default to asterisk rather than using the actual default, which I'm guessing the user could change.
The behaviour you described in Comment #18 is exactly as I described in the first paragraph. This was then changed to a broken situation. The problem is that the GDMSetup UI makes it look as if "asterisks" are the default, with strings such as: "Show visual feedback (asterisks) in the password entry" (!UseInvisibleInEntry) "Use circles instead of asterisks in the password entry" (UseCirclesInEntry) If we change this to: "Hide visual feedback in the password entry" (UseInvisibleInEntry) ...get rid of the UseCirclesInEntry stuff, and commit the GtkSetting for the default invisible character then this problem is solved. How does that sound for you?
We could change the text in gdmsetup so it is more clear. For example: - Show visible feedback in the password entry - Use circles instead of the default invisible character I'm not opposed to removing circles from the configuration if we want, but if we simply remove it, then users who are using the feature will likely complain. Some users may want GDM to show circles, but not want circles to be their default password entry. But if we think that such users are not many, or not important, then I'm open to the suggestion of breaking interface stability here. Especially if we think it makes things work more clearly.
Let's work on the basis that the invisible character will be configurable by some other means (Setting/Style). I think we should /at least/ drop the "Use circles..." from the GDM Setup UI.
Sounds okay to me. You can provide a patch that implements this for us to discuss. Please update gdmsetup.c and gdmsetup.glade along with your patch. I'm happy to remove setting the circles from the GDM default configuration file and also from gdmsetup. However, it would be nice if the user has UseCirclesInEntry=true in their custom configuration file that it is still respected. Just for backwards compatibility.
Created attachment 95865 [details] [review] Patch All done.
This seems reasonable to me. What do others think? Since Jon was hesitant to approve this sort of change in the D-Bus branch, is this still a problem with the new approach? If Jon doesn't respond to this bug report, could you ping him Alex after some time?
Isn't Jon's branch a complete rewrite using GSettings anyway?
Yes, but I'd still like his feedback. I'm hesitant to change how configuration in GDM is managed without knowing how such changes will affect the new branch. I don't want to add further new configuration settings that will be a pain to port when we go to the new version. Unless we decide that there just will be configuration breakages at that point and we are okay with that. I'd like to know what he thinks about this patch and how it affects his branch.
I've not added any new configuration settings. And if GDM3 is going to be GSettings/dconf based, it couldn't possibly support legacy configurations anyway. Not to mention that he has already said he has no plans to support customization of the password character. I'd say configuration compatibility will definitely be broken!
I understand. I'd still like to hear what Jon thinks about this. I'm assuming this patch is for GDM 2.21 anyway, so there is some time to discuss. No?
OK, sure. :)
Ready to go in yet?
Yes, you can feel free to commit to the 2.20 branch.
Can't, no SVN access.
Okay, done. Committed to 2.20 branch. Thanks for waiting.
I backed out this fix, and am reopening. This patch is only valid for GDM 2.20, and it changes strings. Either you need to get approval from the release team to break string freeze or you need to update the patch so it doesn't break the string freeze. It looks like these two old strings were changed from: 1. Show visual feedback in the password entry. Turning this option on can be a security hazard as the length of your password can be guessed. 2. _Show visual feedback (asterisks) in the password entry To: 1. Hide visual feedback in the password entry. Turning this option on can increase security, as the length of your password cannot be guessed by people looking at your screen. 2. _Hide visual feedback in the password entry Perhaps it might be best to just fix the code to use the old strings, rather than making the translaters do the extra work? It seems the only real difference is you changed the GUI from saying "Show" to "Hide", reversing the state.
Can we just apply to 2.21? Here's my justification for the string changes: I clarified exactly how it can be "guessed" on purpose, as the existing documentation makes it sound far more important than it is. I switched "show" to "hide", because the option more closely reflects the config option. It seems to make more sense to me this way around, as the action of "hiding" the password seems to be a forward action, enforced by the "Turning this on" phrase (indeed, "turning this on" seems like a weird thing to say for an option that is by default already on!) I'd appreciate it if you could fudge the patch around to apply against 2.21, assuming that it doesn't already...
2.21 is a complete rewrite. I think Jon already did some work in the 2.21 release to make managing the password character to do pretty much the same thing that this patch does (respect the GTK default, in other words). You can review the 2.21 code and let us know if there is any work to do there. In other words, if there is work to do in 2.21, it won't be a simple "fudge the patch around" to get it to apply there. I'm happy to apply a patch that doesn't break string freeze (or one that does if someone first gets permission from the release team) to the 2.20 branch if that is desired.
Let's not bother. Onwards and upwards!
Since I needed to get string freeze breakage approval for 2 other issues affecting GDM 2.20, I was able to get approval and get this fix into GDM 2.20. This fix should be in the next GDM 2.20.4 release.