From a4462f0ca1d64ca9b9033ae6d38d75b91d886f67 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= <alejorro70@gmail.com>
Date: Mon, 29 Oct 2018 21:34:08 -0300
Subject: [PATCH] Don't reprocess v2 auth tokens

For remote repository calls we receive and pass v2 tokens, so we don't
need (and also it would generate an invalid token) to re-HMAC it.
---
 auth/rpccredentials.go                        | 17 +++++++++-
 auth/token.go                                 |  4 +++
 .../unreleased/remote-repo-token-v2.yml       |  5 +++
 cmd/gitaly-ssh/main.go                        |  2 +-
 .../gitlab/git/gitaly_remote_repository.rb    | 10 ++++--
 .../git/gitaly_remote_repository_spec.rb      | 33 +++++++++++++++++++
 6 files changed, 66 insertions(+), 5 deletions(-)
 create mode 100644 changelogs/unreleased/remote-repo-token-v2.yml
 create mode 100644 ruby/spec/lib/gitlab/git/gitaly_remote_repository_spec.rb

diff --git a/auth/rpccredentials.go b/auth/rpccredentials.go
index c35cd4d6171..12c5258f68a 100644
--- a/auth/rpccredentials.go
+++ b/auth/rpccredentials.go
@@ -41,7 +41,22 @@ type rpcCredentialsV2 struct {
 func (*rpcCredentialsV2) RequireTransportSecurity() bool { return false }
 
 func (rc *rpcCredentialsV2) GetRequestMetadata(context.Context, ...string) (map[string]string, error) {
-	return map[string]string{"authorization": "Bearer " + rc.hmacToken()}, nil
+	// Check if token is already a v2 auth token
+	// TODO: Remove support for direct secret handling (i.e. v1 auth scheme)
+	// https://gitlab.com/gitlab-org/gitaly/issues/1388
+	authInfo, err := parseToken(rc.token)
+	if err != nil {
+		return nil, err
+	}
+
+	var authToken string
+	if authInfo.Version == "v2" {
+		authToken = rc.token
+	} else {
+		authToken = rc.hmacToken()
+	}
+
+	return map[string]string{"authorization": "Bearer " + authToken}, nil
 }
 
 func (rc *rpcCredentialsV2) hmacToken() string {
diff --git a/auth/token.go b/auth/token.go
index e1c196615b4..77127f3515b 100644
--- a/auth/token.go
+++ b/auth/token.go
@@ -76,6 +76,10 @@ func ExtractAuthInfo(ctx context.Context) (*AuthInfo, error) {
 		return nil, err
 	}
 
+	return parseToken(token)
+}
+
+func parseToken(token string) (*AuthInfo, error) {
 	split := strings.SplitN(string(token), ".", 3)
 
 	// v1 is base64-encoded using base64.StdEncoding, which cannot contain a ".".
diff --git a/changelogs/unreleased/remote-repo-token-v2.yml b/changelogs/unreleased/remote-repo-token-v2.yml
new file mode 100644
index 00000000000..396b553db05
--- /dev/null
+++ b/changelogs/unreleased/remote-repo-token-v2.yml
@@ -0,0 +1,5 @@
+---
+title: Don't reprocess v2 tokens in GitalyRemoteRepository
+merge_request: 948
+author:
+type: changed
diff --git a/cmd/gitaly-ssh/main.go b/cmd/gitaly-ssh/main.go
index 0481741332e..2f790568647 100644
--- a/cmd/gitaly-ssh/main.go
+++ b/cmd/gitaly-ssh/main.go
@@ -52,7 +52,7 @@ func main() {
 func dialOpts() []grpc.DialOption {
 	connOpts := client.DefaultDialOpts
 	if token := os.Getenv("GITALY_TOKEN"); token != "" {
-		connOpts = append(connOpts, grpc.WithPerRPCCredentials(gitalyauth.RPCCredentials(token)))
+		connOpts = append(connOpts, grpc.WithPerRPCCredentials(gitalyauth.RPCCredentialsV2(token)))
 	}
 
 	return connOpts
diff --git a/ruby/lib/gitlab/git/gitaly_remote_repository.rb b/ruby/lib/gitlab/git/gitaly_remote_repository.rb
index ab0380ed741..d841c52db4a 100644
--- a/ruby/lib/gitlab/git/gitaly_remote_repository.rb
+++ b/ruby/lib/gitlab/git/gitaly_remote_repository.rb
@@ -56,13 +56,13 @@ module Gitlab
       end
 
       def token
-        gitaly_client.token(storage).to_s
+        @token ||= gitaly_client.token(storage).to_s
       end
 
       def request_kwargs
         @request_kwargs ||= begin
           metadata = {
-            'authorization' => "Bearer #{auhtorization_token}",
+            'authorization' => "Bearer #{authorization_token}",
             'client_name' => CLIENT_NAME
           }
 
@@ -70,7 +70,11 @@ module Gitlab
         end
       end
 
-      def auhtorization_token
+      def authorization_token
+        # TODO: Remove support for direct secret handling (i.e. v1 auth scheme)
+        # https://gitlab.com/gitlab-org/gitaly/issues/1388
+        return token if token.split('.')[0] == 'v2'
+
         issued_at = Time.now.to_i.to_s
         hmac = OpenSSL::HMAC.hexdigest(OpenSSL::Digest::SHA256.new, token, issued_at)
 
diff --git a/ruby/spec/lib/gitlab/git/gitaly_remote_repository_spec.rb b/ruby/spec/lib/gitlab/git/gitaly_remote_repository_spec.rb
new file mode 100644
index 00000000000..e0a5f9a6e78
--- /dev/null
+++ b/ruby/spec/lib/gitlab/git/gitaly_remote_repository_spec.rb
@@ -0,0 +1,33 @@
+require 'spec_helper'
+
+describe Gitlab::Git::GitalyRemoteRepository do
+  include TestRepo
+
+  let(:gitaly_repository) { git_test_repo_read_only }
+  let(:token) { 'foobar' }
+  let(:gitaly_servers) do
+    Base64.strict_encode64({ 'default' => { 'token' => token } }.to_json)
+  end
+  let(:call) { double(:call, metadata: { 'gitaly-servers' => gitaly_servers }) }
+  let(:gitaly_remote_repository) do
+    described_class.new(gitaly_repository, call)
+  end
+
+  describe '#request_kwargs' do
+    subject(:auth_token) do
+      gitaly_remote_repository.send(:request_kwargs)[:metadata]['authorization']
+    end
+
+    it 'generates a v2 authentication token' do
+      expect(auth_token).to match(/Bearer v2\..+/)
+    end
+
+    context 'when the token is already a v2 authentication token' do
+      let(:token) { 'v2.foo.bar' }
+
+      it 'includes the authentication token' do
+        expect(auth_token).to match(token)
+      end
+    end
+  end
+end
-- 
GitLab