freedomofpress / securedrop

GitHub repository for the SecureDrop whistleblower platform. Do not submit tips here!
https://securedrop.org/
Other
3.62k stars 686 forks source link

Document format needed for HOTP secrets #2199

Open heartsucker opened 7 years ago

heartsucker commented 7 years ago

Feature request

Description

Currently we don't say in the UI, the CLI, or docs what format the HOTP secret needs. Based off this code:

    def set_hotp_secret(self, otp_secret):
        self.is_totp = False
        self.otp_secret = base64.b32encode(
            binascii.unhexlify(
                otp_secret.replace(
                    " ",
                    "")))
        self.hotp_counter = 0 

It accepts b32 encoded with or with or without spaces. I'm not positive how this handles upper/lower case (trivial to check) and it's somewhat common to use colons to separate these values. These cases should be handled.

Thus, we should handle exactly these cases and no more:

And have regex or something that verifies it and formats it correctly for the b32encode(unhexlify(secret)) translation in db.py.

Or perhaps only all lowercase colon separated.

Note: this may already "just work" somehow (I suspect not), but in any case I'm opening this ticket so we can investigate it and then make it more robust and more strict.

User Stories

As an admin, I don't want unexpected errors because of misunderstandings on how to format my HOTP secrets.

ghost commented 7 years ago

The caller has decent error messages when the format is incorrect. I think I would also prefer this to be documented on the page explicitly though.

        except TypeError as e:
            if "Non-hexadecimal digit found" in str(e):
                flash(gettext(
                    "Invalid secret format: "
                    "please only submit letters A-F and numbers 0-9."),
                      "error")
            elif "Odd-length string" in str(e):
                flash(gettext(
                    "Invalid secret format: "
                    "odd-length secret. Did you mistype the secret?"),
                      "error")
            else:
                flash(gettext(
                    "An unexpected error occurred! "
                    "Please check the application "
                    "logs or inform your adminstrator."), "error")
                app.logger.error(
                    "set_hotp_secret '{}' (id {}) failed: {}".format(
                        otp_secret, uid, e))
heartsucker commented 7 years ago

This seems like an error prone way to handle this. I think we could either handle more cases, or just have one error message that says "Must be a colon separated hex string of length 64" or whatever it is. Though the point about documenting it still stands.

heartsucker commented 7 years ago

Also, if we keep these error messages, they should be in the set_hotp_secret function so they apply to the CLI too. The flash should just be 'HOTP bad format: ' + str(e)

eloquence commented 3 years ago

This is still a valid issue. Our docs treat YubiKey and HOTP as synonymous, and the UI does not provide any guidance on the format of the secret. This is an issue that covers both docs and UI, so I'm not migrating it to the securedrop-docs repo.

The priority here depends on how much emphasis we want to put on security keys (and of what variety, e.g., HOTP vs. U2F), especially as we potentially shift to the SecureDrop Workstation based on Qubes as the recommended user experience for journalists.