Differential D9875

Extend parsing ssh prompt
ClosedPublic
Actions

Authored by fvogt on Jan 14 2018, 1:18 PM.

Details

Reviewers
pali
lbeltrame
Commits
R105:e19146459b1e: Extend parsing ssh prompt
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.

Diff Detail

Repository
R105 KDE SSH Password Dialog
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
pali created this revision.Jan 14 2018, 1:18 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 14 2018, 1:18 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
pali requested review of this revision.Jan 14 2018, 1:18 PM
pali added a comment.Feb 11 2018, 10:39 PM

Hi! Can somebody review/comment this patch?

fvogt requested changes to this revision.Mar 23 2018, 6:34 PM
fvogt added a subscriber: fvogt.

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."

This revision now requires changes to proceed.Mar 23 2018, 6:34 PM
pali updated this revision to Diff 32615.Apr 19 2018, 10:56 PM

Rebased on top of master, hopefully correctly.

fvogt requested changes to this revision.Apr 20 2018, 6:50 AM

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

This revision now requires changes to proceed.Apr 20 2018, 6:50 AM
pali added a comment.Apr 20 2018, 7:54 AM
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.

fvogt added a comment.Apr 20 2018, 7:59 AM

Use of QStringLiteral and nullptr in some cases, but maybe that's just phabricator doing git diff wrong.

I'll try it out.

fvogt added a comment.Apr 20 2018, 8:12 AM

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.

src/main.cpp
44

I'd prefer an enum instead of two booleans.

pali updated this revision to Diff 32651.Apr 20 2018, 3:45 PM

I changed those two booleans into enum.

fvogt added inline comments.Apr 20 2018, 5:48 PM
src/main.cpp
330

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.

331

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

pali updated this revision to Diff 32670.Apr 20 2018, 6:36 PM
fvogt accepted this revision.Apr 20 2018, 7:05 PM
This revision is now accepted and ready to land.Apr 20 2018, 7:05 PM
pali added a comment.Dec 26 2019, 9:57 AM

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

fvogt added a comment.Dec 26 2019, 10:27 AM
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.

ngraham added a subscriber: ngraham.Dec 26 2019, 3:12 PM
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

fvogt added a comment.Jan 20 2020, 8:05 AM

I'll just land this now...

fvogt commandeered this revision.Jan 20 2020, 8:22 AM
fvogt edited reviewers, added: pali; removed: fvogt.
This revision now requires review to proceed.Jan 20 2020, 8:22 AM
lbeltrame accepted this revision.Jan 20 2020, 8:24 AM
lbeltrame added a subscriber: lbeltrame.

Just because the "accept" flag was cleared.

This revision is now accepted and ready to land.Jan 20 2020, 8:24 AM
fvogt updated this revision to Diff 73912.Jan 20 2020, 8:24 AM

Rebased

Closed by commit R105:e19146459b1e: Extend parsing ssh prompt (authored by pali, committed by fvogt). · Explain WhyJan 20 2020, 8:27 AM
This revision was automatically updated to reflect the committed changes.
mkoller reopened this revision.Jun 13 2020, 8:37 AM
mkoller added a subscriber: mkoller.

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).

This revision is now accepted and ready to land.Jun 13 2020, 8:37 AM
fvogt added a comment.Jun 13 2020, 9:10 AM
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.

mkoller added a comment.Jun 13 2020, 9:37 AM

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

fvogt added a comment.Jun 13 2020, 9:39 AM
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.

Revision Contents
Changeset List

PathPackages
Msrc/main.cpp (354 lines)

Diff 73914

View Options

src/main.cpp

Loading...