gnodet-bot commented on code in PR #26664:
URL: https://github.com/apache/camel/pull/26664#discussion_r4061649980
##########
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:
💡 **Suggestion: add explicit negative assertion on fail-closed path**
The four new tests cover the fail-open marker comprehensively. The existing
`failsClosedWhenThePolicyCannotBeEvaluated` test already verifies that an
exception is thrown when `failOpen=false`, but it does not assert that
`DECISION_FAILED_OPEN` is absent. Adding that assertion turns a silent
assumption into an explicit contract: the marker only fires on the intentional
`failOpen` path, not as a side-effect of any exception in the evaluator.
Suggested addition to the existing test (not a new test — just one more
`assertThat`):
```java
assertThat(out.getMessage().getHeader(OpaConstants.DECISION_FAILED_OPEN)).isNull();
```
##########
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:
⚠️ **`isDecisionHeader()` is not updated — incomplete defensive filter**
`clearDecisionHeaders()` now correctly removes `DECISION_FAILED_OPEN` (this
line), but `isDecisionHeader()` — the method that prevents decision headers
from being fed back into OPA as input — was not updated to include it.
The current code path makes this safe: `clearDecisionHeaders()` runs at line
84, before `buildInput()` at line 87, so by the time `isDecisionHeader()` is
consulted the header is already gone. But the method's contract is "never let
any of our own decision headers into the OPA input document" — and it now
silently fails to cover `DECISION_FAILED_OPEN`. If the call ordering ever
shifts (an early-return path, a subclass that calls `buildInput()` before
`clearDecisionHeaders()`, a test that calls `buildInput()` directly), the gap
becomes exploitable: a sender that pre-sets `CamelOpaDecisionFailedOpen` could
have it included in the OPA input, which defeats the purpose of the header
being a component-only output.
The fix is one line:
```suggestion
message.removeHeader(OpaConstants.DECISION_FAILED_OPEN);
}
private static boolean isDecisionHeader(String name) {
return OpaConstants.DECISION_ALLOW.equalsIgnoreCase(name)
|| OpaConstants.DECISION.equalsIgnoreCase(name)
|| OpaConstants.POLICY_PATH.equalsIgnoreCase(name)
|| OpaConstants.DECISION_FAILED_OPEN.equalsIgnoreCase(name);
```
(The suggestion includes the closing `}` of `clearDecisionHeaders` and the
first lines of `isDecisionHeader` — adjust the diff range as needed if the
suggestion block spans them.)
--
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]
