From 8752691ef75ce42e00ca312aeae789709acd24b8 Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Wed, 26 Jul 2023 09:39:38 -0700 Subject: [PATCH 1/2] token streamline (#449) * eliminate persisting token in TokenRefreshHandlerImpl * more succinct useSharedToken implementation * fix return, cleaner recursion * more conservative wait for lock backoff --- .../OAuthRefreshTokenSourceAuthStrategy.java | 66 +++++++++++-------- .../RefreshTokenTokenRequestBuilder.java | 10 +-- 2 files changed, 40 insertions(+), 36 deletions(-) diff --git a/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/OAuthRefreshTokenSourceAuthStrategy.java b/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/OAuthRefreshTokenSourceAuthStrategy.java index 9360474f9..f5b79e3bc 100644 --- a/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/OAuthRefreshTokenSourceAuthStrategy.java +++ b/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/OAuthRefreshTokenSourceAuthStrategy.java @@ -143,6 +143,7 @@ public interface TokenRequestBuilder { * * q: maybe this should just *always* be true? or should be env var? * + * * @return whether resulting `access_token` should be shared across all instances of * connections to this source. */ @@ -197,9 +198,27 @@ public static class TokenRefreshHandlerImpl implements OAuth2CredentialsWithRefr // open a lambda waiting for a lock private static final String TOKEN_REFRESH_LOCK_ID = "oauth_refresh_token"; private static final int MAX_TOKEN_REFRESH_ATTEMPTS = 3; - private static final long WAIT_AFTER_FAILED_LOCK_ATTEMPTS = 2000L; - private AccessToken currentToken = null; + /** + * how long to wait after a failed lock attempt before trying again; multiplier on the + * attempt. + * + * goal is that this value should be big enough to allow the process that holds the lock to + * 1) refresh the token it + * 2) write it + * 3) release the lock + * 4) have that write be visible to other processes, given eventual consistency in GCP + * Secret Manager case + * + * this includes the allowance for eventual consistency, so wait should be >= that value + * in practice. + */ + private static final long WAIT_AFTER_FAILED_LOCK_ATTEMPTS = 4000L; + + /** + * how long to allow for eventual consistency after write to config + */ + private static final long ALLOWANCE_FOR_EVENTUAL_CONSISTENCY_SECONDS = 2L; /** * implements canonical oauth flow to exchange refreshToken for accessToken @@ -220,43 +239,34 @@ private AccessToken refreshAccessToken(int attempt) throws IOException { CanonicalOAuthAccessTokenResponseDto tokenResponse; - Optional sharedAccessToken = getSharedAccessTokenIfSupported(); - if (this.currentToken == null) { - this.currentToken = sharedAccessToken.orElse(null); - } - - if (sharedAccessToken.isPresent()) { - // we have a token, but shared token is newer. Other instance refreshed, so use it - if (sharedAccessToken.get().getExpirationTime().after(this.currentToken.getExpirationTime())) { - this.currentToken = sharedAccessToken.get(); - } - } + AccessToken token = getSharedAccessTokenIfSupported().orElse(null); - if (shouldRefresh(this.currentToken, clock.instant())) { + if (shouldRefresh(token, clock.instant())) { // only lock if we're using a shared token across processes boolean lockNeeded = payloadBuilder.useSharedToken(); boolean acquired = !lockNeeded || lockService.acquire(TOKEN_REFRESH_LOCK_ID, Duration.ofMinutes(2)); - if (!acquired) { - //NOTE: check shared access token again, in case the instance which held the - // lock refreshed the token + if (acquired) { + tokenResponse = exchangeRefreshTokenForAccessToken(); + token = asAccessToken(tokenResponse); - Uninterruptibles.sleepUninterruptibly(attempt * WAIT_AFTER_FAILED_LOCK_ATTEMPTS, TimeUnit.MILLISECONDS); - refreshAccessToken(attempt + 1); - } + storeSharedAccessTokenIfSupported(token); - tokenResponse = exchangeRefreshTokenForAccessToken(); - this.currentToken = asAccessToken(tokenResponse); - storeSharedAccessTokenIfSupported(this.currentToken); - - if (lockNeeded) { - lockService.release(TOKEN_REFRESH_LOCK_ID); + if (lockNeeded) { + // hold lock extra, to try to maximize the time between token refreshes + Uninterruptibles.sleepUninterruptibly(ALLOWANCE_FOR_EVENTUAL_CONSISTENCY_SECONDS, TimeUnit.SECONDS); + lockService.release(TOKEN_REFRESH_LOCK_ID); + } + } else { + //re-try recursively, w/ linear backoff + Uninterruptibles.sleepUninterruptibly(attempt * WAIT_AFTER_FAILED_LOCK_ATTEMPTS, TimeUnit.MILLISECONDS); + token = refreshAccessToken(attempt + 1); } } - return this.currentToken; + return token; } @@ -425,4 +435,4 @@ public CanonicalOAuthAccessTokenResponseDto parseTokenResponse(HttpResponse resp } -} \ No newline at end of file +} diff --git a/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/RefreshTokenTokenRequestBuilder.java b/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/RefreshTokenTokenRequestBuilder.java index e84bceeeb..2610cdd05 100644 --- a/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/RefreshTokenTokenRequestBuilder.java +++ b/java/core/src/main/java/co/worklytics/psoxy/gateway/impl/oauth/RefreshTokenTokenRequestBuilder.java @@ -43,13 +43,7 @@ public enum ConfigProperty implements ConfigService.ConfigProperty { public boolean useSharedToken() { Optional useSharedTokenConfig = config.getConfigPropertyAsOptional(ConfigProperty.USE_SHARED_TOKEN); - boolean useSharedToken = false; - - if (useSharedTokenConfig.isPresent()) { - useSharedToken = Boolean.parseBoolean(useSharedTokenConfig.get()); - } - - return useSharedToken; + return useSharedTokenConfig.map(Boolean::parseBoolean).orElse(false); } public HttpContent buildPayload() { @@ -87,4 +81,4 @@ public void addHeaders(HttpHeaders httpHeaders) { // But for some APIs (Github) is required to ensure that the response will be in JSON format httpHeaders.setAccept("application/json"); } -} \ No newline at end of file +} From 9613277ea1e28b8ab998c19c61c175514682ce40 Mon Sep 17 00:00:00 2001 From: Erik Schultink Date: Wed, 26 Jul 2023 09:46:17 -0700 Subject: [PATCH 2/2] sha256 encoder (#450) * proper commit of Sha256PseudonymEncoder, wo intellij files * Update java/gateway-core/src/main/java/com/avaulta/gateway/pseudonyms/impl/Sha256PseudonymEncoder.java Co-authored-by: Jose Lorenzo * fix encoding test roundtrip, add proper decoding given base64 addition --------- Co-authored-by: Jose Lorenzo --- .../impl/Sha256PseudonymEncoder.java | 50 ++++++++++++++ .../impl/Sha256PseudonymEncoderTest.java | 65 +++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 java/gateway-core/src/main/java/com/avaulta/gateway/pseudonyms/impl/Sha256PseudonymEncoder.java create mode 100644 java/gateway-core/src/test/java/com/avaulta/gateway/pseudonyms/impl/Sha256PseudonymEncoderTest.java diff --git a/java/gateway-core/src/main/java/com/avaulta/gateway/pseudonyms/impl/Sha256PseudonymEncoder.java b/java/gateway-core/src/main/java/com/avaulta/gateway/pseudonyms/impl/Sha256PseudonymEncoder.java new file mode 100644 index 000000000..188acc23d --- /dev/null +++ b/java/gateway-core/src/main/java/com/avaulta/gateway/pseudonyms/impl/Sha256PseudonymEncoder.java @@ -0,0 +1,50 @@ +package com.avaulta.gateway.pseudonyms.impl; + +import com.avaulta.gateway.pseudonyms.Pseudonym; +import com.avaulta.gateway.pseudonyms.PseudonymEncoder; +import org.apache.commons.lang3.StringUtils; + +import java.nio.charset.StandardCharsets; +import java.util.Base64; + +/** + * implementation of defacto encoding used by BulkDataSanitizerImpl as of v0.4.30 + */ +public class Sha256PseudonymEncoder implements PseudonymEncoder { + + + @Override + public String encode(Pseudonym pseudonym) { + return base64Encode(pseudonym.getHash()); + } + + @Override + public Pseudonym decode(String input) { + if (!canBeDecoded(input)) { + throw new IllegalArgumentException("input cannot be decoded"); + } + + return Pseudonym.builder().hash(base64decode(input)).build(); + } + + @Override + public boolean canBeDecoded(String possiblePseudonym) { + return possiblePseudonym != null && + possiblePseudonym.getBytes(StandardCharsets.UTF_8).length == 43; //43 rather than 32, bc of base64 encoding without padding + } + + //base64 encoding, to match implementation in HashUtils.java from psoxy-core v0.4.30 + String base64Encode(byte[] bytes) { + String encoded = new String( + Base64.getEncoder() + .withoutPadding() + .encode(bytes), + StandardCharsets.UTF_8); + return StringUtils.replaceChars(encoded, "/+", "_."); + } + + byte[] base64decode(String input) { + return Base64.getDecoder() + .decode(StringUtils.replaceChars(input, "_.", "/+")); + } +} diff --git a/java/gateway-core/src/test/java/com/avaulta/gateway/pseudonyms/impl/Sha256PseudonymEncoderTest.java b/java/gateway-core/src/test/java/com/avaulta/gateway/pseudonyms/impl/Sha256PseudonymEncoderTest.java new file mode 100644 index 000000000..9c7b0e227 --- /dev/null +++ b/java/gateway-core/src/test/java/com/avaulta/gateway/pseudonyms/impl/Sha256PseudonymEncoderTest.java @@ -0,0 +1,65 @@ +package com.avaulta.gateway.pseudonyms.impl; + +import com.avaulta.gateway.pseudonyms.Pseudonym; +import com.avaulta.gateway.pseudonyms.PseudonymEncoder; +import com.avaulta.gateway.tokens.impl.Sha256DeterministicTokenizationStrategy; +import org.apache.commons.lang3.RandomStringUtils; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import java.util.Random; +import java.util.function.Function; + +import static org.junit.jupiter.api.Assertions.*; + +class Sha256PseudonymEncoderTest { + + Sha256PseudonymEncoder encoder = new Sha256PseudonymEncoder(); + + + @ParameterizedTest + @ValueSource(strings = { + // examples taken from https://github.com/Worklytics/psoxy/blob/b483e3788d5457398d55cad7934de959b74c7900/java/core/src/test/java/co/worklytics/psoxy/storage/impl/BulkDataSanitizerImplTest.java#L228-L239 + "SappwO4KZKGprqqUNruNreBD2BVR98nEM6NRCu3R2dM", + "mfsaNYuCX__xvnRz4gJp_t0zrDTC5DkuCJvMkubugsI", + ".ZdDGUuOMK.Oy7_PJ3pf9SYX12.3tKPdLHfYbjVGcGk", + ".fs1T64Micz8SkbILrABgEv4kSg.tFhvhP35HGSLdOo" + }) + void canBeDecoded(String encoded) { + assertTrue(encoder.canBeDecoded(encoded)); + } + + + @ParameterizedTest + @ValueSource(strings = { + "asdfasdf", + "1343287afdaskdljf4324sasdfa", + }) + void cannotBeDecoded(String encoded) { + assertFalse(encoder.canBeDecoded(encoded)); + } + + @ParameterizedTest + @ValueSource(strings = { + "asdfasdf", + "1343287afdaskdljf4324sasdfa", + "asdf1234234", + "alice@acme.com" + }) + void roundtrip(String identifier) { + Sha256DeterministicTokenizationStrategy sha256DeterministicTokenizationStrategy = + new Sha256DeterministicTokenizationStrategy("salt"); + + Pseudonym pseudonym = Pseudonym.builder() + .hash(sha256DeterministicTokenizationStrategy.getToken(identifier, Function.identity())) + .build(); + + String encoded = encoder.encode(pseudonym); + + assertTrue(encoder.canBeDecoded(encoded)); + assertEquals(new String(pseudonym.getHash()), + new String(encoder.decode(encoded).getHash())); + + } +}