Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Only run DlsFlsValveImpl.invoke on indices requests #4937

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Only run DlsFlsValveImpl.invoke on indices requests #4937

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eecba3a
Only run DlsFlsValveImpl.invoke on indices requests
cwperks Nov 25, 2024
4769144
Only check prefix
cwperks Nov 25, 2024
d6037f9
Only set once
cwperks Nov 27, 2024
9de9c24
Increment metadata version
cwperks Nov 27, 2024
1369726
Match action name
cwperks Nov 27, 2024
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
6 changes: 6 additions & 0 deletions src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.threadpool.ThreadPool;

import static org.opensearch.security.privileges.PrivilegesEvaluator.isIndexPerm;

public class DlsFlsValveImpl implements DlsFlsRequestValve {

private static final String MAP_EXECUTION_HINT = "map";
Expand Down Expand Up @@ -135,6 +137,10 @@ public DlsFlsValveImpl(
*/
@Override
public boolean invoke(PrivilegesEvaluationContext context, final ActionListener<?> listener) {
if (!isIndexPerm(context.getAction())) {
Copy link
Collaborator

@nibix nibix Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would guess that the valve code needs to be at least also called for scroll requests, otherwise FLS and field masking might be not correctly applied.

Generally, as the issue seems to be of a very specific kind, I am wondering whether the fix should be equally specific.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed a commit to check the action prefix alone. If the action prefix is not indices:, then DlsFlsValveImpl would be skipped.

Copy link
Collaborator

@nibix nibix Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Still, I think the fault is rather on the ISM side: It defines an action which has a request which extends UpdateRequest and thus in turn extends IndicesRequest - while the type string is cluster:admin/opendistro/transform/stop, i.e. it indicates it to be a cluster request. That seems to be a contradiction in itself.

I am a bit surprised that this should only occur after the merge of #4380. At least this basic behavior of DlsFlsValveImpl did not change. If this is really related to #4380, we should find out why #4380 changes this behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Before #4380 , it would get short circuited here because the helpdesk_role has no restrictions. The new code does not use that data structure and instead calls on IndexNameExpressionResolver.getConcreteIndices and passes in * because the resolved request shows * as you pointed out in #4937 (comment)

return true;
}

DlsFlsProcessedConfig config = this.dlsFlsProcessedConfig.get();
ActionRequest request = context.getRequest();
IndexResolverReplacer.Resolved resolved = context.getResolvedRequest();
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,10 @@ public static boolean isClusterPerm(String action0) {
|| (action0.equals(RenderSearchTemplateAction.NAME)));
}

public static boolean isIndexPerm(String action0) {
return (action0.startsWith("indices:") && !isClusterPerm(action0));
}

@SuppressWarnings("unchecked")
private boolean checkFilteredAliases(Resolved requestedResolved, String action, boolean isDebugEnabled) {
final String faMode = dcm.getFilteredAliasMode();// getConfigSettings().dynamic.filtered_alias_mode;
Expand Down
Loading