-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support ssh public keys configuration #10573
Conversation
d69e014
to
2b2c8da
Compare
56aa88c
to
461e496
Compare
a8855a4
to
b465867
Compare
/hold |
started the job as gitpod-build-hw-ssh-keys-s.30 because the annotations in the pull request description changed |
started the job as gitpod-build-hw-ssh-keys-s.31 because the annotations in the pull request description changed |
started the job as gitpod-build-hw-ssh-keys-s.32 because the annotations in the pull request description changed |
I'll not change the description anymore until one build success, since werft is crazy now... 😱 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, LGTM! 👍
/werft run 👍 started the job as gitpod-build-hw-ssh-keys-s.45 |
@mustard-mh wanted to check out UI, but werft fails |
/werft run 👍 started the job as gitpod-build-hw-ssh-keys-s.46 |
6edd014
to
fef5040
Compare
Resolved merge conflict /werft run with-clean-slate-deployment=true 👍 started the job as gitpod-build-hw-ssh-keys-s.48 |
@mustard-mh Could you clean deploy another preview environment or delete some users because we've reached the limit of 10 users and no new users can sign up. |
fef5040
to
725c071
Compare
@gtsiolis Rebased to main again, since some commits may fix this problem (db have only two builtin users) |
/werft run 👍 started the job as gitpod-build-hw-ssh-keys-s.50 |
works now @gtsiolis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, @mustard-mh! 🔮
Took another look at this and left a few more comments below. 👀
<a href="/docs/configure/ssh" target="gitpod-ssh-doc" className="gp-link"> | ||
Learn more | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: Do we need this help link here? Asking because we also link below to the same page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can keep that, they are different part link
try { | ||
getData(value); | ||
} catch (e) { | ||
return "Key is invalid. You must supply a key in OpenSSH public key format."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue(non-blocking): While we catch some cases, we still allow users adding invalid keys. Is this known? If needed, let's open a follow-up issue to resolve this.
)} | ||
{!hasSSHKey && selectSSHKey && ( | ||
<Alert type="warning" className="whitespace-normal"> | ||
You don't have any public SSH keys in your Gitpod account. You can{" "} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: When you don't have an SSH key in place, you still see an SSH command below to connect, which asks for a password. Should we drop (hide) the SSH command below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we should allow to copy it always. If a user adds keys following a warning then they can use it immediately. Otherwise they need to reopen the dialog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we should allow to copy it always. If a user adds keys following a warning then they can use it immediately. Otherwise they need to reopen the dialog
Ah, I see. Then the warning dialog becomes obsolete or no longer relevant after adding a key, right?
Maybe it's not worth doing so, but what do you think of rephrasing the copy then to the following when there are no keys in place:
BEFORE
The following shell command can be used to SSH into this workspace with a ssh key.
AFTER
The following shell command can be used to SSH into this workspace with a ssh key, once you add an SSH key.
Otherwise, let's leave this as is and ignore the comment above in #10573 (comment). ✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with both. I think they will read a warning when prompted for a password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEAL—Let's go with any option for now.
UI looks good, but then I try first time user flow I cannot connect:
Is it because newly installed keys are not automatically propagated to workspaces? Hm it does not work after restart either. |
could your use |
@iQQBot I did everything again and it worked now. So probably some mistake on my side. Oh, I know why, I did not exit another ssh session 😆 so I was in Gitpod container. |
rebase to main and double-checked everything, now we are in gen51 it's time to merge /unhold |
Description
This PR works on allowing users to upload their own ssh public key to Gitpod, make copy-paste ssh connect more conventional, and make sharing of ssh connect command more safety
Link of Figma
TODO
Functions for late used time?Related Issue(s)
Relate #9932
How to test
Modal of ssh copy-paste always shows
SSH Key
as the default selection, if the user has no public key uploaded, it will show a warning in this section.conn A
ssh key
, getconn B
conn C
which use the same command like conn B, will not works - ask a passwordRelease Notes
Documentation
Werft options: