-
Notifications
You must be signed in to change notification settings - Fork 343
Introduce API Tokens with cluster_permissions and index_permissions directly associated with the token
#5443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Introduce API Tokens with cluster_permissions and index_permissions directly associated with the token
#5443
Conversation
…4921) Signed-off-by: Derek Ho <[email protected]>
…ect#4967) Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
…00 tokens outstanding (opensearch-project#5147) Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
cluster_permissions and index_permissions directly associated with the token
Signed-off-by: Craig Perkins <[email protected]>
nibix
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Cool to see API tokens going forward.
One general question: I am wondering whether we also need to modify the DLS/FLS code to be API token aware. Otherwise, the deny-by-default policy might block index access.
| pluginIdToActionPrivileges.put( | ||
| entry.getKey(), | ||
| new SubjectBasedActionPrivileges(entry.getValue(), flattenedActionGroups) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this, we might need to make the pluginIdToActionPrivileges HashMap thread-safe, for example, by converting it into a ConcurrentHashMap.
Alternatively, we could have two HashMaps, one for plugins and one for API tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i vote for single thread-safe map
|
|
||
| private final Map<String, RoleV7> jtis = new ConcurrentHashMap<>(); | ||
|
|
||
| void reloadApiTokensFromIndex(ActionListener<Void> listener) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is getting called when an update action is received. I think I did not see anything regarding initial node startup. Did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache seems to be update only when a token is created or deleted. We should add one that loads on node bootstrap.
| if (tokenCount >= 100) { | ||
| sendErrorResponse( | ||
| channel, | ||
| RestStatus.TOO_MANY_REQUESTS, | ||
| "Maximum limit of 100 API tokens reached. Please delete existing tokens before creating new ones." | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a quite low limit. Is there a reason for that?
| public void createApiToken( | ||
| String name, | ||
| List<String> clusterPermissions, | ||
| List<ApiToken.IndexPermission> indexPermissions, | ||
| Long expiration, | ||
| ActionListener<String> listener | ||
| ) { | ||
| apiTokenIndexHandler.createApiTokenIndexIfAbsent(ActionListener.wrap(() -> { | ||
| ExpiringBearerAuthToken token = securityTokenManager.issueApiToken(name, expiration); | ||
| ApiToken apiToken = new ApiToken(name, clusterPermissions, indexPermissions, expiration); | ||
| apiTokenIndexHandler.indexTokenMetadata( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the parameter name an identifier for the token? Do we need uniqueness guarantees for this?
| private void logDebug(String message, Object... args) { | ||
| if (log.isDebugEnabled()) { | ||
| log.debug(message, args); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This method is not really necessary. It can be replaced just by log.debug() without any disadvantages. As a slight advantage, one also avoids the codecov warning for missing test coverage.
| private final List<TokenListener> tokenListener = new ArrayList<>(); | ||
| private static final Logger log = LogManager.getLogger(ApiTokenRepository.class); | ||
|
|
||
| private final Map<String, RoleV7> jtis = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about changing RoleV7 into SubjectBasedActionPrivileges here? PrivilegesEvaluator could then retrieve these instances just from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. It might not be straightforward since JTIs seem to be populated on getTokenMetadata request and there doesn't seem to be a way to pass flattenedActionGroups to that call. I may be seeing the complete picture here, but something like updateJTIs(FlattenedAGs) from PrivilegeEvaluator to update jtis and then use that to populate pluginIdToActionPrivileges which will then be used to create PrivilegeEvalContext?
| public Map<String, RoleV7> getJtis() { | ||
| return jtis; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this map should be kept private and managed only by this class.
| public XContentBuilder toXContent(XContentBuilder xContentBuilder, Params params) throws IOException { | ||
| xContentBuilder.startObject(); | ||
| xContentBuilder.field("enabled", enabled); | ||
| xContentBuilder.field("signing_key", signing_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a hs512 key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it fails in ApiTokenAuthenticator if it is anything lower than 512 bits.
| public Boolean isRequestAllowed(final SecurityRequest request) { | ||
| Matcher matcher = PATTERN_PATH_PREFIX.matcher(request.path()); | ||
| final String suffix = matcher.matches() ? matcher.group(2) : null; | ||
| if (isAccessToRestrictedEndpoints(request, suffix)) { | ||
| final OpenSearchException exception = ExceptionUtils.invalidUsageOfApiTokenException(); | ||
| log.error(exception.toString()); | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a fragile way to deny certain endpoints, especially with the hardcoded path prefixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, do I understand it correctly that the goal of this code is that we cannot call the "issue API token" API with API tokens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't rest-admin-only restriction block access to endpoints anyway?
DarshitChanpura
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @cwperks for taking this over. Left some comments around testing and general usage.
| public ApiToken(String name, List<String> clusterPermissions, List<IndexPermission> indexPermissions, Long expiration) { | ||
| this.creationTime = Instant.now(); | ||
| this.name = name; | ||
| this.clusterPermissions = clusterPermissions; | ||
| this.indexPermissions = indexPermissions; | ||
| this.expiration = expiration; | ||
| } | ||
|
|
||
| public ApiToken( | ||
| String name, | ||
| List<String> clusterPermissions, | ||
| List<IndexPermission> indexPermissions, | ||
| Instant creationTime, | ||
| Long expiration | ||
| ) { | ||
| this.name = name; | ||
| this.clusterPermissions = clusterPermissions; | ||
| this.indexPermissions = indexPermissions; | ||
| this.creationTime = creationTime; | ||
| this.expiration = expiration; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
can be combined into 1 constructor:
public ApiToken(
String name,
List<String> clusterPermissions,
List<IndexPermission> indexPermissions,
Instant creationTime, // nullable
Long expiration) {
this.name = name;
this.clusterPermissions = clusterPermissions;
this.indexPermissions = indexPermissions;
this.creationTime = (creationTime != null) ? creationTime : Instant.now();
this.expiration = expiration;
}Can the name field be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the first constructor and made it so that creationTime should be supplied by the caller.
src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java
Show resolved
Hide resolved
|
|
||
| public ApiToken(String name, List<String> clusterPermissions, List<IndexPermission> indexPermissions, Long expiration) { | ||
| this.creationTime = Instant.now(); | ||
| this.name = name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be UUID instead of name? or does the name field have any association with users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name is not necessarily unique per token, but it is unique per active token. i.e. on a token's expiry you would be able to regenerate a new one with the same perms.
The metadata which includes the perms for the token is stored as a doc in the .opensearch_security_api_tokens index
| private final List<TokenListener> tokenListener = new ArrayList<>(); | ||
| private static final Logger log = LogManager.getLogger(ApiTokenRepository.class); | ||
|
|
||
| private final Map<String, RoleV7> jtis = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. It might not be straightforward since JTIs seem to be populated on getTokenMetadata request and there doesn't seem to be a way to pass flattenedActionGroups to that call. I may be seeing the complete picture here, but something like updateJTIs(FlattenedAGs) from PrivilegeEvaluator to update jtis and then use that to populate pluginIdToActionPrivileges which will then be used to create PrivilegeEvalContext?
| } | ||
|
|
||
| @Test | ||
| public void shouldNotAuthenticateWithInvalidExpiration() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add a case to test against expired token
| public XContentBuilder toXContent(XContentBuilder xContentBuilder, Params params) throws IOException { | ||
| xContentBuilder.startObject(); | ||
| xContentBuilder.field("enabled", enabled); | ||
| xContentBuilder.field("signing_key", signing_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it fails in ApiTokenAuthenticator if it is anything lower than 512 bits.
| } | ||
|
|
||
| @Test | ||
| public void issueApiToken_success() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add a test for failure path
| Settings settings = Settings.builder().put("signing_key", tooShortKey).build(); | ||
| final Throwable exception = assertThrows(OpenSearchException.class, () -> { new JwtVendor(settings); }); | ||
|
|
||
| assertThat(exception.getMessage(), containsString("The secret length must be at least 256 bits")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't signing key at minimum of 512 bits, based on ApiTokenAuthenticator.MINIMUM_SIGNING_KEY_BIT_LENGTH
| throw new RuntimeException(ex); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should add test to check for index permissions, i.e. ability to search from, write to an index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should do this before we merge this.
|
Hi @cwperks ! First I wanted to thank you for your work on this PR. Really appreciate effort you put it in to move forward with this feature! Just wanted to ask whether is there any plan to soon merge your changes? Is there any blocking issues/ any help you would need? It would be really cool to have API tokens feature working :) |
|
I'll resolve the conflicts ASAP and try to push this forward for V1. Its important to know that this PR will still have a lot of limitations, but paves the way for expansion. For instance, in this PR only the admin can issue tokens. |
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
DarshitChanpura
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for picking this up @cwperks . Left a few comments. Main comment is addition of e2e test which checks authn+authz with API token,
src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java
Show resolved
Hide resolved
|
|
||
| // First check token count | ||
| apiTokenRepository.getTokenCount(ActionListener.wrap(tokenCount -> { | ||
| if (tokenCount >= 100) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this value configurable with default to 100?
maybe maxed at 1k active tokens?
| updateRequest, | ||
| ActionListener.wrap( | ||
| updateResponse -> listener.onResponse(response), | ||
| exception -> listener.onFailure(new ApiTokenException("Failed to refresh cache", exception)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this message should be "Failed to update API token"
| * Create a mechanism for plugins to explicitly declare actions they need to perform with their assigned PluginSubject ([#5341](https://github.com/opensearch-project/security/pull/5341)) | ||
| * Moves OpenSAML jars to a Shadow Jar configuration to facilitate its use in FIPS enabled environments ([#5400](https://github.com/opensearch-project/security/pull/5404)) | ||
| * Replaced the standard distribution of BouncyCastle with BC-FIPS ([#5439](https://github.com/opensearch-project/security/pull/5439)) | ||
| * Introduce API Tokens with `cluster_permissions` and `index_permissions` directly associated with the token ([#5443](https://github.com/opensearch-project/security/pull/5443)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change log needs to be cleaned up to only add this entry.
| if (!issueApiTokenAllowed()) { | ||
| throw new OpenSearchSecurityException("Api token generation is not enabled."); | ||
| } | ||
| final User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't seem like this is used?
| throw new RuntimeException(ex); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should do this before we merge this.
Description
Re-basing #5225 with latest changes from main.
This PR creates a new capability in the security plugin where users can issue tokens and associate permissions directly with the token.
Currently this is limited to security admins.
What is novel about this PR is that we can associated permissions directly with the token instead of OBO tokens which contain a claim with the user's roles that the OBO authenticator uses for authz. With API Tokens an admin can request that the token is scoped down from their own permissions to give the token exactly the permissions it needs. For instance, if the admin only wants the token to have search permissions on a single index, then that can be configured and the token will be unable to perform any other action.
This is a key building block to be able to deprecate and replace Roles Injection which is the current practice for how plugins ensure that tasks or jobs that run asynchronously run with the same restrictions as the user that created the job. Instead, I envision that we require users to issue tokens scoped to exactly the needs for the job to ensure the job runs with only the permissions it needs and no more (principal of least privilege)
I'll update this PR Description with details about new REST APIs with example request and response.
Enhancement
Issues Resolved
Partially resolves #4009, but limited to security admins in initial release
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.