Skip to content

Commit 759130e

Browse files
sauravzgsauravzg
committed
Fixup: 12494 address comments and bring back up to updated ext authz spec
1 parent 1d811a6 commit 759130e

File tree

6 files changed

+297
-392
lines changed

6 files changed

+297
-392
lines changed

‎xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutationFilter.java‎

Lines changed: 84 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,7 @@
1717
package io.grpc.xds.internal.headermutations;
1818

1919
import com.google.common.collect.ImmutableList;
20-
import com.google.common.collect.ImmutableSet;
21-
import io.envoyproxy.envoy.config.core.v3.HeaderValueOption;
20+
import io.grpc.xds.internal.grpcservice.HeaderValue;
2221
import io.grpc.xds.internal.headermutations.HeaderMutations.RequestHeaderMutations;
2322
import io.grpc.xds.internal.headermutations.HeaderMutations.ResponseHeaderMutations;
2423
import java.util.Collection;
@@ -30,28 +29,18 @@
3029
* The HeaderMutationFilter class is responsible for filtering header mutations based on a given set
3130
* of rules.
3231
*/
33-
public interface HeaderMutationFilter {
32+
public class HeaderMutationFilter {
33+
private final Optional<HeaderMutationRulesConfig> mutationRules;
3434

3535
/**
36-
* A factory for creating {@link HeaderMutationFilter} instances.
36+
* Set of HTTP/2 pseudo-headers and the host header that are critical for routing and protocol
37+
* correctness. These headers cannot be mutated by user configuration.
3738
*/
38-
@FunctionalInterface
39-
interface Factory {
40-
/**
41-
* Creates a new instance of {@code HeaderMutationFilter}.
42-
*
43-
* @param mutationRules The rules for header mutations. If an empty {@code Optional} is
44-
* provided, all header mutations are allowed by default, except for certain system
45-
* headers. If a {@link HeaderMutationRulesConfig} is provided, mutations will be
46-
* filtered based on the specified rules.
47-
*/
48-
HeaderMutationFilter create(Optional<HeaderMutationRulesConfig> mutationRules);
49-
}
39+
private static final int MAX_HEADER_LENGTH = 16384;
5040

51-
/**
52-
* The default factory for creating {@link HeaderMutationFilter} instances.
53-
*/
54-
Factory INSTANCE = HeaderMutationFilterImpl::new;
41+
public HeaderMutationFilter(Optional<HeaderMutationRulesConfig> mutationRules) { // NOPMD
42+
this.mutationRules = mutationRules;
43+
}
5544

5645
/**
5746
* Filters the given header mutations based on the configured rules and returns the allowed
@@ -62,111 +51,94 @@ interface Factory {
6251
* @throws HeaderMutationDisallowedException if a disallowed mutation is encountered and the rules
6352
* specify that this should be an error.
6453
*/
65-
HeaderMutations filter(HeaderMutations mutations) throws HeaderMutationDisallowedException;
54+
public HeaderMutations filter(HeaderMutations mutations)
55+
throws HeaderMutationDisallowedException {
56+
ImmutableList<HeaderValueOption> allowedRequestHeaders =
57+
filterCollection(mutations.requestMutations().headers(),
58+
this::isHeaderMutationIgnored,
59+
header -> isHeaderMutationAllowed(header.header().key()));
60+
ImmutableList<String> allowedRequestHeadersToRemove =
61+
filterCollection(mutations.requestMutations().headersToRemove(),
62+
header -> header.isEmpty() || header.length() > MAX_HEADER_LENGTH,
63+
header -> isHeaderMutationAllowed(header) && isHeaderRemovalAllowed(header));
64+
ImmutableList<HeaderValueOption> allowedResponseHeaders =
65+
filterCollection(mutations.responseMutations().headers(),
66+
this::isHeaderMutationIgnored,
67+
header -> isHeaderMutationAllowed(header.header().key()));
68+
return HeaderMutations.create(
69+
RequestHeaderMutations.create(allowedRequestHeaders, allowedRequestHeadersToRemove),
70+
ResponseHeaderMutations.create(allowedResponseHeaders));
71+
}
6672

67-
/** Default implementation of {@link HeaderMutationFilter}. */
68-
final class HeaderMutationFilterImpl implements HeaderMutationFilter {
69-
private final Optional<HeaderMutationRulesConfig> mutationRules;
73+
/**
74+
* A generic helper to filter a collection based on a predicate.
75+
*/
76+
private <T> ImmutableList<T> filterCollection(Collection<T> items,
77+
Predicate<T> isIgnoredPredicate, Predicate<T> isAllowedPredicate)
78+
throws HeaderMutationDisallowedException {
79+
ImmutableList.Builder<T> allowed = ImmutableList.builder();
80+
for (T item : items) {
81+
if (isIgnoredPredicate.test(item)) {
82+
continue;
83+
}
84+
if (isAllowedPredicate.test(item)) {
85+
allowed.add(item);
86+
} else if (disallowIsError()) {
87+
throw new HeaderMutationDisallowedException(
88+
"Header mutation disallowed for header: " + item);
89+
}
90+
}
91+
return allowed.build();
92+
}
7093

71-
/**
72-
* Set of HTTP/2 pseudo-headers and the host header that are critical for routing and protocol
73-
* correctness. These headers cannot be mutated by user configuration.
74-
*/
75-
private static final ImmutableSet<String> IMMUTABLE_HEADERS =
76-
ImmutableSet.of("host", ":authority", ":scheme", ":method");
94+
private boolean isHeaderRemovalAllowed(String headerKey) {
95+
return !isSystemHeaderKey(headerKey);
96+
}
7797

78-
private HeaderMutationFilterImpl(Optional<HeaderMutationRulesConfig> mutationRules) { // NOPMD
79-
this.mutationRules = mutationRules;
80-
}
98+
private boolean isSystemHeaderKey(String key) {
99+
return key.startsWith(":") || key.toLowerCase(Locale.ROOT).equals("host");
100+
}
81101

82-
@Override
83-
public HeaderMutations filter(HeaderMutations mutations)
84-
throws HeaderMutationDisallowedException {
85-
ImmutableList<HeaderValueOption> allowedRequestHeaders =
86-
filterCollection(mutations.requestMutations().headers(),
87-
header -> isHeaderMutationAllowed(header.getHeader().getKey())
88-
&& !appendsSystemHeader(header));
89-
ImmutableList<String> allowedRequestHeadersToRemove =
90-
filterCollection(mutations.requestMutations().headersToRemove(),
91-
header -> isHeaderMutationAllowed(header) && isHeaderRemovalAllowed(header));
92-
ImmutableList<HeaderValueOption> allowedResponseHeaders =
93-
filterCollection(mutations.responseMutations().headers(),
94-
header -> isHeaderMutationAllowed(header.getHeader().getKey())
95-
&& !appendsSystemHeader(header));
96-
return HeaderMutations.create(
97-
RequestHeaderMutations.create(allowedRequestHeaders, allowedRequestHeadersToRemove),
98-
ResponseHeaderMutations.create(allowedResponseHeaders));
102+
private boolean isHeaderMutationIgnored(HeaderValueOption option) {
103+
HeaderValue header = option.header();
104+
String key = header.key();
105+
if (key.isEmpty() || key.length() > MAX_HEADER_LENGTH) {
106+
return true;
99107
}
100-
101-
/**
102-
* A generic helper to filter a collection based on a predicate.
103-
*
104-
* @param items The collection of items to filter.
105-
* @param isAllowedPredicate The predicate to apply to each item.
106-
* @param <T> The type of items in the collection.
107-
* @return An immutable list of allowed items.
108-
* @throws HeaderMutationDisallowedException if an item is disallowed and disallowIsError is
109-
* true.
110-
*/
111-
private <T> ImmutableList<T> filterCollection(Collection<T> items,
112-
Predicate<T> isAllowedPredicate) throws HeaderMutationDisallowedException {
113-
ImmutableList.Builder<T> allowed = ImmutableList.builder();
114-
for (T item : items) {
115-
if (isAllowedPredicate.test(item)) {
116-
allowed.add(item);
117-
} else if (disallowIsError()) {
118-
throw new HeaderMutationDisallowedException(
119-
"Header mutation disallowed for header: " + item);
120-
}
121-
}
122-
return allowed.build();
108+
if (header.value().isPresent() && header.value().get().length() > MAX_HEADER_LENGTH) {
109+
return true;
123110
}
124-
125-
private boolean isHeaderRemovalAllowed(String headerKey) {
126-
return !isSystemHeaderKey(headerKey);
111+
if (header.rawValue().isPresent() && header.rawValue().get().size() > MAX_HEADER_LENGTH) {
112+
return true;
127113
}
128-
129-
private boolean appendsSystemHeader(HeaderValueOption headerValueOption) {
130-
String key = headerValueOption.getHeader().getKey();
131-
boolean isAppend = headerValueOption
132-
.getAppendAction() == HeaderValueOption.HeaderAppendAction.APPEND_IF_EXISTS_OR_ADD;
133-
return isAppend && isSystemHeaderKey(key);
114+
if (key.toLowerCase(Locale.ROOT).startsWith("grpc-")) {
115+
return true;
134116
}
117+
return false;
118+
}
135119

136-
private boolean isSystemHeaderKey(String key) {
137-
return key.startsWith(":") || key.toLowerCase(Locale.ROOT).equals("host");
120+
private boolean isHeaderMutationAllowed(String headerName) {
121+
String lowerCaseHeaderName = headerName.toLowerCase(Locale.ROOT);
122+
if (isSystemHeaderKey(lowerCaseHeaderName)) {
123+
return false;
138124
}
125+
return mutationRules.map(rules -> isHeaderMutationAllowed(lowerCaseHeaderName, rules))
126+
.orElse(true);
127+
}
139128

140-
private boolean isHeaderMutationAllowed(String headerName) {
141-
String lowerCaseHeaderName = headerName.toLowerCase(Locale.ROOT);
142-
if (IMMUTABLE_HEADERS.contains(lowerCaseHeaderName)) {
143-
return false;
144-
}
145-
return mutationRules.map(rules -> isHeaderMutationAllowed(lowerCaseHeaderName, rules))
146-
.orElse(true);
129+
private boolean isHeaderMutationAllowed(String lowerCaseHeaderName,
130+
HeaderMutationRulesConfig rules) {
131+
if (rules.disallowExpression().isPresent()
132+
&& rules.disallowExpression().get().matcher(lowerCaseHeaderName).matches()) {
133+
return false;
147134
}
148-
149-
private boolean isHeaderMutationAllowed(String lowerCaseHeaderName,
150-
HeaderMutationRulesConfig rules) {
151-
// TODO(sauravzg): The priority is slightly unclear in the spec.
152-
// Both `disallowAll` and `disallow_expression` take precedence over `all other
153-
// settings`.
154-
// `allow_expression` takes precedence over everything except `disallow_expression`.
155-
// This is a conflict between ordering for `allow_expression` and `disallowAll`.
156-
// Choosing to proceed with current envoy implementation which favors `allow_expression` over
157-
// `disallowAll`.
158-
if (rules.disallowExpression().isPresent()
159-
&& rules.disallowExpression().get().matcher(lowerCaseHeaderName).matches()) {
160-
return false;
161-
}
162-
if (rules.allowExpression().isPresent()) {
163-
return rules.allowExpression().get().matcher(lowerCaseHeaderName).matches();
164-
}
165-
return !rules.disallowAll();
135+
if (rules.allowExpression().isPresent()) {
136+
return rules.allowExpression().get().matcher(lowerCaseHeaderName).matches();
166137
}
138+
return !rules.disallowAll();
139+
}
167140

168-
private boolean disallowIsError() {
169-
return mutationRules.map(HeaderMutationRulesConfig::disallowIsError).orElse(false);
170-
}
141+
private boolean disallowIsError() {
142+
return mutationRules.map(HeaderMutationRulesConfig::disallowIsError).orElse(false);
171143
}
172144
}

‎xds/src/main/java/io/grpc/xds/internal/headermutations/HeaderMutations.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818

1919
import com.google.auto.value.AutoValue;
2020
import com.google.common.collect.ImmutableList;
21-
import io.envoyproxy.envoy.config.core.v3.HeaderValueOption;
2221

2322
/** A collection of header mutations for both request and response headers. */
2423
@AutoValue

0 commit comments

Comments
 (0)