Skip to content

Commit

Permalink
Make hashed token ids url safe (#42651)
Browse files Browse the repository at this point in the history
This commit changes the way token ids are hashed so that the output is
url safe without requiring encoding. This follows the pattern that we
use for document ids that are autogenerated, see UUIDs and the
associated classes for additional details.
  • Loading branch information
jaymode authored May 30, 2019
1 parent 2bc4363 commit 8d0bae6
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -360,14 +360,14 @@ public boolean verify(SecureString text, char[] hash) {
public char[] hash(SecureString text) {
MessageDigest md = MessageDigests.sha256();
md.update(CharArrays.toUtf8Bytes(text.getChars()));
return Base64.getEncoder().encodeToString(md.digest()).toCharArray();
return Base64.getUrlEncoder().withoutPadding().encodeToString(md.digest()).toCharArray();
}

@Override
public boolean verify(SecureString text, char[] hash) {
MessageDigest md = MessageDigests.sha256();
md.update(CharArrays.toUtf8Bytes(text.getChars()));
return CharArrays.constantTimeEquals(Base64.getEncoder().encodeToString(md.digest()).toCharArray(), hash);
return CharArrays.constantTimeEquals(Base64.getUrlEncoder().withoutPadding().encodeToString(md.digest()).toCharArray(), hash);
}
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public final class TokenService {
TimeValue.MINUS_ONE, Property.NodeScope);

static final String TOKEN_DOC_TYPE = "token";
private static final int HASHED_TOKEN_LENGTH = 44;
private static final int HASHED_TOKEN_LENGTH = 43;
// UUIDs are 16 bytes encoded base64 without padding, therefore the length is (16 / 3) * 4 + ((16 % 3) * 8 + 5) / 6 chars
private static final int TOKEN_LENGTH = 22;
private static final String TOKEN_DOC_ID_PREFIX = TOKEN_DOC_TYPE + "_";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,17 @@
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.core.watcher.watch.ClockMock;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;
import org.junit.After;
import org.elasticsearch.xpack.security.test.SecurityMocks;
import org.hamcrest.Matchers;
import org.junit.After;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;

import javax.crypto.SecretKey;
import java.io.IOException;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.security.GeneralSecurityException;
import java.time.Clock;
import java.time.Instant;
Expand All @@ -70,8 +73,6 @@
import java.util.HashMap;
import java.util.Map;

import javax.crypto.SecretKey;

import static java.time.Clock.systemUTC;
import static org.elasticsearch.repositories.ESBlobStoreTestCase.randomBytes;
import static org.elasticsearch.test.ClusterServiceUtils.setState;
Expand Down Expand Up @@ -721,6 +722,11 @@ public void testCannotValidateTokenIfLicenseDoesNotAllowTokens() throws Exceptio
assertThat(authToken, Matchers.nullValue());
}

public void testHashedTokenIsUrlSafe() {
final String hashedId = TokenService.hashTokenString(UUIDs.randomBase64UUID());
assertEquals(hashedId, URLEncoder.encode(hashedId, StandardCharsets.UTF_8));
}

private TokenService createTokenService(Settings settings, Clock clock) throws GeneralSecurityException {
return new TokenService(settings, clock, client, licenseState, securityMainIndex, securityTokensIndex, clusterService);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import org.apache.http.HttpHeaders;
import org.apache.http.HttpHost;
import org.apache.lucene.util.LuceneTestCase.AwaitsFix;
import org.elasticsearch.Version;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.RequestOptions;
Expand All @@ -25,6 +26,7 @@
import java.util.List;
import java.util.Map;

@AwaitsFix(bugUrl = "need to backport #42651")
public class TokenBackwardsCompatibilityIT extends AbstractUpgradeTestCase {

private Collection<RestClient> twoClients = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
"Get the indexed token and use if to authenticate":
- skip:
features: headers
version: " - 7.99.99"
reason: "Need to backport PR #42651"

- do:
cluster.health:
Expand Down Expand Up @@ -59,6 +61,8 @@
"Get the indexed refreshed access token and use if to authenticate":
- skip:
features: headers
version: " - 7.99.99"
reason: "Need to backport PR #42651"

- do:
get:
Expand Down Expand Up @@ -111,6 +115,8 @@
"Get the indexed refresh token and use it to get another access token and authenticate":
- skip:
features: headers
version: " - 7.99.99"
reason: "Need to backport PR #42651"

- do:
get:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
"Get the indexed token and use if to authenticate":
- skip:
features: headers
version: " - 8.0.0"
reason: "Need to backport PR #42651"

- do:
cluster.health:
Expand Down Expand Up @@ -49,6 +51,8 @@
"Get the indexed refresh token and use if to get another access token and authenticate":
- skip:
features: headers
version: " - 8.0.0"
reason: "Need to backport PR #42651"

- do:
get:
Expand Down

0 comments on commit 8d0bae6

Please sign in to comment.