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

implement bindle login/logout #102

Merged
merged 2 commits into from
Mar 29, 2022
Merged

Conversation

bacongobbler
Copy link
Contributor

@bacongobbler bacongobbler commented Feb 3, 2022

A couple of changes were introduced in this commit:

  • hippo bindle -> hippo bindle push. This was required to free up the hippo bindle command as a subcommand tree.
  • hippo prepare -> hippo bindle prepare (this can be reverted)
  • hippo bindle login was introduced
  • hippo bindle logout was introduced
  • hippo auth login no longer logs you into bindle
  • --danger-accept-invalid-certs now accepts the shortflag -k for compatibility
  • bindle and hippo configuration is stored separately, in
    ~/.config/hippo/hippo.json and ~/.config/hippo/bindle.json,
    respectively

fixes #100

Signed-off-by: Matthew Fisher [email protected]

A couple of changes were introduced in this commit:

- hippo bindle -> hippo bindle push
- hippo prepare -> hippo bindle prepare (this can be reverted)
- hippo bindle login was introduced
- hippo bindle logout was introduced
- hippo auth login no longer logs you into bindle
- bindle and hippo configuration is stored separately, in
  ~/.config/hippo/hippo.json and ~/.config/hippo/bindle.json,
  respectively

Signed-off-by: Matthew Fisher <[email protected]>
Signed-off-by: Matthew Fisher <[email protected]>
Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

Tested against a local bindle server running with basic auth and works great!

I had a few comments/questions:

First, an easy one; just looks like a command description needs updating. Would expect the bindle subcommand to have a more appropriate description:

SUBCOMMANDS:
    app            Add, update, and remove Applications
    auth           Register new accounts and log in/out of Hippo
    bindle         Register new accounts and log in/out of Hippo

Second, a comparison of login flows between bindle and hippo.

  1. hippo bindle login doesn't appear to prompt for username and password like hippo auth login does. Wonder if we can align both? (So that hippo bindle login prompts for both if not provided.)
$  hippo bindle login
Configuration written to /Users/vdice/Library/Application Support/hippo/bindle.json

$  hippo auth login
Enter Hippo username:

Also, hippo bindle login seems to save the entered creds into a file directly without attempting to login to the bindle server, while hippo auth login does attempt to login with the hippo server:

$ hippo bindle login --username bindle --password topsecret
Configuration written to /Users/vdice/Library/Application Support/hippo/bindle.json

$ hippo auth login --username hippo --password topsecret
Error: error in reqwest: error sending request for url (https://localhost:5309/api/account/createtoken): error trying to connect: tcp connect error: Connection refused (os error 61)

(Where I didn't have a hippo server running, but you get the idea.) I wonder if we might want the CLI to also immediately attempt to login to the bindle server on hippo bindle login? If so, it could certainly be a follow-up.

@bacongobbler
Copy link
Contributor Author

bacongobbler commented Feb 4, 2022

  1. hippo bindle login doesn't appear to prompt for username and password like hippo auth login does. Wonder if we can align both? (So that hippo bindle login prompts for both if not provided.)

There is no way to run Hippo without some form of authentication, whereas it is totally OK to run bindle-server in --unauthenticated mode. If we were to force a prompt at hippo bindle login, my concern is that users will have no way of entering an empty username/password to the CLI without being forced to enter empty strings into a terminal prompt. That would prevent those users from using the hippo CLI in a CI environment.

If bindle-server deprecates the --unauthenticated flag and therefore enforces authentication on the backend, then perhaps we can add that to hippo bindle login. It's only a few lines of code:

hippo-cli/src/cli/mod.rs

Lines 206 to 217 in e3c19c0

let h_username: String = match username {
Some(u) => u.to_owned(),
None => Input::new()
.with_prompt("Enter Hippo username")
.interact_text()?,
};
let h_password: String = match password {
Some(p) => p.to_owned(),
None => Password::new()
.with_prompt("Enter Hippo password")
.interact()?,
};

Also, hippo bindle login seems to save the entered creds into a file directly without attempting to login to the bindle server,

This is because logging in with Hippo is a little more sophisticated :) allow me to explain the difference between the two...

When you sign in with Hippo, your username and password is passed to the API, and if successful, you receive a JWT which is valid for 30 minutes after the token has been issued by the server. The Hippo CLI stores this auth token as well as the username in hippo.json so

  1. you know who you logged in as (via hippo auth whoami)
  2. the password is never stored on-disk
  3. the password is never re-used after completing the login flow
  4. the user is forced to re-login every 30 minutes (ensuring compromised tokens cannot be re-used after they've expired)

On the other hand, bindle passes the (optional) username and password in plain text on every API request, so we just store that information on-disk.

I wonder if we might want the CLI to also immediately attempt to login to the bindle server on hippo bindle login? If so, it could certainly be a follow-up.

We certainly could attempt to hit a bindle endpoint and see if the credentials work. That seems like something that could be tackled in a follow-up, and perhaps the bindle maintainers may better understand what the auth flow for bindle should look like. I'm not very familiar with its auth flow, and as I understand it, this is an area that may be replaced or improved upon in future updates, so I'm hesitant to build out a sophisticated auth flow for Bindle at this time.

@vdice
Copy link
Member

vdice commented Feb 4, 2022

Ah, yes, good point re: bindle server able to run --unauthenticated.

We certainly could attempt to hit a bindle endpoint and see if the credentials work. That seems like something that could be tackled in a follow-up, and perhaps the bindle maintainers may better understand what the auth flow for bindle should look like. I'm not very familiar with its auth flow, and as I understand it, this is an area that may be replaced or improved upon in future updates, so I'm hesitant to build out a sophisticated auth flow for Bindle at this time.

Good point. Most likely the basic auth approach in bindle will continue to be, as you've said, dynamic authn on each request w/ the provided creds. See also deislabs/bindle#302

Copy link
Member

@vdice vdice left a comment

Choose a reason for hiding this comment

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

LGTM!

@bacongobbler bacongobbler merged commit 548b309 into deislabs:main Mar 29, 2022
@bacongobbler bacongobbler deleted the bindle-login branch March 29, 2022 17:13
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.

implement hippo bindle login
2 participants