D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt closed this revision.
fvogt added a comment.


  FTR: https://invent.kde.org/plasma/ksshaskpass/-/merge_requests/2

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: fvogt, pali, lbeltrame
Cc: mkoller, lbeltrame, ngraham, fvogt, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added a comment.


  In D9875#675107 , @mkoller wrote:
  
  > They did not overwrite each other since the entry for the Username holds an 
URL _without_ user but the password entry did.
  
  
  That's pretty fragile, but would need to be kept for compatibility indeed.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: fvogt, pali, lbeltrame
Cc: mkoller, lbeltrame, ngraham, fvogt, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D9875: Extend parsing ssh prompt

[Martin Koller]

mkoller added a comment.


  They did not overwrite each other since the entry for the Username holds an 
URL _without_ user but the password entry did.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: fvogt, pali, lbeltrame
Cc: mkoller, lbeltrame, ngraham, fvogt, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added a comment.


  In D9875#675101 , @mkoller wrote:
  
  > This patch breaks usage for git (and probably others):
  >  git first asks for a "Username for 'https:" which leads to ksshaskpass 
open the input dialog but the typed-in user
  >  is no longer stored into the wallet!!
  >  (See case TypeClearText)
  >  This leads to git again and again ask for the Username on each invokation.
  >
  > Please ensure that even the Username is stored again _in the same folder_ 
in kwallet as before (e.g. below Passwords)
  >  otherwise it also breaks using existing passwords. E.g. reading
  >
  >   if (type != TypePassword) {
  >  QByteArray retrievedBytes;
  >  wallet->readEntry(identifier, retrievedBytes);
  >
  > is wrong (backwards incompatible).
  
  
  Can you make a patch?
  
  Having a quick look at the previous code, it seems like username and password 
shared the identifier, so they overwrote each other.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: fvogt, pali, lbeltrame
Cc: mkoller, lbeltrame, ngraham, fvogt, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D9875: Extend parsing ssh prompt

[Martin Koller]

mkoller reopened this revision.
mkoller added a comment.
This revision is now accepted and ready to land.


  This patch breaks usage for git (and probably others):
  git first asks for a "Username for 'https:" which leads to ksshaskpass 
open the input dialog but the typed-in user
  is no longer stored into the wallet!!
  (See case TypeClearText)
  This leads to git again and again ask for the Username on each invokation.
  
  Please ensure that even the Username is stored again _in the same folder_ in 
kwallet as before (e.g. below Passwords)
  otherwise it also breaks using existing passwords. E.g. reading 
   if (type != TypePassword) {
  
QByteArray retrievedBytes;
wallet->readEntry(identifier, retrievedBytes);
  
  is wrong (backwards incompatible).

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: fvogt, pali, lbeltrame
Cc: mkoller, lbeltrame, ngraham, fvogt, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

This revision was automatically updated to reflect the committed changes.
Closed by commit R105:e19146459b1e: Extend parsing ssh prompt (authored by 
pali, committed by fvogt).

REPOSITORY
  R105 KDE SSH Password Dialog

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9875?vs=73912=73914

REVISION DETAIL
  https://phabricator.kde.org/D9875

AFFECTED FILES
  src/main.cpp

To: fvogt, pali, lbeltrame
Cc: lbeltrame, ngraham, fvogt, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt updated this revision to Diff 73912.
fvogt added a comment.


  Rebased

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9875?vs=32670=73912

REVISION DETAIL
  https://phabricator.kde.org/D9875

AFFECTED FILES
  src/main.cpp

To: fvogt, pali, lbeltrame
Cc: lbeltrame, ngraham, fvogt, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D9875: Extend parsing ssh prompt

[Luca Beltrame]

lbeltrame accepted this revision.
lbeltrame added a comment.
This revision is now accepted and ready to land.


  Just because the "accept" flag was cleared.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: fvogt, pali, lbeltrame
Cc: lbeltrame, ngraham, fvogt, plasma-devel, Orage, LeGast00n, 
The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 
alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, 
apol, ahiemstra, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added a comment.


  I'll just land this now...

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: ngraham, fvogt, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added a comment.


  Ping.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: ngraham, fvogt, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D9875: Extend parsing ssh prompt

[Nathaniel Graham]

ngraham added a comment.


  In D9875#583139 , @fvogt wrote:
  
  > In D9875#583106 , @pali wrote:
  >
  > > Is there anything else or something which blocks merging this change into 
upstream git repository?
  >
  >
  > Do you have a contributor account? If not, I can push it.
  
  
  I Recently learned that you can find out this information for yourself by 
searching through https://websvn.kde.org/trunk/kde-common/accounts?view=markup
  
  It looks like @pali does indeed have a contributor account. So @pali, feel 
free to land the patch yourself using `arc land`. For more details, see 
https://community.kde.org/Infrastructure./Phabricator#Step_3:_Land_your_diff

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: ngraham, fvogt, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added a comment.


  In D9875#583106 , @pali wrote:
  
  > Is there anything else or something which blocks merging this change into 
upstream git repository?
  
  
  Do you have a contributor account? If not, I can push it.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: fvogt, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D9875: Extend parsing ssh prompt

[Pali Rohár]

pali added a comment.


  Is there anything else or something which blocks merging this change into 
upstream git repository?

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: fvogt, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D9875: Extend parsing ssh prompt

[Pali Rohár]

pali updated this revision to Diff 32670.

REPOSITORY
  R105 KDE SSH Password Dialog

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9875?vs=32651=32670

REVISION DETAIL
  https://phabricator.kde.org/D9875

AFFECTED FILES
  src/main.cpp

To: pali, fvogt
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added inline comments.

INLINE COMMENTS

> main.cpp:330
> +// Item could not be retrieved from wallet. Open dialog
> +if (item.isEmpty()) {
> +if (type == TypeConfirm) {

Can you replace this if with

  if (!item.isEmpty()) {
  QTextStream(stdout) << item;
  return 0;
  }

That way there's one layer of indentation less on the following code.

> main.cpp:331
> +if (item.isEmpty()) {
> +if (type == TypeConfirm) {
> +if (KMessageBox::questionYesNo(0, dialog, i18n("Ksshaskpass")) 
> != KMessageBox::Yes) {

Please use `switch(type) { case TypeConfirm: { ...} ... }` instead of an 
if..else if..else chain.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Pali Rohár]

pali updated this revision to Diff 32651.
pali added a comment.


  I changed those two booleans into enum.

REPOSITORY
  R105 KDE SSH Password Dialog

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9875?vs=32615=32651

REVISION DETAIL
  https://phabricator.kde.org/D9875

AFFECTED FILES
  src/main.cpp

To: pali, fvogt
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added a comment.


  I gave it a quick test - works fine with ssh(-add). I didn't test anything 
else though.
  
  Code-wise it looks ok (as ok as the old code...), just a remark about the 
reference parameters.

INLINE COMMENTS

> main.cpp:44
>  // has no i18n, so this should work for all languages as long as the string 
> is unchanged.
> -static void parsePrompt(const QString , QString& keyFile, bool& 
> wrongPassphrase)
> +static void parsePrompt(const QString , QString& identifier, bool& 
> ignoreWallet, bool& clearText, bool& confirm)
>  {

I'd prefer an enum instead of two booleans.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt added a comment.


  Use of `QStringLiteral` and `nullptr` in some cases, but maybe that's just 
phabricator doing git diff wrong.
  
  I'll try it out.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Pali Rohár]

pali added a comment.


  In D9875#250214 , @fvogt wrote:
  
  > too many unrelated changes in the diff
  
  
  Which are unrelated? I think all of them are needed. parsePrompt() was needed 
to be rewritten for support plain text inputs and yes/no questions.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  Rebase looks ok - but now there are too many unrelated changes in the diff. 
Can you split them?

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Pali Rohár]

pali updated this revision to Diff 32615.
pali added a comment.


  Rebased on top of master, hopefully correctly.

REPOSITORY
  R105 KDE SSH Password Dialog

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D9875?vs=25313=32615

REVISION DETAIL
  https://phabricator.kde.org/D9875

AFFECTED FILES
  src/main.cpp

To: pali, fvogt
Cc: fvogt, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Fabian Vogt]

fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  Sorry that you had to wait so long - it seems there's no active maintainer 
for this.
  
  I just wanted to give this a try. Unfortunately the patch doesn't apply to 
current master anymore. Can you rebase?
  
  Phab says the same, "Context not available."

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali, fvogt
Cc: fvogt, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Pali Rohár]

pali added a comment.


  Hi! Can somebody review/comment this patch?

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

To: pali
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart

D9875: Extend parsing ssh prompt

[Pali Rohár]

pali created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.
pali requested review of this revision.

REVISION SUMMARY
  - Add support for yes/no questions from ssh-agent and openssh client
  - Add support for more openssh client prompt lines
  - Better support for parsing git credential strings
  
  I wrote this patch 5 years ago and sent it to Armin Berres, current
  maintainer in that year. Seems it was never applied. Now I ported it for
  the last ksshaskpass version.

REPOSITORY
  R105 KDE SSH Password Dialog

REVISION DETAIL
  https://phabricator.kde.org/D9875

AFFECTED FILES
  src/main.cpp

To: pali
Cc: plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas, apol, mart