smolnar82 commented on code in PR #1476:
URL: https://github.com/apache/knox/pull/1476#discussion_r4442067545
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/K8sDestinationAudienceValidator.java:
##########
@@ -399,19 +526,84 @@ public void init(final FilterConfig filterConfig) throws
Exception {
enforceActSubIsServiceAccount =
Boolean.parseBoolean(filterConfig.getInitParameter(ENFORCE_ACT_SUB_IS_SERVICE_ACCOUNT_PARAM));
+ ingressSourceRoutingEnabled = Boolean.parseBoolean(
+
filterConfig.getInitParameter(INGRESS_SOURCE_SPIFFE_ID_ROUTING_ENABLED_PARAM));
+ ingressSourceRoutingSkipTrustDomainMatch = Boolean.parseBoolean(
+
filterConfig.getInitParameter(INGRESS_SOURCE_SPIFFE_IDS_ROUTING_SKIP_TRUST_DOMAIN_MATCH_PARAM));
+ ingressSourceRoutingSkipValidation = Boolean.parseBoolean(
+
filterConfig.getInitParameter(INGRESS_SOURCE_SPIFFE_IDS_ROUTING_SKIP_VALIDATION_PARAM));
+
+ // Parsed strictly whether or not routing is enabled, so a malformed entry
fails the
+ // topology at startup instead of sitting inert.
+ final String ingressSourceSpiffeIdsParam =
+
blankToNull(filterConfig.getInitParameter(INGRESS_SOURCE_SPIFFE_IDS_PARAM));
+ ingressSourceSpiffeIds = new ArrayList<>();
+ if (ingressSourceSpiffeIdsParam != null) {
+ for (final String entry :
parseCommaSeparated(ingressSourceSpiffeIdsParam)) {
+ final Optional<SpiffeId> parsed = SpiffeId.parse(entry);
+ if (parsed.isEmpty()) {
+ throw new ServletException(String.format(Locale.ROOT,
+ "%s contains an invalid SPIFFE id entry: %s",
INGRESS_SOURCE_SPIFFE_IDS_PARAM, entry));
+ }
+ ingressSourceSpiffeIds.add(lowerCased(parsed.get()));
+ }
+ }
+
if (enforceActSubMatchesSourceSpiffeId && sourceSpiffeIdHeader == null) {
throw new ServletException(String.format(Locale.ROOT,
"%s is true but %s is not configured",
ENFORCE_ACT_SUB_MATCHES_SOURCE_SPIFFE_ID_PARAM,
SOURCE_SPIFFE_ID_HEADER_PARAM));
}
+
+ if (ingressSourceRoutingEnabled) {
+ if (sourceSpiffeIdHeader == null) {
+ throw new ServletException(String.format(Locale.ROOT,
+ "%s is true but %s is not configured",
INGRESS_SOURCE_SPIFFE_ID_ROUTING_ENABLED_PARAM,
+ SOURCE_SPIFFE_ID_HEADER_PARAM));
+ }
+ if (ingressSourceSpiffeIds.isEmpty()) {
+ throw new ServletException(String.format(Locale.ROOT,
+ "%s is true but %s is not configured",
INGRESS_SOURCE_SPIFFE_ID_ROUTING_ENABLED_PARAM,
+ INGRESS_SOURCE_SPIFFE_IDS_PARAM));
+ }
+ }
}
@Override
public AudienceValidationResult validate(final HttpServletRequest request,
final JWT token,
final List<String> configuredAudiences) {
final boolean hasActClaim = token.getClaimAsObject(JWTToken.ACT_CLAIM) !=
null;
- if (!hasActClaim && !validateAudiencesWithoutActClaim) {
- return
AudienceValidationResult.of(AbstractJWTFilter.matchesConfiguredAudiences(token,
configuredAudiences));
+
+ // Resolved before the early returns below because the routing decision
needs it. An
+ // absent value is not rejected here: that rejection happens further down,
after the destination
+ // header and server-name checks, so a request whose source header is
missing or unparseable
+ // is reported only once those earlier checks have passed.
+ final Optional<SpiffeId> sourceSpiffeId = sourceSpiffeIdHeader == null
+ ? Optional.empty() : sourceSpiffeIdFromHeader(request);
+ final boolean fromIngressSource = ingressSourceRoutingEnabled
+ && isConfiguredIngressSource(sourceSpiffeId);
+
+ if (!hasActClaim) {
+ if (fromIngressSource) {
+ // A token with no act claim arriving through the ingress names no
in-cluster destination,
+ // so its audience is either not checked at all or checked against the
fixed configured
+ // list. Both decisions are made before the aud claim is required to
be present, because
+ // the fixed-list check has its own handling for a token that carries
no aud claim.
+ return ingressSourceRoutingSkipValidation
+ ? AudienceValidationResult.of(true)
+ : AudienceValidationResult.of(
+ AbstractJWTFilter.matchesConfiguredAudiences(token,
configuredAudiences));
+ }
+ if (!validateAudiencesWithoutActClaim) {
+ return AudienceValidationResult.of(
+ AbstractJWTFilter.matchesConfiguredAudiences(token,
configuredAudiences));
+ }
+ } else if (fromIngressSource) {
+ // An act claim means the token authorizes one workload to call another
inside the cluster.
+ // Arriving from an ingress identity means it was presented from
outside, which no such call
+ // does.
+ return new AudienceValidationResult(false, "Token has an act claim but
its source SPIFFE id in header "
+ + sourceSpiffeIdHeader + " is a configured ingress source: " +
request.getHeader(sourceSpiffeIdHeader));
Review Comment:
**Diverges from the JIRA.** KNOX-3534: *"Delegation tokens are not allowed
on that path, so even if they come from an ingress, still treat them as they
are now."* I read that as "don't send `act` tokens down the ingress route;
destination-validate them as today", not "reject them".
With routing enabled, an RFC 8693-exchanged token presented through the
ingress used to be destination-validated. Now it always gets a 400. That breaks
clients outside the cluster that re-exchange a delegated token through the
public ingress.
Could we either:
- drop this branch, so `act` tokens fall through to destination validation
as before, or
- put the rejection behind its own flag that defaults to `false` (e.g.
`request.audience.k8s.ingress.routing.reject.delegation.tokens`)?
If the stricter behavior is intended, please update the JIRA so the two
agree.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/K8sDestinationAudienceValidator.java:
##########
@@ -399,19 +526,84 @@ public void init(final FilterConfig filterConfig) throws
Exception {
enforceActSubIsServiceAccount =
Boolean.parseBoolean(filterConfig.getInitParameter(ENFORCE_ACT_SUB_IS_SERVICE_ACCOUNT_PARAM));
+ ingressSourceRoutingEnabled = Boolean.parseBoolean(
+
filterConfig.getInitParameter(INGRESS_SOURCE_SPIFFE_ID_ROUTING_ENABLED_PARAM));
+ ingressSourceRoutingSkipTrustDomainMatch = Boolean.parseBoolean(
+
filterConfig.getInitParameter(INGRESS_SOURCE_SPIFFE_IDS_ROUTING_SKIP_TRUST_DOMAIN_MATCH_PARAM));
+ ingressSourceRoutingSkipValidation = Boolean.parseBoolean(
+
filterConfig.getInitParameter(INGRESS_SOURCE_SPIFFE_IDS_ROUTING_SKIP_VALIDATION_PARAM));
+
+ // Parsed strictly whether or not routing is enabled, so a malformed entry
fails the
+ // topology at startup instead of sitting inert.
+ final String ingressSourceSpiffeIdsParam =
+
blankToNull(filterConfig.getInitParameter(INGRESS_SOURCE_SPIFFE_IDS_PARAM));
+ ingressSourceSpiffeIds = new ArrayList<>();
+ if (ingressSourceSpiffeIdsParam != null) {
+ for (final String entry :
parseCommaSeparated(ingressSourceSpiffeIdsParam)) {
+ final Optional<SpiffeId> parsed = SpiffeId.parse(entry);
+ if (parsed.isEmpty()) {
+ throw new ServletException(String.format(Locale.ROOT,
+ "%s contains an invalid SPIFFE id entry: %s",
INGRESS_SOURCE_SPIFFE_IDS_PARAM, entry));
+ }
+ ingressSourceSpiffeIds.add(lowerCased(parsed.get()));
+ }
+ }
+
if (enforceActSubMatchesSourceSpiffeId && sourceSpiffeIdHeader == null) {
throw new ServletException(String.format(Locale.ROOT,
"%s is true but %s is not configured",
ENFORCE_ACT_SUB_MATCHES_SOURCE_SPIFFE_ID_PARAM,
SOURCE_SPIFFE_ID_HEADER_PARAM));
}
+
+ if (ingressSourceRoutingEnabled) {
+ if (sourceSpiffeIdHeader == null) {
+ throw new ServletException(String.format(Locale.ROOT,
+ "%s is true but %s is not configured",
INGRESS_SOURCE_SPIFFE_ID_ROUTING_ENABLED_PARAM,
+ SOURCE_SPIFFE_ID_HEADER_PARAM));
+ }
+ if (ingressSourceSpiffeIds.isEmpty()) {
+ throw new ServletException(String.format(Locale.ROOT,
+ "%s is true but %s is not configured",
INGRESS_SOURCE_SPIFFE_ID_ROUTING_ENABLED_PARAM,
+ INGRESS_SOURCE_SPIFFE_IDS_PARAM));
+ }
+ }
}
@Override
public AudienceValidationResult validate(final HttpServletRequest request,
final JWT token,
final List<String> configuredAudiences) {
final boolean hasActClaim = token.getClaimAsObject(JWTToken.ACT_CLAIM) !=
null;
- if (!hasActClaim && !validateAudiencesWithoutActClaim) {
- return
AudienceValidationResult.of(AbstractJWTFilter.matchesConfiguredAudiences(token,
configuredAudiences));
+
+ // Resolved before the early returns below because the routing decision
needs it. An
+ // absent value is not rejected here: that rejection happens further down,
after the destination
+ // header and server-name checks, so a request whose source header is
missing or unparseable
+ // is reported only once those earlier checks have passed.
+ final Optional<SpiffeId> sourceSpiffeId = sourceSpiffeIdHeader == null
+ ? Optional.empty() : sourceSpiffeIdFromHeader(request);
+ final boolean fromIngressSource = ingressSourceRoutingEnabled
+ && isConfiguredIngressSource(sourceSpiffeId);
+
+ if (!hasActClaim) {
+ if (fromIngressSource) {
+ // A token with no act claim arriving through the ingress names no
in-cluster destination,
+ // so its audience is either not checked at all or checked against the
fixed configured
+ // list. Both decisions are made before the aud claim is required to
be present, because
+ // the fixed-list check has its own handling for a token that carries
no aud claim.
+ return ingressSourceRoutingSkipValidation
+ ? AudienceValidationResult.of(true)
+ : AudienceValidationResult.of(
+ AbstractJWTFilter.matchesConfiguredAudiences(token,
configuredAudiences));
Review Comment:
`matchesConfiguredAudiences(token, null)` returns `true`, so with the
default `skip.validation=false` and no `knox.token.audiences`, this route
behaves exactly like `skip.validation=true`. The docs mention it, but nothing
at runtime does. Could we log a WARN at startup when routing is enabled and the
provider has no `knox.token.audiences`? (`AbstractJWTFilter` sees both.)
Alternatively, consider a dedicated `ingress.source.audiences` list. Reusing
`knox.token.audiences` also changes what mesh tokens without `act` accept when
`validate.audiences.without.act.claim=false`, which makes it hard to restrict
ingress tokens without affecting the rest.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/K8sDestinationAudienceValidator.java:
##########
@@ -331,6 +345,115 @@ public class K8sDestinationAudienceValidator implements
RequestAudienceValidator
public static final String ENFORCE_ACT_SUB_IS_SERVICE_ACCOUNT_PARAM =
PARAM_PREFIX + "enforce.act.sub.is.service.account";
+ /**
+ * Whether a request presented by an ingress gateway is routed to a
different audience check
+ * than validation against the request destination. A request is from an
ingress gateway when
+ * the SPIFFE id in the header named by {@link
#SOURCE_SPIFFE_ID_HEADER_PARAM} matches an entry
+ * of {@link #INGRESS_SOURCE_SPIFFE_IDS_PARAM}. Defaults to {@code false},
the fail-closed
+ * setting: with routing off, a request from an ingress gateway is handled
like a request from
+ * any other source.
+ *
+ * <p>When {@code true}, a request from an ingress gateway is routed by
whether its token
+ * carries an {@code act} claim.
+ *
+ * <p>A token with no {@code act} claim is not destination-validated. What
it is checked
+ * against depends on {@link
#INGRESS_SOURCE_SPIFFE_IDS_ROUTING_SKIP_VALIDATION_PARAM}: either
+ * no audience validation at all, or the fixed configured allowed-audience
list. Either way the
+ * token does not reach destination validation, so the requirement that
every configured header
+ * be present and parseable does not apply to it.
+ *
+ * <p>A token with an {@code act} claim is rejected, because a delegation
token must not
+ * arrive through the ingress. Routing never lets such a token through,
whatever the skip setting.
+ *
+ * <p>This parameter reads {@link #SOURCE_SPIFFE_ID_HEADER_PARAM} and
+ * {@link #INGRESS_SOURCE_SPIFFE_IDS_PARAM}, neither of which has a default.
Setting this to
+ * {@code true} without both of them is a configuration error:
initialization fails, rather than
+ * leaving routing a deployment asked for silently inactive.
+ *
+ * <p>Routing is driven by the value of the header named by
+ * {@link #SOURCE_SPIFFE_ID_HEADER_PARAM}, which is trusted at face value. A
caller able to set
+ * that header can have its request routed as an ingress request, so it must
be set or overwritten by a trusted
+ * component before the request reaches Knox. This matters most with
+ * {@link #INGRESS_SOURCE_SPIFFE_IDS_ROUTING_SKIP_VALIDATION_PARAM}, which
disables audience
+ * checking for the requests it routes.
+ *
+ * <p>A request whose configured source SPIFFE id header is missing or
unparseable does not name
+ * an ingress source, so it is not routed; it continues to the ordinary
flow. There it is
+ * rejected by the existing requirement that a configured header be present
and parseable,
+ * unless {@link #VALIDATE_AUDIENCES_WITHOUT_ACT_CLAIM_PARAM} is {@code
false}, in which case a
+ * token with no {@code act} claim is validated against the fixed configured
allowed-audience
+ * list.
+ */
+ public static final String INGRESS_SOURCE_SPIFFE_ID_ROUTING_ENABLED_PARAM =
+ PARAM_PREFIX + "ingress.source.spiffe-id.routing.enabled";
Review Comment:
Naming nit, but these names become public config once released: this one
says `spiffe-id` (singular) while the others say `spiffe-ids`. The names are
also quite long. Something like this might be clearer: (current -> suggested):
- `ingress.source.spiffe-id.routing.enabled` -> `ingress.routing.enabled`
- `ingress.source.spiffe-ids` -> `ingress.source.spiffe-ids`
- `ingress.source.spiffe-ids.routing.skip.trust.domain.match` ->
`ingress.routing.skip.trust.domain.match`
- `ingress.source.spiffe-ids.routing.skip.validation` ->
`ingress.routing.skip.audience.validation`
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/K8sDestinationAudienceValidator.java:
##########
@@ -49,14 +49,21 @@
* {@link AudienceResource#parseCustomForm(String, String)}.
* <p>The first form is tried at all only when {@link
#DNS_FORMAT_ENABLED_PARAM} is {@code true},
* its default; when it is {@code false}, every entry is evaluated only as the
custom form.
- * <p>A token carrying a delegation {@code act} claim is always routed through
this validator; see
- * {@link #validate(HttpServletRequest, JWT, List)}. A token with no {@code
act} claim is too,
- * unless {@link #VALIDATE_AUDIENCES_WITHOUT_ACT_CLAIM_PARAM} is {@code false}.
+ * <p>A token carrying a delegation {@code act} claim is always
destination-validated; see
+ * {@link #validate(HttpServletRequest, JWT, List)}. The one departure is
ingress source routing:
+ * when {@link #INGRESS_SOURCE_SPIFFE_ID_ROUTING_ENABLED_PARAM} is {@code
true} and the request's
+ * source SPIFFE id names one of the {@link #INGRESS_SOURCE_SPIFFE_IDS_PARAM},
such a token is
+ * rejected outright, because a delegation token must not arrive through the
ingress.
+ * A token with no {@code act} claim is destination-validated too, unless
ingress source routing
+ * applies to it or {@link #VALIDATE_AUDIENCES_WITHOUT_ACT_CLAIM_PARAM} is
{@code false}.
Review Comment:
If the `act`-token rejection changes (see the comment on `validate()`), this
paragraph needs updating too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
