[
https://issues.apache.org/jira/browse/KNOX-3534?focusedWorklogId=1051853&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1051853
]
ASF GitHub Bot logged work on KNOX-3534:
----------------------------------------
Author: ASF GitHub Bot
Created on: 02/Nov/26 12:10
Start Date: 02/Nov/26 12:10
Worklog Time Spent: 10m
Work Description: 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.
Issue Time Tracking
-------------------
Worklog Id: (was: 1051853)
Time Spent: 0.5h (was: 20m)
> Add k8s request audience validator bypass for JWTs from an ingress
> ------------------------------------------------------------------
>
> Key: KNOX-3534
> URL: https://issues.apache.org/jira/browse/KNOX-3534
> Project: Apache Knox
> Issue Type: Sub-task
> Components: JWT
> Reporter: Harrison Sheinblatt
> Assignee: Harrison Sheinblatt
> Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> The existing K8sDestinationAudienceValidator feature focus on service to
> service audience validation. Enable using the validator for the use case
> where a single topology is to validate all jwts, both service to service and
> ingress traffic JWTs.
> Add flags to enable a feature to match JWTs coming from an ingress gateway
> and treat them differently: either validate the audience against the same
> static whitelist used for the rest of the traffic, or skip audience
> validation. This path would be split and evaluated first. Delegation tokens
> are not allowed on that path, so even if they come from an ingress, still
> treat them as they are now.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
