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

Autodetermine if work-path needs to be specified in SSH authorized_keys #23301

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented 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 #19317

Replace #22754

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 <[email protected]>
@zeripath zeripath added type/bug outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels Mar 4, 2023
@zeripath zeripath added this to the 1.20.0 milestone Mar 4, 2023
@wxiaoguang
Copy link
Contributor

I think it's better to simplify the Gitea's path system, instead of making it more complex. It's also worth to introducing breaking changes to simplify the path system.

I can't find a reason why the paths in config should be relative path, and why these paths should be controlled by various variables.

If some people really need the relative paths in config, just tell them to write %AppBinPath/data/.... in the future?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 4, 2023
@lunny
Copy link
Member

lunny commented Mar 5, 2023

Could we assume the base path when it is a relative path? And of course, we can give it a warning or deprecated message. I also don't like introduce a new configuration item.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 5, 2023

The old WorkPath etc are already the "assumed base path" , which already cause many problems. That mechanism was just patched and patched in history, became more and more complex. I believe at the moment few maintainers could tell how these paths work clearly without spending a lot of time on reading code.

In the future, Gitea could do the following steps to clean the legacy "path problems" fundamentally, without breaking most users:

  1. Use ini package to update the app.ini, write absolute paths into it on startup.
  2. If the app.ini is read-only, show Deprecated warning current release and do Fatal on next release.
  3. If some site admin really needs to use relative path, introduce %AppBinPath% for app.ini.

By doing so, most users could upgrade to latest Gitea without any problem. If a site admin set app.ini to read-only, or they need to use relative path, they should be very sure about what they are doing and they must already have the ability to handle the breaking changes.

@codecov-commenter

This comment was marked as off-topic.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 2, 2023

Not only one user reports the WorkDir related bugs: Gitea's path system quirk and bug #24222 and more.

I do not think we should do more hacking and patching, the Gitea's path quirk should be fixed fundamentally.

@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
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 4, 2023
@wxiaoguang wxiaoguang removed this from the 1.20.0 milestone May 27, 2023
@wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang closed this Jun 18, 2023
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
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 16, 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/need 2 This PR needs two approvals by maintainers to be considered for merging. outdated/backport/v1.19 This PR should be backported to Gitea 1.19 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants