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

[ws-proxy, ws-manager] support user upload ssh public key #10617

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

iQQBot
Copy link
Contributor

@iQQBot iQQBot commented Jun 13, 2022

Description

[ws-proxy, ws-manager] support user upload ssh public key
This is part of Epic: Upload users SSH keys to Gitpod #9932
It only contains workspace components change

Related Issue(s)

Relate #9932

How to test

Be sure to check to see if previous authentication has NOT been breached

  1. start a workspace in this preview environment, and get this workspace instanceID (e.g. 90147062-a363-44d1-9516-e7b9bc07ac54)
  2. start a workspace using this branch (in order to get test gpctl) the following command will be execute at this workspace
# replace 90147062-a363-44d1-9516-e7b9bc07ac54 as your workspace Instance ID
# build gpctl
cd /workspace/gitpod/dev/gpctl && go build

# generate a ssh key pair
ssh-keygen

# update local ssh
./gpctl workspaces update-ssh-keys 90147062-a363-44d1-9516-e7b9bc07ac54 ~/.ssh/id_rsa.pub

# check annotation with kubectl
kubectl describe pods ws-90147062-a363-44d1-9516-e7b9bc07ac54 | grep "gitpod.io/sshPublicKeys" -C 3

# test ssh into this workspace, remember don't include ownerToken
ssh workspaceID@workspaceID.ssh.ws.pd-use-userpk2.preview.gitpod-dev.com

Release Notes

NONE

Documentation

/werft analytics=segment|TEZnsG4QbLSxLfHfNieLYGF4cDwyFWoe

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 13, 2022

/hold
need remove debug commit

@iQQBot iQQBot marked this pull request as ready for review June 13, 2022 09:02
@iQQBot iQQBot requested a review from a team June 13, 2022 09:02
@iQQBot iQQBot requested review from aledbf and sagor999 as code owners June 13, 2022 09:02
@iQQBot iQQBot requested a review from a team June 13, 2022 09:02
@github-actions github-actions bot added team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Jun 13, 2022
@geropl geropl self-assigned this Jun 13, 2022
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Reviewed the core.proto changes, and those LGTM. 👍
(did NOT test)

/hold to ensure others can review as well

@mustard-mh mustard-mh self-requested a review June 13, 2022 09:42
@akosyakov
Copy link
Member

akosyakov commented Jun 13, 2022

@iQQBot Should not How to test have tests for backward compatibility with current approach for all interfaces?

or you mean Be sure to check to see if previous authentication has NOT been breached implies it? and reviewer should know how to test it? Maybe we should have a Notion page for manual smoke testing of SSH client and share it on PRs and workspace team to verify after deployment?

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 13, 2022

@iQQBot Should not How to test have tests for backward compatibility with current approach for all interfaces?

I put a very large title in how to test section Be sure to check to see if previous authentication has NOT been breached , but I assume your are already known how to test previous authentication

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 13, 2022

@mustard-mh Thanks for your point, I changed the code

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 14, 2022

will add some CDWP test and remote debug commit, then merge it!

@mustard-mh
Copy link
Contributor

mustard-mh commented Jun 15, 2022

/hold

Why only allow to add? How about set keys? (since we can delete keys in dashboard)

If we allow set keys after workspaces start, is that mean we need to kick conn out if their public keys have been deleted

Need more changes or explain that keys only work on the moment workspaces start and remove add function

Sorry, I'm working on dashboard and forget that what ws-manger-api provided is update, not add (add is in dashboard)

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 15, 2022

Why only allow to add? How about set keys? (since we can delete keys in dashboard)
If we allow set keys after workspaces start, is that mean we need to kick conn out if their public keys have been deleted
Need more changes or explain that keys only work on the moment workspaces start and remove add function

Any management of public keys will only work on newly established connections, adding or deleting public keys will real-time change the annotation of the workspace pod (need server calls UpdateSSHKey API), if the user deletes a key then the connection corresponding to this key will not be kicked out, just that no new connection can be auth with this key.

@mustard-mh
Copy link
Contributor

Why only allow to add? How about set keys? (since we can delete keys in dashboard)
If we allow set keys after workspaces start, is that mean we need to kick conn out if their public keys have been deleted
Need more changes or explain that keys only work on the moment workspaces start and remove add function

Any management of public keys will only work on newly established connections, adding or deleting public keys will real-time change the annotation of the workspace pod (need server calls UpdateSSHKey API), if the user deletes a key then the connection corresponding to this key will not be kicked out, just that no new connection can be auth with this key.

I know what this PR doing, but want to know why we choose this way, and this behavior needs to be documented: Remove will not work for all started workspaces

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 15, 2022

I know what this PR doing, but want to know why we choose this way, and this behavior needs to be documented: Remove will not work for all started workspaces

This does not mean Remove will not work for all started workspaces

Remove will apply to all started workspaces, by updating annotation, i.e. Deleted ssh keys are not allowed to connect to these workspaces again.

However, connections that have been successfully established will not be affected, this behavior is consistent with openssh-server i.e. If you connect to a server via ssh, edit your ~/.ssh/authorized_keys in your server remove your public key, it will not affect this connection, but if you want to start a new ssh client, it will fail.

I will put this in the document. or put this as warning dialog when users try to delete their public key

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 15, 2022

/werft run

👍 started the job as gitpod-build-pd-use-userpk2.20
(with .werft/ from main)

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 15, 2022

Also test it using multi public keys, see screenshot key1, key2 is uploaded, key3 is new generate and not upload

image
image
image
image

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 15, 2022

fix a bug that let segment missing some trace

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 15, 2022

I would like to merge this

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 15, 2022

....Ah, again Conflicting files

@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 15, 2022

/werft run

👍 started the job as gitpod-build-pd-use-userpk2.23
(with .werft/ from main)

Co-authored-by: Huiwen <[email protected]>
Co-authored-by: Christian Weichel <[email protected]>
Co-authored-by: Pavel Tumik <[email protected]>
@iQQBot
Copy link
Contributor Author

iQQBot commented Jun 15, 2022

/unhold

@roboquat roboquat merged commit fe6e39e into main Jun 15, 2022
@roboquat roboquat deleted the pd/use-userpk2 branch June 15, 2022 17:28
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production labels Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production release-note-none size/XXL team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants