This issue is to track the status of the contributed EFL-based pinentry.
Description
Description
Details
Details
Related Objects
Related Objects
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
I changed it back to EFL from Enlightenment. Enlightenment is a desktop/application coded using the EFL. Like Gnome is coded in GTK. This does not require Enlightenment at all. Just the EFL. Which can be used in GTK/Gnome, or KDE/QT based environments just the same. Also Tizen, etc. Thus I would not call it Enlightenment anywhere, as it really has no relation.
Comment Actions
For reference: D426: Initial patch for EFL based pinentry. Thanks for the explanations of the terminology in the E project!
Comment Actions
Sure thing, and it is "semi" animated. If you fail to enter a correct pin/passphrase. The error message is animated, it will slowly move back and forth from left to right. I can provide further screenshots of its various options, confirm, quality, double entry, etc. The quality bar changes color from red to green, with every shade in between based on quality, 0 red, 100 green, in between other colors/shades between the two.
Comment Actions
The underscores on the Cancel and OK button come from Pinentry. Not sure if I am not handling Locale correctly, or something along those lines. The other stuff does not have it, and setting text the same way.
Comment Actions
I have noticed some issues, minor code fixes, and a major issue with failing to fall back to tty/ncurses interface when a GUI is not available. I will make the changes, cleanup the code format, etc and re-attach patch.
Active work can be seen, and diff'ed against master my wltjr/efl branch on a forked repo on github.
Comment Actions
I have updated the code and patch. It is ready for review, modification, and ideally inclusion
Comment Actions
If the dialog's show a bit off centered. The center of the screen being top/left of dialog. Which makes it offset to the bottom right. That is a bug in EFL (T5481) that is fixed and should be in EFL 0.19.1. Not anything related to this code though I did try to address in this code.
Comment Actions
Hi @wltjr, thanks for picking this up. If we want to merge your code, we'll need a DCO from you. If you agree, please send https://dev.gnupg.org/source/gnupg/browse/master/doc/DCO to gnupg-devel@.
Do you want to preserve the history of your work on the efl pinentry, or do you prefer that all patches are folded into one?
Comment Actions
I sent the DCO per request.
As for folded/squashing into one. I figured it would be preferred that way. I can submit as a series of patches or a single patch. Ideally it would be nice to preserve the individual commits. But if that is not of use to pinentry project, history wise. Then I am fine with folding/squashed into one. Which is how I have it now. I am good either way. Thanks!
Comment Actions
"wltjr (William L Thomson Jr)" <noreply@dev.gnupg.org> writes:
What version of the patch or EFL?
While we're at it, both :)
I tried your wltjr/efl branch, with efl from debian experimental, and it
failed with:
../../efl/pinentry-efl.c: In function ‘create_window’:
../../efl/pinentry-efl.c:227:3: error: implicit declaration of function ‘elm_obj_table_padding_set’ [-Werror=implicit-function-declaration]
[...]
$ apt-cache policy libeina1a
libeina1a:
Installed: 1.18.4-1
Candidate: 1.18.4-1
Version table:
*** 1.18.4-1 300
300 http://httpredir.debian.org/debian experimental/main amd64 Packages
100 /var/lib/dpkg/status
$ git describe
pinentry-1.0.0-42-g1be1b9fComment Actions
Very sorry! I already fixed that. I just had not updated the patch. This one is updated
https://github.com/Obsidian-StudiosInc/pinentry/commit/0fb3104c3ab27112aad70668c5828f9d435e10d4.patch
I will update the patch in phab with that one.
Legacy call
elm_obj_table_padding_set -> elm_table_padding_set
I did not want to spam the list with another update... :)
Comment Actions
Ok you should be good to go now. There are 2 issues I am aware of.
- The Ok/Cancel buttons have a underscore for some reason, also maybe using wrong icons. I have a bit more experience with EFL so can address at least the icon aspect. I am not sure where the underscore comes from. Seems to come from pinentry, but GTK and QT do not have that, so I think its something I am doing wrong.
- I think it is an EFL bug, but maybe in my code. The dialog sometimes is not center of the screen. The top left corner of the dialog is centered. On dual displays sometimes its in the middle, but stuck to the side of one display. I am pretty sure these are EFL issues, at least one attempt to address was recently done. Issue centering dialog windows EFL task #T5481
Comment Actions
Forgot EFL version...
This should work with EFL >=1.18.4, but may work with older EFL. I am not really using anything new or specific to one version. I have not tested on anything older than 1.18.4. The window centering may not be addressed till 1.19.2, as it seems to be the same in 1.19.1.
Comment Actions
The underscores are for shortcut keys. If EFL doesn't support shortcuts, they need to be removed.
Comment Actions
Ok, so the patch from the differential works. Could you please address these warnings?
../../efl/pinentry-efl.c:66:1: warning: ‘static’ is not at beginning of declaration [-Wold-style-declaration]
const static int WIDTH = 480;
^~~~~
../../efl/pinentry-efl.c:67:1: warning: ‘static’ is not at beginning of declaration [-Wold-style-declaration]
const static int BUTTON_HEIGHT = 27;
^~~~~
../../efl/pinentry-efl.c:68:1: warning: ‘static’ is not at beginning of declaration [-Wold-style-declaration]
const static int BUTTON_WIDTH = 60;
^~~~~
../../efl/pinentry-efl.c:69:1: warning: ‘static’ is not at beginning of declaration [-Wold-style-declaration]
const static int PADDING = 5;
^~~~~
../../efl/pinentry-efl.c:86:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
quit ()
^~~~
../../efl/pinentry-efl.c: In function ‘create_window’:
../../efl/pinentry-efl.c:216:27: warning: unused parameter ‘ctx’ [-Wunused-parameter]
create_window (pinentry_t ctx)
^~~Also, could you please add a NEWS item?
Also, I noticed that the pinentry makes no effort to grab the keyboard. For me that is an essential feature that prevents the passphrase or part of that from leaking into other windows that pop up and get focused, e.g. chat windows.
Furthermore, if the prompt is very small, toggling the 'show password' checkbox changes the size of the dialaog window, making it look jumpy. To reproduce that, try echo GETPIN | efl/pinentry-efl.
On my machine, the lock on the left looks different from your screenshots, and more like a shopping bag.
Comment Actions
I will see about removing the underscores now that I understand their meaning. I am not sure if EFL has any means to interpret such at this time. I will look into it and address either way. Thank you for that information!
I will address the warnings.
I will add a NEWS item.
I brought up the grab keyboard mouse stuff on list. I know the GTK version does this, but I am pretty sure QT one does not or did not. I used that for sometime. Also did not see it in the QT code but may have missed that. Just checked out the GTK one recently for comparison. No one responded to my post on list. I have code I can add to that, The EFL pinentry Mike Blumenkrantz did had grab code but was missing some other stuff. I can add that logic if needed. I just dislike it because I am usually typing or working in another window/screen. I run scripts that commit sign via git. Which can pop up at times while I am doing other stuff. But I understand the security aspects. Though if someone is able to grab your mouse/keyboard or another thing, likely already exposed. I can always comment it out and/or make it conditional for my needs. If it is a requirement I will add it, simple as that :)
The size of most things other than buttons is controlled by EFL. I will see if I can improve that. EFL at times starts out tiny. In Enlightenment I have to increase scaling of everything by 1.25 for it to be legible/usable. I will have to see if that was an EFL issue or not. Could also be edje as that controls the sizes of stuff. The look and feel/theme. I assume you were running it in GTK or QT desktop. Just curious so I can replicate the issue described.
The lock icon was one Mike chose. I can change to a different icon that maybe more universal in its appearance. Its just what ever the Icon set for the Theme or user selected maps to the icon name being used. I need to see about correcting the ones for the buttons as well. Those should be icons and text, or just icons if available.
Thank you for the feedback. I will address the things mentioned. If I can just get confirmation on the keyboard/mouse grabbing. Thanks!
Comment Actions
"wltjr (William L Thomson Jr)" <noreply@dev.gnupg.org> writes:
wltjr added a comment.
I will see about removing the underscores now that I understand their meaning. I am not sure if EFL has any means to interpret such at this time. I will look into it and address either way. Thank you for that information! I will address the warnings. I will add a NEWS item.
Thanks!
I brought up the grab keyboard mouse stuff on listhttps://lists.gnupg.org/pipermail/gnupg-devel/2017-April/032820.html. I
know the GTK version does this, but I am pretty sure QT one does not or
did not. I used that for sometime. Also did not see it in the QT code
but may have missed that. Just checked out the GTK one recently for
comparison. No one responded to my post on list.
Sorry for not replying.
I have code I can add to that, The EFL pinentry Mike Blumenkrantz did
had grab code but was missing some other stuff. I can add that logic
if needed. I just dislike it because I am usually typing or working in
another window/screen. I run scripts that commit sign via git. Which
can pop up at times while I am doing other stuff.
[...]
I can always comment it out and/or make it conditional for my
needs. If it is a requirement I will add it, simple as that :)
There is a flag to turn grabbing off. No need to comment that out.
But I understand the security aspects. Though if someone is able to
grab your mouse/keyboard or another thing, likely already exposed.
It is not about someone being able to grab my keyboard, it is about my
instant messanger popping up a new chat window while I'm entering my
passphrase, which I do fast and without thinking, hence I won't
necessarily note my passphrase being typed into the chat window before I
hit enter, and *boom* there goes the passphrase.
I consider it an important security feature. I will not use a pinentry
that does not do this.
The size of most things other than buttons is controlled by EFL. I
will see if I can improve that. EFL at times starts out tiny. In
Enlightenment I have to increase scaling of everything by 1.25 for it
to be legible/usable. I will have to see if that was an EFL issue or
not. Could also be edje as that controls the sizes of stuff. The look
and feel/theme. I assume you were running it in GTK or QT
desktop. Just curious so I can replicate the issue described.
Actually, I'm using enligthenment as packaged in Debian experimental
(0.21.5).
The lock icon was one Mike chose. I can change to a different icon
that maybe more universal in its appearance. Its just what ever the
Icon set for the Theme or user selected maps to the icon name being
used. I need to see about correcting the ones for the buttons as
well. Those should be icons and text, or just icons if available.
I was just wondering why my lock looks so different from yours. I can
post screenshots if that helps.
Thanks!
Comment Actions
Ok I can add the keyboard/mouse grab stuff. I have the code already. I get your point, mine is the opposite of yours. I would say don't launch something if your typing in your pin or about to :)
Check the QT version. I am pretty 100% it does not have such code or function. Why it was new to me with GTK, and seeing it in the EFL version Mike did.
Interesting that you are running Enlightenment desktop/window manager. I haven't see the size change. Let me see if I can replicate that. I do not believe I have seen it, so need to replicate to fix,
As for the icon. I am using KDE Oxygen icons, not the standard EFL/Enlightenment ones.
If I change my Icons set, it changes the lock to something else. The shopping bag you are seeing is some system-lock-image that the system is finding. I looked on my system but do not have that image. If you want to do a screenshot you can. Its an icon from some Icon set. FYI all windows in Enlightenment have screenshot option built in, just right click on window title bar.
The icon will change from system to system based on what ever Icon set the user has selected. Default should be the EFL/Enlightenment one.
Comment Actions
No, based on what you just wrote you did not get my point. It is not about launching something when you are about to type in your pin, it is about existing applications, most notoriously instant messangers, that may create popups or grab your focus at any point.
Also, I have been using your pinentry in my development setup. When it pops up, it doesn't even get the focus in my enlightenment setup, so I have to manually focus it before I can even input my pin. I don't think my configuration is too exotic. Focus follows mouse, tiling mode.
Furthermore, the way enlightenment focuses windows is sometimes erratic, I won't use any pinentry that does not globally grab the input for that reason alone. ymmv of course, and key grabbing is apparently not essential.
I noticed some other strange things. If a pinentry with two password fields is popped up, the second field is sometimes populated with stars before I even enter anything. Also, I just got a popup saying "warning: you have entered an insecure passphrase...", and it got two buttons "enter new passphrase", "take this one anyway", but both buttons are too small, and the texts extend beyond the buttons and bleed into each other.
Comment Actions
I got your point, I was saying do not have a chat client or program that would create pop ups and grab focus away. Its a highly debatable and personal preference type of thing. I have run into such already.
I may need to move the focus code to focus on the entry. I tend to do that last, but in pinentry doing it in some conditionals. I may need to add another at the end. Other things maybe grabbing it away. Though the window should be focused. That maybe an EFL issue I think. I will see if there is something I can do to request window focus beyond the field.
Keyboard/mouse grabbing is something I could only do in X11 and not Wayland. Seems none can do it in Wayland as it does not have such abilities. Rather do it for both than one for consistent behavior.
I have not seen that issue with two password fields.
The text overflowing buttons is because I have them at a fixed size. EFL is different in that the buttons will be varying size otherwise. I will see if I can address so they can resize will still having padding, and being the same size.
Thanks for the feedback I will address the issues and provide an update!
Comment Actions
@justus Can you tell me how you got the two passwords with extra text and the long button text? I can replicate the long button text via cli. Not sure about the two passwords and extra unwanted characters. I would like to be able to replicate as you did. Thank you!
Comment Actions
So are you also saying that I should better not use e17 because its focus handling is so fubar that it does not focus the pinentry when it pops up? FTR, the gtk pinentry handles this just fine by grabbing the focus.
gpg --quick-generate-key foo@bad.example.org
yields
and the long button text?
Enter a short passphrase like 'foo', I get
I got this from light use of the pinentry while developing GnuPG.
Comment Actions
That maybe the case, which is clearly a bug. I am just not sure if its a bug in Pinentry, EFL/Elementary, or the theme/EDJE. I have seen other issues, and I recall seeing others talking about it being issues for other stuff. I just have not confirmed where the focus issue resides. I just know it has focusing issues. Not just pinentry but some other stuff as well. I another app I have some issues with focus as well.
The GTK is grabbing keyboard and mouse, but yes GTK nor QT versions have focus issues. This is a EFL/Elementary/EDJE issue or possibly my pinentry code. Though I have ran into focus issues in another application.
In T2905#98415, @justus wrote:
gpg --quick-generate-key foo@bad.example.org
Thanks!
I could replicate the long button text by setting the button text to other stuff via enter pinentry shell session. Your way is much faster and I can test out both issues. Thanks!
I will get on this and provide an updated patch.
Comment Actions
I updated the patch, fixed all issues mentioned and a couple others I noticed. Things not being centered vertically labels/entries, and ok not being fired on pressing enter on entry, or confirm when present. That should fix all outstanding issues.
I have an attempt at fixing the focus issue. Calling focus last. Seems I was copying others code ot much or something. I had focus being set when a confirm field was present....
The unwanted chars in confirm was the label text.... I was putting the label text into the entry instead of having a label before the field. I corrected that with a label before the field.
For a direct link, same as updated patch in phab. Link to current patch
Make sure your using the latest version. The icon in your last screenshot still shows lock/shopping bag, but I switched to a key icon in the previous patch. Its moot as this replaces that one. Just make sure via that visual icon that your using the right one :)
Comment Actions
../../efl/pinentry-efl.c: In function ‘create_window’:
../../efl/pinentry-efl.c:493:7: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
int ok_len = ELM_SCALE_SIZE(strlen(txt) * (PADDING * 1.5));
^~~
Comment Actions
The password quality bar is not working. There are spaces missing in the texts.
The password quality algorithm is a joke and is probably more dangerous than helpful. (Try entering the password 12345678...) AIUI, it was added because a client had a specific requirement. I'd prefer that we either fix the algorithm (complicated and depends on the user's threat model) or we deprecate the quality bar.
Comment Actions
I will fix that warning, I should have caught that, I do no think I am using that compiler flag/option.
As for the spaces. It seems like maybe its a line return or something not being interpreted correctly. If they are actually spaces. Nothing in the code would strip such out. I will have to look further into that one, but seems like its the text containing a line return or something else stripping out the space.
With regard to quality algorithm, I assume I do not need to do anything there? I can adjust the math for the percentage aspect. But that is based on what I get back from pinentry so if that is off, it maybe what is effecting the quality of the quality bar :)
Seems like we are getting close now! Busy today but will address these issues tomorrow and confirm one way or another on the spaces issues. Thanks!
Comment Actions
How is that icon by the way? Like the key better than the lock/shopping bag? :) The icons will change based on the users selected icon set.
Comment Actions
No, you should not adjust what you are getting. My point is only that the password quality bar may not only be useless, it may, in fact, be dangerous.
Comment Actions
So currently the efl bar always displays 0%. It must either be fixed (i.e. display the same as the gtk one), or be removed. I don't care either way, but it cannot be broken like it is now.
Comment Actions
In my tests it worked, you just have to type a decent amount to get it to kick in. It seems to accelerate really quick, like it jumps from 0 to 60% and then 100%, not really smooth from 0 - 100, as intended. But I think that is due the quality value returned from Pinentry.
I can remove just the same.
Comment Actions
No, the quality bar is required for pinentries which want to get in wide use. Even if the quality algorithm seems to be too trivial, it is required for external checks like gpg-check-pattern.
Comment Actions
Note that it depends on a gpg-agent configure option. Thus Neal and you may see a different thing.
Comment Actions
The quality bar should be working, please try typing in more characters till it does something. It should at some point.
Comment Actions
The gtk pinentry displays 10% improvements per character, and if one character is a digit, then at 80% the bar turns green.
The efl pinentry displays nothing until i entered 8 characters (of which one must be a digit), the it jumps to 80% and turns green, and increses up to 100%. If I don't enter a digit, the bar never shows anything but 0% no matter how many characters I enter.
I don't know if the algorithm is clever or dump, that is clearly a bug.
Comment Actions
gpg-agent waits for a QUALITY inquiry via Assuan from the pjnentry and replies with an integer giving the percent value. A negative percent value requests "red" indication. The argument for the quality inquirement is the passphrase as already entered. The idea is that this inquiry is send after each keystroke.
Here is what the gtk pinentry does:
percent = length? pinentry_inq_quality (pinentry, s, length) : 0;
if (!length)
{
strcpy(textbuf, QUALITYBAR_EMPTY_TEXT);
color.red = 0xffff;
}
else if (percent < 0)
{
snprintf (textbuf, sizeof textbuf, "(%d%%)", -percent);
textbuf[sizeof textbuf -1] = 0;
color.red = 0xffff;
percent = -percent;
}
else
{
snprintf (textbuf, sizeof textbuf, "%d%%", percent);
textbuf[sizeof textbuf -1] = 0;
color.green = 0xffff;
}
gtk_progress_bar_set_fraction (GTK_PROGRESS_BAR (qualitybar),
(double)percent/100.0);
gtk_progress_bar_set_text (GTK_PROGRESS_BAR (qualitybar), textbuf);Comment Actions
Why I was saying maybe my math is off or something. I am doing basically the same. Should be the same code. I calculate the percent exactly as they do for GTK. I also set the value the same. Maybe something I am not doing correctly in EFL.
length = strlen (s);
percent = length? pinentry_inq_quality (pinentry, s, length) : 0;
evas_object_color_set(qualitybar,
255 - ( 2.55 * percent ),
2.55 * percent, 0, 255);
elm_progressbar_value_set (qualitybar, (double) percent / 100.0);Comment Actions
I just tested this out. It seems to be based on what you enter and what is returned from Assuan/Pinentry. If I enter, 2 spaces, then a 1, and repeat that pattern. By the 6th space, you get 20%, and from there it increments by 10% or so to 100% as you continue to enter space space 1,
space space 1 space space 1 space space = 20%
space space 1 space space 1 space space 1 = 30%
space space 1 space space 1 space space 1 space space 1 = 40%
space space 1 space space 1 space space 1 space space 1 space space 1 = 50%
.....
Try entering in that, and you should get the exact values above. I can type in a full sentence 0%, but soon as I hit a single number, it jumps to 80%.
Comment Actions
Here is the code:
int goodlength = opt.min_passphrase_len + opt.min_passphrase_len/3;
int length;
const char *s;
if (goodlength < 1)
return 0;
for (length = 0, s = pw; *s; s++)
if (!spacep (s))
length ++;
if (length > goodlength)
return 100;
return ((length*10) / goodlength)*10;So with GOODLENGTH being 10 by default you get 10% for each non-space. But something must be wrong. I see that even with the gtk pinentry.
Comment Actions
ok so I just need to fix the compiler warning and we should be good to go. Was there anything else I needed to address?
Comment Actions
I have updated the patch in D426, direct link to it on Github, to address the compiler warning from comment T2905#98802 .
I think that takes care of the last issue. If there is anything else let me know. Thanks!
Comment Actions
No I wont. I'm constantly testing your code. Please read my feedback. I'm growing a bit impatient with you, because I feel like you are developing a piece of software that you are not using, because as soon as I test it I instantly find problems with it.
This is even more worryingly because you actually have multiple pinentries to compare with. The gtk version clearly behaves very differently wrt the quality bar. You need to fix this.
Please re-read my feedback. For example, if you enter "1234567", the gtk one says in red 70%, whereas yours says 0%.
Comment Actions
Completely uncalled for, did you even read T2905#99021? It clearly shows the problem is NOT in my code. As it is clearly stated that the GTK pinentry is clearly effected as well.... Maybe go try what you are with GTK like werner did and you will see the same....
Comment Actions
I am comparing your work with the gtk pinentry as shipped by Debian. Maybe Debian is shipping a patched pinentry, I don't know, and frankly I don't care.
Comment Actions
FYI I use this daily as I have been since my first submission. Gentoo ebuild I made which uses the patch. Why would I make that if I am not using? Every commit I make is GPG signed.
Comment Actions
You do not care about comments in T2905#99021? So werner is completely incorrect there? He stated something is wrong and GTK is effected as well...
Comment Actions
Wait, you said Debian maybe patching YOUR code to fix an issue? Maybe get the patch and apply to pinentry and correct the issue werner found in pinentry. Rather than going off on me for something I have zero control over.
Comment Actions
Ok I just tested this out and BOTH GTK and QT are messed up. Maybe the others as well. I have to check FLTK and not sure about ncurcses/tty. I simply typed 10 characters and I got 100%. It did not matter what those 10 characters were at all. Like 10 of the same character. That is not correct!!!! That is not saying what I typed was of any quality. This functionality is completely jacked in all. No wonder my version is having issue. Seems others hacked around this broken function and eliminated the entire purposes of qualifying an entry.....
Pinentry needs to be fixed!!!!!
Comment Actions
Even with that being said I see no difference here
gtk_progress_bar_set_fraction (GTK_PROGRESS_BAR (qualitybar), (double)percent/100.0); elm_progressbar_value_set (qualitybar, (double) percent / 100.0);
I am not seeing anything that would make the percent for GTK be any different than percent for EFL.
The GTK code is basically the same as my EFL code.
Neither change the percent value. GTK does only if it is below zero. Which seems like a hack, make a negative value positive?
else if (percent < 0)
{
...
percent = -percent;
}
...Maybe that where the difference comes from. I am not making that value positive. Seems based on werners comments about 10% per char would go inline with that. If percent is returning a negative value, and they take that and flip it to be positive. But that is not correct. It is not qualifying the quality of the entry.
Comment Actions
Yes. This is not a hack. Please read Werners message. And this exactly explains the behavior that I have been seeing. It is a bug in your code, and it needs to be fixed. No need to make such a fuss.
Comment Actions
FLTK
GTK-2
This seems so wrong... entering 1's and a's This would fail a lot of sites that require minimum stuff on passwords like upper/lower, number, special character, etc. This makes NO sense for a quality meter to say junk is quality. Think others have hacked around this. It is not correct.
QT
FLTK
GTK-2
I can make EFL be the same as the others. But seems to be broken across the board. Basically you want 10% per character. Which is not validating or qualifying the quality of the password. Rather than a quality progress bar, should say length or characters. Quality implies what you enter must meet some criteria.
Given this is for GNUPG saying that 10 of any single character is 100% quality. That seems like a major bug and issue to address. Which others would need to correct as well. Potentially making EFL the only correct one.
Also noticed FLTK has those accelerators like EFL used to have but I fixed. Interesting how FLTK made it in with such remaining.
Comment Actions
I have also noticed that there is a line return after "to" before protect. Which explains why those words run together on the EFL version. I will have to see about replacing the new line characters with something that works for EFL. It does not support new line characters in labels.
Comment Actions
Just confirmed it is this that causes it to move 10% per char, and is wrong IMHO
if(percent<0)
percent = -percent;But since you wall want this broken behavior consistent I will add that along with my fixes for new line characters. Which should be the last of the issues.
Comment Actions
Either the quality algorithm needs to be fixed, or this deprecated all around. I have a hard time committing the above knowing it is wrong.
Comment Actions
T2103 is the right place to discuss the password quality algorithm, not here.
If you want to not implement the password quality indicator for the EFL pinentry (which seems like a reasonable choice to me) then it would be in line with pinentry-curses, which also doesn't implement it, afaict.
Comment Actions
Sure but that issue is seeming to cause issues for this contribution.
If you want to not implement the password quality indicator for the EFL pinentry (which seems like a reasonable choice to me) then it would be in line with pinentry-curses, which also doesn't implement it, afaict.
It is currently implemented and I do not have a problem leaving it as is. The question then becomes a matter of its functionality. Do I leave as is, for when the password quality algorithm is fixed? Or should I make it like the others?
It is really up to you all. I am just trying to get this accepted. If you want the EFL version without I can remove. If it is to remain. I would prefer to leave as is, and see that the password algorithm is fixed. Which means others will need to update GTK, Gnome, FLTK, and Qt interfaces. If I modify EFL to be like the others. That is one more to fix, in addition to the others. I think its best to leave as is.
Though I do have an update for handling new line characters. Which beyond the progress bar/quality issue is the last remaining issue.
Comment Actions
There are two separate issues here:
- How password quality is generated.
- Whether the EFL pinentry correctly displays the returned quality metric (where correct might mean not showing it at all)
T2103 is the correct place to discuss the first issue. If we change how the password quality is calculated, then we'll change it in gpg-agent. Once that happens, all pinentries that show the quality will automatically use the new algorithm without any change to the code.
Thus, if the EFL pinentry shows the returned quality metric, then it should do it in the same way as pinentry-gtk, etc. Unless of course pinentry-gtk, for example, has a bug. In that case, pinentry-gtk should be fixed.
I would prefer to leave as is, and see that the password algorithm is fixed. Which means others will need to update GTK, Gnome, FLTK, and Qt interfaces. If I modify EFL to be like the others. That is one more to fix, in addition to the others. I think its best to leave as is.
Unfortunately, I don't understand the issue that you are raising. It sounds to me like you are claiming that pinentry-gtk, pinentry-qt, pinentry-fltk all have a bug and you don't want to copy that bug. If that's the case, please open a new issue and describe the bug. Unfortunately, it is not clear to me what the bug is, but perhaps I just missed its description in the wall of text. If so, apologies.
Comment Actions
Let me clarify, what all are doing now to make the progress bar move is the following
if(percent<0)
percent = -percent;That inverts the value if below zero a negative with 2 negatives become positive. That ends up moving it 10% per 1 character entered. That is the code mine does not have. I have tested it with that code and it functions like all others.
Once the password algorithm is corrected. That if condition should not be hit long as the returned value is greater than 0. The value, in theory will always be above 0. HOWEVER, if it is below zero, legitimately from what ever is checking the "quality" of the passphrase. Then this will be a bug in all. Since it will revert to old behavior, every wrong character entered that causes a negative value to be returned, will hit that condition and move by 10% per 1 character.
That means at best the code becomes useless code for a condition never hit. But at worst it cause the same behavior as seen now even with a new algorithm, a bug. If something returns negative values for bad entries.
Thus I rather leave out code that would become obsolete/useless, or potentially cause the same behavior with a corrected algorithm. Which will happen now for all the other interfaces.
Comment Actions
Said another way, the only thing that should make the progress bar move in any is the "quality" value. The use of percent for the value is the hack. Because the quality value cannot be used. They grab the percent value and increment based on number of characters.
This logic should not be needed.
length = strlen (s); percent = length? pinentry_inq_quality (pinentry, s, length) : 0;
That should just be like
length = strlen (s); quality = pinentry_inq_quality (pinentry, s, length);
Or something along those lines. Then the value can still be multiplied by 100 if needed. It really depends on the value returned from pinentry_inq_quality.
Comment Actions
Sorry, you've misunderstood the point of the minus sign. It is not saying that the quality is bad, but it is simply a boolean value encoded using the sign bit. The boolean value means: the red indication should be shown. Please review https://dev.gnupg.org/T2905#99007 . Thanks.
Comment Actions
I am pretty sure I understand it clearly. If I add those two lines it makes the EFL version function like the others. Without it does not. I just debugged this on first character entered, pinentry_inq_quality returns -10. Which again negating -10 becomes 10, and thus the first character gets you 10%, and continues from there.
Comment Actions
Looking further into this, pinentry_inq_quality can return a value in the range of -100 to 100. Thus getting -10 from pinentry_inq_quality seems quite normal. Which explains why each are doing <0, since the value can be less than zero, negative quality.
Docs even say so
"Returns: A value between -100 and 100 to give an estimate of the passphrase's quality. Negative values are use if the caller won't even accept that passphrase."
Saying the caller will not accept, should mean that the progress bar does not move with such value. If that is the case, then all are broken, as they move with a negated value of -10, by 10% per 1 char.
Comment Actions
With all that said, if someone could let me know how you want me to proceed, 2 options.
- I add the 2 lines to make EFL function like others, 1 char = 10%
- Omit the 2 lines, and leave it as is, requiring fix of pinentry_inq_quality, and underlying passphrase/password quality algorithm
I am fine either way. I prefer #2, as #1 will introduce a bug when the algorithm is fixed. Though all of the interfaces will need to be fixed. Not sure if that is a factor or not.
I have some other fixes for new lines in text that was not handled properly. Beyond that and the issue about the progress bar asked above. That should be all of the remaining issues, and can look to have this included.
Thank you for everyone's time, effort testing, and comments on this task/bug. Sorry for the noise!
Comment Actions
Yes.
- Omit the 2 lines, and leave it as is, requiring fix of pinentry_inq_quality, and underlying passphrase/password quality algorithm
No.
I am fine either way. I prefer #2, as #1 will introduce a bug when the algorithm is fixed. Though all of the interfaces will need to be fixed. Not sure if that is a factor or not.
There is nothing to fix in the way the underlying algorithm communicates its value to the frontend. Negative values mean red, positive values green. After that, you have to normalize that to 0...100. Werner has pointed that out, Neal has, and so have I. If we have somehow failed to communicate that to you, then I am sorry, but I honestly do not know how to do that better.
Thanks for your efforts.
Comment Actions
That is not what the document say. That is simply how 1 interface was made, and all others have carried on that broken functionality. Not to mention they are crude going from just red to green, when there are colors in between. Should a 50% quality be green or less?
Werner has pointed that out, Neal has, and so have I. If we have somehow failed to communicate that to you, then I am sorry, but I honestly do not know how to do that better.
Given what I have encountered, what the docs say, how the other interfaces function, and the unwillingness to acknowledge. It has made me think differently of GnuGP and the quality of the software all around. I will be negating this function for the Gentoo ebuild. I have also commented the code to reflect the hack. Which is very much is one and is not proper. But seems GnuPG does not care that the quality progress bar has any real meaning.
That said, I have updated the patch, direct link to github.
That should be the last of any issues.
Comment Actions
Not sure this should remain open. Months later a release was done excluding this. Originally mentioned on list in October 2016. Over a year later still not included. Very discouraging. I guess I can just see about having this external for myself. Shocking that FLTK and QTK see more usage than EFL which is part of Tizen OS. Clearly issues with either me, or EFL. Some reason it was excluded and being ignored. Seems nothing I can do either way. Oh well, I did all I could for months. On a very small contribution...
Comment Actions
Moving on, I will just look to make a stand along project for efl-pinentry interface. I withdraw my previous submission. Welcome to resume and move forward with Mike Blumenkrantz version. Thanks!
Comment Actions
Proceeding with a fork, and likely will remove other interfaces and just maintain another version of pinentry for EFL. Maybe renamed to pinentry-efl, and only have that and tty and curses interfaces in addition to EFL.
Comment Actions
Hello @wltjr,
I have not followed this bug for the last 6 months and meanwhile @justus and @neal moved on to the pEp company and are not any longer available to work on this. Although, I made the last pinentry release I do no closely follow the development. What I noticed is that we still don't have an EFL based pinentry despite that I explained them several times that I would like to see EFL in pinentry proper. I can't remember what the Mike Blumenkrantz version is or that there have been two pending versions at all. The thread is pretty long and I have note read it in its full length.
AFAICS, the problem is about the quality bar and that I once claimed that a pinentry needs to have such a quality bar. In some recent offline discussions we agreed that not showing the quality bar by default would be better than having a bad one. Thus @gouttegd posted two patches to gnupg-devel to do just this and enable the quality bar only if certain other options are enabled. This new policy should make a pinentry without a quality bar acceptable.
Would you be so nice to explain (again) the other open things for the EFL pinentry and on how your EFL version differs from Mike's version?
Comment Actions
Oh yes, I should re-open this because we should keep on tracking the status - either for an included EFL version or an external version.
Comment Actions
@werner no clue, I thought it was merged in at some point. I could have sworn something happened there. I went on advising others like the TQT interface assuming EFL was already added. I was shocked it was not when release came out and no explanation as to why it was excluded.
There are 4 differences between Mike Blumenkrantz version and mine. I have his original in a branch in my fork. I made a minor change but other wise that is his version untouched. I will list the differences in order of severity/interest.
- Keyboard/Mouse Grab
- Mikes has it, mine does not as Wayland has no support for such making it X only requiring ifdef, etc.
- libsecret support
- Mikes has it mind does not, I assumed it was related to GTK/Gnome Keyring it is not I can add that if needed
- Box vs Table
- Mike's uses boxes mine uses a table with fixed sizes, IMHO bit better UI look and feel, think different icon and text for buttons
- Aminations
- Mine has animated error text slides back and forth, and quality progress bar changes color from red to green with all shades in between, Mikes is not animated, and progress is regular red to green no shades.
Not sure about support, I assume Mike would be readily available to address any issues. Though he is pretty busy with core EFL/Enlightement stuff. I maybe more accessible to address any issues. That is pretty moot but maybe a factor as well. I have been using mine daily for months. I haven't ever used Mikes version but I assume it is fine as well.
That covers the main differences. Mostly 1 and 2, and 3 is some what in looks. 4 is moot, but point of EFL vs others IMHO is for animations and its tastefully animated. Not fast so should not effect those with disabilities.
The first one is a bit contentious. I went with EFL developers preference that there is no X/Wayland specific code in EFL projects/applications. EFL handles are we running on X or Wayland. Wayland approach is to let the Window Manger handle things like Keyboard/Mouse grab for particular dialogs etc. I can add back the X specific code for Keyboard/Mouse grabbing. That would be a difference in say Pinentry dev needs/preference vs EFL devs. Though maybe moot since I think pinentry itself default was switched from grab to no-grab. That is likely the biggest difference and is more a Wayland issue. That effects all GTK, QT, etc. Any that have grab will not work under Wayland. Thus I opted for consistency under either, at the expense of grabbing under X.
Comment Actions
@werner no problem with re-opening. I closed as it seemed it was not of interest or wanted. I wasn't get any responses like asking why it was left out of 1.1.0 release. To my knowledge other than preferences of GnuPG devs, changes to suit your needs, grabbing, libsecret, etc. It should be good to go without any issues. Thus I was waiting next release, assuming it was already committed . May have confused it with some other PR that was committed. But there should not be any outstanding issues preventing it from inclusion. If there are it was never relayed to me. It should be ready for inclusion, less any requested changes.
Comment Actions
Thanks for the long explanation. I think it should go into pinentry proper. I will have a closer look on it.
Comment Actions
Your welcome, I can remake another unified patch if need be. I was starting to prepare things to be a stand alone fork. Did an initial .travis.yml file, and initial stuff for Coverity. Though never did get a build uploaded to Coverity. Not sure if you have ever run pinentry through Coverity or other GnuPG stuff, may be a good idea just to see if it catches anything.
If you would ilke a PR or other for the main repo for travis etc I can help work on that. I assume you have an internal CI setup. Though can't hurt to have travis as well running on public repo, and ideally coverity as well.
Either way just let me know what you need and I will provide what I can ASAP. Thanks!
Comment Actions
After fighting with Coverity over a fork of pinentry that has EFL. I setup to have Coverity scan. Which found some like 22 defects. Coverity unable to identify that I have any affiliation, after I spent/wasted hours getting a build to upload to Coverity to scan. Just to fight with some unhelpful person basically standing in the way of FOSS project, a wonderful Mel Llaguno. Decided for security reasons I be denied ability to use Coverity to scan pinentry for defects, even in the EFL interface I made and am the author of. Which also means I cannot fix other issues with pinentry or aide further in development....
Really quite detested at Coverity and how they are just causing problems here where they need not be. I highly recommend NEVER using Coverity. Do not waste time with their stupid scan approval process. I recommend using others ilke SonarCloud which I will be migrating my own Coverity stuff to SonarCloud. Having just wasted many hours on this, due to Coverity's own build failing and not notifying Travis of the build failure. Not to mention how it double builds, once for Travis and then again to upload to Coverity.
I was considering helping further with the project and joining officially. Thanks to Coverity, I will not and likely just continue with my own fork and remove other interfaces etc. I haven't time to waste like this all around. It is a small contribution. The code base clearly has other issues not addressed. I was trying to be helpful working for free. Thanks to Coverity again for causing problems where there need not be any.
Once again personal thanks to Mel Llaguno <Mel.Llaguno@synopsys.com>.... seriously rude person.... Wastes developers time trying to work on free stuff.... Not like I am paid for any of this. Least they could do is be helpful....
Pinentry has defects some maybe serious that need fixing. You can spend your time to integrate with Coverity and fix such issues. I have wasted enough time on this. Sorry Werner, I know your down a few people. I was going to step up, but plenty of other things to work on I need more and can have more control over and not waste time.
Comment Actions
Okay. Thanks for the report. I once looked at Coverty but decided not to use it because of their rules which would not allow me to document and fix a possible security vulnerability without following their process. If there is a security problem I will fix it according to my schedule and not allow anyone to delay it.
Comment Actions
No clue what their problem is, I have a few projects scanned by Coverity. Most are forks that I took over, but one is not really. Not sure why they took such issues here.
I moved onto Sonar Cloud. You maybe interested in the results there. I did not see any major issues. Though a few bugs to maybe address and re-code some things. Not sure if you interested in any of the Travis stuff. I have it setup so with a few more modifications it can build all the various options. I was mostly concerned with EFL, but can test GTK, Gnome, etc. I addressed most the issues Sonar pointed out with the EFL interface. I can address some of the others as well. May look to use other analysis as well. Now that Coverity has me looking at others.
Rather interesting another project of mine asspr, has no issues on Coverity, but Sonar identified several. Though I think Coverity found some leaks for Pinentry I did not see offhand under Sonar.
Sonar seems nice, has multi-user access, nice unified dashboard. No scan approval or other processes of Coverity. Seems pretty useful. I was mostly concerned with the EFL interface issues/defects, but could not hurt to find others just he same. Only good intentions!