Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

Remove HOTP/TOTP support #806

Merged
merged 6 commits into from
May 28, 2020
Merged

Remove HOTP/TOTP support #806

merged 6 commits into from
May 28, 2020

Conversation

msfjarvis
Copy link
Member

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Remove all references to HOTP/TOTP and all the associated code. Updates PasswordEntryTest to remove all non-applicable test cases and moves it to the JVM unit tests since it now has zero Android dependencies.

💡 Motivation and Context

The HOTP/TOTP code is a bit of a nightmare and none of the active maintainers either endorse or use this particular functionality, so it makes sense for us to remove the feature rather than offer a subpar implementation.

💚 How did you test it?

Unit tests pass and password decryption works as before.

📝 Checklist

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

🔮 Next steps

I have a few more UX concerns around the decrypt screen that shall be addressed in a followup PR.

📸 Screenshots / GIFs

msfjarvis added 3 commits May 28, 2020 20:39
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis msfjarvis added this to the 1.9.0 milestone May 28, 2020
@msfjarvis msfjarvis self-assigned this May 28, 2020
msfjarvis added 2 commits May 28, 2020 21:02
I interpreted the check around this as "for HOTP" when it is "for everything but HOTP"

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member Author

GitHub's green and red has the magical ability of just screaming where you fucked up

fmeum
fmeum previously approved these changes May 28, 2020
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis msfjarvis merged commit e7463ec into master May 28, 2020
@msfjarvis msfjarvis deleted the refactor/remove-otp-feature branch May 28, 2020 17:12
msfjarvis added a commit to fmeum/Android-Password-Store that referenced this pull request May 30, 2020
* master:
  github: run PSL update workflow weekly (android-password-store#809)
  Update Public Suffix List data (android-password-store#808)
  Remove HOTP/TOTP support (android-password-store#806)
  Configure IME options and focus direction (android-password-store#805)
@chvp
Copy link

chvp commented Jun 22, 2020

Is there any way this decision could be revised? I use this feature quite a lot, and the proposed alternatives don't provide the same ease-of-use. The reason I store my TOTP secrets in my pass store is that I sometimes don't have access to my phone when on my laptop (and vice-versa). So I would somehow have to keep the Aegis/AndOTP data in sync with my pass store (or yet another program on my laptop).

I also don't feel like buying yet another piece of hardware that I can't verify the supply chain of (with regards to child/forced labor and environmental practices) that adds minimal security benefits when used instead of a software solution.

@msfjarvis
Copy link
Member Author

Is there any way this decision could be revised? I use this feature quite a lot, and the proposed alternatives don't provide the same ease-of-use. The reason I store my TOTP secrets in my pass store is that I sometimes don't have access to my phone when on my laptop (and vice-versa). So I would somehow have to keep the Aegis/AndOTP data in sync with my pass store (or yet another program on my laptop).

I also don't feel like buying yet another piece of hardware that I can't verify the supply chain of (with regards to child/forced labor and environmental practices) that adds minimal security benefits when used instead of a software solution.

I'm sorry, but this is final. I understand your concerns completely but we cannot be maintaining a feature that none of us use or recommend.

@chvp
Copy link

chvp commented Jun 22, 2020

Not trying to argue, but what does "recommend" mean in this case? Are there security concerns with the way I've (been) using TOTP?

@msfjarvis
Copy link
Member Author

Not trying to argue, but what does "recommend" mean in this case? Are there security concerns with the way I've (been) using TOTP?

Hopefully not, but FIDO tokens are inherently more secure. Entrusting TOTP to the same security as your passwords will mean that if your GPG key gets pwned, two factor authentication will not be able to prevent anything.

@fmeum
Copy link
Member

fmeum commented Jun 22, 2020

Not trying to argue, but what does "recommend" mean in this case? Are there security concerns with the way I've (been) using TOTP?

I think that your strategy is quite reasonable.

In case you are using TOTP, how about reregistering both your phone (with an app such as Aegis/AndOTP) and your laptop with the same TOTP secret/QR code at the time of enabling two-factor authentication? I know this could take quite some effort, but the secret would be protected optimally on each platform (e.g., be stored in the Android Keystore) and there would be no need for syncing.

I personally have never used HOTP, so I don't really know anything about its security guarantees or best practices.

I also don't feel like buying yet another piece of hardware that I can't verify the supply chain of (with regards to child/forced labor and environmental practices) that adds minimal security benefits when used instead of a software solution.

I found Nitrokey and Solo to be much more transparent vendors than Yubico, so you might want to inquire with them if you haven't so far. In general, FIDO tokens should be pretty durable (no batteries, just a single button, simple protocol that will be supported for many years) and do offer a substantial security benefit compared to HOTP/TOPT: they fully prevent account takeovers through phishing.

@erayd
Copy link

erayd commented Jun 27, 2020

Completely removing this feature is IMO a bad call. I can understand the security concerns, but I do think it would be better to still provide the feature, but simply default it to disabled behind a warning screen. If users wish to proceed regardless, then that should be their choice.

We went through the same security debate when rewriting browserpass, and there were too many annoyed users to simpy leave it out - a fork was a real possibility. In the end, we decided to continue providing the feature, but as a secondary extension - users must actively seek out the feature; it's not present by default, but it's still there if they really want to use it. We do have security advice in the readme explaining the issues with it.

/cc @maximbaz

@msfjarvis
Copy link
Member Author

Completely removing this feature is IMO a bad call. I can understand the security concerns, but I do think it would be better to still provide the feature, but simply default it to disabled behind a warning screen. If users wish to proceed regardless, then that should be their choice.

We went through the same security debate when rewriting browserpass, and there were too many annoyed users to simpy leave it out - a fork was a real possibility. In the end, we decided to continue providing the feature, but as a secondary extension - users must actively seek out the feature; it's not present by default, but it's still there if they really want to use it. We do have security advice in the readme explaining the issues with it.

Really appreciate you weighing in on this. My concern with this code was multi-fold and extended beyond the clear security problem. The way this was architectured blocked other meaningful improvements like #776, and since neither of us used or wrote this code, it was hard to argue in its favor against the "obvious" choice of using a better, dedicated solution. I'd consider bringing it back, but that again gets stuck on the point of us not being consumers of this code and thus not in a position to guarantee its efficacy. We can probably provide TOTP support without a lot of trouble, but HOTP will require some strong arguments in its favor for me to re-open that can of worms.

@erayd
Copy link

erayd commented Jun 27, 2020

Does anyone actually use HOTP? It's rare in my experience; I've never actually encountered a website that used it.

TOTP seems to be the predominant standard, and as you say, it's nice and simple to implement. Would a TOTP-only solution be a viable option?

@msfjarvis
Copy link
Member Author

Does anyone actually use HOTP? It's rare in my experience; I've never actually encountered a website that used it.

TOTP seems to be the predominant standard, and as you say, it's nice and simple to implement. Would a TOTP-only solution be a viable option?

TOTP-only is certainly viable, I can spin up an implementation within the day. We're awaiting reporter confirmation before our 1.9.1 patch release goes out so maybe I'll be able to sneak TOTP support into it. Will get started now.

@fmeum
Copy link
Member

fmeum commented Jun 27, 2020

TOTP-only is certainly viable, I can spin up an implementation within the day. We're awaiting reporter confirmation before our 1.9.1 patch release goes out so maybe I'll be able to sneak TOTP support into it. Will get started now.

Minimal-but-working TOTP support should satisfy the needs of 99% of our users that want 2FA in PS. This seems to strike the right balance between maintenance burden and usefulness. IMO HOTP should not make a return to Android Password Store though.

@msfjarvis
Copy link
Member Author

I've started #890 with my initial draft of reimplementing TOTP support with a cleaner interface. It's very WIP and completely untested so I'd appreciate any and all help with figuring things out :)

@erayd
Copy link

erayd commented Jun 27, 2020

@msfjarvis I'm not too familiar with Android dev, but if someone compiles a test apk I'm happy to test the feature.

@msfjarvis
Copy link
Member Author

@msfjarvis I'm not too familiar with Android dev, but if someone compiles a test apk I'm happy to test the feature.

I'll attach an APK to the PR once I get the unit tests down.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants