Encrypt oauth token at rest
The oauth token provided by Github is used together with the
`github_oauth` schema, to generate a new external identity for the
Gerrit user.
The external-id is then persisted on disk as a note under the
`refs/meta/external-ids` ref of the `All-Users.git` repo, as documented
in [1].
All credentials should be stored encrypted and protected at-rest, as the
token could be used to impersonate the user associated with it.
This vulnerability has been exploited in the past on other OAuth-based
integrations [2].
Avoid using plaintext oauth tokens for storing github external-ids by
encrypting them first.
[1] https://gerrit-documentation.storage.googleapis.com/Documentation/3.6.1/config-accounts.html#external-ids
[2] https://github.blog/2022-04-15-security-alert-stolen-oauth-user-tokens/
Bug: Issue 16192
Change-Id: I69c2add9170159ab8b9a31098cf35f184e678401
diff --git a/github-oauth/pom.xml b/github-oauth/pom.xml
index dc8b456..21631aa 100644
--- a/github-oauth/pom.xml
+++ b/github-oauth/pom.xml
@@ -120,5 +120,10 @@
<version>4.4</version>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <scope>test</scope>
+ </dependency>
</dependencies>
</project>
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/CipherException.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/CipherException.java
new file mode 100644
index 0000000..6ab74d0
--- /dev/null
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/CipherException.java
@@ -0,0 +1,34 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.googlesource.gerrit.plugins.github.oauth;
+
+import java.io.IOException;
+
+/**
+ * Signals that a cipher exception has occurred. This class can be used to represent exception for
+ * both encryption and decryption failures
+ */
+public class CipherException extends IOException {
+ private static final long serialVersionUID = 1L;
+
+ /**
+ * Constructs a {@code CipherException} with the specified detail message and cause
+ *
+ * @param message The detail message of the failure
+ * @param cause The cause of the failure
+ */
+ public CipherException(String message, Exception cause) {
+ super(message, cause);
+ }
+}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java
index 91a17d8..a826886 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java
@@ -24,22 +24,32 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.Scope;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
import java.util.stream.Collectors;
import javax.servlet.http.HttpServletRequest;
import lombok.Getter;
import org.eclipse.jgit.lib.Config;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
@Singleton
public class GitHubOAuthConfig {
+ private static Logger logger = LoggerFactory.getLogger(GitHubOAuthConfig.class);
+
private final Config config;
private final CanonicalWebUrl canonicalWebUrl;
public static final String CONF_SECTION = "github";
+ public static final String CONF_KEY_SECTION = "github-key";
public static final String GITHUB_OAUTH_AUTHORIZE = "/login/oauth/authorize";
public static final String GITHUB_OAUTH_ACCESS_TOKEN = "/login/oauth/access_token";
public static final String GERRIT_OAUTH_FINAL = "/oauth";
@@ -68,6 +78,8 @@
public final long httpConnectionTimeout;
public final long httpReadTimeout;
+ private final Map<String, KeyConfig> keyConfigMap;
+ private final KeyConfig currentKeyConfig;
@Inject
protected GitHubOAuthConfig(@GerritServerConfig Config config, CanonicalWebUrl canonicalWebUrl) {
@@ -121,6 +133,30 @@
ConfigUtil.getTimeUnit(
config, CONF_SECTION, null, "httpReadTimeout", 30, TimeUnit.SECONDS),
TimeUnit.SECONDS);
+
+ Map<String, KeyConfig> configuredKeyConfig =
+ config.getSubsections(CONF_KEY_SECTION).stream()
+ .map(KeyConfig::new)
+ .collect(Collectors.toMap(KeyConfig::getKeyId, Function.identity()));
+
+ if (configuredKeyConfig.isEmpty()) {
+ logger.warn(
+ "No configured '{}' sections found. Default configuration should NOT be used in production code.",
+ CONF_KEY_SECTION);
+ currentKeyConfig = new KeyConfig();
+ keyConfigMap = Map.of(currentKeyConfig.getKeyId(), currentKeyConfig);
+ } else {
+ keyConfigMap = configuredKeyConfig;
+ List<KeyConfig> currentKeyConfigs =
+ keyConfigMap.values().stream().filter(KeyConfig::isCurrent).collect(Collectors.toList());
+ if (currentKeyConfigs.size() != 1) {
+ throw new IllegalStateException(
+ String.format(
+ "Expected exactly 1 subsection of '%s' to be configured as 'current', %d found",
+ CONF_KEY_SECTION, currentKeyConfigs.size()));
+ }
+ currentKeyConfig = currentKeyConfigs.get(0);
+ }
}
public String getOAuthFinalRedirectUrl(HttpServletRequest req) {
@@ -173,4 +209,106 @@
}
return scopes.get("scopes").toArray(new Scope[0]);
}
+
+ public KeyConfig getCurrentKeyConfig() {
+ return currentKeyConfig;
+ }
+
+ public KeyConfig getKeyConfig(String subsection) {
+ return keyConfigMap.get(subsection);
+ }
+
+ public class KeyConfig {
+
+ public static final String PASSWORD_DEVICE_DEFAULT = "/dev/zero";
+ public static final int PASSWORD_LENGTH_DEFAULT = 16;
+ public static final String CIPHER_ALGORITHM_DEFAULT = "AES/ECB/PKCS5Padding";
+ public static final String SECRET_KEY_ALGORITHM_DEFAULT = "AES";
+ public static final boolean IS_CURRENT_DEFAULT = false;
+ public static final String KEY_ID_DEFAULT = "current";
+
+ public static final String PASSWORD_DEVICE_CONFIG_LABEL = "passwordDevice";
+ public static final String PASSWORD_LENGTH_CONFIG_LABEL = "passwordLength";
+ public static final String SECRET_KEY_CONFIG_LABEL = "secretKeyAlgorithm";
+ public static final String CIPHER_ALGO_CONFIG_LABEL = "cipherAlgorithm";
+ public static final String CURRENT_CONFIG_LABEL = "current";
+
+ public static final String KEY_DELIMITER = ":";
+
+ private final String passwordDevice;
+ private final Integer passwordLength;
+ private final String cipherAlgorithm;
+ private final String secretKeyAlgorithm;
+ private final String keyId;
+ private final Boolean isCurrent;
+
+ KeyConfig(String keyId) {
+
+ if (keyId.contains(KEY_DELIMITER)) {
+ throw new IllegalStateException(
+ String.format(
+ "Configuration error. %s.%s should not contain '%s'",
+ CONF_KEY_SECTION, keyId, KEY_DELIMITER));
+ }
+
+ this.passwordDevice =
+ trimTrailingSlash(
+ MoreObjects.firstNonNull(
+ config.getString(CONF_KEY_SECTION, keyId, PASSWORD_DEVICE_CONFIG_LABEL),
+ PASSWORD_DEVICE_DEFAULT));
+ this.passwordLength =
+ config.getInt(
+ CONF_KEY_SECTION, keyId, PASSWORD_LENGTH_CONFIG_LABEL, PASSWORD_LENGTH_DEFAULT);
+ isCurrent =
+ config.getBoolean(CONF_KEY_SECTION, keyId, CURRENT_CONFIG_LABEL, IS_CURRENT_DEFAULT);
+
+ this.cipherAlgorithm =
+ MoreObjects.firstNonNull(
+ config.getString(CONF_KEY_SECTION, keyId, CIPHER_ALGO_CONFIG_LABEL),
+ CIPHER_ALGORITHM_DEFAULT);
+
+ this.secretKeyAlgorithm =
+ MoreObjects.firstNonNull(
+ config.getString(CONF_KEY_SECTION, keyId, SECRET_KEY_CONFIG_LABEL),
+ SECRET_KEY_ALGORITHM_DEFAULT);
+ this.keyId = keyId;
+ }
+
+ private KeyConfig() {
+ passwordDevice = PASSWORD_DEVICE_DEFAULT;
+ passwordLength = PASSWORD_LENGTH_DEFAULT;
+ isCurrent = true;
+ cipherAlgorithm = CIPHER_ALGORITHM_DEFAULT;
+ keyId = KEY_ID_DEFAULT;
+ secretKeyAlgorithm = SECRET_KEY_ALGORITHM_DEFAULT;
+ }
+
+ public byte[] readPassword() throws IOException {
+ Path devicePath = Paths.get(passwordDevice);
+ try (FileInputStream in = new FileInputStream(devicePath.toFile())) {
+ byte[] passphrase = new byte[passwordLength];
+ if (in.read(passphrase) < passwordLength) {
+ throw new IOException("End of password device has already been reached");
+ }
+
+ return passphrase;
+ }
+ }
+
+ public String getCipherAlgorithm() {
+ return cipherAlgorithm;
+ }
+
+ public String getSecretKeyAlgorithm() {
+ return secretKeyAlgorithm;
+ }
+
+ public Boolean isCurrent() {
+ return isCurrent;
+ }
+
+ public String getKeyId() {
+ return keyId;
+ }
+ }
}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/IdentifiedUserGitHubLoginProvider.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/IdentifiedUserGitHubLoginProvider.java
index b953531..5449347 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/IdentifiedUserGitHubLoginProvider.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/IdentifiedUserGitHubLoginProvider.java
@@ -39,17 +39,20 @@
private final GitHubOAuthConfig config;
private final AccountCache accountCache;
private final GitHubHttpConnector httpConnector;
+ private final OAuthTokenCipher oAuthTokenCipher;
@Inject
public IdentifiedUserGitHubLoginProvider(
Provider<IdentifiedUser> identifiedUserProvider,
GitHubOAuthConfig config,
GitHubHttpConnector httpConnector,
- AccountCache accountCache) {
+ AccountCache accountCache,
+ OAuthTokenCipher oAuthTokenCipher) {
this.userProvider = identifiedUserProvider;
this.config = config;
this.accountCache = accountCache;
this.httpConnector = httpConnector;
+ this.oAuthTokenCipher = oAuthTokenCipher;
}
@Override
@@ -75,16 +78,17 @@
}
}
- private AccessToken newAccessTokenFromUser(String username) {
+ private AccessToken newAccessTokenFromUser(String username) throws IOException {
AccountState account = accountCache.getByUsername(username).get();
Collection<ExternalId> externalIds = account.externalIds();
for (ExternalId accountExternalId : externalIds) {
String key = accountExternalId.key().get();
if (key.startsWith(EXTERNAL_ID_PREFIX)) {
- return new AccessToken(key.substring(EXTERNAL_ID_PREFIX.length()));
+ String encryptedOauthToken = key.substring(EXTERNAL_ID_PREFIX.length());
+ String decryptedOauthToken = oAuthTokenCipher.decrypt(encryptedOauthToken);
+ return new AccessToken(decryptedOauthToken);
}
}
-
return null;
}
}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipher.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipher.java
new file mode 100644
index 0000000..a8133dc
--- /dev/null
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipher.java
@@ -0,0 +1,168 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.googlesource.gerrit.plugins.github.oauth;
+
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.KEY_DELIMITER;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Splitter;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.util.Base64;
+import java.util.List;
+import javax.crypto.BadPaddingException;
+import javax.crypto.Cipher;
+import javax.crypto.IllegalBlockSizeException;
+import javax.crypto.NoSuchPaddingException;
+import javax.crypto.spec.SecretKeySpec;
+
+/** Provides the ability to encrypt and decrypt an OAuth token */
+@Singleton
+public class OAuthTokenCipher {
+ private final String currentCipherAlgorithm;
+ private final SecretKeySpec currentSecretKey;
+ private final String currentKeyId;
+ private final GitHubOAuthConfig config;
+
+ /**
+ * Constructs a {@code OAuthTokenCipher} using cipher algorithm specified in configuration
+ *
+ * @param config the github oauth configuration object
+ * @throws IOException when the cipher could not be constructed
+ */
+ @Inject
+ public OAuthTokenCipher(GitHubOAuthConfig config) throws IOException {
+ GitHubOAuthConfig.KeyConfig currentKeyConfig = config.getCurrentKeyConfig();
+ currentKeyId = currentKeyConfig.getKeyId();
+ currentCipherAlgorithm = currentKeyConfig.getCipherAlgorithm();
+ currentSecretKey =
+ new SecretKeySpec(
+ currentKeyConfig.readPassword(), currentKeyConfig.getSecretKeyAlgorithm());
+ this.config = config;
+ }
+
+ /**
+ * Encrypts the provided string and returns its base64 representation, prefixed with the name of
+ * the configuration subsection used to encrypt it, separated by ':'.
+ *
+ * <p>For example:
+ *
+ * <p>current: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
+ *
+ * @param plainText the string to encrypt
+ * @return the base64-encoded encrypted string
+ * @throws CipherException when the string could not be encrypted
+ */
+ public String encrypt(final String plainText) throws CipherException {
+ try {
+ return prependCurrentKeyId(
+ Base64.getEncoder()
+ .encodeToString(
+ initCurrentCipherForEncryption()
+ .doFinal(plainText.getBytes(StandardCharsets.UTF_8))));
+ } catch (IllegalBlockSizeException | BadPaddingException e) {
+ throw new CipherException("Could not encrypt oauth token", e);
+ }
+ }
+
+ /**
+ * Decrypts the provided base64-encoded encrypted string, prefixed with the name of the
+ * configuration subsection used to encrypt it, separated by ':'.
+ *
+ * <p>For example:
+ *
+ * <p>current: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
+ *
+ * <p>In order to provide back-compatibility with plaintext oauth tokens that were stored before
+ * encryption was introduced, it will return the input string as-is, when the string is not
+ * prefixed by a key-id
+ *
+ * @param base64EncryptedString the string to decrypt
+ * @return the plainText string
+ * @throws CipherException when the string could not be decrypted
+ */
+ public String decrypt(final String base64EncryptedString) throws CipherException {
+ try {
+
+ if (isPrefixedWithKeyId(base64EncryptedString)) {
+ List<String> keyIdAndMaterial = splitKeyIdFromMaterial(base64EncryptedString);
+ String keyId = keyIdAndMaterial.get(0);
+ String material = keyIdAndMaterial.get(1);
+ Cipher decryptCipher = getCipherFor(keyId);
+ return new String(
+ decryptCipher.doFinal(Base64.getDecoder().decode(material)), StandardCharsets.UTF_8);
+ }
+ return base64EncryptedString;
+
+ } catch (IllegalStateException
+ | IllegalArgumentException
+ | IllegalBlockSizeException
+ | BadPaddingException
+ | IOException e) {
+ throw new CipherException("Could not decrypt oauth token", e);
+ }
+ }
+
+ private static Cipher initCipher(String cipherAlgorithm, SecretKeySpec secretKey, int mode)
+ throws CipherException {
+ try {
+ Cipher cipher = Cipher.getInstance(cipherAlgorithm);
+ cipher.init(mode, secretKey);
+ return cipher;
+ } catch (NoSuchPaddingException | NoSuchAlgorithmException | InvalidKeyException e) {
+ throw new CipherException("Could not init cipher", e);
+ }
+ }
+
+ private Cipher initCurrentCipherForEncryption() throws CipherException {
+ return initCipher(currentCipherAlgorithm, currentSecretKey, Cipher.ENCRYPT_MODE);
+ }
+
+ private String prependCurrentKeyId(String base64EncodedString) {
+ return String.format("%s%s%s", currentKeyId, KEY_DELIMITER, base64EncodedString);
+ }
+
+ private static boolean isPrefixedWithKeyId(String maybeEncryptedString) {
+ return maybeEncryptedString.contains(KEY_DELIMITER);
+ }
+
+ @VisibleForTesting
+ static List<String> splitKeyIdFromMaterial(String base64EncryptedString) {
+ List<String> tokens = Splitter.on(KEY_DELIMITER).splitToList(base64EncryptedString);
+ int nOfTokens = tokens.size();
+ if (nOfTokens != 2) {
+ throw new IllegalStateException(
+ String.format(
+ "The encrypted key is expected to contain 2 tokens (keyId:key), whereas it contains %d tokens",
+ nOfTokens));
+ }
+ return tokens;
+ }
+
+ private Cipher getCipherFor(String keyId) throws IOException {
+ GitHubOAuthConfig.KeyConfig keyConfig = config.getKeyConfig(keyId);
+ if (keyConfig == null) {
+ throw new IllegalStateException(
+ String.format("Could not find key-id '%s' in configuration", keyId));
+ }
+ return initCipher(
+ keyConfig.getCipherAlgorithm(),
+ new SecretKeySpec(keyConfig.readPassword(), keyConfig.getSecretKeyAlgorithm()),
+ Cipher.DECRYPT_MODE);
+ }
+}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java
index 87f56e5..3c72141 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthWebFilter.java
@@ -49,6 +49,7 @@
private final SitePaths sites;
private final ScopedProvider<GitHubLogin> loginProvider;
private final OAuthProtocol oauth;
+ private final OAuthTokenCipher oAuthTokenCipher;
@Inject
public OAuthWebFilter(
@@ -57,11 +58,13 @@
OAuthProtocol oauth,
// We need to explicitly tell Guice the correct implementation
// as this filter is instantiated with a standard Gerrit WebModule
- GitHubLogin.Provider loginProvider) {
+ GitHubLogin.Provider loginProvider,
+ OAuthTokenCipher oAuthTokenCipher) {
this.config = config;
this.sites = sites;
this.oauth = oauth;
this.loginProvider = loginProvider;
+ this.oAuthTokenCipher = oAuthTokenCipher;
}
@Override
@@ -88,13 +91,14 @@
}
if (ghLogin != null && ghLogin.isLoggedIn()) {
+ String hashedToken = oAuthTokenCipher.encrypt(ghLogin.getToken().accessToken);
httpRequest =
new AuthenticatedHttpRequest(
httpRequest,
config.httpHeader,
ghLogin.getMyself().getLogin(),
config.oauthHttpHeader,
- GITHUB_EXT_ID + ghLogin.getToken().accessToken);
+ GITHUB_EXT_ID + hashedToken);
}
chain.doFilter(httpRequest, httpResponse);
diff --git a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java
new file mode 100644
index 0000000..94d1a43
--- /dev/null
+++ b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfigTest.java
@@ -0,0 +1,151 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.googlesource.gerrit.plugins.github.oauth;
+
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.CONF_KEY_SECTION;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.CONF_SECTION;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.CIPHER_ALGORITHM_DEFAULT;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.CIPHER_ALGO_CONFIG_LABEL;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.CURRENT_CONFIG_LABEL;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.KEY_DELIMITER;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.KEY_ID_DEFAULT;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.SECRET_KEY_ALGORITHM_DEFAULT;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.SECRET_KEY_CONFIG_LABEL;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
+
+import com.google.gerrit.extensions.client.AuthType;
+import com.google.gerrit.httpd.CanonicalWebUrl;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.util.Providers;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Before;
+import org.junit.Test;
+
+public class GitHubOAuthConfigTest {
+
+ CanonicalWebUrl canonicalWebUrl;
+ Config config;
+
+ @Before
+ public void setUp() {
+ config = new Config();
+ config.setString(CONF_SECTION, null, "clientSecret", "theSecret");
+ config.setString(CONF_SECTION, null, "clientId", "theClientId");
+ config.setString("auth", null, "httpHeader", "GITHUB_USER");
+ config.setString("auth", null, "type", AuthType.HTTP.toString());
+
+ canonicalWebUrl =
+ Guice.createInjector(
+ new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(String.class)
+ .annotatedWith(com.google.gerrit.server.config.CanonicalWebUrl.class)
+ .toProvider(Providers.of(null));
+ }
+ })
+ .getInstance(CanonicalWebUrl.class);
+ }
+
+ @Test
+ public void shouldDefaultKeyConfigWhenNoSpecificConfigurationIsSet() {
+ GitHubOAuthConfig objectUnderTest = objectUnderTest();
+ assertEquals(objectUnderTest.getCurrentKeyConfig().isCurrent(), true);
+ assertEquals(
+ objectUnderTest.getCurrentKeyConfig().getCipherAlgorithm(), CIPHER_ALGORITHM_DEFAULT);
+ assertEquals(
+ objectUnderTest.getCurrentKeyConfig().getSecretKeyAlgorithm(),
+ SECRET_KEY_ALGORITHM_DEFAULT);
+ assertEquals(objectUnderTest.getCurrentKeyConfig().getKeyId(), KEY_ID_DEFAULT);
+ }
+
+ @Test
+ public void shouldDBeAbleToLookupDefaultKeyWhenNoSpecificConfigurationIsSet() {
+ assertEquals(objectUnderTest().getKeyConfig(KEY_ID_DEFAULT).isCurrent(), true);
+ }
+
+ @Test
+ public void shouldReadASpecificKeyConfig() {
+ String keySubsection = "someKeyConfig";
+ String cipherAlgorithm = "AES/CFB8/NoPadding";
+ String secretKeyAlgorithm = "DES";
+ config.setBoolean(CONF_KEY_SECTION, keySubsection, CURRENT_CONFIG_LABEL, true);
+ config.setString(CONF_KEY_SECTION, keySubsection, CIPHER_ALGO_CONFIG_LABEL, cipherAlgorithm);
+ config.setString(CONF_KEY_SECTION, keySubsection, SECRET_KEY_CONFIG_LABEL, secretKeyAlgorithm);
+
+ GitHubOAuthConfig objectUnderTest = objectUnderTest();
+
+ assertEquals(objectUnderTest.getCurrentKeyConfig().isCurrent(), true);
+ assertEquals(objectUnderTest.getCurrentKeyConfig().getCipherAlgorithm(), cipherAlgorithm);
+ assertEquals(objectUnderTest.getCurrentKeyConfig().getSecretKeyAlgorithm(), secretKeyAlgorithm);
+ assertEquals(objectUnderTest.getCurrentKeyConfig().getKeyId(), keySubsection);
+ }
+
+ @Test
+ public void shouldReturnTheExpectedKeyConfigAsCurrent() {
+ config.setBoolean(CONF_KEY_SECTION, "currentKeyConfig", CURRENT_CONFIG_LABEL, true);
+ config.setBoolean(CONF_KEY_SECTION, "someOtherKeyConfig", CURRENT_CONFIG_LABEL, false);
+
+ assertEquals(objectUnderTest().getCurrentKeyConfig().getKeyId(), "currentKeyConfig");
+ }
+
+ @Test
+ public void shouldReadMultipleKeyConfigs() {
+ String currentKeyConfig = "currentKeyConfig";
+ String someOtherKeyConfig = "someOtherKeyConfig";
+ config.setBoolean(CONF_KEY_SECTION, currentKeyConfig, CURRENT_CONFIG_LABEL, true);
+ config.setBoolean(CONF_KEY_SECTION, someOtherKeyConfig, CURRENT_CONFIG_LABEL, false);
+
+ GitHubOAuthConfig objectUnderTest = objectUnderTest();
+
+ assertEquals(objectUnderTest.getKeyConfig(currentKeyConfig).getKeyId(), currentKeyConfig);
+ assertEquals(objectUnderTest.getKeyConfig(someOtherKeyConfig).getKeyId(), someOtherKeyConfig);
+ }
+
+ @Test
+ public void shouldThrowWhenNoKeyConfigIsSetAsCurrent() {
+ config.setBoolean(CONF_KEY_SECTION, "someKeyConfig", CURRENT_CONFIG_LABEL, false);
+
+ assertThrows(IllegalStateException.class, this::objectUnderTest);
+ }
+
+ @Test
+ public void shouldThrowWhenKeyConfigContainsDelimiterCharacter() {
+ String invalidSubsection = "foo" + KEY_DELIMITER + "bar";
+ config.setBoolean(CONF_KEY_SECTION, invalidSubsection, CURRENT_CONFIG_LABEL, false);
+
+ IllegalStateException illegalStateException =
+ assertThrows(IllegalStateException.class, this::objectUnderTest);
+
+ assertEquals(
+ illegalStateException.getMessage(),
+ String.format(
+ "Configuration error. %s.%s should not contain '%s'",
+ CONF_KEY_SECTION, invalidSubsection, KEY_DELIMITER));
+ }
+
+ @Test
+ public void shouldThrowWhenMoreThanOneKeyConfigIsSetAsCurrent() {
+ config.setBoolean(CONF_KEY_SECTION, "someKeyConfig", CURRENT_CONFIG_LABEL, true);
+ config.setBoolean(CONF_KEY_SECTION, "someOtherKeyConfig", CURRENT_CONFIG_LABEL, true);
+
+ assertThrows(IllegalStateException.class, this::objectUnderTest);
+ }
+
+ private GitHubOAuthConfig objectUnderTest() {
+ return new GitHubOAuthConfig(config, canonicalWebUrl);
+ }
+}
diff --git a/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipherTest.java b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipherTest.java
new file mode 100644
index 0000000..9621fc5
--- /dev/null
+++ b/github-oauth/src/test/java/com/googlesource/gerrit/plugins/github/oauth/OAuthTokenCipherTest.java
@@ -0,0 +1,165 @@
+// Copyright (C) 2022 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.googlesource.gerrit.plugins.github.oauth;
+
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.CONF_KEY_SECTION;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.CONF_SECTION;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.CIPHER_ALGO_CONFIG_LABEL;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.CURRENT_CONFIG_LABEL;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.PASSWORD_DEVICE_CONFIG_LABEL;
+import static com.googlesource.gerrit.plugins.github.oauth.GitHubOAuthConfig.KeyConfig.SECRET_KEY_CONFIG_LABEL;
+import static com.googlesource.gerrit.plugins.github.oauth.OAuthTokenCipher.splitKeyIdFromMaterial;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertThrows;
+
+import com.google.gerrit.extensions.client.AuthType;
+import com.google.gerrit.httpd.CanonicalWebUrl;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.util.Providers;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import java.util.List;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Before;
+import org.junit.Test;
+
+public class OAuthTokenCipherTest {
+
+ CanonicalWebUrl canonicalWebUrl;
+ Config config;
+
+ private static final String VERSION1_KEY_ID = "version1";
+ private static final String VERSION2_KEY_ID = "version2";
+
+ @Before
+ public void setUp() {
+ config = new Config();
+
+ config.setString(CONF_SECTION, null, "clientSecret", "theSecret");
+ config.setString(CONF_SECTION, null, "clientId", "theClientId");
+ config.setString("auth", null, "httpHeader", "GITHUB_USER");
+ config.setString("auth", null, "type", AuthType.HTTP.toString());
+
+ config.setBoolean(CONF_KEY_SECTION, VERSION1_KEY_ID, CURRENT_CONFIG_LABEL, true);
+ config.setBoolean(CONF_KEY_SECTION, VERSION2_KEY_ID, CURRENT_CONFIG_LABEL, false);
+
+ canonicalWebUrl =
+ Guice.createInjector(
+ new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(String.class)
+ .annotatedWith(com.google.gerrit.server.config.CanonicalWebUrl.class)
+ .toProvider(Providers.of(null));
+ }
+ })
+ .getInstance(CanonicalWebUrl.class);
+ }
+
+ @Test
+ public void shouldEncryptAndDecryptAToken() throws IOException {
+ String someOauthToken = "someToken";
+ OAuthTokenCipher objectUnderTest = objectUnderTest();
+
+ String encrypt = objectUnderTest.encrypt(someOauthToken);
+ assertNotEquals(encrypt, someOauthToken);
+ assertEquals(objectUnderTest.decrypt(encrypt), someOauthToken);
+ }
+
+ @Test
+ public void shouldEncryptWithKeyId() throws IOException {
+ assertEquals(takeKeyId(objectUnderTest().encrypt("someToken")), VERSION1_KEY_ID);
+ }
+
+ @Test
+ public void shouldReturnAPrefixedBase64EncodedEncryptedString() throws IOException {
+ String someOauthToken = "someToken";
+ List<String> keyAndMaterial = splitKeyIdFromMaterial(objectUnderTest().encrypt(someOauthToken));
+ String keyId = keyAndMaterial.get(0);
+ String material = keyAndMaterial.get(1);
+
+ assertEquals(keyId, VERSION1_KEY_ID);
+ assertNotEquals(
+ Base64.getDecoder().decode(material), someOauthToken.getBytes(StandardCharsets.UTF_8));
+ }
+
+ @Test
+ public void shouldStillBeAbleToDecryptATokenEncryptedWithANonCurrentKey() throws IOException {
+ String someToken = "someToken";
+ String encryptedWithV1 = objectUnderTest().encrypt(someToken);
+
+ config.setBoolean(CONF_KEY_SECTION, VERSION1_KEY_ID, CURRENT_CONFIG_LABEL, false);
+ config.setBoolean(CONF_KEY_SECTION, VERSION2_KEY_ID, CURRENT_CONFIG_LABEL, true);
+
+ assertEquals(objectUnderTest().decrypt(encryptedWithV1), someToken);
+ }
+
+ @Test
+ public void shouldPassThroughWhenDecryptingPlainTextStrings() throws IOException {
+ String somePlainTextToken = "someToken";
+ assertEquals(objectUnderTest().decrypt(somePlainTextToken), somePlainTextToken);
+ }
+
+ @Test
+ public void shouldThrowWhenDecryptingATokenEncryptedANoLongerAvailableKey() {
+ CipherException cipherException =
+ assertThrows(
+ CipherException.class, () -> objectUnderTest().decrypt("non-existing-key:foobar"));
+
+ assertEquals(
+ cipherException.getCause().getMessage(),
+ "Could not find key-id 'non-existing-key' in configuration");
+ }
+
+ @Test
+ public void shouldThrowWhenCipherAlgorithmIsNotValid() {
+ config.setString(
+ CONF_KEY_SECTION, VERSION1_KEY_ID, CIPHER_ALGO_CONFIG_LABEL, "Invalid cipher algorithm");
+
+ assertThrows(CipherException.class, () -> objectUnderTest().encrypt("some token"));
+ }
+
+ @Test
+ public void shouldThrowWhenKeyAlgorithmIsNotValid() {
+ config.setString(
+ CONF_KEY_SECTION, VERSION1_KEY_ID, SECRET_KEY_CONFIG_LABEL, "Invalid Key algorithm");
+
+ assertThrows(CipherException.class, () -> objectUnderTest().encrypt("some token"));
+ }
+
+ @Test
+ public void shouldThrowWhenPasswordCouldNotBeRead() {
+ config.setString(
+ CONF_KEY_SECTION, VERSION1_KEY_ID, PASSWORD_DEVICE_CONFIG_LABEL, "/some/unexisting/file");
+
+ assertThrows(IOException.class, this::objectUnderTest);
+ }
+
+ @Test
+ public void shouldThrowWhenDecryptingANonBase64String() {
+ assertThrows(
+ IOException.class, () -> objectUnderTest().decrypt("current:some non-base64 string"));
+ }
+
+ private static String takeKeyId(String base64EncryptedString) {
+ return splitKeyIdFromMaterial(base64EncryptedString).get(0);
+ }
+
+ private OAuthTokenCipher objectUnderTest() throws IOException {
+ return new OAuthTokenCipher(new GitHubOAuthConfig(config, canonicalWebUrl));
+ }
+}
diff --git a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java
index 4fc1533..b025737 100644
--- a/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java
+++ b/github-plugin/src/main/java/com/googlesource/gerrit/plugins/github/filters/GitHubOAuthFilter.java
@@ -26,6 +26,7 @@
import com.googlesource.gerrit.plugins.github.oauth.IdentifiedUserGitHubLoginProvider;
import com.googlesource.gerrit.plugins.github.oauth.OAuthFilter;
import com.googlesource.gerrit.plugins.github.oauth.OAuthProtocol.AccessToken;
+import com.googlesource.gerrit.plugins.github.oauth.OAuthTokenCipher;
import com.googlesource.gerrit.plugins.github.oauth.OAuthWebFilter;
import com.googlesource.gerrit.plugins.github.oauth.ScopedProvider;
import java.io.IOException;
@@ -47,15 +48,18 @@
private final ScopedProvider<GitHubLogin> loginProvider;
private final Provider<CurrentUser> userProvider;
private final AccountCache accountCache;
+ private final OAuthTokenCipher oAuthTokenCipher;
@Inject
public GitHubOAuthFilter(
ScopedProvider<GitHubLogin> loginProvider,
Provider<CurrentUser> userProvider,
- AccountCache accountCache) {
+ AccountCache accountCache,
+ OAuthTokenCipher oAuthTokenCipher) {
this.loginProvider = loginProvider;
this.userProvider = userProvider;
this.accountCache = accountCache;
+ this.oAuthTokenCipher = oAuthTokenCipher;
}
@Override
@@ -78,7 +82,8 @@
.get()
.substring(
ExternalId.SCHEME_EXTERNAL.length() + OAuthWebFilter.GITHUB_EXT_ID.length() + 1);
- hubLogin.login(new AccessToken(oauthToken));
+ String decryptedToken = oAuthTokenCipher.decrypt(oauthToken);
+ hubLogin.login(new AccessToken(decryptedToken));
}
chain.doFilter(request, response);
diff --git a/github-plugin/src/main/resources/Documentation/config.md b/github-plugin/src/main/resources/Documentation/config.md
index e8fc5fa..ea32890 100644
--- a/github-plugin/src/main/resources/Documentation/config.md
+++ b/github-plugin/src/main/resources/Documentation/config.md
@@ -83,4 +83,60 @@
* s, sec, second, seconds
* m, min, minute, minutes
* h, hr, hour, hours
- Default value: 30 seconds
\ No newline at end of file
+ Default value: 30 seconds
+
+Key Configuration
+-------------
+
+Since this plugin obtains credentials from Github and persists them in Gerrit,
+it also takes care of encrypting them at rest. The Gerrit admin can configure
+how this is done by setting the relevant configuration parameters.
+
+github-key.<key-id>.passwordDevice
+: The device or file where to retrieve the encryption passphrase.\
+Default: /dev/zero
+
+*NOTE*: such configuration is considered insecure and should *not be used in
+production*, always set a non-zero password device for deriving the key.
+
+github-key.<key-id>.passwordLength
+: The length in bytes of the password read from the passwordDevice.\
+Default: 16
+
+github-key.<key-id>.cipherAlgorithm
+: The algorithm to be used for encryption/decryption. Available algorithms are
+described in
+the [Cipher section](https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html#cipher-algorithm-names)
+of the Java Cryptography Architecture Standard Algorithm Name Documentation.\
+Default: AES/ECB/PKCS5Padding
+
+github-key.<key-id>.secretKeyAlgorithm
+: the algorithm to be used to encrypt the provided password. Available
+algorithms are described in
+the [Cipher section](https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html#cipher-algorithm-names)
+of the Java Cryptography Architecture Standard Algorithm Name Documentation.\
+Default: AES
+
+github-key.<key-id>.current
+: Whether this configuration is the current one, and it should be used to
+encrypt new Github credentials. Note that _exactly_ one github-key configuration
+must be set to `current`, otherwise an error exception will be thrown.\
+Default: false
+
+As you can observe, in order to support key rotations, multiple `github-key`
+can be specified in configuration. credentials encrypted with a `<key-id>` key
+can still be decrypted as long as the `github-key.<key-id>` stanza is available
+in the configuration. New credentials will always be encrypted with
+the `current` `<key-id>`.
+
+Note that unencrypted oauth tokens will be handled gracefully and just passed
+through to github by the decryption algorithm. This is done so that oauth tokens
+that were persisted _before_ the encryption feature was implemented will still
+be considered valid until their natural expiration time.
+
+If no `github-key.<key-id>` exists in configuration, then a default current key
+configuration
+(named `current`) will be inferred, using the defaults documented above.
+
+*NOTE* such configuration is considered insecure and should *not be used in
+production*.
\ No newline at end of file
diff --git a/pom.xml b/pom.xml
index 6202ae3..34b1018 100644
--- a/pom.xml
+++ b/pom.xml
@@ -302,4 +302,14 @@
<scope>provided</scope>
</dependency>
</dependencies>
+ <dependencyManagement>
+ <dependencies>
+ <dependency>
+ <groupId>junit</groupId>
+ <artifactId>junit</artifactId>
+ <version>4.13.2</version>
+ <scope>test</scope>
+ </dependency>
+ </dependencies>
+ </dependencyManagement>
</project>
