Support for OTP

[qbit]

I am making this a PR before it gets bitrot :D

As I mentioned in #69, this is likely not the best way to display the OTP - Also there are concerns to be had about storing your OTP secret in the same store as your password!

[roddhjav]

I think you should just support the pass extension pass-otp. To display the OTP, why don't you submit it like the extension does it with passwords and logins?

Regarding the concerns about storing OTP secret in the same store as your password, it has been disputed in the pass mailing list. Due to the nature of pass it is the user responsibility to use different key and/or devices to store the otp secret.

[qbit]

I think you should just support the pass extension pass-otp.

Meaning the format they use in the encrypted file, or the actual pass-otp script? The latter would be difficult to use via the browserpass extension.

[maximbaz]

It is not impossible to use pass-otp script, this whole extension was using pass binary to retrieve passwords, however just like pass was ditched in favor of plain gpg for various reasons, I don't think you should introduce a dependency on pass-otp script. Your approach is good here, keep it as it is.

However, I do think supporting the format of otp string that pass-otp is using is a must, it is too popular of an extension to be ignored. Also, we already have a feature request for this - #99 🙂

@qbit are you still interested in getting this PR merged? Could you add the support for pass-otp format, and fix the code conflicts? I'll make sure to review it ASAP 😉

[qbit]

Sure! I will get something re-pushed this week hopefully!

[qbit]

OK, I have an update for this that makes it work with the pass-otp url style. I am getting some weird errors when talking to the native extension though (not just on my otp branch, but on current master for this repo..). Did something else break?

[qbit]

Also - this disables the alert for the OTP - as I can't test :D

[maximbaz]

So you decided to support only pass-otp format, and not any other?

If we don't have alerts, how will users retrieve the OTP code? 🙂

I don't have any issues on master, could you try maybe to remove everything you have, and try again from scratch?

[qbit]

@maximbaz how are you loading the extensions for testing?

[maximbaz]

This is a clean set of installation steps:

- Delete all artifacts from all previous installations
$ yarn
$ make js
$ make release
$ ./install.sh
- Load `browserpass/chrome` with `Load unpacked extension`.

[qbit]

Yep.. looks like I still haven't had enough coffee!

[qbit]

OK, this is working - but I have found a few OTPs that are encoded oddly (they truncate the base32 padding stuff, which angers Go)

[maximbaz]

maybe let's use a non-strict if (login.digits)? don't want to deal with bugs in future if at some point absence of OTP makes login.digits equal to undefined or null instead of an empty string :)

[maximbaz]

Do you have an example of such OTP url? I wonder if pass-otp itself can handle those, because if it can't - I'm fine with also not supporting them.

[qbit]

Looks like it's an issue with gokyle/twofactor#7 - I have submitted a PR that should fix it! Thanks for the suggestion re pass-otp - I looked at oath-toolkit to figure out what it was doing differently!

Also - this begs the question - do we want to vendor the go 3rd party libs? Anyone have a pref on the tool to use (I am for dep)

[maximbaz]

I'm fine with dep, but I don't want to wait until the gokyle/twofactor#7 is merged and a new version is released... can we just uppercase the secret on our side too? You will know when gokyle/twofactor#7 is merged, just come back and remove the unnecessary case change when this happens.

[qbit]

Done and done!

[maximbaz]

So regarding dep, will you add Gopkg.toml here on in a separate PR? :)

And could you look at my question in the js file?

[qbit]

Less strict check for digits and package deps using dep! Makes sense to do the dep stuff in this PR (imo) since the changes are related to otp anyway.

[maximbaz]

Thanks. Shouldn't the vendor folder be gitignored? I would rather add a note to install dep and run dep ensure in CONTRIBUTING.md, to keep the repo clean

[maximbaz]

I realize we had vendor folder in the repo, but now that we have dep, why not clean it up 🙂
dep ensure can always bring back the correct packages, right?

[qbit]

So the big reason to include the vendor directory is for package maintainers.

Package frameworks like OpenBSD's ports require a complete "tarball" for a given package and using external mechanisms to pull stuff down is forbidden.

I know it adds extra clutter to the repo - but it makes the porting of the app dramatically easier.

Also - I had improperly tested the previous uppercasing - had an old bin. Hence the "clean" target in this commit :P

[maximbaz]

Interesting, but aren't they using browserpass-openbsd64.zip provided in the release? That archive has never contained vendor folder, because the binary is pre-build.

Or there are some packages that don't use the release archive, and instead build from tarball?

I'm fine with keeping vendor, just curious to understand this stuff :)

I'll test this tomorrow morning and merge if all is okay.

[qbit]

With OpenBSD specifically it's typically better to build ports from source, even if binaries are available (net/syncthing for example). This is particularly important because OpenBSD developers are running snapshots while most users run release. During heavy dev cycles, snapshots can contain ABI breakage - rendering binary ports completely useless.

[maximbaz]

Thanks for the explanation!

Tested, works great. I extended the make clean goal a little bit and merged everything in 2cd4c8e.