Skip to content

Fix internal lfs_authenticate API for non-project repositories

Markus Koller requested to merge lfs-ssh-authentication into master

What does this MR do?

This endpoint is used for LFS over SSH and had two problems:

  1. We were generating the repository URL from project rather than container, which means we'd return the wrong URL for e.g. project wiki repositories.

    In practice this didn't break LFS, as we're not touching the repository itself and just associating the objects with the project record, which works the same with project wikis.

  2. We did not check if LFS is enabled, which means we'd still return a URL for project snippets. This is less of a problem as the subsequent HTTP requests to the LFS controllers would still fail, but it's better to be consistent and abort early here.

Follow-up to !45874 (merged)

Manual testing

  1. Create a project, make sure LFS is enabled.
  2. Create a project snippet or wiki page, and clone its repository over SSH.
  3. Add an LFS file locally (git lfs track ...) and commit your changes.
  4. Push with GIT_TRACE=2 git push to see the LFS HTTP requests.
    • There's a line run_command: ssh -- git@gitlab.com git-lfs-authenticate #{repository_path} upload, this is what's triggering our lfs_authenticate endpoint (via gitlab-shell).
    • After that there will be some HTTP POST requests to:
      • #{repository_path}/info/lfs/locks/verify
      • #{repository_path}/info/lfs/objects/batch
    • The repository_path in these three requests should all be the same, and point to the snippet/wiki and NOT the project itself.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Markus Koller

Merge request reports

Loading