Clean up Go proxy implementation

- Various fixes and improvements
- Hide proxy behind a feature flag, due to performance issues: #218083
- Add a feature flag for testing to disable strict go.mod validation
- Document why case en/decoding is necessary
- Refactor pseudo-version processing
  - Move logic from VersionFinder to ModuleHelpers
  - Document reasoning for matching and validation
- Replace BasicAuthHelper and custom method with route setting
- Use serach_files_by_name instead of tree to improve performance
- Use correct content type for zip: closes #214876

doc/user/packages/go_proxy/index.md

@@ -56,7 +56,6 @@ Follow [this issue](https://gitlab.com/gitlab-org/gitlab/-/issues/213770) for de
 
 ### Fetch modules from private projects
 
-NOTE: **Note:**
 `go` does not support transmitting credentials over insecure connections. The
 steps below work only if GitLab is configured for HTTPS.
 

ee/app/finders/packages/go/module_finder.rb

@@ -5,29 +5,31 @@ module Packages
     class ModuleFinder
       include ::API::Helpers::Packages::Go::ModuleHelpers
 
-      GITLAB_GO_URL = (Settings.build_gitlab_go_url + '/').freeze
-
       attr_reader :project, :module_name
 
       def initialize(project, module_name)
-        module_name = CGI.unescape(module_name)
         module_name = Pathname.new(module_name).cleanpath.to_s
 
         @project = project
         @module_name = module_name
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def execute
-        return if @module_name.blank? || !@module_name.start_with?(GITLAB_GO_URL)
+        return if @module_name.blank? || !@module_name.start_with?(gitlab_go_url)
 
-        module_path = @module_name[GITLAB_GO_URL.length..].split('/')
+        module_path = @module_name[gitlab_go_url.length..].split('/')
         project_path = project.full_path.split('/')
-        return unless module_path.take(project_path.length) == project_path
+        module_project_path = module_path.shift(project_path.length)
+        return unless module_project_path == project_path
+
+        Packages::GoModule.new(@project, @module_name, module_path.join('/'))
+      end
+
+      private
 
-        Packages::GoModule.new(@project, @module_name, module_path.drop(project_path.length).join('/'))
+      def gitlab_go_url
+        @gitlab_go_url ||= Settings.build_gitlab_go_url + '/'
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 end

ee/app/models/packages/go_module.rb

 # frozen_string_literal: true
 
 class Packages::GoModule
+  include Gitlab::Utils::StrongMemoize
+
   attr_reader :project, :name, :path
 
   def initialize(project, name, path)
@@ -10,7 +12,7 @@ class Packages::GoModule
   end
 
   def versions
-    @versions ||= Packages::Go::VersionFinder.new(self).execute
+    strong_memoize(:versions) { Packages::Go::VersionFinder.new(self).execute }
   end
 
   def version_by(ref: nil, commit: nil)
@@ -40,6 +42,10 @@ class Packages::GoModule
   end
 
   def gomod_valid?(gomod)
+    if Feature.enabled?(:go_proxy_disable_gomod_validation, @project)
+      return gomod&.start_with?("module ")
+    end
+
     gomod&.split("\n", 2)&.first == "module #{@name}"
   end
 
@@ -47,8 +53,8 @@ class Packages::GoModule
 
   def version_by_name(name)
     # avoid a Gitaly call if possible
-    if defined?(@versions)
-      v = @versions.find { |v| v.name == ref }
+    if strong_memoized?(:versions)
+      v = versions.find { |v| v.name == ref }
       return v if v
     end
 
@@ -60,8 +66,8 @@ class Packages::GoModule
 
   def version_by_ref(ref)
     # reuse existing versions
-    if defined?(@versions)
-      v = @versions.find { |v| v.ref == ref }
+    if strong_memoized?(:versions)
+      v = versions.find { |v| v.ref == ref }
       return v if v
     end
 

ee/app/models/packages/go_module_version.rb

 # frozen_string_literal: true
 
 class Packages::GoModuleVersion
+  include Gitlab::Utils::StrongMemoize
   include ::API::Helpers::Packages::Go::ModuleHelpers
 
   VALID_TYPES = %i[ref commit pseudo].freeze
@@ -42,8 +43,8 @@ class Packages::GoModuleVersion
   end
 
   def gomod
-    @gomod ||=
-      if defined?(@blobs)
+    strong_memoize(:gomod) do
+      if strong_memoized?(:blobs)
         blob_at(@mod.path + '/go.mod')
       elsif @mod.path.empty?
         @mod.project.repository.blob_at(@commit.sha, 'go.mod')&.data
@@ -51,16 +52,31 @@ class Packages::GoModuleVersion
         @mod.project.repository.blob_at(@commit.sha, @mod.path + '/go.mod')&.data
       end
     end
+  end
 
   def archive
     suffix_len = @mod.path == '' ? 0 : @mod.path.length + 1
 
     Zip::OutputStream.write_buffer do |zip|
       files.each do |file|
-        zip.put_next_entry "#{full_name}/#{file.path[suffix_len...]}"
-        zip.write blob_at(file.path)
+        zip.put_next_entry "#{full_name}/#{file[suffix_len...]}"
+        zip.write blob_at(file)
+      end
+    end
+  end
+
+  def files
+    strong_memoize(:files) do
+      ls_tree.filter { |e| !excluded.any? { |n| e.start_with? n } }
     end
   end
+
+  def excluded
+    strong_memoize(:excluded) do
+      ls_tree
+        .filter { |f| f.end_with?('/go.mod') && f != @mod.path + '/go.mod' }
+        .map    { |f| f[0..-7] }
+    end
   end
 
   def valid?
@@ -78,15 +94,19 @@ class Packages::GoModuleVersion
   end
 
   def blobs
-    @blobs ||= @mod.project.repository.batch_blobs(files.map { |x| [@commit.sha, x.path] })
+    strong_memoize(:blobs) { @mod.project.repository.batch_blobs(files.map { |x| [@commit.sha, x] }) }
   end
 
-  def files
-    return @files if defined?(@files)
+  def ls_tree
+    strong_memoize(:ls_tree) do
+      path =
+        if @mod.path.empty?
+          '.'
+        else
+          @mod.path
+        end
 
-    sha = @commit.sha
-    tree = @mod.project.repository.tree(sha, @mod.path, recursive: true).entries.filter { |e| e.file? }
-    nested = tree.filter { |e| e.name == 'go.mod' && !(@mod.path == '' && e.path == 'go.mod' || e.path == @mod.path + '/go.mod') }.map { |e| e.path[0..-7] }
-    @files = tree.filter { |e| !nested.any? { |n| e.path.start_with? n } }
+      @mod.project.repository.gitaly_repository_client.search_files_by_name(@commit.sha, path)
+    end
   end
 end

ee/app/models/packages/sem_ver.rb

 # frozen_string_literal: true
 
 class Packages::SemVer
-  # basic semver, but bounded (^expr$)
-  PATTERN = /\A(v?)(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-([-.a-z0-9]+))?(?:\+([-.a-z0-9]+))?\z/i.freeze
-
   attr_accessor :major, :minor, :patch, :prerelease, :build
 
   def initialize(major = 0, minor = 0, patch = 0, prerelease = nil, build = nil, prefixed: false)
@@ -37,11 +34,11 @@ class Packages::SemVer
   end
 
   def self.match(str, prefixed: false)
-    m = PATTERN.match(str)
-    return unless m
-    return if prefixed == m[1].empty?
+    return unless str&.start_with?('v') == prefixed
+
+    str = str[1..] if prefixed
 
-    m
+    Gitlab::Regex.semver_regex.match(str)
   end
 
   def self.match?(str, prefixed: false)
@@ -52,6 +49,6 @@ class Packages::SemVer
     m = match str, prefixed: prefixed
     return unless m
 
-    new(m[2].to_i, m[3].to_i, m[4].to_i, m[5], m[6], prefixed: prefixed)
+    new(m[1].to_i, m[2].to_i, m[3].to_i, m[4], m[5], prefixed: prefixed)
   end
 end

ee/lib/api/go_proxy.rb

 # frozen_string_literal: true
 module API
   class GoProxy < Grape::API
-    helpers ::API::Helpers::PackagesManagerClientsHelpers
-    helpers ::API::Helpers::Packages::BasicAuthHelpers
+    helpers ::API::Helpers::PackagesHelpers
     helpers ::API::Helpers::Packages::Go::ModuleHelpers
 
     # basic semver, except case encoded (A => !a)
@@ -13,27 +12,27 @@ module API
     before { require_packages_enabled! }
 
     helpers do
-      # support personal access tokens for HTTP Basic in addition to the usual methods
-      def find_personal_access_token
-        pa = find_personal_access_token_from_http_basic_auth
-        return pa if pa
+      def find_project!(id)
+        # based on API::Helpers::Packages::BasicAuthHelpers#authorized_project_find!
 
-        # copied from Gitlab::Auth::AuthFinders
-        token =
-          current_request.params[::Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_PARAM].presence ||
-          current_request.env[::Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER].presence ||
-          parsed_oauth_token
-        return unless token
+        project = find_project(id)
 
-        # Expiration, revocation and scopes are verified in `validate_access_token!`
-        PersonalAccessToken.find_by_token(token) || raise(::Gitlab::Auth::UnauthorizedError)
+        return project if project && can?(current_user, :read_project, project)
+
+        if current_user
+          not_found!('Project')
+        else
+          unauthorized!
+        end
       end
 
       def find_module
+        not_found! unless Feature.enabled?(:go_proxy, user_project)
+
         module_name = case_decode params[:module_name]
         bad_request!('Module Name') if module_name.blank?
 
-        mod = ::Packages::Go::ModuleFinder.new(authorized_user_project, module_name).execute
+        mod = ::Packages::Go::ModuleFinder.new(user_project, module_name).execute
 
         not_found! unless mod
 
@@ -55,13 +54,13 @@ module API
 
     params do
       requires :id, type: String, desc: 'The ID of a project'
-      requires :module_name, type: String, desc: 'Module name'
+      requires :module_name, type: String, desc: 'Module name', coerce_with: ->(val) { CGI.unescape(val) }
     end
-    route_setting :authentication, job_token_allowed: true
+    route_setting :authentication, job_token_allowed: true, basic_auth_personal_access_token: true
     resource :projects, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do
       before do
-        authorize_read_package!(authorized_user_project)
-        authorize_packages_feature!(authorized_user_project)
+        authorize_read_package!
+        authorize_packages_feature!
       end
 
       namespace ':id/packages/go/*module_name/@v' do
@@ -110,10 +109,10 @@ module API
         get ':module_version.zip', requirements: MODULE_VERSION_REQUIREMENTS do
           ver = find_version
 
-          # TODO: Content-Type should be application/zip, see #214876
+          content_type 'application/zip'
+          env['api.format'] = :binary
           header['Content-Disposition'] = ActionDispatch::Http::ContentDisposition.format(disposition: 'attachment', filename: ver.name + '.zip')
           header['Content-Transfer-Encoding'] = 'binary'
-          content_type 'text/plain'
           status :ok
           body ver.archive.string
         end

ee/lib/api/helpers/packages/go/module_helpers.rb

@@ -6,10 +6,24 @@ module API
       module Go
         module ModuleHelpers
           def case_encode(str)
+            # Converts "github.com/Azure" to "github.com/!azure"
+            #
+            # From `go help goproxy`:
+            #
+            # > To avoid problems when serving from case-sensitive file systems,
+            # > the <module> and <version> elements are case-encoded, replacing
+            # > every uppercase letter with an exclamation mark followed by the
+            # > corresponding lower-case letter: github.com/Azure encodes as
+            # > github.com/!azure.
+
             str.gsub(/A-Z/) { |s| "!#{s.downcase}"}
           end
 
           def case_decode(str)
+            # Converts "github.com/!azure" to "github.com/Azure"
+            #
+            # See #case_encode
+
             str.gsub(/![[:alpha:]]/) { |s| s[1..].upcase }
           end
 
@@ -29,7 +43,7 @@ module API
 
             pre = version.prerelease
 
-            # valid pseudo-versions are
+            # Valid pseudo-versions are:
             #   vX.0.0-yyyymmddhhmmss-sha1337beef0, when no earlier tagged commit exists for X
             #   vX.Y.Z-pre.0.yyyymmddhhmmss-sha1337beef0, when most recent prior tag is vX.Y.Z-pre
             #   vX.Y.(Z+1)-0.yyyymmddhhmmss-sha1337beef0, when most recent prior tag is vX.Y.Z
@@ -41,7 +55,41 @@ module API
               pre = pre[m[0].length..]
             end
 
-            /\A\d{14}-[A-Za-z0-9]+\z/.freeze.match? pre
+            # This pattern is intentionally more forgiving than the patterns
+            # above. Correctness is verified by #pseudo_version_commit.
+            /\A\d{14}-\h+\z/.freeze.match? pre
+          end
+
+          def pseudo_version_commit(project, semver)
+            # Per Go's implementation of pseudo-versions, a tag should be
+            # considered a pseudo-version if it matches one of the patterns
+            # listed in #pseudo_version?, regardless of the content of the
+            # timestamp or the length of the SHA fragment. However, an error
+            # should be returned if the timestamp is not correct or if the SHA
+            # fragment is not exactly 12 characters long. See also Go's
+            # implementation of:
+            #
+            # - [*codeRepo.validatePseudoVersion](https://github.com/golang/go/blob/daf70d6c1688a1ba1699c933b3c3f04d6f2f73d9/src/cmd/go/internal/modfetch/coderepo.go#L530)
+            # - [Pseudo-version parsing](https://github.com/golang/go/blob/master/src/cmd/go/internal/modfetch/pseudo.go)
+            # - [Pseudo-version request processing](https://github.com/golang/go/blob/master/src/cmd/go/internal/modfetch/coderepo.go)
+
+            # Go ignores anything before '.' or after the second '-', so we will do the same
+            timestamp, sha = semver.prerelease.split('-').last 2
+            timestamp = timestamp.split('.').last
+            commit = project.repository.commit_by(oid: sha)
+
+            # Error messages are based on the responses of proxy.golang.org
+
+            # Verify that the SHA fragment references a commit
+            raise ArgumentError.new 'invalid pseudo-version: unknown commit' unless commit
+
+            # Require the SHA fragment to be 12 characters long
+            raise ArgumentError.new 'invalid pseudo-version: revision is shorter than canonical' unless sha.length == 12
+
+            # Require the timestamp to match that of the commit
+            raise ArgumentError.new 'invalid pseudo-version: does not match version-control timestamp' unless commit.committed_date.strftime('%Y%m%d%H%M%S') == timestamp
+
+            commit
           end
 
           def parse_semver(str)

ee/spec/factories/go_module_commits.rb

@@ -75,21 +75,27 @@ FactoryBot.define do
       transient do
         name { nil }
         message { 'Add module' }
-      end
 
-      service do
-        port = ::Gitlab.config.gitlab.port
-        host = ::Gitlab.config.gitlab.host
-        domain = case port when 80, 443 then host else "#{host}:#{port}" end
+        url do
+          v = "#{::Gitlab.config.gitlab.host}/#{project.path_with_namespace}"
 
-        url = "#{domain}/#{project.path_with_namespace}"
-        if name.nil?
-          path = ''
+          if name
+            v + '/' + name
           else
-          url += '/' + name
-          path = name + '/'
+            v
+          end
         end
 
+        path do
+          if name
+            name + '/'
+          else
+            ''
+          end
+        end
+      end
+
+      service do
         Files::MultiService.new(
           project,
           project.owner,

ee/spec/finders/packages/go/module_finder_spec.rb

@@ -58,13 +58,6 @@ describe Packages::Go::ModuleFinder do
     end
   end
 
-  context 'with a URL encoded relative path component' do
-    it_behaves_like 'an invalid path' do
-      let(:module_name) { base_url(project) + '/%2E%2E%2Fxyz' }
-      let(:expected_name) { base_url(project.namespace) + '/xyz' }
-    end
-  end
-
   context 'with many relative path components' do
     it_behaves_like 'an invalid path' do
       let(:module_name) { base_url(project) + ('/..' * 10) + '/xyz' }

ee/spec/finders/packages/go/version_finder_spec.rb

@@ -13,11 +13,16 @@ describe Packages::Go::VersionFinder do
     create :go_module_commit, :module,  project: project, tag: 'v1.0.1'
     create :go_module_commit, :package, project: project, tag: 'v1.0.2', path: 'pkg'
     create :go_module_commit, :module,  project: project, tag: 'v1.0.3', name: 'mod'
+    create :go_module_commit, :module,  project: project, tag: 'v1.0.4', name: 'bad-mod', url: 'example.com/go-lib'
     create :go_module_commit, :files,   project: project, tag: 'c1',     files: { 'y.go' => "package a\n" }
     create :go_module_commit, :module,  project: project, tag: 'c2',     name: 'v2'
     create :go_module_commit, :files,   project: project, tag: 'v2.0.0', files: { 'v2/x.go' => "package a\n" }
   end
 
+  before do
+    stub_feature_flags(go_proxy_disable_gomod_validation: false)
+  end
+
   shared_examples '#execute' do |*expected|
     it "returns #{expected.empty? ? 'nothing' : expected.join(', ')}" do
       actual = finder.execute.map { |x| x.name }
@@ -35,7 +40,7 @@ describe Packages::Go::VersionFinder do
     context 'for the root module' do
       let(:mod) { create :go_module, project: project }
 
-      it_behaves_like '#execute', 'v1.0.1', 'v1.0.2', 'v1.0.3'
+      it_behaves_like '#execute', 'v1.0.1', 'v1.0.2', 'v1.0.3', 'v1.0.4'
     end
 
     context 'for the package' do
@@ -47,7 +52,7 @@ describe Packages::Go::VersionFinder do
     context 'for the submodule' do
       let(:mod) { create :go_module, project: project, path: 'mod' }
 
-      it_behaves_like '#execute', 'v1.0.3'
+      it_behaves_like '#execute', 'v1.0.3', 'v1.0.4'
     end
 
     context 'for the root module v2' do
@@ -55,6 +60,22 @@ describe Packages::Go::VersionFinder do
 
       it_behaves_like '#execute', 'v2.0.0'
     end
+
+    context 'for the bad module' do
+      let(:mod) { create :go_module, project: project, path: 'bad-mod' }
+
+      context 'with gomod checking enabled' do
+        it_behaves_like '#execute'
+      end
+
+      context 'with gomod checking disabled' do
+        before do
+          stub_feature_flags(go_proxy_disable_gomod_validation: true)
+        end
+
+        it_behaves_like '#execute', 'v1.0.4'
+      end
+    end
   end
 
   describe '#find' do

ee/spec/models/packages/go_module_spec.rb

@@ -3,6 +3,10 @@
 require 'spec_helper'
 
 describe Packages::GoModule, type: :model do
+  before do
+    stub_feature_flags(go_proxy_disable_gomod_validation: false)
+  end
+
   describe '#path_valid?' do
     context 'with root path' do
       let_it_be(:package) { create(:go_module) }

ee/spec/models/packages/go_module_version_spec.rb

@@ -19,7 +19,7 @@ describe Packages::GoModuleVersion, type: :model do
 
   shared_examples '#files' do |desc, *entries|
     it "returns #{desc}" do
-      actual = version.files.map { |x| x.path }.to_set
+      actual = version.files.map { |x| x }.to_set
       expect(actual).to eq(entries.to_set)
     end
   end

ee/spec/requests/api/go_proxy_spec.rb

@@ -29,7 +29,9 @@ describe API::GoProxy do
 
   before do
     project.add_developer(user)
+
     stub_licensed_features(packages: true)
+    stub_feature_flags(go_proxy_disable_gomod_validation: false)
 
     modules
   end
@@ -54,7 +56,7 @@ describe API::GoProxy do
     end
   end
 
-  shared_examples 'a missing module version list resource' do |*versions, path: ''|
+  shared_examples 'a missing module version list resource' do |path: ''|
     let(:module_name) { "#{base}#{path}" }
     let(:resource) { "list" }
 
@@ -168,6 +170,18 @@ describe API::GoProxy do
     context 'for the root module v2' do
       it_behaves_like 'a module version list resource', 'v2.0.0', path: '/v2'
     end
+
+    context 'with a URL encoded relative path component' do
+      it_behaves_like 'a missing module version list resource', path: '/%2E%2E%2Fxyz'
+    end
+
+    context 'with the feature disabled' do
+      before do
+        stub_feature_flags(go_proxy: false)
+      end
+
+      it_behaves_like 'a missing module version list resource'
+    end
   end
 
   describe 'GET /projects/:id/packages/go/*module_name/@v/:module_version.info' do
@@ -356,26 +370,31 @@ describe API::GoProxy do
 
       it 'returns ok with an oauth token' do
         get_resource(oauth_access_token: oauth)
+
         expect(response).to have_gitlab_http_status(:ok)
       end
 
       it 'returns ok with a job token' do
         get_resource(oauth_access_token: job)
+
         expect(response).to have_gitlab_http_status(:ok)
       end
 
       it 'returns ok with a personal access token' do
         get_resource(personal_access_token: pa_token)
+
         expect(response).to have_gitlab_http_status(:ok)
       end
 
       it 'returns ok with a personal access token and basic authentication' do
         get_resource(headers: build_basic_auth_header(user.username, pa_token.token))
+
         expect(response).to have_gitlab_http_status(:ok)
       end
 
       it 'returns unauthorized with no authentication' do
         get_resource
+
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end
@@ -393,6 +412,7 @@ describe API::GoProxy do
 
       it 'returns ok with no authentication' do
         get_resource
+
         expect(response).to have_gitlab_http_status(:ok)
       end
     end
@@ -406,26 +426,31 @@ describe API::GoProxy do
     describe 'GET /projects/:id/packages/go/*module_name/@v/list' do
       it 'returns not found with a user' do
         get_resource(user)
+
         expect(response).to have_gitlab_http_status(:not_found)
       end
 
       it 'returns not found with an oauth token' do
         get_resource(oauth_access_token: oauth)
+
         expect(response).to have_gitlab_http_status(:not_found)
       end
 
       it 'returns not found with a job token' do
         get_resource(oauth_access_token: job)
+
         expect(response).to have_gitlab_http_status(:not_found)
       end
 
       it 'returns not found with a personal access token' do
         get_resource(personal_access_token: pa_token)
+
         expect(response).to have_gitlab_http_status(:not_found)
       end
 
       it 'returns unauthorized with no authentication' do
         get_resource
+
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
     end