Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CLI support #580

Merged
merged 1 commit into from
Sep 23, 2020
Merged

Add CLI support #580

merged 1 commit into from
Sep 23, 2020

Conversation

grobie
Copy link
Contributor

@grobie grobie commented Sep 5, 2020

This change introduces a small cli program to support use cases like
sharing the initial password for a new account created by some other
command or script (e.g. awscli) with someone else.

Secret messages are commonly read from stdin, but (small) files can also
be shared directly via the --file flag. The generated sharing URL will
be printed on stdout. Extending the default storage expiration time,
disabling one-time passwords and using custom URL and API locations are
all supported. Existing secret URLs can be decrypted with --decrypt.

Existing limitations: no configurable output options and decrypted
messages will always be printed on stdout (even for files).

Tests are provided but not all error flows are covered.

```console
$ yopass --help
Yopass - Secure sharing for secrets, passwords and files

Flags:
      --api string          Yopass API server location (default "https://api.yopass.se")
      --decrypt string      Decrypt secret URL
      --expiration string   Duration after which secret will be deleted [1h, 1d, 1w] (default "1h")
      --file string         Read secret from file instead of stdin
      --key string          Manual encryption/decryption key
      --one-time            One-time download (default true)
      --url string          Yopass public URL (default "https://yopass.se")

Settings are read from flags, environment variables, or a config file located at
~/.config/yopass/defaults.<json,toml,yml,hcl,ini,...> in this order. Environment
variables have to be prefixed with YOPASS_ and dashes become underscores.

Examples:
      # Encrypt and share secret from stdin
      printf 'secret message' | yopass

      # Encrypt and share secret file
      yopass --file /path/to/secret.conf

      # Share secret multiple time a whole day
      cat secret-notes.md | yopass --expiration=1d --one-time=false

      # Decrypt secret to stdout
      yopass --decrypt https://yopass.se/#/...

Website: https://yopass.se

Original draft description

Hej @jhaals!

Today I wanted to quickly securely share the output of a CLI command with someone else and thought "I wish I could do that directly without having to copy&paste it into the browser". As a curious developer on a rainy Saturday, I first implemented a proof of concept and then searched on Github for prior work. Of course I'm not the first who had that idea. Though it was quite easy to add full support for text/file secrets and all options.

$ cat cmd/yopasscli/main.go | ./yopasscli --expiration=1week --one-time=false
https://yopass.se/#/s/5000f555-b0e4-43da-b7f2-3ac5b15d907a/a7Fv0M2yepSKq43tDhHJFd
$ ./yopasscli --file=cmd/yopasscli/main.go --expiration=1week --one-time=false
https://yopass.se/#/f/824cabd5-88de-481e-8216-49964695ad72/ELeAQneP8k1bstEuhLqoW5

The beauty of yopass is that it's so simple. Following the guideline I'd love to have a single binary for all interactions with yopass and invoke the CLI with yopass encrypt. So here is my proposal:

  • CLI support is added to the existing Go executable
  • 3 commands will be supported encrypt, decrypt, server (breaking change to start server: yopass server)
  • Maybe also re-organizing the Go package structure into pkg/yopass (encrypt + client primitives) and pkg/yopass/server (current server dependencies)
  • Full test coverage for the CLI

What do you think about that, would you accept such contribution? Anything you'd like to keep stable without changes (server interface, package structure)?

Have a great weekend!

@jhaals
Copy link
Owner

jhaals commented Sep 6, 2020

Hej @grobie!

First of all thanks for the suggestion, I think this is a great idea especially since the more recent releases no longer uses sjcl.
Splitting dependencies between server and client is a good idea. I do think they should be separate binaries given that 99% don't want to run the server on their laptop 👍 Except for the public API I don't see any problems with changing the package structure given that most run the docker container.

@grobie
Copy link
Contributor Author

grobie commented Sep 6, 2020

Fair enough. I guess having separate commands for encryption and decryption is also not necessary. Supporting decrypting a URL is probably already a very esoteric use case as well, and could be handled with a flag: echo https://yopass.se/#/s/5000f555-b0e4-43da-b7f2-3ac5b15d907a/a7Fv0M2yepSKq43tDhHJFd | yopass --decrypt.

I'd find some beauty in having the CLI binary be called yopass so that the invocation would become

$ ./create-initial-credentials.sh | yopass
https://yopass.se/#....

Would you be cool calling the binaries yopass and yopass-server, or do you want the to keep the current server binary name and prefer yopass-cli? And what to do with the Docker image, include both binaries in there? What entrypoint to use?

@grobie
Copy link
Contributor Author

grobie commented Sep 6, 2020

Running the CLI in a Docker container is not very convenient in the first place given all the flags and arguments which need to be set. So my proposal would be to go forward with binary names yopass / yopass-server and a package structure pkg/yopass / pkg/yopass/server, releasing a single Docker image with both binaries and using /yopass-server as default Docker entrypoint.

If someone wants to use the CLI from the docker image they'll likely configure a shell alias anyways and could do the following alias yopass="docker run --rm --interactive --entrypoint=/yopass jhaals/yopass".

@grobie grobie mentioned this pull request Sep 6, 2020
@jhaals
Copy link
Owner

jhaals commented Sep 6, 2020

Sorry if my previous reply was unclear. I meant that the server is and should be continue to be delivered as a docker container. I agree that using docker for a CLI is a poor experience, I rather go get or brew install the cli yopass cli. I'm completely fine with calling them server and simply yopass for the cli. The package structure is not sensitive as long as the docker container has an entrypoint which starts the server.

@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

Merging #580 into master will decrease coverage by 17.21%.
The diff coverage is 58.91%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #580       +/-   ##
===========================================
- Coverage   86.62%   69.41%   -17.22%     
===========================================
  Files           3        6        +3     
  Lines         157      340      +183     
===========================================
+ Hits          136      236      +100     
- Misses         12       73       +61     
- Partials        9       31       +22     
Impacted Files Coverage Δ
cmd/yopass/main.go 43.29% <43.29%> (ø)
pkg/yopass/client.go 61.53% <61.53%> (ø)
pkg/yopass/yopass.go 77.21% <77.21%> (ø)
pkg/server/memcached.go 68.42% <0.00%> (-5.50%) ⬇️
pkg/server/redis.go 66.66% <0.00%> (-4.77%) ⬇️
pkg/server/server.go 92.63% <0.00%> (-0.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 809df31...9e859bc. Read the comment docs.

@grobie
Copy link
Contributor Author

grobie commented Sep 14, 2020

Good evening @jhaals! I finally found some time today to write tests and restructure the code a bit. I'm not completely happy with the results, but I won't have much time the next days to continue working on that, so I wanted to push it at least for first review.

The test coverage could be better, but it gets a lot more time consuming to set up the test infrastructure to cover all the error flows. I wonder if codeclimate's Cognitive Complexity is tuned for Go, but I'm happy to extract more functionality into smaller functions if you think that will help readability. Please feel free to comment where you'd like to see changes. The integration test currently uses the life demo site (if it can be reached), we could set a web server for these tests as well.

Regarding package structure, I wanted to only provide the most important building blocks in a single yopass package, but had to introduce a third package to avoid cyclic dependencies between server and yopass in tests. Writing this comment, I realize I could also just have put the tests into a third package yopasstest or so. Again, let me know what you'd prefer.

Hope this goes into a direction you like. Cheers!

@grobie
Copy link
Contributor Author

grobie commented Sep 23, 2020

@jhaals Anything I can help with to make this review easier?

@jhaals
Copy link
Owner

jhaals commented Sep 23, 2020

Hi @grobie! Sorry for the lack of updated on my side, I have completely missed this in the flood of dependabot updates.
I think this looks good, I have no problems having the tests in the same package as it covers unless you think it would cause issues down the line.

This change introduces a small cli program to support use cases like
sharing the initial password for a new account created by some other
command or script (e.g. awscli) with someone else.

Secret messages are commonly read from stdin, but (small) files can also
be shared directly via the --file flag. The generated sharing URL will
be printed on stdout. Extending the default storage expiration time,
disabling one-time passwords and using custom URL and API locations are
all supported. Existing secret URLs can be decrypted with --decrypt.

Existing limitations: no configurable output options and decrypted
messages will always be printed on stdout (even for files).

Tests are provided but not all error flows are covered.
@grobie
Copy link
Contributor Author

grobie commented Sep 23, 2020

@jhaals I just took a quick look and it was actually pretty straight forward to move the tests to a yopass_test package, as I was only testing the exported functions anyway. This made it possible to merge the yopass and client packages again, which I believe makes it easier to use than splitting the functionality up in even more packages.

I also removed one of codelimate's reported issues (extracting the flag parsing into a separate parse() function). Not sure that helped a lot, but it resolved one issue. How do you feel about the other codeclimate/coverage issues, can we ignore these for now?

@jhaals
Copy link
Owner

jhaals commented Sep 23, 2020

I think this looks good 👍

@jhaals jhaals merged commit 036b714 into jhaals:master Sep 23, 2020
@jhaals
Copy link
Owner

jhaals commented Sep 28, 2020

@grobie thanks again for doing this work!
I will try to see if I can address some of the testing or configure codecov since it has been blocking other PRs due to lower coverage.

I played around a bit with the cli and noticed that --one-time false did not have any effect (true) is still the default we want.

Secondly the nice usageTemplate docs is not showing up with --help.

@grobie
Copy link
Contributor Author

grobie commented Sep 28, 2020

@jhaals ooops. Looks like some last-minute changes / extractions (in order to satisfy more codeclimate settings) broke this! I'll add a test to verify the correct flag parsing and fix both things today.

@grobie grobie deleted the cli branch September 28, 2020 13:18
@jhaals
Copy link
Owner

jhaals commented Sep 28, 2020

Awesome 👍

@grobie
Copy link
Contributor Author

grobie commented Sep 28, 2020

See #612

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.

None yet

2 participants