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.

Sign up for GitHub

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

Jump to bottom

rad-profile: the rad-profile CLI library #728

Merged
merged 9 commits into from
Sep 8, 2021
Merged

rad-profile: the rad-profile CLI library #728

merged 9 commits into from
Sep 8, 2021

Conversation

FintanH
Copy link
Contributor

@FintanH FintanH commented Jul 16, 2021

Currently ssh_add is a stub since there is some reworking in thrussh required to get radicle-dev/radicle-keystore#17 in.

@FintanH FintanH force-pushed the finto/profile-mgmt branch from f4d94d2 to fb03315 Compare July 27, 2021 10:22
Base automatically changed from finto/profile-mgmt to master July 27, 2021 11:49
@FintanH FintanH force-pushed the finto/rad-profile branch from e4686e1 to 00e5d03 Compare July 27, 2021 12:00
@FintanH FintanH force-pushed the finto/rad-profile branch from 00e5d03 to 4c24357 Compare August 9, 2021 10:34
@FintanH FintanH force-pushed the finto/rad-profile branch from 4c24357 to 3a3196d Compare August 19, 2021 14:46
@FintanH FintanH marked this pull request as ready for review August 19, 2021 14:47
@FintanH FintanH requested a review from a team as a code owner August 19, 2021 14:47
@FintanH
Copy link
Contributor Author

FintanH commented Aug 19, 2021

This is ready for some inspection 🕵️‍♀️

@FintanH FintanH changed the title rad-profile: first draft of the rad-profile CLI rad-profile: the rad-profile CLI library Aug 19, 2021
FintanH
FintanH commented Aug 19, 2021
View reviewed changes
link-crypto/Cargo.toml Outdated Show resolved Hide resolved
@FintanH FintanH mentioned this pull request Aug 20, 2021
Open
@FintanH
Copy link
Contributor Author

FintanH commented Aug 23, 2021

oh lol, what do we do for Windows? 😆 thrussh has no implementation for not(unix) 😬

@kim
Copy link
Contributor

kim commented Aug 23, 2021 via email

@FintanH
Copy link
Contributor Author

FintanH commented Aug 23, 2021

Is this because std doesn't export UNIX domain sockets under windows? That's
more a matter of policy it seems [0]. Oh look, a yak!

Thanks for the pointer :) Well, this would mean that we'd have to also get tokio and smol to add the Windows support so that in turn it can be supported by thrussh.

image

@kim
Copy link
Contributor

kim commented Aug 23, 2021 via email

@FintanH
Copy link
Contributor Author

FintanH commented Aug 23, 2021

I think this is probably the point in time where we no longer pretend we could
support Windows eventually (outside WSL, that is). Or else, someone steps up to
become a maintainer for Windows-compat.

I think we can at least punt on this for now with a04a954, but ya it doesn't seem feasible with all of us working on unix based machines as well.

The only other option I see is hiding all ssh-agent functionality behind a cfg(unix) and forcing windows users to use the prompt method of key access 🙃

xla
xla reviewed Aug 30, 2021
View reviewed changes
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

Cool beans! Are we concerned that there are no destructive actions at the moment, e.g. profile delete or ssh remove?

link-crypto/Cargo.toml Outdated Show resolved Hide resolved
rad-profile/src/cli.rs Show resolved Hide resolved
rad-profile/src/cli/args.rs Outdated Show resolved Hide resolved
@FintanH
Copy link
Contributor Author

FintanH commented Aug 30, 2021

Cool beans! Are we concerned that there are no destructive actions at the moment, e.g. profile delete or ssh remove?

Very good point. Aside: it's funny ssh-add can also delete a key haha. But I don't see why we couldn't add destructive operations.

@kim
Copy link
Contributor

kim commented Aug 30, 2021 via email

@FintanH
Copy link
Contributor Author

FintanH commented Aug 30, 2021

From the agent, which is ephemeral storage. Think of it more as "unload key".

Ya, good way of putting it. I was just musing about the name of add :)

I believe it was mentioned in RFC #682 that secure long term storage of key
material is still a concern.

Long-term storage as well as rotation I would imagine

@FintanH FintanH force-pushed the finto/rad-profile branch from a04a954 to dd6af72 Compare August 30, 2021 11:11
@FintanH FintanH requested a review from a team September 1, 2021 12:40
xla
xla previously approved these changes Sep 1, 2021
View reviewed changes
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

🎤 🗑 📏 🛎

xla
xla reviewed Sep 7, 2021
View reviewed changes
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

This yak looks sufficiently shaved.

⛏ 📘 📙 🎠

xla
xla previously approved these changes Sep 8, 2021
View reviewed changes
Copy link
Contributor

@xla xla left a comment

Choose a reason for hiding this comment

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

⚱ 🖨 🏺 🍂

@FintanH
rad-profile: first draft of the rad-profile CLI
77f8c1e 
Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
@FintanH
Add profile to workspace
0a7d4c2 
Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
@FintanH
profile: use structopt/clap instead of argh
889fee1 
Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
@FintanH
rad-profile: implement ssh-add
358cf8c 
With the changes to thrussh and radicle-keystore, we can now add
a key to the ssh-agent.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
@FintanH
rad-profile: use the clib library
090b7ea 
Using the clib library for getting a prompt and keystore.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
@FintanH
rad-profile: rename to rad-profile
8566a9c 
Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
@FintanH
rad-profile: use generic client stream
626f78c 
We punt on what runtime should be used in the library and leave it to
the caller to decide.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
@FintanH
rad-profile: allow get to specify which profile to get
744f5c3 
Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
@FintanH
rad-exe: intergate rad-profile
719b594 
We add the rad-profile Args to the subcommands of rad-exe allowing the
binary to call into the rad-profile functionality.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
@FintanH FintanH merged commit 719b594 into master Sep 8, 2021
@FintanH FintanH deleted the finto/rad-profile branch September 8, 2021 09:51
@github-pages github-pages bot temporarily deployed to github-pages September 8, 2021 09:52 Inactive
@FintanH FintanH mentioned this pull request Sep 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xla xla xla left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FintanH @kim @xla