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

Specify work-path in SSH authorized keys #22754

Closed
wants to merge 4 commits into from

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Feb 4, 2023

Gusted added a commit to Gusted/gitea that referenced this pull request Feb 4, 2023
- Backport go-gitea#22754
  - This should prevent SSH failures from happening as described in go-gitea#19317
@yardenshoham yardenshoham added backport/done All backports for this PR have been created outdated/backport/v1.18 This PR should be backported to Gitea 1.18 labels Feb 4, 2023
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

I think this is a bodge but I guess we have to do it. It would be better to not have to depend on the work-path at all in when using gitea serv/hook and I'm surprised that this has sneaked in once again.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 15, 2023
@zeripath
Copy link
Contributor

I'll say this again:

I don't immediately understand why the work-path is even needed in gitea serv and I would much rather it was not necessary.

If anyone can determine why this has happened it would be very helpful because I'd rather get rid of this requirement COMPLETELY instead of adding more crap to the authorized_keys.

There may be a lot of keys in the authorized_keys file and this will make that file bigger and slower to parse. Whilst the authorizedkeyscommand functionality exists precisely to avoid slowdowns here - many people will not be aware and I'm fairly sure that our dockers still use authorized_keys instead of that command.

@wxiaoguang
Copy link
Contributor

I don't immediately understand why the work-path is even needed in gitea serv and I would much rather it was not necessary.

I do not understand either, is there any detailed comment about why the SSH failures happens?

If it's a rare case, site admin could fine tune the config according to the document: "Possible keys are: AppPath, AppWorkPath, CustomConf, CustomPath", there are even more options for them.

@lunny
Copy link
Member

lunny commented Feb 17, 2023

RepoRootPath's default value depending on AppDataPath, AppDataPath's default value depending on AppWorkPath.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 17, 2023

RepoRootPath's default value depending on AppDataPath, AppDataPath's default value depending on AppWorkPath.

Yup, there are too many path configs in Gitea's config system.

The question is: by default, these paths just work, the WorkPath is also auto detected by Gitea's binary file.

In my opinion, if and only if a site admin:

  • put Gitea binary in a separate directory
  • use relative path in app.ini
  • set WorkPath by env to start gitea web
  • then all other gitea commands also need to set WorkPath too.

In this case:

  • The site admin should be able to set SSH_AUTHORIZED_KEYS_COMMAND_TEMPLATE to a correct value with -w manually
  • All other gitea commands (like doctor?) also need to set the -w accordingly (but not only the serv)
  • Adding more default cli arguments to SSH_AUTHORIZED_KEYS_COMMAND_TEMPLATE seems not resolving the problem fundamentally (there are other commands ....)
  • To make the problem clear, from Gitea side:
    • Like 19317, add a clear error message to tell the user to use absolute paths in app.ini
    • Improve documents, tell users how to deal with such sitution if they use relative path in app.ini
    • Detect and deprecate relative paths in app.ini if the WorkPath doesn't match the binary path, or make Gitea accept WorkPath from config, then only one -c argument is needed for all cases.

That's why I think it's not necessary to make SSH_AUTHORIZED_KEYS_COMMAND_TEMPLATE more complex at the moment. Just my opinion, not blocker. Correct me if I am wrong.

@lunny
Copy link
Member

lunny commented Feb 18, 2023

Since the file is controlled by Gitea, I think we can merge this at the moment. And we can rewrite it after all paths have adjusted.

@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 19, 2023
@lunny
Copy link
Member

lunny commented Feb 19, 2023

Please resolve the conflicts.

@zeripath
Copy link
Contributor

How about we attempt to see if the AppDataPath is dependent on the WorkPath and only if it is dependent should we add the work-path option?

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.

Blocking per @zeripath 's comment. Mergers please feel free to dismiss this as needed. Just needed to set the blocker so it isn't merged without the additional investigation requested.

@techknowlogick techknowlogick added status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR and removed outdated/backport/v1.18 This PR should be backported to Gitea 1.18 labels Feb 25, 2023
@yardenshoham yardenshoham removed the backport/done All backports for this PR have been created label Feb 25, 2023
zeripath added a commit to zeripath/gitea that referenced this pull request Mar 4, 2023
If a user's `app.ini` contains a `APP_DATA_PATH` which refers to a
non-absolute path then `gitea serv` etc. become dependent on the
`AppWorkPath`.

`gitea serv` may then require `--work-path` to be set in the
`authorized_keys` if the `AppWorkPath` determined by `gitea web` and
`gitea serv` are different.

This would occur if `GITEA_WORK_DIR` is set, `--work-path` is used to
run `gitea web` or if the AppPath cannot be determined at start-up.

This PR adds some code to attempt to automatically determine if this is
necessary and adds some documentation to suggest adding `--work-path` to
the template.

This should prevent SSH failures from happening as described in go-gitea#19317

Replace go-gitea#22754

Signed-off-by: Andrew Thornton <art27@cantab.net>
@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label May 2, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 2, 2023

Quote my comments here (and close this one)

#23301 (comment)

#23301 (comment)

Instead of hacking and patching, we need a fundamental solution.

lunny pushed a commit that referenced this pull request Jun 21, 2023
# The problem

There were many "path tricks":

* By default, Gitea uses its program directory as its work path
* Gitea tries to use the "work path" to guess its "custom path" and
"custom conf (app.ini)"
* Users might want to use other directories as work path
* The non-default work path should be passed to Gitea by GITEA_WORK_DIR
or "--work-path"
* But some Gitea processes are started without these values
    * The "serv" process started by OpenSSH server
    * The CLI sub-commands started by site admin
* The paths are guessed by SetCustomPathAndConf again and again
* The default values of "work path / custom path / custom conf" can be
changed when compiling

# The solution

* Use `InitWorkPathAndCommonConfig` to handle these path tricks, and use
test code to cover its behaviors.
* When Gitea's web server runs, write the WORK_PATH to "app.ini", this
value must be the most correct one, because if this value is not right,
users would find that the web UI doesn't work and then they should be
able to fix it.
* Then all other sub-commands can use the WORK_PATH in app.ini to
initialize their paths.
* By the way, when Gitea starts for git protocol, it shouldn't output
any log, otherwise the git protocol gets broken and client blocks
forever.

The "work path" priority is: WORK_PATH in app.ini > cmd arg --work-path
> env var GITEA_WORK_DIR > builtin default

The "app.ini" searching order is: cmd arg --config > cmd arg "work path
/ custom path" > env var "work path / custom path" > builtin default


## ⚠️ BREAKING

If your instance's "work path / custom path / custom conf" doesn't meet
the requirements (eg: work path must be absolute), Gitea will report a
fatal error and exit. You need to set these values according to the
error log.



----

Close #24818
Close #24222
Close #21606
Close #21498
Close #25107
Close #24981
Maybe close #24503

Replace #23301
Replace #22754

And maybe more
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Jun 21, 2023
# The problem

There were many "path tricks":

* By default, Gitea uses its program directory as its work path
* Gitea tries to use the "work path" to guess its "custom path" and
"custom conf (app.ini)"
* Users might want to use other directories as work path
* The non-default work path should be passed to Gitea by GITEA_WORK_DIR
or "--work-path"
* But some Gitea processes are started without these values
    * The "serv" process started by OpenSSH server
    * The CLI sub-commands started by site admin
* The paths are guessed by SetCustomPathAndConf again and again
* The default values of "work path / custom path / custom conf" can be
changed when compiling

# The solution

* Use `InitWorkPathAndCommonConfig` to handle these path tricks, and use
test code to cover its behaviors.
* When Gitea's web server runs, write the WORK_PATH to "app.ini", this
value must be the most correct one, because if this value is not right,
users would find that the web UI doesn't work and then they should be
able to fix it.
* Then all other sub-commands can use the WORK_PATH in app.ini to
initialize their paths.
* By the way, when Gitea starts for git protocol, it shouldn't output
any log, otherwise the git protocol gets broken and client blocks
forever.

The "work path" priority is: WORK_PATH in app.ini > cmd arg --work-path
> env var GITEA_WORK_DIR > builtin default

The "app.ini" searching order is: cmd arg --config > cmd arg "work path
/ custom path" > env var "work path / custom path" > builtin default

## ⚠️ BREAKING

If your instance's "work path / custom path / custom conf" doesn't meet
the requirements (eg: work path must be absolute), Gitea will report a
fatal error and exit. You need to set these values according to the
error log.

----

Close go-gitea#24818
Close go-gitea#24222
Close go-gitea#21606
Close go-gitea#21498
Close go-gitea#25107
Close go-gitea#24981
Maybe close go-gitea#24503

Replace go-gitea#23301
Replace go-gitea#22754

And maybe more
# Conflicts:
#	cmd/web.go
@Gusted Gusted deleted the workpath-ssh-authorizedkeys branch June 24, 2023 13:14
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants