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 separate SSH_USER config option #17584

Merged
merged 2 commits into from Feb 7, 2022
Merged

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Nov 8, 2021

This lets the user shown in SSH URLs be customised even when the built-in server is disabled. In my case, I customised my SSH server to accept logins from a username other than the one that gitea is logged into, and the way I currently do this is by running the built-in SSH server to override the displayed URL without actually using it. I think that this method is much cleaner, and doesn't add too much complexity to the configuration. In fact, IMHO, it actually simplifies things for non-niche users as well.

The value of this will default to the BUILTIN_SSH_SERVER_USER if not provided, which will default to RUN_USER if that's also not provided, meaning that for most people this will "just work." However, I did put some caveats below.

I technically filed a ticket for this a while back but it got closed due to confusion about the actual implementation, so, I don't think it's worth it to link it here. But if anyone is curious, I can probably find it.

⚠️ BREAKING ⚠️

This is technically a breaking change, since people who have modified BUILTIN_SSH_SERVER_USER but don't actually have the built-in server running will have their URLs show the value they chose for that, which will presumably not work. Initially I considered modifying this change to default to BUILTIN_SSH_SERVER_USER only when the built-in server was enabled, and RUN_USER otherwise, but I ultimately chose to abandon this approach since:

  1. It's more-complicated in both code and explanation.
  2. The number of people who have made these changes is likely small
  3. The people who have BUILTIN_SSH_SERVER_USER set to something other than RUN_USER without the built-in server running probably just forgot to remove that section from their config, and probably should fix that
  4. Explaining the extra default rules adds more complexity to the ruleset when it makes more sense to just let the variables default in the order of SSH_USER -> BUILTIN_SSH_SERVER_USER -> RUN_USER.

Clarification of my specific use case

This is mostly for the curious people who are wondering how I have this set up, and is not important to the PR itself. Here, I explain how I allow SSH to connect to gitea via a user different than the one gitea uses.

  1. I'm running on Arch Linux where gitea is running under a gitea user and group.
  2. For the sake of simplicity, let's say that the extra user for SSH is g.
  3. I modified SSH_AUTHORIZED_KEYS_COMMAND_TEMPLATE to run a custom script that wraps the gitea serv key command. (Before this option was added, I wrapped the authorized keys command in my SSH config directly to modify the output.)
  4. The g user has no permissions, although in the sudoers file it's allowed to run commands as the gitea user without a password and while passing its environment.
  5. My wrapper script runs sudo -Eu gitea gitea serv key-$1, where $1 is the passed key ID. It's only allowed to execute as the g user.
  6. This user is normally not allowed to connect via SSH, but is allowed to connect via SSH on a separate IP and port only to execute these commands.

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

One tiny nitpick but otherwise looks good. Thanks for this PR!

@@ -78,6 +78,9 @@ RUN_MODE = ; prod
;; Domain name to be exposed in clone URL
;SSH_DOMAIN = %(DOMAIN)s
;;
;; User to be exposed in clone URL. If blank, then it is the value of BUILTIN_SSH_SERVER_USER.
;SSH_USER =
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 match what you have in the markdown docs here %(...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main reason I didn't was because the style for the docs in both places is inconsistent, and I was mostly copying what I saw. Can still make the change if you think it'll be better that way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @techknowlogick since it's been a few days, just wanna ask whether you want me to make the change or not

Copy link
Member

Choose a reason for hiding this comment

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

Apologies for the disappearance.

Could you make these changes? I know that there are inconsistencies between these two files, although hopefully they will be aligned, and to make the effort of that future person less that is why I'm requesting they be aligned.

I don't have super strong feelings about this, and so I will add a LGTM and leave it up to you if you wish to change it so I'm not blocking this PR from being merged :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for my disappearance!

Reading this again, I'm not 100% sure what you meant by copying the markdown docs, so, I did what made the most sense to me and put the default directly as the variable value. Also did the same for SSH_BUILTIN_SERVER_USER as well since this directly uses that. Let me know if this is what you'd prefer.

@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 8, 2021
@lunny lunny added pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Nov 22, 2021
@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 Nov 22, 2021
@clarfonthey
Copy link
Contributor Author

Sorry for taking so long; I rebased this on the latest main and made the docs changes that were requested, at least what I think was requested. Let me know if you have any extra feedback.

@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 Feb 7, 2022
@techknowlogick techknowlogick merged commit 99d14f6 into go-gitea:main Feb 7, 2022
@clarfonthey clarfonthey deleted the ssh_user branch February 7, 2022 22:35
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 8, 2022
* giteaofficial/main: (28 commits)
  Added auto-save whitespace behavior if it changed manually (go-gitea#15566)
  Support custom ACME provider (go-gitea#18340)
  Refactor i18n, use Locale to provide i18n/translation related functions (go-gitea#18648)
  Only request write when necessary (go-gitea#18657)
  [skip ci] Updated translations via Crowdin
  Add separate SSH_USER config option (go-gitea#17584)
  Be more lenient with label colors (go-gitea#17752)
  remove redundant call to UpdateRepoStats during migration (go-gitea#18591)
  more repo dump/restore tests, including pull requests (go-gitea#18621)
  No longer show the db-downgrade SQL in production (go-gitea#18653)
  Fix the missing i18n key for update checker (go-gitea#18646)
  Update gitea-vet (go-gitea#18640)
  Future proof for 1.18 (go-gitea#18644)
  Add `contrib/upgrade.sh` (go-gitea#18286)
  If rendering has failed due to a net.OpError stop rendering (go-gitea#18642)
  Delete old git.NewCommand() and use it as git.NewCommandContext() (go-gitea#18552)
  Update JS dependencies (go-gitea#18636)
  fix commits_list_small.tmpl (go-gitea#18641)
  Fix `make fmt` and `make fmt-check` (go-gitea#18633)
  Frontport of changelog for v1.16.1 (go-gitea#18615)
  ...
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
Co-authored-by: zeripath <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
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! type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants