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>