apptension / onetimepass Goto Github PK

In the README's section about the Google Authenticator, there's info that:

The main issue with this app is that it does not offer any way to backup the secrets, and synchronize them between the
devices.

This is not true anymore, as according to the app's official changelog, it is now possible to move accounts between the devices (although, I didn't check the details yet on how good it works).

We should update the README, then.

The official https://github.com/pre-commit/pre-commit-hooks has some nice additional hooks that we don't use. We should consider adding some of them.

We should also consider adding flake8 hook, as at the moment, we don't have any tool in the CI that controls the actual code quality (although we have to make sure all the checks that are conflicting with black are disabled).

As you can see on this GIF included in the README (!), the show-all command automatically increases all HOTP counters.
This automatic behavior makes sense when printing the single HOTP using show, but in this case, it basically breaks the functionality.

This is a critical bug.

EDIT:
Summary of the discussion: #27 (comment)

The current GIFs we have in the README were created using the macOS recording tool and converted using https://gifs.com/.

This is probably not the optimal solution, as the region has to be set manually (and as a result, it's not consistent between the recordings), and the converter leaves the watermark. And it takes a lot of time to do all of that.

We will definitely create new GIFs if we introduce the major changes to the interface, so we should research some better tooling.

We could consider using the tools recommended here (https://asciinema.org/ + https://github.com/marionebl/svg-term-cli).

According to the https://github.com/google/google-authenticator/wiki/Key-Uri-Format#digits, digits parameter should be optional. It is not, e.g. after using this URI:

otpauth://totp/Example:[email protected]?secret=JBSWY3DPEHPK3PXP&issuer=Example

I get:

[2023-12-22 18:36:00,369] DEBUG:otp:352: 1 validation error for Uri
schema_ -> TOTPUriSchema -> parameters -> digits
  field required (type=value_error.missing)
Usage: otp add uri [OPTIONS] ALIAS
Try 'otp add uri -h' for help.

Error: invalid URI

After #68, if URI contains the algorithm optional parameter, e.g.

otpauth://totp/ACME%20Co:[email protected]?secret=HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ&issuer=ACME%20Co&algorithm=SHA1&digits=6&period=30

the application will raise an error:

[2023-12-21 09:42:02,769] DEBUG:otp:352: 1 validation error for Uri
schema_ -> TOTPUriSchema -> parameters -> algorithm
  extra fields not permitted (type=value_error.extra)

I went too far with renaming all the variables, without enough testing.

Based on the https://github.com/apptension/onetimepass/actions, the last working Action was on 4 Aug, then the next one, on 5 Sep, failed.

It fails on the build step, during Setup PDM:
https://github.com/apptension/onetimepass/actions/runs/2996025879/jobs/4806324623#step:4:73
with an exception ModuleNotFoundError: No module named 'pip._vendor.html5lib'.

It looks like it's connected to pdm-project/pdm#1261

It seems it was fixed in pdm-project/pdm#1266, which according to pdm-project/pdm@79e9160 should be in PDM 1.15.5, however, I do not see 1.15.5 in the releases page https://github.com/pdm-project/pdm/releases?page=2 – the next release after 1.15.4 is 2.x 😕 Also, I do not see bug pdm-project/pdm#1261 being mentioned in any release's changelog.

I will try to contact the maintainers of PDM, while in the meantime, bumping up the version of PDM in the CI.

Related to #68.

-a comes from a historical version of the app, where it was short for --algorithm. Since we've renamed the variables and the related option to a more clear --hash_algorithm, -a may be somehow confusing (or not). I'm not yet sure if we should change this or not, and it seems like a minor thing at the moment, so I'm leaving this issue report to be able to track it as tech debt, but won't do anything with it for now.

The app does accept these as the options to both add hotp and add totp, but does not store them in the database as a result:

$ pdm run otp add totp foo -l foo -i [email protected]
Enter secret:
Repeat for confirmation:
foo added
$ pdm run otp export  | jq
{
  "otp": {
    "foo": {
      "secret": "foo",
      "digits_count": 6,
      "hash_algorithm": "sha1",
      "otp_type": "TOTP",
      "params": {
        "initial_time": "1970-01-01T00:00:00+00:00",
        "time_step_seconds": 30
      }
    }
  },
  "version": "1.0.0"
}

From what I see in the code, we pass these parameters to the underlying onetimepass.db.models.AliasSchema, but the model simply does not have these fields defined.
This is proven by the fact, that if I redefine the AliasSchema model to forbid extra fields (class AliasSchema(BaseModel, extra=Extra.forbid), based on this), it will raise this exception:

$ pdm run otp add totp foo -l foo -i [email protected]
Enter secret:
Repeat for confirmation:
Traceback (most recent call last):
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/bin/otp", line 8, in <module>
    sys.exit(otp())
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/onetimepass/otp.py", line 414, in _wrapped
    return fn(*args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/onetimepass/otp.py", line 502, in add_totp
    alias_data = AliasSchema(
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/pydantic/main.py", line 406, in __init__
    raise validation_error
pydantic.error_wrappers.ValidationError: 2 validation errors for AliasSchema
issuer
  extra fields not permitted (type=value_error.extra)
label
  extra fields not permitted (type=value_error.extra)

Based on the https://github.com/apptension/onetimepass/actions, the last working Action was on 31 Jan, then the next one, on 14 Mar, failed.

It fails on the build step, during installation of the dependencies:
https://github.com/apptension/onetimepass/runs/5544580363
Even if there was no actual changes to the code or the dependencies:
https://github.com/apptension/onetimepass/runs/5710372114

Because it tries to use Python 3.8, not 3.10 defined in the earlier step 😕

It seems that something changed in the https://github.com/pdm-project/setup-pdm that forces Python 3.8.
The last commit was on 15 Feb, so it may be the reason.

While looking into their repo, I saw that note in the README:

You don't need actions/setup-python actually.

Maybe we could actually remove the usage of actions/setup-python as suggested and define the Python version explicitly for the setup-pdm step instead.

Also, we should maybe consider freezing the actions versions, if they happen to break between the versions 🤔

We should actually document how to install the app in the production mode on all the supported platforms (including macOS, Linux, and Windows), but the mobile phone is especially important, as using shell there is not something that typical user, even the developers, do, as you have to use Termux on Android, or some alternative on iPhone.

This is probably something that will be much easier if we do #35 first.

Currently, the only part of the codebase that is unit tested is the otp_algorithm module. It is understandable that the unit testing was omitted during the hackathon, but now it should be a priority to introduce decent test coverage.

As a later part of this, we should probably also introduce the test coverage check in the CI.

The current default path, DB_PATH = "test.db", causes the app to always read/write the database in the current directory:

onetimepass$ rg DB_PATH onetimepass/settings.py
3:DB_PATH = "test.db"
onetimepass$ ls -l test.db
-rw-r--r--  1 dstasczak  staff  1080 Dec  5 18:09 test.db
onetimepass$ pdm run bash
onetimepass$ cd
~$ ls -l test.db
ls: test.db: No such file or directory
~$ otp -q init
~$ ls -l test.db
-rw-r--r--  1 dstasczak  staff  120 Dec  9 00:10 test.db

This is a critical bug.

We should change the path to the absolute one, preferably to the OS-dependent, standard directory for such things (see this package).

https://github.com/apptension/onetimepass/blob/1ff20eac46925e7ae3b54dd1af3bf5a475462608/onetimepass/otp.py shadows the name algorithm from the import:

I suggest to not simply rename one of these to algorithm_, but actually rename both of them:
probably to something along the lines of respectivelyotp_algorithm and hash_algorithm.

Because let's be honest, algorithm is too much of a general name, it's worth to distinct both of them in a more clear way, and we actually use the hash_algorithm name already in some other parts of the codebase.

The biggest issue is that the secret parameter is supposed to be:

an arbitrary key value encoded in Base32 according to RFC 3548. The padding specified in RFC 3548 section 2.2 is not required and should be omitted.
~ source

While the application handles this as plain text, not Base32.

Apart from that, parameters that are specified as optional in the specification, are required in the application. I didn't go through all of them yet, but at least algorithm is required, I got this exception after I didn't include it in the URI:

Traceback (most recent call last):
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/bin/otp", line 8, in <module>
    sys.exit(otp())
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/onetimepass/otp.py", line 374, in add_uri
    uri_parsed = otp_auth_uri.parse(input_uri)
  File "/Users/dstasczak/development/projects/onetimepass/onetimepass/otp_auth_uri.py", line 46, in parse
    algorithm = OTPAlgorithm(query_parsed.get("algorithm")[0].lower())
TypeError: 'NoneType' object is not subscriptable

In case the user passes the invalid value for the -a, --algorithm, the app shows the possible values as the canonical string representation of the enum objects:

$ pdm run otp add hotp foo -a sha
Usage: otp add hotp [OPTIONS] ALIAS
Try 'otp add hotp -h' for help.

Error: Invalid value for '-a' / '--algorithm': 'sha' is not one of <OTPAlgorithm.SHA1: 'sha1'>, <OTPAlgorithm.SHA256: 'sha256'>, <OTPAlgorithm.SHA512: 'sha512'>.

which are not the actual values expected from the user, but their implementation details.

The expected is a representation that directly translates to the expected values:

$ pdm run otp add hotp foo -a sha
Usage: otp add hotp [OPTIONS] ALIAS
Try 'otp add hotp -h' for help.

Error: Invalid value for '-a' / '--algorithm': 'sha' is not one of 'sha1', 'sha256', 'sha512'.

The goal of this issue ticket is to collect all valuable info regarding the current release candidate for the 0.2.0 version.

The release candidate is:
https://github.com/apptension/onetimepass/tree/b763554383f3bf449158f3651dc92d728d57e294.

The goals of 0.2.0 version are:
https://github.com/apptension/onetimepass/blob/19637af2e3dfdec57b8d0d2545a36756d037bb42/ROADMAP.md#020

The plan is to manually test¹ the version, verify all the goals are met, and document it here, ideally with a video recording presenting the successful tests.

This includes places like log files, error outputs, etc.
If we need to inform the user in these that there was some kind of error in the secret's value, the actual value should not be shown.

The suggested solution is to rely on the SecretStr and the SecretBytes data types provided by the Pydantic.

Although the app is still in active development and not stable, I think it's a sane thing to do to publish it to the PyPI, as I plan to test it thoroughly on multiple platforms (macOS, Linux, Windows, Android), which will be difficult if I have to install it everywhere using development tools.

Partially connected to the #37.

I thought that the Base32 is only required for the Key URI format, but I've done a small research on a few websites (AWS, GitLab) and it seems that when the user requests the secret directly instead of using QR Code, the services still provide the Base32-encoded string. I've also checked in the Google Authenticator, which allows to either scan the QR Code or input the secret manually, and it also requires the Base32.

In that case, we should reimplement the secret input field to require Base32, validate it and decode it before generating the OTP (value could be actually stored as Base32 in the database, as this would probably improve the cross-compatibility if someone decides to, unfortunately, migrate from onetimepass to the other tool).

Example secret provided by Google when trying to enable MFA for Google account (with the note that spaces can be ignored):

qea4 ptmn kvd5 ampv wm6d rc6m bbh4 urbo

Example respective URI:

otpauth://totp/Google%3Adaniel%40buttercms.com?secret=qea4ptmnkvd5ampvwm6drc6mbbh4urbo&issuer=Google

Result:

$ otp add uri google
Enter URI:
Repeat for confirmation:
Usage: otp add uri [OPTIONS] ALIAS
Try 'otp add uri -h' for help.

Error: invalid Base32 value; Non-base32 digit found

Is it possible that in some cases, secret is not Base32-encoded? 😕

$ pdm run otp add hotp -h
Usage: otp add hotp [OPTIONS] ALIAS

  Add the new HOTP secret as the specified ALIAS.

Options:
  -c, --counter INTEGER
  -l, --label TEXT
  -i, --issuer TEXT
  -a, --algorithm [sha1|sha256|sha512]
                                  [default: sha1]
  -d, --digits-count INTEGER      [default: 6]
  -h, --help                      Show this message and exit.
$ pdm run otp add hotp foo
Enter secret:
Repeat for confirmation:
Traceback (most recent call last):
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/bin/otp", line 8, in <module>
    sys.exit(otp())
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1053, in main
    rv = self.invoke(ctx)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1659, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 1395, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/core.py", line 754, in invoke
    return __callback(*args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/onetimepass/otp.py", line 414, in _wrapped
    return fn(*args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/dstasczak/development/projects/onetimepass/onetimepass/otp.py", line 452, in add_hotp
    params=HOTPParams(counter=counter),
  File "/Users/dstasczak/development/projects/onetimepass/__pypackages__/3.10/lib/pydantic/main.py", line 406, in __init__
    raise validation_error
pydantic.error_wrappers.ValidationError: 1 validation error for HOTPParams
counter
  none is not an allowed value (type=type_error.none.not_allowed)

We should either support the default value (e.g. -c 0) or reimplement this as the command argument (otp add hotp [OPTIONS] ALIAS COUNTER).

Some details are here #27 (comment) and here #27 (comment).

We should probably make the operation atomic: if there's any unexpected error before the OTP is shown, the counter should be decremented to the original value before calling the command.

I suggest implementing these two commands:

1. db version - that prints to STDOUT the version of the currently used local database.
   This will ofc require the master key to be provided.

   This was actually suggested by @khanek during the hackathon.

   Alternatively, we could implement something like otp version --include-db-version that prints the app's version and the database version.

   That would be helpful for debugging (especially if we ever introduce the new database version).

2. db path or something similar - that prints to STDOUT the file path where the local database is stored.

   This would be helpful when moving the database between devices and for debugging.

This could be either otp --version or otp version, although I would subjectively prefer the former. See #67 (comment) for reasoning.