Skip to content

Conversation

@znd4
Copy link
Contributor

@znd4 znd4 commented Apr 5, 2024

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
Copy link
Owner

emersion commented Apr 5, 2024

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
Copy link
Contributor Author

znd4 commented Apr 5, 2024

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
Copy link
Contributor Author

znd4 commented Apr 5, 2024

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 znd4 mentioned this pull request Apr 5, 2024
@znd4
Copy link
Contributor Author

znd4 commented Apr 5, 2024

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
Copy link
Owner

emersion commented Apr 8, 2024

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
Copy link
Contributor Author

znd4 commented Apr 9, 2024

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
Copy link
Owner

emersion commented Apr 9, 2024

I'd be fine with either FWIW.

@znd4
Copy link
Contributor Author

znd4 commented Apr 25, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

znd4 commented Apr 25, 2024

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

@simonfelding
Copy link

This would be handy and would work.

Another way to solve it is:

  • do the auth part locally, with the env variable HYDROXIDE_BRIDGE_PASS="whatever"
  • save the .config/hydroxide folder
  • mount the content to your CI/CD pipelines in /root/.config/hydroxide (or /.config/hydroxide if you dont run as root), with the env variable HYDROXIDE_BRIDGE_PASS="whatever"

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.

3 participants