Add identifiable prefixes to API tokens#11187
Add identifiable prefixes to API tokens#11187daniel-beck wants to merge 1 commit intojenkinsci:masterfrom
Conversation
timja
left a comment
There was a problem hiding this comment.
I think token prefix should probably be configurable for OEM, e.g. cloudbees CI. but no UI and there should be warnings about don't do this unless you know what you are doing?
|
|
||
| private static final String HASH_ALGORITHM = "SHA-256"; | ||
|
|
||
| private static final String TOKEN_PREFIX = "jenkins_apitoken_"; |
There was a problem hiding this comment.
this seems very long.
Have you done research into what other vendors use?
e.g. GitHub:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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_.
I considered this, even had it in the code as a Java system property. The reason I removed it again is that I thought this could cause is that I do not really want prefixes to be optional besides a transitional period after introduction (the flag that currently exists). With a user-configurable prefix, we end up with these options:
I liked none of them a lot. In terms of the OEM argument, I'll definitely check with colleagues what the situation with CloudBees CI should be before I finalize this PR. There may be a middle ground, like a resource file in |
| if (REQUIRE_API_TOKEN_PREFIX && !token.startsWith(TOKEN_PREFIX)) { | ||
| // not allowed, so will not match | ||
| return null; | ||
| } |
There was a problem hiding this comment.
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).
|
Please take a moment and address the merge conflicts of your pull request. Thanks! |
Identifiable prefixes makes it easier to identify poor secret handling. See e.g. https://github.blog/engineering/platform-security/behind-githubs-new-authentication-token-formats/ for an (outdated at this point) description of benefits.
To really have users benefit from this, we can't allow the prefixes to be configurable though. WDYT?
Additionally, does it make sense to require prefixes by default already, or should that be done at a later point?
WIP, no real test coverage yet.
Testing done
Proposed changelog entries
Proposed changelog category
/label
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered (see query).