gnodet-bot commented on code in PR #27719:
URL: https://github.com/apache/camel/pull/27719#discussion_r4247491316
##########
components/camel-lra/src/main/java/org/apache/camel/service/lra/LRAClient.java:
##########
@@ -191,6 +207,16 @@ protected HttpRequest.Builder prepareRequest(URI uri,
Exchange exchange) {
return HttpRequest.newBuilder().uri(uri);
}
+ /**
+ * The query parameters of the participant callback URLs of a step, as the
participant receives them back.
+ */
+ private static Map<String, String> callbackParameters(LRASagaStep step) {
+ Map<String, String> parameters = new HashMap<>(step.getOptions());
+ step.getCompensation().ifPresent(endpoint ->
parameters.put(URL_COMPENSATION_KEY, endpoint.getEndpointUri()));
+ step.getCompletion().ifPresent(endpoint ->
parameters.put(URL_COMPLETION_KEY, endpoint.getEndpointUri()));
+ return parameters;
+ }
Review Comment:
⚠️ **Missing guard against reserved key collision.** `step.getOptions()` is
copied verbatim. If a saga option key matches `URL_COMPENSATION_KEY`,
`URL_COMPLETION_KEY`, or `URL_SIGNATURE_KEY`, `LRAUrlBuilder.query()` will
append both values, producing a duplicate query parameter. When the coordinator
calls back, `parseQuery()` uses `Collectors.toMap()` which throws
`IllegalStateException` on duplicates — caught by the `catch` block and
converted to `parameters = null` → `verify()` returns `false` → 403.
Consider filtering reserved keys from `step.getOptions()` or throwing early
at registration time:
```suggestion
private static Map<String, String> callbackParameters(LRASagaStep step) {
Set<String> reserved = Set.of(URL_COMPENSATION_KEY,
URL_COMPLETION_KEY, URL_SIGNATURE_KEY);
Map<String, String> parameters =
step.getOptions().entrySet().stream()
.filter(e -> !reserved.contains(e.getKey()))
.collect(Collectors.toMap(Map.Entry::getKey,
Map.Entry::getValue));
step.getCompensation().ifPresent(endpoint ->
parameters.put(URL_COMPENSATION_KEY, endpoint.getEndpointUri()));
step.getCompletion().ifPresent(endpoint ->
parameters.put(URL_COMPLETION_KEY, endpoint.getEndpointUri()));
return parameters;
}
```
##########
components/camel-lra/src/main/java/org/apache/camel/service/lra/LRACallbackSigner.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.camel.service.lra;
+
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.security.GeneralSecurityException;
+import java.security.MessageDigest;
+import java.util.Base64;
+import java.util.Map;
+import java.util.StringJoiner;
+import java.util.TreeMap;
+
+import javax.crypto.Mac;
+import javax.crypto.spec.SecretKeySpec;
+
+/**
+ * Signs the participant callback URLs enlisted with the LRA coordinator, and
verifies the signature when the
+ * coordinator calls them back, using HMAC-SHA256 with the callback secret of
the LRA saga service.
+ * <p>
+ * The signature covers the callback path and every query parameter of the
URL: the step endpoint URIs and the saga
+ * options. Parameters are sorted and encoded individually, so the signature
does not depend on the order or the
+ * encoding used by the coordinator when it calls the URL back.
+ */
+final class LRACallbackSigner {
+
+ private static final String ALGORITHM = "HmacSHA256";
+
+ private final SecretKeySpec key;
+
+ LRACallbackSigner(String secret) {
+ this.key = new SecretKeySpec(secret.getBytes(StandardCharsets.UTF_8),
ALGORITHM);
+ }
+
+ /**
+ * Computes the signature of a callback URL.
+ *
+ * @param callbackPath the participant path the URL points to (compensate
or complete)
+ * @param parameters the query parameters of the URL, without the
signature
+ * @return the signature, Base64 URL-safe encoded without
padding
+ */
+ String sign(String callbackPath, Map<String, String> parameters) {
+ try {
+ Mac mac = Mac.getInstance(ALGORITHM);
+ mac.init(key);
+ byte[] digest = mac.doFinal(canonicalForm(callbackPath,
parameters).getBytes(StandardCharsets.UTF_8));
+ return
Base64.getUrlEncoder().withoutPadding().encodeToString(digest);
+ } catch (GeneralSecurityException e) {
+ throw new IllegalStateException("Cannot compute the signature of
the LRA participant callback URL", e);
+ }
+ }
+
+ /**
+ * Checks the signature of a callback URL, in constant time.
+ *
+ * @param callbackPath the participant path that was called (compensate
or complete)
+ * @param parameters the query parameters of the called URL, without
the signature
+ * @param signature the signature received with the URL, may be
<tt>null</tt>
+ * @return true if the signature matches the path and the
parameters
+ */
+ boolean verify(String callbackPath, Map<String, String> parameters, String
signature) {
+ if (signature == null) {
+ return false;
+ }
+ byte[] expected = sign(callbackPath,
parameters).getBytes(StandardCharsets.UTF_8);
+ return MessageDigest.isEqual(expected,
signature.getBytes(StandardCharsets.UTF_8));
+ }
+
+ private static String canonicalForm(String callbackPath, Map<String,
String> parameters) {
+ StringJoiner joiner = new StringJoiner("&", callbackPath + "?", "");
+ new TreeMap<>(parameters).forEach((name, value) ->
joiner.add(encode(name) + "=" + encode(value)));
+ return joiner.toString();
Review Comment:
💡 `new TreeMap<>(parameters)` will throw `NullPointerException` if
`parameters` is `null`. Currently unreachable (all callers guard against null
upstream), but a defensive precondition would make the invariant explicit and
protect against future callers:
```suggestion
new TreeMap<>(Objects.requireNonNull(parameters, "parameters must
not be null")).forEach((name, value) -> joiner.add(encode(name) + "=" +
encode(value)));
```
--
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]
