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

Refactor LFS SSH and internal routers #32473

Merged
merged 3 commits into from
Nov 12, 2024
Merged

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Nov 11, 2024

My Gitea instance keeps reporting a lot of errors like "LFS SSH transfer connection denied, pure SSH protocol is disabled"

When I started debugging the problem, I found more problems to fix. This PR addresses most of them:

  1. unnecessary server side error logs (change fail() to not log them)
  2. the broken user2/lfs.git (added comments)
  3. migratePushMirrors could fail when a repository doesn't exist (ignore them)
  4. "Authorization" (internal&lfs) header conflicts, remove the tricky "swapAuth" and use "X-Gitea-Internal-Auth"
  5. internal token comparing is not constant time (use ConstantTimeCompare, it wasn't a serous problem because in a real world it's nearly impossible to timing-attack the token, but good to fix and backport)
  6. duplicate routers (introduce AddOwnerRepoGitLFSRoutes)
  7. "internal (private)" routes shouldn't use web context, they should use private context
  8. incorrect "path" usages (use "filepath")
  9. incorrect mocked route point handling (need to check func nil correctly)

By the way, split some tests from "git general tests" to "git misc tests" (to keep "git_general_test.go" simple)

Then I tried to write a test for "LFS SSH Transfer" but I guess I haven't gotten the correct result. So the code is kept there (tests/integration/git_lfs_ssh_test.go) and a FIXME explains the details. Indeed it's worth to take a deep look (in another PR)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 11, 2024
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 11, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/migrations labels Nov 11, 2024
@wxiaoguang wxiaoguang force-pushed the refactor-lfs branch 2 times, most recently from 12a7661 to 3557be7 Compare November 11, 2024 05:14
@wxiaoguang wxiaoguang added this to the 1.23.0 milestone Nov 11, 2024
@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Nov 11, 2024
@wxiaoguang wxiaoguang force-pushed the refactor-lfs branch 2 times, most recently from 857e57d to 469dd72 Compare November 11, 2024 05:51
@lunny
Copy link
Member

lunny commented Nov 11, 2024

cmd/web.go:231:func1() [F] PANIC: runtime error: invalid memory address or nil pointer dereference
/Users/xxxxx/data/Projects/gitea/gitea/modules/web/handler.go:133 (0x103bbca54)
	toHandlerProvider.wrapHandlerProvider[...].func2: h := hp(next) // this handle could be dynamically generated, so we can't use it for debug info
/Users/xxxxx/data/Projects/gitea/gitea/modules/web/route.go:117 (0x103bbd6ff)
	(*Router).Methods: r.chiRouter.With(middlewares...).Method(methods, fullPattern, handlerFunc)
/Users/xxxxx/data/Projects/gitea/gitea/modules/web/route.go:150 (0x103bbdc23)
	(*Router).Post: r.Methods("POST", pattern, h...)
/Users/xxxxx/data/Projects/gitea/gitea/routers/common/lfs.go:16 (0x104795427)
	Routes.func1.AddOwnerRepoGitLFSRoutes.3: m.Post("/objects/batch", lfs.CheckAcceptMediaType, lfs.BatchHandler)
/Users/xxxxx/data/Projects/gitea/gitea/modules/web/route.go:68 (0x103bbcedb)
	(*Router).Group: fn()
/Users/xxxxx/data/Projects/gitea/gitea/routers/common/lfs.go:15 (0x10479539f)
	AddOwnerRepoGitLFSRoutes: m.Group("/{username}/{reponame}/info/lfs", func() {

@wxiaoguang
Copy link
Contributor Author

Fixed in 9a876b7, Golang has some strange rules for "nil" comparing.

@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 11, 2024
@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 Nov 12, 2024
@wolfogre wolfogre added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 12, 2024
@wxiaoguang wxiaoguang enabled auto-merge (squash) November 12, 2024 02:23
@wxiaoguang wxiaoguang merged commit 580e21d into go-gitea:main Nov 12, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 12, 2024
@wxiaoguang wxiaoguang deleted the refactor-lfs branch November 12, 2024 02:40
@lunny
Copy link
Member

lunny commented Nov 12, 2024

Can we backport this to v1.22?

@wxiaoguang
Copy link
Contributor Author

-> Refactor LFS SSH and internal routers (#32473) #32479

wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Nov 12, 2024
@lunny lunny added backport/done All backports for this PR have been created backport/v1.22 This PR should be backported to Gitea 1.22 labels Nov 12, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 13, 2024
* giteaofficial/main:
  Disable Oauth check if oauth disabled (go-gitea#32368)
  Update JS and PY dependencies (go-gitea#32482)
  Update `github.com/meilisearch/meilisearch-go` (go-gitea#32484)
  Fix test fixtures for user2/lfs.git (go-gitea#32477)
  Limit org member view of restricted users (go-gitea#32211)
  cargo registry - respect renamed dependencies (go-gitea#32430)
  Refactor LFS SSH and internal routers (go-gitea#32473)
  Fix a number of typescript issues (go-gitea#32459)
  Harden runner updateTask and updateLog api (go-gitea#32462)
  Move some functions from issue.go to standalone files (go-gitea#32468)
lunny pushed a commit that referenced this pull request Nov 13, 2024
…mparing) (#32473) (#32479)

Partially backport #32473. LFS related changes are not in 1.22, so skip
them.

1. Ignore non-existing repos during migrations
2. Improve ReadBatchLine's comment
3. Use `X-Gitea-Internal-Auth` header for internal API calls and make
the comparing constant time (it wasn't a serous problem because in a
real world it's nearly impossible to timing-attack the token, but indeed
security related and good to fix and backport)
4. Fix route mock nil check
@wxiaoguang
Copy link
Contributor Author

Next: Fix LFS route mock and realm #32488

TKaxv-7S added a commit to TKaxv-7S/gitea that referenced this pull request Dec 1, 2024
* SECURITY
  * Fix basic auth with webauthn (go-gitea#32531) (go-gitea#32536)
  * Refactor internal routers (partial backport, auth token const time comparing) (go-gitea#32473) (go-gitea#32479)
* PERFORMANCE
  * Remove transaction for archive download (go-gitea#32186) (go-gitea#32520)
* BUGFIXES
  * Fix `missing signature key` error when pulling Docker images with `SERVE_DIRECT` enabled (go-gitea#32365) (go-gitea#32397)
  * Fix get reviewers fails when selecting user without pull request permissions unit (go-gitea#32415) (go-gitea#32616)
  * Fix adding index files to tmp directory (go-gitea#32360) (go-gitea#32593)
  * Fix PR creation on forked repositories via API (go-gitea#31863) (go-gitea#32591)
  * Fix missing menu tabs in organization project view page (go-gitea#32313) (go-gitea#32592)
  * Support HTTP POST requests to `/userinfo`, aligning to OpenID Core specification (go-gitea#32578) (go-gitea#32594)
  * Fix debian package clean up cron job (go-gitea#32351) (go-gitea#32590)
  * Fix GetInactiveUsers (go-gitea#32540) (go-gitea#32588)
  * Allow the actions user to login via the jwt token (go-gitea#32527) (go-gitea#32580)
  * Fix submodule parsing (go-gitea#32571) (go-gitea#32577)
  * Refactor find forks and fix possible bugs that weaken permissions check (go-gitea#32528) (go-gitea#32547)
  * Fix some places that don't respect org full name setting (go-gitea#32243) (go-gitea#32550)
  * Refactor push mirror find and add check for updating push mirror (go-gitea#32539) (go-gitea#32549)
  * Fix basic auth with webauthn (go-gitea#32531) (go-gitea#32536)
  * Fix artifact v4 upload above 8MB (go-gitea#31664) (go-gitea#32523)
  * Fix oauth2 error handle not return immediately (go-gitea#32514) (go-gitea#32516)
  * Fix action not triggered when commit message is too long (go-gitea#32498) (go-gitea#32507)
  * Fix `GetRepoLink` nil pointer dereference on dashboard feed page when repo is deleted with actions enabled (go-gitea#32501) (go-gitea#32502)
  * Fix `missing signature key` error when pulling Docker images with `SERVE_DIRECT` enabled (go-gitea#32397) (go-gitea#32397)
  * Fix the permission check for user search API and limit the number of returned users for `/user/search` (go-gitea#32310)
  * Fix SearchIssues swagger docs (go-gitea#32208) (go-gitea#32298)
  * Fix dropdown content overflow (go-gitea#31610) (go-gitea#32250)
  * Disable Oauth check if oauth disabled (go-gitea#32368) (go-gitea#32480)
  * Respect renamed dependencies of Cargo registry (go-gitea#32430) (go-gitea#32478)
  * Fix mermaid diagram height when initially hidden (go-gitea#32457) (go-gitea#32464)
  * Fix broken releases when re-pushing tags (go-gitea#32435) (go-gitea#32449)
  * Only provide the commit summary for Discord webhook push events (go-gitea#32432) (go-gitea#32447)
  * Only query team tables if repository is under org when getting assignees (go-gitea#32414) (go-gitea#32426)
  * Fix created_unix for mirroring (go-gitea#32342) (go-gitea#32406)
  * Respect UI.ExploreDefaultSort setting again (go-gitea#32357) (go-gitea#32385)
  * Fix broken image when editing comment with non-image attachments (go-gitea#32319) (go-gitea#32345)
  * Fix disable 2fa bug (go-gitea#32320) (go-gitea#32330)
  * Always update expiration time when creating an artifact (go-gitea#32281) (go-gitea#32285)
  * Fix null errors on conversation holder (go-gitea#32258) (go-gitea#32266) (go-gitea#32282)
  * Only rename a user when they should receive a different name (go-gitea#32247) (go-gitea#32249)
  * Fix checkbox bug on private/archive filter (go-gitea#32236) (go-gitea#32240)
  * Add a doctor check to disable the "Actions" unit for mirrors (go-gitea#32424) (go-gitea#32497)
  * Quick fix milestone deadline 9999 (go-gitea#32423)
  * Make `show stats` work when only one file changed (go-gitea#32244) (go-gitea#32268)
  * Make `owner/repo/pulls` handlers use "PR reader" permission (go-gitea#32254) (go-gitea#32265)
  * Update scheduled tasks even if changes are pushed by "ActionsUser" (go-gitea#32246) (go-gitea#32252)
* MISC
  * Remove unnecessary code: `GetPushMirrorsByRepoID` called on all repo pages (go-gitea#32560) (go-gitea#32567)
  * Improve some sanitizer rules (go-gitea#32534)
  * Update nix development environment vor v1.22.x (go-gitea#32495)
  * Add warn log when deleting inactive users (go-gitea#32318) (go-gitea#32321)
  * Update github.com/go-enry/go-enry to v2.9.1 (go-gitea#32295) (go-gitea#32296)
  * Warn users when they try to use a non-root-url to sign in/up (go-gitea#32272) (go-gitea#32273)

# -----BEGIN PGP SIGNATURE-----
#
# iQIzBAABCAAdFiEEumb2f9c/cFjXEtMIw7fJG2Mvc4oFAmdEyeoACgkQw7fJG2Mv
# c4pythAAn57Z9Csfd8UrHbCd87SBlEGydhlng5Oc99pQIAvExR0hc9VFWjt5pFr4
# aXTajtzb/sDQkAPZEiL45CL471z+Ga81ixaKRfrBeMiSECB0wBaL4+XH94qQ3lw3
# /dNfQsc9bUnomGWQyEIbQ6mT85fJdvBD1nibUSH3b5P4WqOBHbY9YlehPmE96KY2
# 9k1IYvBvcfCjK6njVQ7m+sFOr7/Y2ZHe9FeN8hEf/1Bfnc75wtkeNyeXnlNe67Eo
# ViFzcA35WyTXw4NRY+TG/8xZEXHl8DuOuUdPoBqkpFw9TzxR2svO0QLzRIHgJP+t
# /Cdd16zZd6fQ+ET+DV8IaF2wlXdEgVDWs2aT04VDLGpSw9czxsUEUQ0ETWFFomXN
# //goTLu1B3fVQYrE9MK2vfUQGe2Su3ChGwNtNEK9bMQpO6sLFGRE0nPgBJMPJ0yA
# bfPhRlsVxnyEToqeKoC77wv0kPiOkzPfDm6sFLAt+tATcij5UlTU4nVXyXsELk14
# p5mtsTfaEqiH3U+JW0Drz8wV7nk8F599lZbYO92M3Z59bqC5TsOVYgqb1ODTpqQO
# 7gLdgdKmQbKWTPHLA9Hz+0/3bT1MirMRdtXW7TmgW83TuN37wOuElCmXmJTN2feY
# LG4k417kVrBwF+fdGPXo+T7H0MqxX1fTkVftG3C63sdaRQrUM1M=
# =jyQM
# -----END PGP SIGNATURE-----
# gpg: Signature made Tue, Nov 26, 2024  3:03:06 AM
# gpg:                using RSA key BA66F67FD73F7058D712D308C3B7C91B632F738A
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: keydb_search failed: Connection timed out
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: Note: database_open 134217901 waiting for lock (held by 1152) ...
# gpg: keydb_search failed: Connection timed out
# gpg: Can't check signature: No public key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/done All backports for this PR have been created backport/v1.22 This PR should be backported to Gitea 1.22 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/go Pull requests that update Go code modifies/migrations size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants