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 ssh certificate support #12281

Merged
merged 47 commits into from
Oct 11, 2020
Merged

Add ssh certificate support #12281

merged 47 commits into from
Oct 11, 2020

Conversation

42wim
Copy link
Member

@42wim 42wim commented Jul 20, 2020

Fixes #12224

New KeyType KeyTypePrincipal and struct PrincipalKey is created.
It's reusing the PublicKey table, changes are:

  • content/name is the same
  • fingerprint is empty
  • has KeyTypePrincipal
  • creates authorized_principals file instead of authorized_keys

PublicKey is modified so it doesn't write KeyTypes with KeyTypePrincipal to the authorized_keys.

New section is added to the SSH / GPG keys section.

image

The sshd_config is also modified by adding a AuthorizedPrincipalsFile managed by gitea and a TrustedUserCAKeys pointing to /data/ssh/trusted-user-ca-keys.pem which should be managed by the gitea administrator. (this contains the publickey of the CA that signs your ssh certificates)

Updates to the original PR:

  • built-in ssh server supported
  • SSH_TRUSTED_USER_CA_KEYS option has been added to the config for specifying your CA keys (and have gitea manage them)
  • SSH_TRUSTED_USER_CA_KEYS_FILENAME has been added to the config for specifying the filename gitea will manage the CA keys in
  • Extra SSH_AUTHORIZED_PRINCIPALS_ALLOW has been added to config which by default only allows principals that are equal
    to the user's email or username (to avoid malicious activity). Option to allow anything is also available
  • Option to update the authorized principals file has been added to the admin dashboard (like the authorized keys file)

@lafriks
Copy link
Member

lafriks commented Jul 20, 2020

Support for this should be also added to built-in ssh server or at least support for this should be hidden if built-in ssh server is u

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 20, 2020
@42wim
Copy link
Member Author

42wim commented Jul 20, 2020

@lafriks good point, just updated the PR by adding a ErrSSHBuiltinNotSupported error and check.

It now shows it's not supported when running the builtin server. (and disabled when ssh is disabled)

image

@lafriks
Copy link
Member

lafriks commented Jul 20, 2020

It would be great to add option to manage also CA certs in Gitea admin section to easier add support to built in ssh server later on

@42wim
Copy link
Member Author

42wim commented Jul 20, 2020

@lafriks yes, but that should be for another PR :)

@lafriks
Copy link
Member

lafriks commented Jul 20, 2020

If it is left out it will than later on become breaking change as it would not be possible to migrate automatically to new one

@lafriks lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jul 20, 2020
@lafriks lafriks added this to the 1.13.0 milestone Jul 20, 2020
@42wim
Copy link
Member Author

42wim commented Jul 21, 2020

I've added support for the builtin SSH.

Added a new key SSH_TRUSTED_USER_CA_KEYS which expects comma-separated CA keys.
This works for the builtin server now, but the external one still has a separate file.

Should I let gitea write into /data/ssh/trusted-user-ca-keys.pem based upon the contents of SSH_TRUSTED_USER_CA_KEYS (when the builtin ssh is not used) or should this be the responsibility of the administrator?

@lafriks
Copy link
Member

lafriks commented Jul 21, 2020

I would say that for extendability later on it would be better if Gitea would generate that file from setting value

@42wim
Copy link
Member Author

42wim commented Jul 22, 2020

Updated and the ca file is now getting generated.

models/ssh_key.go Outdated Show resolved Hide resolved
models/ssh_key.go Outdated Show resolved Hide resolved
@silverwind
Copy link
Member

AuthorizedPrincipalsFile requires OpenSSH 6.9 or higher which notably excludes CentOS 6 which has 5.3. I don't think we state a minimum OpenSSH version yet, I guess this PR is a good reason to do so.

@42wim
Copy link
Member Author

42wim commented Jul 26, 2020

We're actually using certificates on centos 6 too :-)
It's been backported since 2013

* Fri Jul 19 2013 Petr Lautrbach <plautrba@redhat.com> 5.3p1-86
  - Add support for certificate key types for users and hosts (#906872)

ed25519 support on the other hand..

@silverwind
Copy link
Member

silverwind commented Jul 26, 2020

Yeah, I too once manually compiled openssh just for ed25519 😉

models/ssh_key.go Outdated Show resolved Hide resolved
models/ssh_key.go Outdated Show resolved Hide resolved
models/ssh_key.go Outdated Show resolved Hide resolved
@techknowlogick
Copy link
Member

I'm 99% sure I'm doing something wrong when testing this using opensshd in docker, but am having troubles figuring out what I'm doing wrong. Is anyone else able to test this scenario?

@42wim
Copy link
Member Author

42wim commented Sep 27, 2020

@techknowlogick it was the 1% my fault, sorry :(
I made a typo in the docker sshd_config template. It should work correctly now.

As this depends on the content of TrustedUserCAKeys we should allow all
signature algorithms as admins can choose the specific algorithm on their
signing CA
Co-authored-by: Lauris BH <lauris@nix.lv>
@6543
Copy link
Member

6543 commented Sep 29, 2020

pleace resolve conflicts :)

@lafriks
Copy link
Member

lafriks commented Oct 5, 2020

@techknowlogick can you review again please?

@6543
Copy link
Member

6543 commented Oct 5, 2020

It would be nice to ad some Docs about ssh-certificate (tld howto and some "read more" links ...)

@42wim
Copy link
Member Author

42wim commented Oct 7, 2020

@6543 something like #12281 (comment) ?
Where should I add those docs?

@6543
Copy link
Member

6543 commented Oct 10, 2020

@42wim would be a nice to have 👍

I think you can reuse parts of this comment ...

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

nice ...!

still some docs like you described in your comment would be better ...

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 10, 2020
@codecov-io
Copy link

Codecov Report

Merging #12281 into master will decrease coverage by 0.08%.
The diff coverage is 15.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12281      +/-   ##
==========================================
- Coverage   42.60%   42.52%   -0.09%     
==========================================
  Files         673      673              
  Lines       73862    74114     +252     
==========================================
+ Hits        31470    31514      +44     
- Misses      37275    37483     +208     
  Partials     5117     5117              
Impacted Files Coverage Δ
cmd/serv.go 2.66% <0.00%> (-0.06%) ⬇️
modules/ssh/ssh.go 46.32% <0.00%> (-8.94%) ⬇️
routers/private/serv.go 29.30% <0.00%> (ø)
models/ssh_key.go 45.11% <6.76%> (-9.21%) ⬇️
routers/user/setting/keys.go 10.79% <11.11%> (+0.11%) ⬆️
modules/setting/setting.go 46.24% <31.81%> (-1.38%) ⬇️
models/user.go 53.52% <33.33%> (-0.06%) ⬇️
modules/cron/tasks_extended.go 69.47% <66.66%> (-0.30%) ⬇️
modules/upload/upload.go 77.35% <83.33%> (+0.76%) ⬆️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
... and 16 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 7eb8460...9a896de. Read the comment docs.

@techknowlogick techknowlogick merged commit 9066d09 into go-gitea:master Oct 11, 2020
@techknowlogick
Copy link
Member

docs could be a different PR

@6543
Copy link
Member

6543 commented Oct 11, 2020

@42wim ☝️ :)

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: support ssh certificates
10 participants