gnodet-bot commented on code in PR #26430:
URL: https://github.com/apache/camel/pull/26430#discussion_r4006263822
##########
components/camel-opa/src/main/java/org/apache/camel/component/opa/OpaPolicyEvaluator.java:
##########
@@ -185,6 +192,22 @@ private static boolean includesAnything(Set<String>
filter) {
return filter == null || !filter.isEmpty();
}
+ /**
+ * A credential header is only sent when the configuration names it, never
through the wildcard.
+ */
+ private boolean isWithheldCredential(String name) {
+ return includedHeaders == null && CREDENTIAL_HEADERS.contains(name);
+ }
+
+ private static Set<String> credentialHeaders() {
+ Set<String> names = new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
+ names.add("Authorization");
+ names.add("Proxy-Authorization");
+ names.add("Cookie");
Review Comment:
π‘ `CREDENTIAL_HEADERS` is a `static final` constant, but the `TreeSet` it
points to is mutable β any in-process code can `.add()` or `.clear()` it. Wrap
with `Collections.unmodifiableSet()` (and add the import):
```suggestion
private static Set<String> credentialHeaders() {
Set<String> names = new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
names.add("Authorization");
names.add("Proxy-Authorization");
names.add("Cookie");
names.add("Set-Cookie");
return Collections.unmodifiableSet(names);
}
```
##########
components/camel-opa/src/main/docs/opa-component.adoc:
##########
@@ -120,7 +120,15 @@
https://github.com/apache/camel/blob/main/components/camel-opa/src/test/resource
which is a worked example of both a plain boolean rule and a decision object
with deny reasons; `OpaIT`
alongside it shows the matching routes end to end.
-`headers` carries every message header by default. Set `includeHeaders` to a
comma-separated list of names
+`headers` carries every message header by default, with one exception: the
headers that carry a caller credential
+verbatim β `Authorization`, `Proxy-Authorization`, `Cookie` and `Set-Cookie` β
are *withheld* from the wildcard.
+OPA's decision logging ships the whole `input` document, frequently to a
remote collector, so the wildcard should
+not quietly export credentials off the box. A policy that genuinely needs one
can still have it by naming the
+header: `includeHeaders=Authorization,user` sends it. Matching is
case-insensitive, so `authorization` is withheld
+too.
+
+Prefer `includeProperties` for identity: a token that an earlier step has
already *verified* belongs there, as
+<<authorizing-an-identity>> describes, rather than handing the raw credential
to the policy to re-check. Set `includeHeaders` to a comma-separated list of
names
(matched case-insensitively) when the policy only needs a few of them. `body`
is *not* sent unless `includeBody`
Review Comment:
β οΈ Sentence splice β the new paragraph ending "β¦to re-check." runs directly
into the old sentence "Set `includeHeaders` to a comma-separated list of
names", producing a run-on. Looks like the old opening was meant to be replaced
but both survived the edit. The catalog copy at
`catalog/camel-catalog/src/generated/resources/β¦/opa-component.adoc` has the
same issue.
##########
components/camel-opa/src/test/java/org/apache/camel/component/opa/OpaInputDocumentTest.java:
##########
@@ -90,6 +90,37 @@ void convertsANonJsonBodyToItsStringForm() throws Exception {
assertThat(input).containsEntry("body", "the payload");
}
+ @Test
+ void withholdsCredentialHeadersFromTheWildcard() throws Exception {
+ Map<String, Object> input = inputSentFor(ENDPOINT, e -> {
+ e.getMessage().setHeader("user", "alice");
+ e.getMessage().setHeader("Authorization", "Bearer s3cr3t");
+ e.getMessage().setHeader("Cookie", "session=s3cr3t");
+ e.getMessage().setHeader("Proxy-Authorization", "Basic s3cr3t");
+ });
+
Review Comment:
π‘ The withhold list has four headers but this test only sets three β
`Set-Cookie` is missing. Since the assertion is `containsOnlyKeys("user")`, a
broken `Set-Cookie` filter would go unnoticed. Add it for completeness:
```suggestion
@Test
void withholdsCredentialHeadersFromTheWildcard() throws Exception {
Map<String, Object> input = inputSentFor(ENDPOINT, e -> {
e.getMessage().setHeader("user", "alice");
e.getMessage().setHeader("Authorization", "Bearer s3cr3t");
e.getMessage().setHeader("Cookie", "session=s3cr3t");
e.getMessage().setHeader("Proxy-Authorization", "Basic s3cr3t");
e.getMessage().setHeader("Set-Cookie", "session=abc; Path=/");
});
assertThat(headersOf(input)).containsOnlyKeys("user");
}
```
--
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]
