Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ability to save OTP Token URI #437

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

trowgundam
Copy link
Contributor

With the recent release of 5.5, the ability to use 3rd party authentication token applications, like the Google Authenticator app, was added. With this change I've added the ability to provide the OTP Secret or, better yet, entire OTP URI. This allows this launcher to automatically generate the OTP code. The URI is stored in the same manner as the Password when saving, and I've also semi-hidden the setup window in the context menu of the Account Switcher. I did this to dissuade users, as this would technically reduce the security of their account if say their computer was compromised.

In order to accomplish this, I added the Otp.Net NuGet package to calculate the OTP. I could have implemented the generation logic directly, but I felt it would be better to use an established library, that is known to work. I have taken other precautions as well. I provide feedback on the relevant sections of the URI in the setup Window, and whether they are valid values. Plus I have included an actively updating read out of the actual OTP, so that the value can be checked against actual authentication apps. Also, if the user's login fails once, it will disable the logic. This is to prevent users from potentially locking themselves out of their accounts by repeatedly hitting the Login button.

@trowgundam trowgundam force-pushed the otp_2fa branch 2 times, most recently from 4885499 to f5f79f7 Compare June 9, 2021 04:47
{
var credentials = CredentialManager.GetCredentials($"FINAL FANTASY XIV-{UserName.ToLower()}-OTP");

return credentials != null ? credentials.Password : string.Empty;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious: how long does this value stick around? I'd be more comfortable using this feature if I knew it was only kept in memory for strictly as long as necessary. Would it be possible to never have a get for the URI value, and just have a get for the current OTP itself instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OTP URI is only kept in memory when it is used to generate your 2FA code on Login OR when on its Setup screen. Otherwise it should never be loaded. Of course you do have to worry about the GC to actually remove it from memory, but it shouldn't be in memory for an inordinate amount of time.

@mkolassa
Copy link

Confirmed working for me, and how glorious it is! Thanks @trowgundam!

TestAriaDownload and TestVersionDecode tests pass, while TestTorrentDownload does not, although that one is commented as WIP.

@mkolassa
Copy link

What is preventing this PR from being merged at this point?

@Caraxi
Copy link

Caraxi commented Aug 20, 2021

other than being a bad idea?

@mkolassa
Copy link

I don't see this as being a bad idea on a device that I trust. Why not allow the user decide what they want to do?

@reiichi001
Copy link
Contributor

The fact that it defeats the entire point of having an OTP? If your computer is compromised, someone would have full and easy access to your account.

@mkolassa
Copy link

Once again, why not let the user decide what they want to do? If a user's computer is compromised, a game is going to be the least of their worries.

@ackwell
Copy link

ackwell commented Aug 20, 2021

As developers, it's our job to do the right thing by the end user, not just supply the means to shoot themselves in the proverbial foot.

MFA/2FA/OTP is designed the way it is for an explicit purpose. Let's not undermine the entire purpose of these tools because it's "inconvenient" or "i'm just doing it for the free teleport". Security is, by its very nature, unfortunately inconvenient.

@mkolassa
Copy link

Think of the Steam and Battle.net launchers, and how once they are authenticated, they remember that they were because the device is trusted by the user.

In my opinion, the design of the FFXIV launcher to exclude this is an unfortunate oversight.

If Steam and Battle.net, two of the most popular launchers in the world, can trust a computer after the initial authentication, why not allow FFXIV to do the same?

@reiichi001
Copy link
Contributor

We have an experimental UID cache. That's completely separate from using an OTP

@ackwell
Copy link

ackwell commented Aug 20, 2021

Both Steam and Battle.net require an off-device 2FA for initial authentication. While it's unfortunate that XIV's authentication logic does not support extended sessions like those two examples do, that's a little apples to oranges.

This isn't an attempt to cache an existing session - that's a feature already implemented in XL. This PR is trying to completely omit the 2FA system entirely.

@nebularg
Copy link

The concern is someone having access to your unlocked physical computer being able to log in? Security is basically moot at that point. Bitwarden, a popular password manager, also supports saving the otp secret key along with your password to further automate logins because it's convenient. Authy syncs otp tokens across any number devices out of convenience. Imposing your view of strict security on a user because you think they can't make the decision for themselves is pretty awkward.

I use 2fa to prevent access to unknowns over the internet, not to lock down my physical computer. If you share your computer, you probably want to take steps to prevent siblings or what-not from accessing your account, but for a majority of people it's just a QoL improvement to not have to pull out your phone every time you login.

@reiichi001
Copy link
Contributor

Use bluestacks then. Or authy desktop. Or merge the PR into a custom fork, forsake official support, and use it anyways.

@nebularg
Copy link

You're missing the point. Why have auto-login at all?

@NotAdam
Copy link

NotAdam commented Aug 20, 2021

password managers like bitwarden don't store credentials on your local computer and authenticating provides you with the ability to fetch secrets from the remote server (normally for a limited period) which contains your passwords, 2fa secrets etc.

this has none of the benefits a password manager would provide has because your password and 2fa secret are now stored on your local computer in an easy place to get to, and the first thing basically any competent malware scrapes.

Why have auto-login at all?

because if you're using 2fa you still need proof that you are who you say you are with something that you have. you just skip having to enter a password. it's incredibly common practice nowadays with the advent of 2fa/mfa, skipping the password prompt and requiring you to acknowledge the login or otherwise provide a TOTP to login.

@trowgundam
Copy link
Contributor Author

trowgundam commented Aug 20, 2021

I agree this isn't the best idea from a security standpoint, I fully understood that when I made this change. However, I have a special use case where, while not essential, greatly eases the use. I have a system called the GPD Win 3. The keyboard on it is atrocious, to the level of being unusable at times. Combine that with such a tiny screen and it is an incredible pain to type in 2FA codes. It was eased some what with the ability to use OTP as I can just swap over to my Password Manager and copy the code and then paste to login, but even that isn't the quickest process, and I've actually hit the time limit several times, its happened so often if the remaining time on a 2FA code is less than 10 seconds, I wait. As a result I made this change, to alleviate that. Is it the most security conscious solution? Probably not, but that is the age old problem of convenience vs security, I just chose convenience. This was one of the reasons I hid the option in the UI. Heck before I did this, I didn't even know about the profiler switcher or the ability to right click for more options in it, because I had never had a reason to go there before.

If you feel it isn't a fit for the project, that's fine. Please deny the PR. I fully understand the reasoning behind that, and even support to an extent. If that is the case, I'll just maintain my own personal build for my own special use case. No I'm not gonna make any kind of long term supported fork for others, although, I am likely to keep it fairly even with the master branch here. So, they are more than welcome to clone my own repo and build their own versions.

@mkolassa
Copy link

To the best of my knowledge, password managers such as BitWarden DO cache everything locally, and are only synchronized with a remote server. So this puts them in the same league as the Windows Credentials store.

@trowgundam
Copy link
Contributor Author

Yes, most password managers keep your database encrypted locally and only use the server to synchronize your password store. The store is only decrypted when the master password is entered. No secrets are ever sent over network traffic, secure or not.

@NotAdam
Copy link

NotAdam commented Aug 20, 2021

I don't use BitWarden, but 1password doesn't store them locally :) Comes over as an encrypted blob and is decrypted using your keys locally.

@mkolassa
Copy link

mkolassa commented Aug 20, 2021

As a user of 1Password, I can confirm that it does indeed store everything locally on Windows, macOS, and iOS, and in fact only synchronizes to the cloud. It also supports entirely offline vaults.

Edit: I say this because I can disable my network interfaces before I unlock it and it still works.

@chyyran
Copy link

chyyran commented Aug 20, 2021

Could we not compromise here and support having some sort of biometrics through Windows Hello before inputting the OTP?

I am of the opinion that saving the 2FA secret locally does not actually compromise the security of the account, if it is stored securely on the local device. An attacker would have to gain physical access to the device to otherwise extract the secret, so the "one-time-ness" of the OTP still fulfills the "something you have" requirement, being the computer you're logging in from. This is the same reasoning that Windows Hello uses for the PIN authenticator, and why storing TOTP in a 1Password or Bitwarden vault is still secure despite it being stored next to the password.

I would assume that those savvy enough to enroll a TOTP authenticator are already using a password vault and storing their TOTP code alongside, negating the benefit of having 2 factors anyways.

@trowgundam
Copy link
Contributor Author

I actually just had this thought myself. The GPD Win 3 has a Fingerprint Scanner that is compatible with Windows Hello. I thought about seeing if I could figure out how to use that to unlock the OTP secret. Something I might look into over the weekend.

@chyyran
Copy link

chyyran commented Aug 20, 2021

Biometrics would be really cool to have but to be honest, I don't think storing the TOTP secret omits or skips the 2FA system in any meaningful way. If the user has the TOTP secret stored anywhere locally on the device or even on the same device as the password (like with a password manager), it hardly changes the threat model at all, as long as the key stays securely stored locally on the device.

@trowgundam trowgundam force-pushed the otp_2fa branch 2 times, most recently from 23504a1 to 19bc099 Compare August 28, 2021 02:24
@Centzilius
Copy link
Contributor

Just wanted to add that integrating Windows Hello support would potentially break the wine compability. To be fair that is not our main target audience (I think although I am one of those pesky linux users) it should still be considered that if we add this feature it should at least not crash the app if Windows Hello does not exist.

On other note I wanted to add that TOTP still protects your account even if it is stored on the same device from certain thread scenarios. That would be (which imo is the likeliest of all threats) an attacker that got your username and password through a breach (where you share username and password) and tries to log in that way.

@trowgundam trowgundam force-pushed the otp_2fa branch 2 times, most recently from 07f8da6 to cbc1c8a Compare January 19, 2022 16:12
@trowgundam trowgundam force-pushed the otp_2fa branch 2 times, most recently from ff1dc9f to 7c97379 Compare January 27, 2022 05:49
@trowgundam trowgundam force-pushed the otp_2fa branch 4 times, most recently from fe7424d to 7f91b59 Compare March 9, 2022 11:36
@davepinto
Copy link

Damn, I really wish this was included in the main repo. Been using trowgundam's build for a while and I absolutely love it. Saves me 30 seconds of clicking around and finding my OTP key in my password manager (which is unsecure, and right next to the "play ffxiv" button on my taskar). Does your branch have an installer or auto-update @trowgundam ?

@trowgundam
Copy link
Contributor Author

No, I don't provide binaries, and I'm not even sure how the auto-update works, mostly because I've never looked into it. I did not feel comfortable providing binaries for this due to the security implications and out of respect for the original maintainers not wanting the functionality. It try to keep my fork up-to-date for my own personal use, but there are times I forget to check on updates, and a lot of the more recent changes weren't a typical rebase/update and had to have some actual little reworks to fit upstream. You're welcome to keep up with my fork if you like, but I can't make guarantees on it.

@goaaats
Copy link
Member

goaaats commented Mar 9, 2022

I'll try to merge the XIVLauncher Authenticator windows stuff soon, so you can run the auto-otp app on windows, should be just as good

@trowgundam
Copy link
Contributor Author

Will that work for SteamDeck? Because that is one use case that would be attractive for most, I'd think. I had to do things a little differently to make it work under WINE since the .Net libraries for generating OTP wouldn't work under WINE.

@goaaats
Copy link
Member

goaaats commented Mar 9, 2022

We're building a new launcher that runs natively on Linux for steam deck, should be fine

@trowgundam trowgundam force-pushed the otp_2fa branch 2 times, most recently from c329779 to 2ffb1fa Compare March 13, 2022 07:26
@trowgundam trowgundam force-pushed the otp_2fa branch 2 times, most recently from 885eeaa to 1b4b6a8 Compare March 22, 2022 09:54
@trowgundam trowgundam force-pushed the otp_2fa branch 2 times, most recently from b20cade to 3e9a4aa Compare April 13, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.