oscerd commented on code in PR #26664:
URL: https://github.com/apache/camel/pull/26664#discussion_r4062254835
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaPolicyEvaluator.java:
##########
@@ -233,6 +237,9 @@ private static void clearDecisionHeaders(Exchange exchange)
{
message.removeHeader(OpaConstants.DECISION_ALLOW);
message.removeHeader(OpaConstants.DECISION);
message.removeHeader(OpaConstants.POLICY_PATH);
+ // as attacker-settable as the verdict itself: left in place, a sender
could preload it false and make a
+ // fail-open read as a decision a policy actually made
+ message.removeHeader(OpaConstants.DECISION_FAILED_OPEN);
}
Review Comment:
Fixed. You're right that the current path is safe and right that the
contract isn't.
`clearDecisionHeaders()` at line 84 runs before `buildInput()` at 87, so
today the header is already gone when the filter is consulted — which means
`isDecisionHeader()` returning false for it changes nothing *now*. But the
filter is what documents the rule, and anyone reordering those two calls, or
setting the marker from somewhere other than `evaluate()`, reintroduces the
leak with nothing to catch it. Added:
```java
|| OpaConstants.DECISION_FAILED_OPEN.equalsIgnoreCase(name);
```
Same reasoning as CAMEL-24732, which withheld credential headers from the
wildcard: OPA's decision logging ships the whole input document off the box, so
anything that can reach `input.headers` deserves a deliberate answer rather
than an incidental one.
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaPolicyEvaluator.java:
##########
@@ -233,6 +237,9 @@ private static void clearDecisionHeaders(Exchange exchange)
{
message.removeHeader(OpaConstants.DECISION_ALLOW);
message.removeHeader(OpaConstants.DECISION);
message.removeHeader(OpaConstants.POLICY_PATH);
+ // as attacker-settable as the verdict itself: left in place, a sender
could preload it false and make a
+ // fail-open read as a decision a policy actually made
+ message.removeHeader(OpaConstants.DECISION_FAILED_OPEN);
Review Comment:
Same fix as the adjacent thread — `DECISION_FAILED_OPEN` is in
`isDecisionHeader()` now.
On the pipeline scenario you describe: within a single `evaluate()` the
clear always precedes `buildInput`, so a stale sender-injected value cannot
reach the input document even before this change. Where it would have mattered
is a second evaluator in the same route reading it, or any future path that
builds input without clearing first. Closing the contract rather than relying
on call order is the right call either way.
_Claude Code on behalf of @oscerd_
##########
components/camel-opa/src/test/java/org/apache/camel/component/opa/OpaProducerTest.java:
##########
@@ -157,6 +157,52 @@ void deniesWhenTheVerdictIsNotABoolean() throws Exception {
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_ALLOW)).isEqualTo(false);
}
+ @Test
+ void marksAnExchangeThatOnlyProceededBecauseOfFailOpen() throws Exception {
+ when(client.evaluate(eq(PATH), anyMap(),
eq(Object.class))).thenThrow(new OPAException("connection refused"));
+
+ Exchange out = template.request(ENDPOINT + "&failOpen=true", e -> {
+ });
+
+ assertThat(out.getException()).isNull();
+
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_ALLOW)).isEqualTo(true);
+
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_FAILED_OPEN)).isEqualTo(true);
+ }
+
+ @Test
+ void doesNotMarkADecisionAPolicyActuallyMade() throws Exception {
+ // the point of the marker is that it separates the two, so an allow
from a real policy must not carry it
+ givenDecision(Boolean.TRUE);
+
+ Exchange out = template.request(ENDPOINT + "&failOpen=true", e -> {
+ });
+
+
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_ALLOW)).isEqualTo(true);
+
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_FAILED_OPEN)).isNull();
+ }
+
+ @Test
+ void doesNotLetAnInboundMessageClaimItDidNotFailOpen() throws Exception {
+ // as settable by a sender as the verdict was: left in place,
"FailedOpen=false" would disguise an
+ // unauthorized exchange as one a policy allowed - which is the audit
trail this header exists to give
+ when(client.evaluate(eq(PATH), anyMap(),
eq(Object.class))).thenThrow(new OPAException("connection refused"));
+
+ Exchange out = template.request(ENDPOINT + "&failOpen=true",
+ e ->
e.getMessage().setHeader(OpaConstants.DECISION_FAILED_OPEN, false));
+
+
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_FAILED_OPEN)).isEqualTo(true);
+ }
+
+ @Test
+ void clearsAClaimedFailOpenMarkerOnAnOrdinaryDecision() throws Exception {
+ givenDecision(Boolean.TRUE);
+
+ Exchange out = template.request(ENDPOINT,
+ e ->
e.getMessage().setHeader(OpaConstants.DECISION_FAILED_OPEN, true));
+
+
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_FAILED_OPEN)).isNull();
+ }
+
@Test
void failsClosedWhenThePolicyCannotBeEvaluated() throws Exception {
Review Comment:
Added — that was a real gap between what the tests check and what the PR
claims.
```java
// the marker is for the deliberate failOpen path only, not for any
exception the evaluator happens to hit
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_FAILED_OPEN)).isNull();
```
"Set only on the fail-open path" was carried by three tests asserting its
presence and none asserting its absence on the closed path, so the separation
the header exists to provide was an assumption. 86 tests in the module.
_Claude Code on behalf of @oscerd_
--
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]
