Support multiple authentication methods #825

fmeum · 2020-06-01T17:36:59Z

📢 Type of change

Bugfix
New feature
Enhancement
Refactoring

📜 Description

Add support for multiple authentication methods by offering password authentication after public key authentication.

Along the way, refactor GitOperation to use a (somewhat) proper builder
pattern. This uncovered that we were previously not using SSHJ for the
case of no authentication, which is now rectified.

💡 Motivation and Context

Fixes #333, fixes #548 and fixes #782 if it works.

💚 How did you test it?

I lack the setup to test this properly and only verified that SSH public key authentication by itself still works as expected. Password authentication, public key followed by password authentication and no authentication still need testing.

📝 Checklist

I formatted the code with the IDE's reformat action (Ctrl + Shift + L/Cmd + Shift + L)
I reviewed submitted code

🔮 Next steps

📸 Screenshots / GIFs

msfjarvis · 2020-06-02T09:41:03Z

I can't get this to work for me. After setting AuthenticationMethods publickey,password in my sshd_config, trying to clone my repository with the SSH Key connection mode fails here

~~https://github.com/android-password-store/Android-Password-Store/pull/825/files#diff-efe0564ca4779571d8e3340b0d88a26dR43~~ Ugh, didn't work. GitOperation.kt, line 43.

fmeum · 2020-06-02T09:52:16Z

I can't get this to work for me. After setting AuthenticationMethods publickey,password in my sshd_config, trying to clone my repository with the SSH Key connection mode fails here

~~https://github.com/android-password-store/Android-Password-Store/pull/825/files#diff-efe0564ca4779571d8e3340b0d88a26dR43~~ Ugh, didn't work. GitOperation.kt, line 43.

Sorry, that require shouldn't have made it in in that form: #826.

msfjarvis · 2020-06-02T10:09:10Z

Doesn't throw anymore but still doesn't seem to work, tried both my SSH key as well as password and SSHJ always fails saying all authentication methods were exhausted. I'll try to debug some more on my end...

msfjarvis · 2020-06-02T10:41:34Z

uhh

msfjarvis · 2020-06-12T19:55:00Z

@FabianHenneke can you rebase this and #807 when you get a chance? I have the weekend off so I'm going to try and create a docker setup to allow me to test this.

fmeum · 2020-06-13T06:24:49Z

@FabianHenneke can you rebase this and #807 when you get a chance? I have the weekend off so I'm going to try and create a docker setup to allow me to test this.

Done, but EDCSA keys remain broken for now. Everything else should work as before.

msfjarvis · 2020-09-06T08:38:38Z

@FabianHenneke would you be able to rebase this some time over the next week? Should leave us enough time to be able to review and test this before the September release.

fmeum · 2020-09-07T07:58:48Z

@FabianHenneke would you be able to rebase this some time over the next week? Should leave us enough time to be able to review and test this before the September release.

Please review and merge #1084 first.
We are currently offering password authentication implicitly after publickey authentication in both possible cases: As a follow-up authentication mode when publickey auth succeeded and as an alternative authentication mode when it failed. I don't think there is an easy way to differntiate between the two without delving into SSHJ internals, so we should first agree on whether this behavior is okay or not.

msfjarvis · 2020-09-07T08:04:53Z

@FabianHenneke would you be able to rebase this some time over the next week? Should leave us enough time to be able to review and test this before the September release.

Please review and merge #1084 first.
We are currently offering password authentication implicitly after publickey authentication in both possible cases: As a follow-up authentication mode when publickey auth succeeded and as an alternative authentication mode when it failed. I don't think there is an easy way to differntiate between the two without delving into SSHJ internals, so we should first agree on whether this behavior is okay or not.

I think it's acceptable. I assume we'd only be offering password authentication for servers that do advertise password or keyboard-interactive authentication methods alongside publickey?

fmeum · 2020-09-07T08:17:37Z

@FabianHenneke would you be able to rebase this some time over the next week? Should leave us enough time to be able to review and test this before the September release.

Please review and merge #1084 first.
We are currently offering password authentication implicitly after publickey authentication in both possible cases: As a follow-up authentication mode when publickey auth succeeded and as an alternative authentication mode when it failed. I don't think there is an easy way to differntiate between the two without delving into SSHJ internals, so we should first agree on whether this behavior is okay or not.

I think it's acceptable. I assume we'd only be offering password authentication for servers that do advertise password or keyboard-interactive authentication methods alongside publickey?

That should be true based on my understanding of the SSHJ code, but I don't have the setup to test it properly.

fmeum · 2020-09-07T08:19:46Z

Maybe we should rethink our AuthMode switches though. If we could make the switches shown depend on whether or not the URL starts with http:// or https://, we could show either a choice between "Password" and "None" (HTTP) or "Public key", "Public key (via OpenKeychain)" or "Password only" (SSH).

msfjarvis · 2020-09-07T08:21:38Z

Maybe we should rethink our AuthMode switches though. If we could make the switches shown depend on whether or not the URL starts with http:// or https://, we could show either a choice between "Password" and "None" (HTTP) or "Public key", "Public key (via OpenKeychain)" or "Password only" (SSH).

Should certainly not be impossible, I can take a stab at it. Do you want it to go in with this PR?

fmeum · 2020-09-07T08:24:32Z

Maybe we should rethink our AuthMode switches though. If we could make the switches shown depend on whether or not the URL starts with http:// or https://, we could show either a choice between "Password" and "None" (HTTP) or "Public key", "Public key (via OpenKeychain)" or "Password only" (SSH).

Should certainly not be impossible, I can take a stab at it. Do you want it to go in with this PR?

I would prefer to have this go in as is. The other change would be almost purely UI-related while this does not touch the UI at all, so I would prefer them to remain separate.

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* ux-improvements: DecryptActivity: hide menu items until decrypt finishes Address review comments GitCommandExecutor: cleanup and address build warning Hide unsupported authentication methods git: re-add back button handling

msfjarvis · 2020-09-07T23:39:31Z

I tried cloning from a server which only had password authentication after setting authentication mode to SSH key in the app and did not get prompted for a password, but from my understanding of the changes here, that shouldn't have been the case?

fmeum · 2020-09-08T08:45:19Z

I tried cloning from a server which only had password authentication after setting authentication mode to SSH key in the app and did not get prompted for a password, but from my understanding of the changes here, that shouldn't have been the case?

Yes, if that doesn't work and there is nothing else that's wrong here, then I fear sshj doesn't properly deal with multiple auth methods.

Could you send me the sshj log output?

* develop: Use same checks as BiometricAuthenticator in UserPreference (android-password-store#1088) UX fixups and improvements (android-password-store#1086)

msfjarvis · 2020-09-08T08:57:12Z

I tried cloning from a server which only had password authentication after setting authentication mode to SSH key in the app and did not get prompted for a password, but from my understanding of the changes here, that shouldn't have been the case?

Yes, if that doesn't work and there is nothing else that's wrong here, then I fear sshj doesn't properly deal with multiple auth methods.

Could you send me the sshj log output?

Obviously simply taking a log made it behave 😑

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

fmeum requested review from msfjarvis and Skrilltrax as code owners June 1, 2020 17:36

msfjarvis self-assigned this Jun 1, 2020

msfjarvis added the 🎁 feature label Jun 1, 2020

msfjarvis added this to the 1.9.0 milestone Jun 1, 2020

fmeum mentioned this pull request Jun 2, 2020

Remove a misguided require from GitOperation #826

Merged

6 tasks

msfjarvis closed this in #826 Jun 2, 2020

msfjarvis reopened this Jun 2, 2020

fmeum force-pushed the fhenneke_multiple_ssh branch from 612d626 to 545e4e7 Compare June 13, 2020 06:23

msfjarvis modified the milestones: 1.9.0, 1.10.0 Jun 17, 2020

msfjarvis closed this Jun 24, 2020

msfjarvis reopened this Jun 24, 2020

msfjarvis changed the base branch from master to develop June 24, 2020 19:00

fmeum added blocked labels Jul 2, 2020

msfjarvis removed this from the 1.10.0 milestone Jul 16, 2020

msfjarvis removed the blocked label Aug 17, 2020

msfjarvis added this to the 1.12.0 milestone Aug 17, 2020

fmeum force-pushed the fhenneke_multiple_ssh branch 2 times, most recently from 545e4e7 to c5dd328 Compare September 7, 2020 07:56

Offer password SSH authentication after publickey

b680fc4

msfjarvis force-pushed the fhenneke_multiple_ssh branch from c5dd328 to b680fc4 Compare September 7, 2020 08:19

msfjarvis added 2 commits September 7, 2020 14:04

git: re-add back button handling

cf1471d

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

Hide unsupported authentication methods

6f2d510

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

msfjarvis mentioned this pull request Sep 7, 2020

UX fixups and improvements #1086

Merged

8 tasks

msfjarvis added 4 commits September 7, 2020 16:52

GitCommandExecutor: cleanup and address build warning

21537ac

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

Address review comments

afbe284

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

DecryptActivity: hide menu items until decrypt finishes

2f25116

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

Merge branch 'ux-improvements' into fhenneke_multiple_ssh

e837d77

* ux-improvements: DecryptActivity: hide menu items until decrypt finishes Address review comments GitCommandExecutor: cleanup and address build warning Hide unsupported authentication methods git: re-add back button handling

Merge branch 'develop' into fhenneke_multiple_ssh

a1a0b2e

* develop: Use same checks as BiometricAuthenticator in UserPreference (android-password-store#1088) UX fixups and improvements (android-password-store#1086)

msfjarvis previously approved these changes Sep 8, 2020

View reviewed changes

Add changelog entry

c606b36

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

msfjarvis dismissed their stale review via c606b36 September 8, 2020 08:58

msfjarvis approved these changes Sep 8, 2020

View reviewed changes

msfjarvis removed the in progress label Sep 8, 2020

msfjarvis merged commit 9e0fb93 into android-password-store:develop Sep 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple authentication methods #825

Support multiple authentication methods #825

fmeum commented Jun 1, 2020 •

edited

msfjarvis commented Jun 2, 2020 •

edited

fmeum commented Jun 2, 2020

msfjarvis commented Jun 2, 2020

msfjarvis commented Jun 2, 2020

msfjarvis commented Jun 12, 2020

fmeum commented Jun 13, 2020

msfjarvis commented Sep 6, 2020

fmeum commented Sep 7, 2020

msfjarvis commented Sep 7, 2020

fmeum commented Sep 7, 2020

fmeum commented Sep 7, 2020

msfjarvis commented Sep 7, 2020

fmeum commented Sep 7, 2020

msfjarvis commented Sep 7, 2020

fmeum commented Sep 8, 2020

msfjarvis commented Sep 8, 2020

Support multiple authentication methods #825

Support multiple authentication methods #825

Conversation

fmeum commented Jun 1, 2020 • edited

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

📸 Screenshots / GIFs

msfjarvis commented Jun 2, 2020 • edited

fmeum commented Jun 2, 2020

msfjarvis commented Jun 2, 2020

msfjarvis commented Jun 2, 2020

msfjarvis commented Jun 12, 2020

fmeum commented Jun 13, 2020

msfjarvis commented Sep 6, 2020

fmeum commented Sep 7, 2020

msfjarvis commented Sep 7, 2020

fmeum commented Sep 7, 2020

fmeum commented Sep 7, 2020

msfjarvis commented Sep 7, 2020

fmeum commented Sep 7, 2020

msfjarvis commented Sep 7, 2020

fmeum commented Sep 8, 2020

msfjarvis commented Sep 8, 2020

fmeum commented Jun 1, 2020 •

edited

msfjarvis commented Jun 2, 2020 •

edited