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

restrict certificate type for builtin SSH server #26789

Merged
merged 2 commits into from
Sep 1, 2023

Conversation

earl-warren
Copy link
Contributor

@earl-warren earl-warren commented Aug 29, 2023

  • While doing some sanity checks over OpenSSH's code for how they handle certificates authentication. I stumbled on an condition that checks the certificate type is really an user certificate on the server-side authentication. This checks seems to be a formality and just for the sake of good domain seperation, because an user and host certificate don't differ in their generation, verification or flags that can be included.
  • Add this check to the builtin SSH server to stay close to the unwritten SSH specification.
  • This is an breaking change for setups where the builtin SSH server is being used and for some reason host certificates were being used for authentication.

(cherry picked from commit de35b141b79a3d6efe2127ed2c73fd481515e481)

Refs: https://codeberg.org/forgejo/forgejo/pulls/1172

⚠️ BREAKING ⚠️

The built-in SSH server will now only accept SSH user certificates, not server certificates. This behaviour matches OpenSSH.

- While doing some sanity checks over OpenSSH's code for how they
handle certificates authentication. I stumbled on an condition that
checks the certificate type is really an user certificate on the
server-side authentication. This checks seems to be a formality and just
for the sake of good domain seperation, because an user and host
certificate don't differ in their generation, verification or flags that
can be included.
- Add this check to the builtin SSH server to stay close to the
unwritten SSH specification.
- This is an breaking change for setups where the builtin SSH server is
being used and for some reason host certificates were being used for
authentication.

(cherry picked from commit de35b141b79a3d6efe2127ed2c73fd481515e481)

Refs: https://codeberg.org/forgejo/forgejo/pulls/1172
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 29, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 29, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 29, 2023
Comment on lines +195 to +196
log.Warn("Certificate Rejected: Not a user certificate")
log.Warn("Failed authentication attempt from %s", ctx.RemoteAddr())
Copy link
Member

Choose a reason for hiding this comment

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

could you merge these in one.

Copy link
Member

Choose a reason for hiding this comment

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

Think it's fine. Go is not really made for multi-line strings.

@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 Aug 29, 2023
@KN4CK3R KN4CK3R added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Aug 29, 2023
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 1, 2023
@silverwind
Copy link
Member

Breaking label added, good to go imho.

@silverwind silverwind enabled auto-merge (squash) September 1, 2023 13:17
@silverwind
Copy link
Member

silverwind commented Sep 1, 2023

In the unlikely event this is breaking someone's workflow, I guess a option could be introduced to skip this check, but as I see it this is not configurable in OpenSSH either, so we likely shouldn't bother either.

@silverwind silverwind added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Sep 1, 2023
@silverwind silverwind merged commit 4ab8e56 into go-gitea:main Sep 1, 2023
23 checks passed
@GiteaBot GiteaBot added this to the 1.21.0 milestone Sep 1, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 1, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 1, 2023
* giteaoffical/main: (22 commits)
  Use case-insensitive regex for all webpack assets (go-gitea#26867)
  restrict certificate type for builtin SSH server (go-gitea#26789)
  feat(API): add secret deletion functionality for repository (go-gitea#26808)
  Avoid double-unescaping of form value (go-gitea#26853)
  Move web/api context related testing function into a separate package (go-gitea#26859)
  Remove some unused CSS styles (go-gitea#26852)
  [skip ci] Updated translations via Crowdin
  Minor dashboard tweaks, fix flex-list margins (go-gitea#26829)
  Update team invitation email link (go-gitea#26550)
  Redirect from `{repo}/issues/new` to `{repo}/issues/new/choose` when blank issues are disabled (go-gitea#26813)
  Remove "TODO" tasks from CSS file (go-gitea#26835)
  User details page (go-gitea#26713)
  Render code blocks in repo description (go-gitea#26830)
  Remove joinPaths function (go-gitea#26833)
  Remove polluted `.ui.right` (go-gitea#26825)
  Sync tags when adopting repos (go-gitea#26816)
  rm comment about hugo (go-gitea#26832)
  Fix filename for .spectral.yaml (go-gitea#26828)
  [skip ci] Updated translations via Crowdin
  Check blocklist for emails when adding them to account (go-gitea#26812)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 30, 2023
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. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants