[BUG] Blobless clones don't work because .gitconfig isn't used for SSH clients #869

Closed
opened 2023-06-17 03:18:02 +00:00 by DanielGibson · 10 comments

Dependency references

Context

  • Can you reproduce the problem on Forgejo Next?
    I think this requires access to the server itself to reproduce
  • Forgejo version (or commit ref): 1.19.3
  • Git version: 2.39.2
  • Operating system: Linux
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • How are you running Forgejo?

I'm running the binary release for amd64 in a systemwide installation, as described in https://forgejo.org/docs/v1.20/admin/installation/#installation-from-binary

Description

Blobless clones (git clone --filter=blob:none ...) didn't work (warning: filtering not recognized by server, ignoring), even though both
/home/git/.gitconfig and /var/lib/forgejo/data/home/.gitconfig have

[uploadpack]
        allowfilter = true
        allowAnySHA1InWant = true

set.

Analysis

On the server, I ran strings /proc/$(pidof git)/environ | grep HOME (while the git clone operation of the client was running) and it printed HOME=/usr/local/bin/data/home.
I did not set [Git] HOME_PATH in the app.ini, so it defaults to %(APP_DATA_PATH)s/home, which should be $FORGEJO_WORK_DIR/data/home/.

So in this case obviously $FORGEJO_WORK_DIR wasn't set, even though the forgejo web process has it set correctly.

The problem appears to be that the entries that forgejo generates in /home/git/.ssh/authorized_keys look like:
command="/usr/local/bin/forgejo --config=/etc/forgejo/app.ini serv key-1",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict ssh-rsa AAAA....
So forgejo is called with the path to the config, but without setting --work-path, so the Git HOME_PATH that forgejo generates is wrong.

So you should generate the authorized_keys entries in a way that also sets --work-path (or -w), and ideally you should support setting the work path in the app.ini (and do set it when generating it in the web-based installation step), so it doesn't have to be specified on commandline all the time, see also forgejo/discussions#28

Reproducing

Use a systemwide forgejo installation (or any where --work-path/$FORGEJO_WORK_DIR is explicitly set, and you haven't explicitly configured [Git] HOME_PATH in the app.ini) and try to clone a repo bloblessly from it, i.e. with git clone HOME=/usr/local/bin/data/home git@yourhost:foo/bar.git, note that while cloning generally works, it will do a full clone and show this message at the beginning: warning: filtering not recognized by server, ignoring

## Dependency references * v1.21 https://github.com/go-gitea/gitea/pull/25331 * v1.20 https://github.com/go-gitea/gitea/pull/25347 * https://github.com/go-gitea/gitea/issues/24222#issuecomment-1595599546 ## Context - Can you reproduce the problem on [Forgejo Next](https://next.forgejo.org/)? I think this requires access to the server itself to reproduce - Forgejo version (or commit ref): 1.19.3 - Git version: 2.39.2 - Operating system: Linux - Database (use `[x]`): - [ ] PostgreSQL - [ ] MySQL - [ ] MSSQL - [x] SQLite - How are you running Forgejo? I'm running the binary release for amd64 in a systemwide installation, as described in https://forgejo.org/docs/v1.20/admin/installation/#installation-from-binary ## Description Blobless clones (`git clone --filter=blob:none ...`) didn't work (`warning: filtering not recognized by server, ignoring`), even though both `/home/git/.gitconfig` and `/var/lib/forgejo/data/home/.gitconfig` have ``` [uploadpack] allowfilter = true allowAnySHA1InWant = true ``` set. ## Analysis On the server, I ran `strings /proc/$(pidof git)/environ | grep HOME` (while the git clone operation of the client was running) and it printed `HOME=/usr/local/bin/data/home`. I did *not* set `[Git] HOME_PATH` in the app.ini, so it defaults to `%(APP_DATA_PATH)s/home`, which *should* be `$FORGEJO_WORK_DIR/data/home/`. So in this case obviously $FORGEJO_WORK_DIR wasn't set, even though the `forgejo web` process has it set correctly. The problem appears to be that the entries that forgejo generates in `/home/git/.ssh/authorized_keys` look like: `command="/usr/local/bin/forgejo --config=/etc/forgejo/app.ini serv key-1",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty,no-user-rc,restrict ssh-rsa AAAA....` So forgejo is called with the path to the config, but without setting `--work-path`, so the Git HOME_PATH that forgejo generates is wrong. So you should generate the authorized_keys entries in a way that also sets `--work-path` (or `-w`), and ideally you should support setting the work path in the app.ini (and do set it when generating it in the web-based installation step), so it doesn't have to be specified on commandline all the time, see also https://codeberg.org/forgejo/discussions/issues/28 ## Reproducing Use a systemwide forgejo installation (or any where `--work-path`/`$FORGEJO_WORK_DIR` is explicitly set, and you haven't explicitly configured `[Git] HOME_PATH` in the app.ini) and try to clone a repo bloblessly from it, i.e. with `git clone HOME=/usr/local/bin/data/home git@yourhost:foo/bar.git`, note that while cloning generally works, it will do a *full* clone and show this message at the beginning: `warning: filtering not recognized by server, ignoring`
earl-warren added the
bug
label 2023-06-17 05:10:37 +00:00
Poster
Collaborator

BTW, explicitly setting

[git]
HOME_PATH = /home/git

(or HOME_PATH = /var/lib/forgejo/data/home) in the app.ini works around this problem - just in case anyone else runs into this before it's fixed..

I think this should be fixed before the next release.

BTW, explicitly setting ```ini [git] HOME_PATH = /home/git ``` (or `HOME_PATH = /var/lib/forgejo/data/home`) in the app.ini works around this problem - just in case anyone else runs into this before it's fixed.. I think this should be fixed before the next release.

The [git] section in app.ini was refactored in v1.20 but I suspect this particular problem persists. I see you have linked this issue to Gitea as well and maybe there will be more input there.

If you confirm the problem still shows in v1.20 (the release candidate is here) and @wxiaoguang does not beat me to it, I'll implement a fix and backport it so it is released with v1.20.

The `[git]` section in `app.ini` was refactored in v1.20 but I suspect this particular problem persists. I see you have [linked this issue to Gitea](https://github.com/go-gitea/gitea/issues/24222#issuecomment-1595599546) as well and maybe there will be more input there. If you confirm the problem still shows in v1.20 (the release candidate is [here](https://codeberg.org/forgejo-experimental/forgejo/releases/tag/v1.20.0-0-rc0)) and @wxiaoguang does not beat me to it, I'll implement a fix and backport it so it is released with v1.20.
earl-warren added the
issue
do-not-exist-yet
dependency
Gitea
backport/v1.20
labels 2023-06-17 05:57:07 +00:00
earl-warren added this to the Forgejo v1.21.0-0-rc0 milestone 2023-06-17 05:57:16 +00:00
Poster
Collaborator

I finally got around to test 1.20rc0, and to my big surprise the issue seems to be fixed?!

$ strings /proc/$(pidof git)/environ | grep HOME
HOME=/var/lib/forgejo/data/home

The app.ini (that apparently is modified by forgejo migrate, or when first starting a new version?) now under server contains STATIC_ROOT_PATH=/var/lib/forgejo/ and APP_DATA_PATH=/var/lib/forgejo/data, which I guess is used to set those paths.

Weird I've never seen that mentioned in the chat or in https://github.com/go-gitea/gitea/issues/24222#issuecomment-1595599546 or forgejo/discussions#28

I finally got around to test 1.20rc0, and to my big surprise the issue seems to be fixed?! ``` $ strings /proc/$(pidof git)/environ | grep HOME HOME=/var/lib/forgejo/data/home ``` The app.ini (that apparently is modified by `forgejo migrate`, or when first starting a new version?) now under server contains `STATIC_ROOT_PATH=/var/lib/forgejo/` and `APP_DATA_PATH=/var/lib/forgejo/data`, which I guess is used to set those paths. Weird I've never seen that mentioned in the chat or in https://github.com/go-gitea/gitea/issues/24222#issuecomment-1595599546 or https://codeberg.org/forgejo/discussions/issues/28
Poster
Collaborator

Of course, if (as suggested in the official installation instructions of both gitea and forgejo) /etc/forgejo/app.ini is set to readonly after the initial installation, upgrading to 1.20 will not automatically fix this bug for users.

But it might suffice to adapt the migration guide to tell people to make the app.ini writable before upgrading (and running forgejo -w /path/to/working_dir -c /path/to/app.ini migrate)?
Also, how come https://forgejo.org/docs/v1.20/admin/upgrade/ doesn't even mention forgejo migrate?

Of course, if (as suggested in the official installation instructions of both gitea and forgejo) /etc/forgejo/app.ini is set to readonly after the initial installation, upgrading to 1.20 will not automatically fix this bug for users. But it might suffice to adapt the migration guide to tell people to make the app.ini writable before upgrading (and running `forgejo -w /path/to/working_dir -c /path/to/app.ini migrate`)? Also, how come https://forgejo.org/docs/v1.20/admin/upgrade/ doesn't even mention `forgejo migrate`?

This will be fixed by https://github.com/go-gitea/gitea/pull/25330 and a temporary fix is being implemented at https://github.com/go-gitea/gitea/pull/25331.

This will be fixed by https://github.com/go-gitea/gitea/pull/25330 and a temporary fix is being implemented at https://github.com/go-gitea/gitea/pull/25331.
earl-warren added
issue
open
and removed
issue
do-not-exist-yet
labels 2023-06-18 07:15:24 +00:00

Also, how come https://forgejo.org/docs/v1.20/admin/upgrade/ doesn't even mention forgejo migrate

The migration implicitly happens when the newer version is started. I suspect very few people use forgejo migrate and I would not be surprised if it does not do exactly the same thing.

And the forgejo doctor can be run after to identify / fix issues. If there is a difficult situation discovered that cannot be resolved, the Forgejo instance is restored from the backup done before the upgrade.

> Also, how come https://forgejo.org/docs/v1.20/admin/upgrade/ doesn't even mention forgejo migrate The migration implicitly happens when the newer version is started. I suspect very few people use `forgejo migrate` and I would not be surprised if it does not do **exactly** the same thing. And the `forgejo doctor` can be run after to identify / fix issues. If there is a difficult situation discovered that cannot be resolved, the Forgejo instance is restored from the backup done before the upgrade.
Poster
Collaborator

I explicitly had to call forgejo migrate for the database to be migrated to the newer version.
forgejo doctor complained about wrong DB version and suggested running gitea migrate (you should change that to forgejo migrate!).

I explicitly had to call `forgejo migrate` for the database to be migrated to the newer version. `forgejo doctor` complained about wrong DB version and suggested running `gitea migrate` (**you should change that to `forgejo migrate`!**).
Poster
Collaborator

I explicitly had to call forgejo migrate for the database to be migrated to the newer version.

I can't reproduce that issue, no idea what went wrong, maybe I ran forgejo doctor before running forgejo web, so the migration hadn't run yet? Not sure anymore (and unfortunately I didn't make a backup before the update, because it was just a test repo).

Still, that warning in forgejo doctor should recommend calling forgejo migrate, not gitea migrate :)

> I explicitly had to call forgejo migrate for the database to be migrated to the newer version. I can't reproduce that issue, no idea what went wrong, *maybe* I ran `forgejo doctor` *before* running `forgejo web`, so the migration hadn't run yet? Not sure anymore (and unfortunately I didn't make a backup before the update, because it was just a test repo). Still, that warning in `forgejo doctor` should recommend calling `forgejo migrate`, not `gitea migrate` :)

Still, that warning in forgejo doctor should recommend calling forgejo migrate, not gitea migrate :)

This was fixed with #871 yesterday.

The ideal solution would be for the Forgejo to be a white label software with the theming (name, logo, etc.) cleanly separated in a well documented directory. Eventually the forgejo-branding feature branch will make it possible to do that but there is a long way to go before it happens.

> Still, that warning in forgejo doctor should recommend calling forgejo migrate, not gitea migrate :) This was fixed with https://codeberg.org/forgejo/forgejo/pulls/871 yesterday. The ideal solution would be for the Forgejo to be a white label software with the theming (name, logo, etc.) cleanly separated in a well documented directory. Eventually the `forgejo-branding` feature branch will make it possible to do that but there is a long way to go before it happens.

@DanielGibson can this be closed?

@DanielGibson can this be closed?
Sign in to join this conversation.
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: forgejo/forgejo#869
There is no content yet.