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

Fix ssh deploy and user key constraints (#1357) #5939

Merged
merged 6 commits into from Feb 3, 2019

Conversation

6 participants
@zeripath
Copy link
Contributor

zeripath commented Feb 2, 2019

My understanding is that the intention is:

  1. A key can either be an ssh user key or a deploy key. It cannot be both.
  2. If a key is a user key - it can only be associated with one user.
  3. If a key is a deploy key - it can be used in multiple repositories and the permissions it has on those repositories can be different.
  4. If a repository is deleted, its deploy keys must be deleted too.

We currently don't enforce any of this and multiple repositories access with different permissions doesn't work at all. This PR enforces the following constraints:

  • You should not be able to add the same user key as another user
  • You should not be able to add a ssh user key which is being used as a deploy key
  • You should not be able to add a ssh deploy key which is being used as a user key
  • If you add an ssh deploy key to another repository you should be able to use it in different modes without losing the ability to use it in the other mode.
  • If you delete a repository you must delete all its deploy keys.

Fix #1357

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 2, 2019

Codecov Report

Merging #5939 into master will increase coverage by 0.14%.
The diff coverage is 47.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5939      +/-   ##
==========================================
+ Coverage   38.54%   38.68%   +0.14%     
==========================================
  Files         330      330              
  Lines       48526    48573      +47     
==========================================
+ Hits        18703    18792      +89     
+ Misses      27115    27061      -54     
- Partials     2708     2720      +12
Impacted Files Coverage Δ
routers/repo/setting.go 10.35% <0%> (-0.07%) ⬇️
routers/api/v1/repo/key.go 20.5% <0%> (+3.83%) ⬆️
routers/private/internal.go 56.45% <100%> (+0.71%) ⬆️
models/repo.go 47.24% <15.38%> (-0.22%) ⬇️
routers/private/key.go 31.88% <35.71%> (-11.76%) ⬇️
models/ssh_key.go 45.48% <64.44%> (+2.72%) ⬆️
modules/lfs/locks.go 47.23% <0%> (-0.51%) ⬇️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 634cbaa...6d56c3b. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 label Feb 2, 2019

@zeripath zeripath modified the milestones: 1.7.2, 1.8.0 Feb 2, 2019

@techknowlogick
Copy link
Member

techknowlogick left a comment

Looks great! One nit, but I'm ok if this PR remains same.

cmd/serv.go Outdated
fail("Key access denied", "Deploy key access denied: [key_id: %d, repo_id: %d]", key.ID, repo.ID)
}

if deployKey.Mode < requestedMode {
fail("Key permission denied", "Cannot push with deployment key: %d to repo_id: %d", key.ID, repo.ID)

This comment has been minimized.

@techknowlogick

techknowlogick Feb 3, 2019

Member

One tiny nit: I know this was the error message before your change, but could you update it to include something about this specific deploy key being read-only?

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Feb 3, 2019

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Feb 3, 2019

The deploykey should be deleted from public_key table if last repo which use it is deleted?

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Feb 3, 2019

Hi @lunny the call to deleteDeployKey manages the deletion of the PublicKey entry as necessary.

zeripath added some commits Feb 2, 2019

Fingerprint should be indexed
Signed-off-by: Andrew Thornton <art27@cantab.net>
Add integration tests for ssh keys
Signed-off-by: Andrew Thornton <art27@cantab.net>

@zeripath zeripath force-pushed the zeripath:issue-1357-fix-deploy-keys-semantics branch from 01a9fad to d2e9331 Feb 3, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Feb 3, 2019

OK I've added some tests and changed the failure message as per @techknowlogick

If you look at the tests - I've created them in a more declarative style similar to that of TestGit.

@lafriks

lafriks approved these changes Feb 3, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Feb 3, 2019

@zeripath zeripath merged commit 01c10a9 into go-gitea:master Feb 3, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@zeripath zeripath deleted the zeripath:issue-1357-fix-deploy-keys-semantics branch Feb 3, 2019

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Feb 4, 2019

please backport to release/v1.7

zeripath added a commit to zeripath/gitea that referenced this pull request Feb 4, 2019

Fix ssh deploy and user key constraints (go-gitea#1357) (go-gitea#5939)
1. A key can either be an ssh user key or a deploy key. It cannot be both.
2. If a key is a user key - it can only be associated with one user.
3. If a key is a deploy key - it can be used in multiple repositories and the permissions it has on those repositories can be different.
4. If a repository is deleted, its deploy keys must be deleted too.

We currently don't enforce any of this and multiple repositories access with different permissions doesn't work at all. This PR enforces the following constraints:

- [x] You should not be able to add the same user key as another user
- [x] You should not be able to add a ssh user key which is being used as a deploy key
- [x] You should not be able to add a ssh deploy key which is being used as a user key
- [x] If you add an ssh deploy key to another repository you should be able to use it in different modes without losing the ability to use it in the other mode.
- [x] If you delete a repository you must delete all its deploy keys.

Fix go-gitea#1357

@zeripath zeripath referenced this pull request Feb 4, 2019

Merged

Fix ssh deploy and user key constraints (#1357) (#5939) #5966

5 of 5 tasks complete

zeripath added a commit that referenced this pull request Feb 4, 2019

Fix ssh deploy and user key constraints (#1357) (#5939) (#5966)
Backport of #5939 

1. A key can either be an ssh user key or a deploy key. It cannot be both.
2. If a key is a user key - it can only be associated with one user.
3. If a key is a deploy key - it can be used in multiple repositories and the permissions it has on those repositories can be different.
4. If a repository is deleted, its deploy keys must be deleted too.

We currently don't enforce any of this and multiple repositories access with different permissions doesn't work at all. This PR enforces the following constraints:

- [x] You should not be able to add the same user key as another user
- [x] You should not be able to add a ssh user key which is being used as a deploy key
- [x] You should not be able to add a ssh deploy key which is being used as a user key
- [x] If you add an ssh deploy key to another repository you should be able to use it in different modes without losing the ability to use it in the other mode.
- [x] If you delete a repository you must delete all its deploy keys.

Fix #1357

@zeripath zeripath referenced this pull request Feb 17, 2019

Closed

Deployment keys should not collide with user keys #938

2 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment