emersion / hydroxide

A third-party, open-source ProtonMail CardDAV, IMAP and SMTP bridge
MIT License
1.63k stars 126 forks source link

Add -password and -2fa-totp options and pre-commit linting / formatting #269

Open znd4 opened 7 months ago

znd4 commented 7 months ago

Hi, I'd like to be able to automate my hydroxide setup, e.g.:

hydroxide auth \
    -password $(op item get Protonmail --fields password) \
    -2fa-totp $(op item get Protonmail --otp) \
    $(op item get Protonmail --fields username)

I also probably went a bit overboard with a bit of refactoring (the if a == nil got flagged by gopls as tautological), and adding the pre-commit hooks (especially gofumpt, which if run on every file with pre-commit run --all would generate a lot of changes), but I'll leave them in in case you appreciate some of them.

Also, obviously open to different flag names.

pre-commit comment

If you are interested in keeping pre-commit, pre-commit.ci is pretty cool, although I'd recommend setting ci.autoupdate_branch to quarterly, because IMO it's really noisy at weekly

emersion commented 7 months ago

This was intentionally left out because CLI flags leak sensitive information in shell history files. It should be possible to pass that info via stdin instead.

znd4 commented 7 months ago

This was intentionally left out because CLI flags leak sensitive information in shell history files. It should be possible to pass that info via stdin instead.

That is a reasonable enough concern, especially for such something this important. Unfortunately, piping to stdin hasn't worked for me so far (at least in fish, bash, and nushell):

❯ echo foo | hydroxide auth username
Password:
$ echo foo | hydroxide auth username
Password:
❯ echo foo | hydroxide auth username
Password:

Would you be open to either

  1. environment variables (e.g. HYDROXIDE_LOGIN_PASSWORD and HYDROXIDE_2FA_TOTP)
  2. A change in the implementation of the password prompt that enables piping
znd4 commented 7 months ago

I'll split my more recent changes into a separate PR. a quick comment on them though:

basically, hydroxide doesn't actually build any more for go <=1.17, and under some conditions, go install automatically adds -lang=go1.16, which causes things to break. My proposed fix is to increase the go statement in go.mod to go 1.17

znd4 commented 7 months ago

Switched over to environment variables, in case that seems more secure (FWIW, I feel like environment variable secrets seem somewhat less secure if people are just leaving them in their shell all the time, although I guess that's more obvious than the ~/.bash_history risk of arguments).

Also, I looked into supporting piped stdin, and it seems less trivial than I'd expected (e.g. charmbracelet/huh's inputs don't seem to support it), so environment variables or arguments seem like the lower hanging fruits. If you'd prefer to include neither of them, no hard feelings; I can always just maintain a slightly-deviated personal fork)

emersion commented 7 months ago

I'd prefer to fix the stdin read issue. Here's an example of how to do it:

https://git.sr.ht/~emersion/chathistorysync/tree/master/item/askpass.go

znd4 commented 7 months ago

Hmm it doesn't seem too difficult, but I think that supporting multiple passwords (e.g. login password and then the 2FA TOTP) will require either of two funky decisions:

  1. use a single bufio.Scanner throughout case "auth" if os.Stdin isn't a tty
  2. don't use bufio if os.Stdin isn't a tty (e.g. os.Stdin.Read(1)). This way askPass doesn't consume more than a line at a time from os.Stdin

I think 1 is probably the better option, even though the performance hit for 2 won't matter most of the time

emersion commented 7 months ago

I'd be fine with either FWIW.

znd4 commented 7 months ago

use a single bufio.Scanner throughout case "auth" if os.Stdin isn't a tty

@emersion , when you get a chance to review, I've implemented this option

emersion commented 7 months ago

Oh, I remember now why we used /dev/tty in the past: it's to be able to read passwords from the terminal even when piping data into hydroxide. Something like hydroxide import-messages <archive.mbox would still ask the user for the password interactively.

znd4 commented 7 months ago

hmm. I think we could support that by implementing something like a singleton for accessing os.Stdin, but the only way I can think to prevent direct access to os.Stdin is adding a grep invocation to the CI pipeline.

Also, it'd require replacing every existing use of os.Stdin with the new singleton