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 commands for credential management #68

Closed
rgerganov opened this issue Mar 25, 2020 · 15 comments
Closed

Add commands for credential management #68

rgerganov opened this issue Mar 25, 2020 · 15 comments

Comments

@rgerganov
Copy link
Contributor

Since we are adding credential management support in Solo, I believe we also need relevant commands in solo-python. I propose the following CLI interface:

solo key cred --pin <pin>
  get resident keys metadata

solo key cred get-rps --pin <pin>
  list of relying parties with resident keys

solo key cred get-rks --pin <pin> <rpId>
  list of resident keys with the specified rpId

solo key cred rm-rk --pin <pin> <credId>
  deletes the resident key with the specified credential id

I can easily implement this one if others are OK with that.

@conorpp
Copy link
Member

conorpp commented Mar 25, 2020

Great idea! I'd suggest some changes:

solo key cred info --pin <pin>
  get resident keys metadata

solo key cred ls --pin <pin>
  list of relying parties and associated resident keys

solo key cred rm --pin <pin> <credId>
  deletes the resident key with the specified credential id

solo key cred
  Prints this message

@My1
Copy link
Contributor

My1 commented Mar 25, 2020

solo key cred --help should probably also go to printing

and as noted on other places I am not really a fan of passing the pin directly as an argument.

but generally speaking i would TOTALLY dig this, especially as the tool built into chrome (unless you are using windows 10 1903 or higher, where it is hidden due to w10 intercepting fido calls) doesnt seem to work for some reason

@nickray
Copy link
Member

nickray commented Mar 26, 2020

I'd suggest bunching everything under solo key credential (in particular, subsume solo key make-credential. Generally, this tool avoids abbreviations and "tech speak". The idea is that you can "discover" commands, e.g. solo outputs all commands, solo key lists all subcommands.

How PIN entry is handled is orthogonal, the important thing is that it's consistent with other functionality. Ideally I think it should use either an env like SOLO_PIN or a parameter --pin.

@My1
Copy link
Contributor

My1 commented Mar 26, 2020

an env doesnt really make this awesome as it makes it harder to use for people who aren't THAT well versed with computers, and would you enter your password as a parameter to sudo or passwd? I doubt it.
(but as you said that that's something seperate.)

but yeah having all under a seperate credential "submenu" makes sense. although I would keep the old solo key make-credential as an alias for at least a while for compatibility and stuff.

also talking about make credential, it might be nice to have an option to choose whether to create resident or "normal" credentials.

@nickray
Copy link
Member

nickray commented Mar 26, 2020

Please open a separate issue for your recurring comments on how to pass PIN :)

@My1
Copy link
Contributor

My1 commented Mar 26, 2020

okay done -> #70

rgerganov added a commit to rgerganov/solo-python that referenced this issue Mar 27, 2020
@rgerganov
Copy link
Contributor Author

I combined the feedback from @nickray and @conorpp and came up with this:

$ solo key credential     
Usage: solo key credential [OPTIONS] COMMAND [ARGS]...

  Credential management, see subcommands.

Options:
  --help  Show this message and exit.

Commands:
  info    Get credentials metadata
  list    List stored credentials
  remove  Remove stored credential
$ solo key credential info
PIN: 
Existing resident keys: 4
Remaining resident keys: 46
$ solo key credential list
PIN: 
Relying Party       Username            Credential ID
-----------------------------------------------------
ssh:                openssh             YMMIsYUTnMh0bopZi4vHlLLoQDh3e/D1zc7RZqLWJS4DWeMGEOihYhFZYP4ewiPmUpyfS26AIA3LXlwyHIrx4rG/GgAAAA==
ssh:                openssh             UWusy6Fn4TGSc5qvxmzBKor5vZeM2aF0j52VCiZTfIbp+uMGEOihYhFZYP4ewiPmUpyfS26AIA3LXlwyHIrx4rG/HgAAAA==
webauthn.io         batepesho           zY8ZdeKtuZ7lmqm+KSpZss2eHIO+6mrsYKbbu9vGPiRpV3Sm6pITyZwvdLIkkrMgz0AmKpTBqVCgOX8pJQtghB7wGwAAAA==
webauthn.io         xakcop              qzv4d4kid88d0hWvNc5MAF+DhRZJgvNop7nBhnl8xFDS53Sm6pITyZwvdLIkkrMgz0AmKpTBqVCgOX8pJQtghB7wHwAAAA==
$ solo key credential remove --cred-id zY8ZdeKtuZ7lmqm+KSpZss2eHIO+6mrsYKbbu9vGPiRpV3Sm6pITyZwvdLIkkrMgz0AmKpTBqVCgOX8pJQtghB7wGwAAAA==
PIN: 
$ solo key credential list                                                                                                             
PIN: 
Relying Party       Username            Credential ID
-----------------------------------------------------
ssh:                openssh             YMMIsYUTnMh0bopZi4vHlLLoQDh3e/D1zc7RZqLWJS4DWeMGEOihYhFZYP4ewiPmUpyfS26AIA3LXlwyHIrx4rG/GgAAAA==
ssh:                openssh             UWusy6Fn4TGSc5qvxmzBKor5vZeM2aF0j52VCiZTfIbp+uMGEOihYhFZYP4ewiPmUpyfS26AIA3LXlwyHIrx4rG/HgAAAA==

Solo credential IDs are very long, so I choose to show them in base64 instead of hex string. fido2-token is also using base64. As you can see from the output, the firmware still has bugs 😄

Let me know what you think.

@My1
Copy link
Contributor

My1 commented Mar 27, 2020

I have seen credential IDs (and U2F Keyhandles) mostly in base64 or base64url, so that's actually ideal.

but this looks fun, maybe add the create credential command too so all is in one nice place, but interesting that the relying party can be even something that isnt a hostname/domain ("ssh:") especially as the CTAP2 document links the "relying party identifier" directly over to webauthn, which calls it a "valid domain string"

https://fidoalliance.org/specs/fido2/fido-client-to-authenticator-protocol-v2.1-rd-20191217.html
https://www.w3.org/TR/webauthn/#relying-party-identifier

although I think having a bit of uniformity across the commands might be helpful, like why is listing solos solo ls but listing credentials solo key credential list, shouldnt ideally both be ls or both be list?

@rgerganov
Copy link
Contributor Author

although I think having a bit of uniformity across the commands might be helpful, like why is listing solos solo ls but listing credentials solo key credential list, shouldnt ideally both be ls or both be list?

Good point. We should either:
a) keep the current interface and go with ls and rm everywhere
or
b) change solo ls to solo list

I think I prefer a) but it's up to the Solo guys to decide

@My1
Copy link
Contributor

My1 commented Mar 27, 2020

but if b keep a documented (but not listed in help) alias to maintain compatibility
(as well as maybe throw make credential into here), while also keeping a hidden compatibility alias

@nickray
Copy link
Member

nickray commented Mar 27, 2020

It's fine to have synonyms, but we should keep the existing commands as they're in docs, and changing would be a breakimg semver change. Maybe if you really want list etc. too, add them as (hidden) commands linking to the same actual command?

Some tools even let you use just as many characters to disambiguate, (e.g. solo l would work...), but I don't think click supports that.

One comment on arguments vs. options, it seems to me that --cred-id in solo key credential remove --cred-id <...> is mandatory, hence not optional, hence would be an argument? :)

@conorpp
Copy link
Member

conorpp commented Mar 27, 2020

I like using ls/rm.

@nickray
Copy link
Member

nickray commented Mar 27, 2020

I like a lot that solo key credential ls in this proprosal lists all RKs of all RPs, I guess it would be nice to have an optional --relying-party <...> to filter down to only one.

Also for optional arguments, so far we've used --very-long-form with short one-letter (e.g. -i) synonyms. In that vein it would be --credential-id not --cred-id etc.

Alright, enough bikeshedding XD.

I should write down documentation of the philosophy in the repo to share knowledge...

rgerganov added a commit to rgerganov/solo-python that referenced this issue Mar 30, 2020
@rgerganov
Copy link
Contributor Author

rgerganov commented Mar 30, 2020

I have updated the PR with the following changes:

  • use ls and rm instead of list and remove
  • handle empty usernames
  • handle no credentials when running solo key credential ls
  • use mandatory argument instead of option for solo key credential rm

@michaelblyons
Copy link

Closed with merge of #71? Or waiting on #85?

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

No branches or pull requests

5 participants