Repository: Add getIdentifier() method to avoid instanceof operator

This change is needed to implement permission aware ref database in
Gerrit: [1], that is a pre-requisite to re-enable Git v2 protocol in
Gerrit: [2].

Background: Last year Git v2 protocol was enabled in Gerrit. The fact,
that JGit layer was not calling ref advertise filter for Git v2
protocol, introduced security vulnerability.

The lesson learned from this security incident: Gerrit should not rely
on ref advertise filter being called by JGit to implement crictical
security checks. Instead, the idea is to use the same approach as
currently used by Google's internal code on googlesource.com that
didn't suffer from this vulnerability: provide a custom repository to
JGit. The repository provides a RefDatabase that is permission-aware
and will only ever return refs that the user has access to.

However, due to hard coded instanceof operator usages in JGit code
base, some tests in Gerrit are failing with: [1] in place. This change
addresses this problem.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/212874
[2] https://gerrit-review.googlesource.com/c/gerrit/+/226754

Change-Id: I67c0f53ca33b149442e7ee3e51910d19e3f348d5
Signed-off-by: David Ostrovsky <david@ostrovsky.org>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ServletUtils.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ServletUtils.java
index b6d73b5591c08e51a1b4665ec04086beffbf8524..256279bfed508b5881804ac3cfe7f877a7503771 100644 (file)
--- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ServletUtils.java
+++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ServletUtils.java
@@ -64,7 +64,6 @@ import javax.servlet.ServletRequest;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-import org.eclipse.jgit.internal.storage.dfs.DfsRepository;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
@@ -276,12 +275,11 @@ public final class ServletUtils {
        }
 
        static String identify(Repository git) {
-               if (git instanceof DfsRepository) {
-                       return ((DfsRepository) git).getDescription().getRepositoryName();
-               } else if (git.getDirectory() != null) {
-                       return git.getDirectory().getPath();
+               String identifier = git.getIdentifier();
+               if (identifier == null) {
+                       return "unknown";
                }
-               return "unknown";
+               return identifier;
        }
 
        private ServletUtils() {

diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters
index dc1df596330222bb6a83d6ce2bb1d9b9624ea25f..7f93191ca6ba6141573e922abf586011c3872a34 100644 (file)
--- a/org.eclipse.jgit/.settings/.api_filters
+++ b/org.eclipse.jgit/.settings/.api_filters
@@ -22,6 +22,14 @@
             </message_arguments>
         </filter>
     </resource>
+    <resource path="src/org/eclipse/jgit/lib/Repository.java" type="org.eclipse.jgit.lib.Repository">
+        <filter id="336695337">
+            <message_arguments>
+                <message_argument value="org.eclipse.jgit.lib.Repository"/>
+                <message_argument value="getIdentifier()"/>
+            </message_arguments>
+        </filter>
+    </resource>
     <resource path="src/org/eclipse/jgit/revwalk/ObjectWalk.java" type="org.eclipse.jgit.revwalk.ObjectWalk">
         <filter comment="ignore the risk subclasses could define the same field and cause a name clash" id="336658481">
             <message_arguments>

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRepository.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRepository.java
index 5169e929e4e5b82ec89ee0f15bd10c38d5c8ffe1..8e5c5a7f75b3e6b14a6a1fb7c60e810379ef78fa 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRepository.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRepository.java
@@ -124,6 +124,12 @@ public abstract class DfsRepository extends Repository {
                return config;
        }
 
+       /** {@inheritDoc} */
+       @Override
+       public String getIdentifier() {
+               return getDescription().getRepositoryName();
+       }
+
        /** {@inheritDoc} */
        @Override
        public void scanForRepoChanges() throws IOException {

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileRepository.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileRepository.java
index d82d29e4cf82e5ff8a8e4f947ab01ad8b2fb13d4..90772970ae7f27597c023afb6fbf7928106cf49e 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileRepository.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileRepository.java
@@ -388,6 +388,17 @@ public class FileRepository extends Repository {
                return refs;
        }
 
+       /** {@inheritDoc} */
+       @Override
+       public String getIdentifier() {
+               File directory = getDirectory();
+               if (directory != null) {
+                       return directory.getPath();
+               } else {
+                       throw new IllegalStateException();
+               }
+       }
+
        /** {@inheritDoc} */
        @Override
        public FileBasedConfig getConfig() {

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
index aac63e9d2435f9f604d2306a697a07192fd8ab6e..d53b0c926a6c655f06e515103aab1970b72fa0c5 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
@@ -239,6 +239,15 @@ public abstract class Repository implements AutoCloseable {
                return gitDir;
        }
 
+       /**
+        * Get repository identifier.
+        *
+        * @return repository identifier. The returned identifier has to be unique
+        *         within a given Git server.
+        * @since 5.4
+        */
+       public abstract String getIdentifier();
+
        /**
         * Get the object database which stores this repository's data.
         *

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/HMACSHA1NonceGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/HMACSHA1NonceGenerator.java
index 53eaa6a7f9151e4c23e0f9a59190ff86ccd4b61d..01f6fec7e48f671c0578c01f19d7b78f9bc1b059 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/HMACSHA1NonceGenerator.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/HMACSHA1NonceGenerator.java
@@ -45,14 +45,12 @@ package org.eclipse.jgit.transport;
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
-import java.io.File;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
 
 import javax.crypto.Mac;
 import javax.crypto.spec.SecretKeySpec;
 
-import org.eclipse.jgit.internal.storage.dfs.DfsRepository;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.PushCertificate.NonceStatus;
 
@@ -87,19 +85,7 @@ public class HMACSHA1NonceGenerator implements NonceGenerator {
        @Override
        public synchronized String createNonce(Repository repo, long timestamp)
                        throws IllegalStateException {
-               String path;
-               if (repo instanceof DfsRepository) {
-                       path = ((DfsRepository) repo).getDescription().getRepositoryName();
-               } else {
-                       File directory = repo.getDirectory();
-                       if (directory != null) {
-                               path = directory.getPath();
-                       } else {
-                               throw new IllegalStateException();
-                       }
-               }
-
-               String input = path + ":" + String.valueOf(timestamp); //$NON-NLS-1$
+               String input = repo.getIdentifier() + ":" + String.valueOf(timestamp); //$NON-NLS-1$
                byte[] rawHmac = mac.doFinal(input.getBytes(UTF_8));
                return Long.toString(timestamp) + "-" + toHex(rawHmac); //$NON-NLS-1$
        }