Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import jenkins.security.Messages;
import jenkins.util.SystemProperties;
import net.jcip.annotations.Immutable;
import net.sf.json.JSONObject;
import org.kohsuke.accmod.Restricted;
Expand All @@ -69,6 +70,10 @@

private static final String HASH_ALGORITHM = "SHA-256";

private static final String TOKEN_PREFIX = "jenkins_apitoken_";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. GitHub

Have you checked what their new tokens look like recently? 😉 Their prefix is now github_pat_ instead of ghp.

I considered the prefix jio_ (for jenkins.io) for a while, then I saw GH made much longer token prefixes, so I thought why not. jio in isolation is not identifiable as a Jenkins API token. Neither might jenkins_ alone.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe OT but we could rename the Jenkins feature in GUI and docs to personal access tokens for consistency with GH and to emphasize that these are bound to a “user” (regular principal). Thus, jenkins_pat_.


private static /* non-final for script console */ boolean REQUIRE_API_TOKEN_PREFIX = SystemProperties.getBoolean(ApiTokenStore.class.getName() + ".REQUIRE_API_TOKEN_PREFIX", true);

private List<HashedToken> tokenList;

public ApiTokenStore() {
Expand Down Expand Up @@ -177,8 +182,8 @@
RANDOM.nextBytes(random);
// 32-char in hex
String secretValue = Util.toHexString(random);
String tokenTheUserWillUse = HASH_VERSION + secretValue;
assert tokenTheUserWillUse.length() == 2 + 32;
String tokenTheUserWillUse = TOKEN_PREFIX + HASH_VERSION + secretValue;
assert tokenTheUserWillUse.length() == TOKEN_PREFIX.length() + 2 + 32;

Check warning on line 186 in core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 186 is only partially covered, one branch is missing

HashedToken token = prepareAndStoreToken(name, secretValue);

Expand All @@ -195,9 +200,19 @@
*/
@SuppressFBWarnings(value = "UNSAFE_HASH_EQUALS", justification = "Comparison only validates version of the specified token")
public synchronized @NonNull String addFixedNewToken(@NonNull String name, @NonNull String tokenPlainValue) {
if (REQUIRE_API_TOKEN_PREFIX && !tokenPlainValue.startsWith(TOKEN_PREFIX)) {

Check warning on line 203 in core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 203 is only partially covered, one branch is missing
throw new IllegalArgumentException("The token must consist of: the prefix '" + TOKEN_PREFIX + "', 2 characters for the version, and 32 hex-characters for the secret");
}
if (tokenPlainValue.startsWith(TOKEN_PREFIX)) {

Check warning on line 206 in core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 206 is only partially covered, one branch is missing
tokenPlainValue = tokenPlainValue.substring(TOKEN_PREFIX.length());
}
if (tokenPlainValue.length() != VERSION_LENGTH + HEX_CHAR_LENGTH) {
LOGGER.log(Level.INFO, "addFixedNewToken, length received: {0}" + tokenPlainValue.length());
throw new IllegalArgumentException("The token must consist of 2 characters for the version and 32 hex-characters for the secret");
// TODO clean this logic up
if (REQUIRE_API_TOKEN_PREFIX) {

Check warning on line 212 in core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 212 is only partially covered, one branch is missing
throw new IllegalArgumentException("The token must consist of: the prefix '" + TOKEN_PREFIX + "', 2 characters for the version, and 32 hex-characters for the secret");
}
throw new IllegalArgumentException("The token must consist of: the prefix '" + TOKEN_PREFIX + "' (optional), 2 characters for the version, and 32 hex-characters for the secret");

Check warning on line 215 in core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 215 is not covered by tests
}

String hashVersion = tokenPlainValue.substring(0, VERSION_LENGTH);
Expand Down Expand Up @@ -255,6 +270,14 @@
if (isLegacyToken(token)) {
plainToken = token;
} else {
if (REQUIRE_API_TOKEN_PREFIX && !token.startsWith(TOKEN_PREFIX)) {

Check warning on line 273 in core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 273 is only partially covered, 2 branches are missing
// not allowed, so will not match
return null;

Check warning on line 275 in core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 275 is not covered by tests
}
Comment on lines +273 to +276
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will prevent people from upgrading unless I misunderstand?

IMO:

  • There is no need for a system property.
  • All new tokens should start with the new prefix.
  • Any existing tokens should still be honored (until regenerated).

if (token.startsWith(TOKEN_PREFIX)) {

Check warning on line 277 in core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 277 is only partially covered, one branch is missing
// Identifiable Jenkins API token prefix
token = token.substring(TOKEN_PREFIX.length());
}
plainToken = getHashOfToken(token);
}

Expand All @@ -265,7 +288,7 @@
* Determine if the given token was generated by the legacy system or the new one
*/
private boolean isLegacyToken(@NonNull String token) {
return token.length() != TOKEN_LENGTH_V2;
return token.length() != TOKEN_LENGTH_V2 && token.length() != TOKEN_PREFIX.length() + TOKEN_LENGTH_V2;

Check warning on line 291 in core/src/main/java/jenkins/security/apitoken/ApiTokenStore.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 291 is only partially covered, one branch is missing
}

/**
Expand Down
16 changes: 9 additions & 7 deletions test/src/test/java/jenkins/security/ApiTokenPropertyTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ void script_addFixedNewToken_Regular() throws Exception {

Collection<ApiTokenStore.HashedToken> beforeTokenList = apiTokenProperty.getTokenStore().getTokenListSortedByName();

String tokenPlainTextValue = "110123456789abcdef0123456789abcdef";
String tokenPlainTextValue = "jenkins_apitoken_110123456789abcdef0123456789abcdef";
apiTokenProperty.addFixedNewToken("fixed-token", tokenPlainTextValue);

Collection<ApiTokenStore.HashedToken> afterTokenList = apiTokenProperty.getTokenStore().getTokenListSortedByName();
Expand All @@ -492,10 +492,12 @@ void script_addFixedNewToken_Invalid() {

Collection<ApiTokenStore.HashedToken> beforeTokenList = apiTokenProperty.getTokenStore().getTokenListSortedByName();

checkInvalidTokenValue(apiTokenProperty, "invalid-token: too-long", "110123456789abcdef0123456789abcdefg");
checkInvalidTokenValue(apiTokenProperty, "invalid-token: too-short", "110123456789abcdef0123456789abcde");
checkInvalidTokenValue(apiTokenProperty, "invalid-token: non-hex", "110123456789abcdef0123456789abcdeg");
checkInvalidTokenValue(apiTokenProperty, "invalid-token: invalid-version", "120123456789abcdef0123456789abcdef");
checkInvalidTokenValue(apiTokenProperty, "invalid-token: no prefix", "120123456789abcdef0123456789abcdef");
checkInvalidTokenValue(apiTokenProperty, "invalid-token: wrong prefix", "jenkins_120123456789abcdef0123456789abcdef");
checkInvalidTokenValue(apiTokenProperty, "invalid-token: too-long", "jenkins_apitoken_110123456789abcdef0123456789abcdefg");
checkInvalidTokenValue(apiTokenProperty, "invalid-token: too-short", "jenkins_apitoken_110123456789abcdef0123456789abcde");
checkInvalidTokenValue(apiTokenProperty, "invalid-token: non-hex", "jenkins_apitoken_110123456789abcdef0123456789abcdeg");
checkInvalidTokenValue(apiTokenProperty, "invalid-token: invalid-version", "jenkins_apitoken_120123456789abcdef0123456789abcdef");

Collection<ApiTokenStore.HashedToken> afterTokenList = apiTokenProperty.getTokenStore().getTokenListSortedByName();
// ensure there is no new tokens
Expand Down Expand Up @@ -549,7 +551,7 @@ void script_revokeAllTokens() throws Exception {
tokenList = apiTokenProperty.getTokenStore().getTokenListSortedByName();
assertThat(tokenList, empty());

String tokenPlainTextValue = "110123456789abcdef0123456789abcdef";
String tokenPlainTextValue = "jenkins_apitoken_110123456789abcdef0123456789abcdef";
apiTokenProperty.addFixedNewToken("fixed-token", tokenPlainTextValue);
checkTokenIsWorking(user.getId(), tokenPlainTextValue);
apiTokenProperty.revokeAllTokens();
Expand Down Expand Up @@ -585,7 +587,7 @@ void script_revokeAllTokensExceptOne() throws Exception {
assertThat(tokenList, hasSize(1));
assertEquals(token1.tokenUuid, tokenList.iterator().next().getUuid());

String tokenPlainTextValue = "110123456789abcdef0123456789abcdef";
String tokenPlainTextValue = "jenkins_apitoken_110123456789abcdef0123456789abcdef";
apiTokenProperty.addFixedNewToken("fixed-token", tokenPlainTextValue);
TokenUuidAndPlainValue token4 = apiTokenProperty.generateNewToken("token4");
apiTokenProperty.revokeAllTokensExceptOne(token4.tokenUuid);
Expand Down
Loading