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

Alternative solution for tracking unconsumed parameters #22

Merged
merged 2 commits into from
Feb 21, 2024
Merged

Alternative solution for tracking unconsumed parameters #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5dc47ba
Alternative solution for tracking unconsumed parameters
peternied Feb 14, 2024
b0b354c
SpotlessApply
peternied Feb 14, 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
4 changes: 2 additions & 2 deletions src/main/java/com/amazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,10 @@ private AuthCredentials extractCredentials0(final SecurityRequest request) {
}

if ((jwtToken == null || jwtToken.isEmpty()) && jwtUrlParameter != null) {
jwtToken = request.param(jwtUrlParameter);
jwtToken = request.params().get(jwtUrlParameter);
} else {
// just consume to avoid "contains unrecognized parameter"
request.param(jwtUrlParameter);
request.params().get(jwtUrlParameter);
}

if (jwtToken == null || jwtToken.length() == 0) {
Expand Down
40 changes: 29 additions & 11 deletions src/main/java/org/opensearch/security/filter/NettyRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import java.util.TreeMap;
import javax.net.ssl.SSLEngine;

import com.google.common.base.Supplier;
import com.google.common.base.Suppliers;

import org.opensearch.http.netty4.Netty4HttpChannel;
import org.opensearch.rest.RestRequest.Method;
import org.opensearch.rest.RestUtils;
Expand All @@ -36,8 +39,7 @@ public class NettyRequest implements SecurityRequest {

protected final HttpRequest underlyingRequest;
protected final Netty4HttpChannel underlyingChannel;

private final Set<String> consumedParams = new HashSet<>();
protected final Supplier<CheckedAccessMap> parameters = Suppliers.memoize(() -> new CheckedAccessMap(params(uri())));

NettyRequest(final HttpRequest request, final Netty4HttpChannel channel) {
this.underlyingRequest = request;
Expand Down Expand Up @@ -86,18 +88,12 @@ public String uri() {

@Override
public Map<String, String> params() {
return params(underlyingRequest.uri());
return parameters.get();
}

@Override
public String param(String key) {
Map<String, String> urlParams = params();
consumedParams.add(key);
return urlParams != null ? params().get(key) : null;
}

public Set<String> getConsumedParams() {
return consumedParams;
public Set<String> getUnconsumedParams() {
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't this method remain as getConsumedParams()? The method returns the keys that have been accessed.

Copy link
Author

Choose a reason for hiding this comment

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

I ran into this when thinking about the interface - I think parameters are either unconsumed and consumed; furthermore unconsumed parameters need to be consumed by something. This makes the overall design to transition parameters from accessed -> unconsumed -> consumed.

Since OpenSearchRequest has a way to consume parameters, returning an empty set is correct, whereas if it was accessed or consumed parameters it would return a list to be actioned on - which isn't needed.

I'd prefer the new name, but don't want to hold up your on this topic. What do you think about this?

Copy link
Owner

Choose a reason for hiding this comment

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

This makes the overall design to transition parameters from accessed -> unconsumed -> consumed

I see what you are saying. I'm getting confused since it clashes with the RestRequest object where these are referred to as consumed params, but in this case they are accessed in the header verifier, but finally consumed in the SecurityRestFilter. I wonder if we should refer to them as accessed params to avoid a naming clash, but I see what you mean now.

return parameters.get().accessedKeys();
}

private static Map<String, String> params(String uri) {
Expand All @@ -115,4 +111,26 @@ private static Map<String, String> params(String uri) {

return params;
}

/** Records access of any keys if explicetly requested from this map */
private static class CheckedAccessMap extends HashMap<String, String> {
private final Set<String> accessedKeys = new HashSet<>();

public CheckedAccessMap(final Map<String, String> map) {
super(map);
}

@Override
public String get(final Object key) {
// Never noticed this about java's map interface the getter is not generic
if (key instanceof String) {
accessedKeys.add((String) key);
}
return super.get(key);
}

public Set<String> accessedKeys() {
return accessedKeys;
}
}
}
6 changes: 4 additions & 2 deletions src/main/java/org/opensearch/security/filter/OpenSearchRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import javax.net.ssl.SSLEngine;

import org.opensearch.rest.RestRequest;
Expand Down Expand Up @@ -79,8 +80,9 @@ public String get(Object key) {
}

@Override
public String param(String key) {
return underlyingRequest.param(key);
public Set<String> getUnconsumedParams() {
// params() Map consumes explict parameter access
return Set.of();
}

/** Gets access to the underlying request object */
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/opensearch/security/filter/SecurityRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.stream.Stream;
import javax.net.ssl.SSLEngine;

Expand Down Expand Up @@ -50,6 +51,6 @@ default String header(final String headerName) {
/** The parameters associated with this request */
Map<String, String> params();

/** Gets the parameter for the given key */
String param(String key);
/** The list of parameters that have been accessed but not recorded as being consumed */
Set<String> getUnconsumedParams();
}
8 changes: 4 additions & 4 deletions src/main/java/org/opensearch/security/filter/SecurityRestFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,10 @@

import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX;
import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX;
import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS;
import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE;
import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE;
import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED;
import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS;

public class SecurityRestFilter {

Expand Down Expand Up @@ -145,10 +145,10 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
}
});

NettyAttribute.popFrom(request, CONSUMED_PARAMS).ifPresent(consumedParams -> {
for (String param : consumedParams) {
NettyAttribute.popFrom(request, UNCONSUMED_PARAMS).ifPresent(unconsumedParams -> {
for (String unconsumedParam : unconsumedParams) {
// Consume the parameter on the RestRequest
request.param(param);
request.param(unconsumedParam);
}
});

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/http/SecurityHttpServerTransport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
public class SecurityHttpServerTransport extends SecuritySSLNettyHttpServerTransport {

public static final AttributeKey<SecurityResponse> EARLY_RESPONSE = AttributeKey.newInstance("opensearch-http-early-response");
public static final AttributeKey<Set<String>> CONSUMED_PARAMS = AttributeKey.newInstance("opensearch-http-request-consumed-params");
public static final AttributeKey<Set<String>> UNCONSUMED_PARAMS = AttributeKey.newInstance("opensearch-http-request-consumed-params");
public static final AttributeKey<ThreadContext.StoredContext> CONTEXT_TO_RESTORE = AttributeKey.newInstance(
"opensearch-http-request-thread-context"
);
Expand Down
7 changes: 2 additions & 5 deletions src/main/java/org/opensearch/security/ssl/http/netty/Netty4HttpRequestHeaderVerifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.http.netty4.Netty4HttpChannel;
import org.opensearch.http.netty4.Netty4HttpServerTransport;
import org.opensearch.security.filter.NettyRequest;
import org.opensearch.security.filter.SecurityRequestChannel;
import org.opensearch.security.filter.SecurityRequestChannelUnsupported;
import org.opensearch.security.filter.SecurityRequestFactory;
Expand All @@ -33,11 +32,11 @@
import io.netty.handler.codec.http.HttpRequest;
import io.netty.util.ReferenceCountUtil;

import static org.opensearch.security.http.SecurityHttpServerTransport.CONSUMED_PARAMS;
import static org.opensearch.security.http.SecurityHttpServerTransport.CONTEXT_TO_RESTORE;
import static org.opensearch.security.http.SecurityHttpServerTransport.EARLY_RESPONSE;
import static org.opensearch.security.http.SecurityHttpServerTransport.IS_AUTHENTICATED;
import static org.opensearch.security.http.SecurityHttpServerTransport.SHOULD_DECOMPRESS;
import static org.opensearch.security.http.SecurityHttpServerTransport.UNCONSUMED_PARAMS;

@Sharable
public class Netty4HttpRequestHeaderVerifier extends SimpleChannelInboundHandler<DefaultHttpRequest> {
Expand Down Expand Up @@ -86,9 +85,7 @@ public void channelRead0(ChannelHandlerContext ctx, DefaultHttpRequest msg) thro
// If request channel is completed and a response is sent, then there was a failure during authentication
restFilter.checkAndAuthenticateRequest(requestChannel);

if (requestChannel instanceof NettyRequest) {
ctx.channel().attr(CONSUMED_PARAMS).set(((NettyRequest) requestChannel).getConsumedParams());
}
ctx.channel().attr(UNCONSUMED_PARAMS).set(requestChannel.getUnconsumedParams());

ThreadContext.StoredContext contextToRestore = threadPool.getThreadContext().newStoredContext(false);
ctx.channel().attr(CONTEXT_TO_RESTORE).set(contextToRestore);
Expand Down
Loading