Copilot commented on code in PR #8185: URL: https://github.com/apache/incubator-seata/pull/8185#discussion_r3653002929
########## server/src/main/java/org/apache/seata/server/security/AllowedCallerRegistry.java: ########## @@ -0,0 +1,53 @@ +/* + * 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.seata.server.security; + +import java.util.Collection; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; + +/** + * In-memory table of NamingServer identities that TC trusts. Supports online replacement so + * operators can hot-swap secrets during rotation without a restart. + */ +public final class AllowedCallerRegistry { + + private final ConcurrentHashMap<String, AllowedCaller> byId = new ConcurrentHashMap<>(); + + public void register(AllowedCaller caller) { + byId.put(caller.getId(), caller); + } + + /** + * Swap the whole set. During a rotation the new secret and the old secret can coexist + * — the caller passes both entries in and this method installs them atomically. + */ + public void reload(Collection<AllowedCaller> callers) { + byId.clear(); + for (AllowedCaller c : callers) { + byId.put(c.getId(), c); + } + } Review Comment: AllowedCallerRegistry.reload() clears and repopulates the map, so concurrent find() calls can observe a transient empty/partial state during reload (contradicts the comment that it installs atomically). This can cause intermittent 401 UNKNOWN_CLUSTER_ID during key rotation/reload. ########## namingserver/src/main/java/org/apache/seata/namingserver/security/ClusterIdentityRegistry.java: ########## @@ -0,0 +1,65 @@ +/* + * 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.seata.namingserver.security; + +import java.util.Collection; +import java.util.Collections; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; + +/** + * In-memory table of {@link ClusterIdentity} keyed by cluster-id. Reads are lock-free; writes + * only happen at startup or on config reload, so this is heavily biased for the read path. + * + * <p>Kept as a small stand-alone class (rather than a full-blown "IdentityService") so it can + * be unit-tested without Spring context and shared between the inbound filter and the + * outbound signer. + */ +public final class ClusterIdentityRegistry { + + private final Map<String, ClusterIdentity> byId = new ConcurrentHashMap<>(); + + /** Register (or replace) an identity. Callers must have already validated the secret length. */ + public void register(ClusterIdentity identity) { + byId.put(identity.getId(), identity); + } + + /** Bulk-load, replacing everything atomically-ish (individual puts, not a swap). */ + public void reload(Collection<ClusterIdentity> identities) { + byId.clear(); + for (ClusterIdentity id : identities) { + byId.put(id.getId(), id); + } + } Review Comment: ClusterIdentityRegistry.reload() does clear()+put() which allows concurrent reads to see an empty/partial registry during reload. That can intermittently reject legitimate traffic during dynamic config updates. ########## server/src/main/java/org/apache/seata/server/security/RouteAuthorizer.java: ########## @@ -0,0 +1,57 @@ +/* + * 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.seata.server.security; + +/** + * Maps TC's inbound HTTP routes to a required {@link CallerPermission}. Deny-by-default: + * any path that isn't matched here returns {@code null} and the filter treats that as + * forbidden. + * + * <p>Kept intentionally small — only paths that could conceivably be reached by + * NamingServer or by an operator's console proxy need protection here. Netty ports and + * anything auto-served by Spring Boot infrastructure (actuator, static files) are + * expected to be either excluded up-front or protected by other means. + */ Review Comment: RouteAuthorizer Javadoc says unknown routes are treated as forbidden, but SeataServerAuthFilter passes through when requiredPermission() returns null. This mismatch can confuse operators/auditors about what is actually protected. ########## namingserver/src/main/java/org/apache/seata/namingserver/security/SecurityFilter.java: ########## @@ -0,0 +1,323 @@ +/* + * 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.seata.namingserver.security; + +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.BufferedReader; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; +import java.util.Enumeration; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletInputStream; +import jakarta.servlet.http.HttpServletRequestWrapper; +import org.apache.seata.common.security.CanonicalRequest; +import org.apache.seata.common.security.SecurityConstants; +import org.apache.seata.common.security.SignatureAlgorithm; +import org.apache.seata.common.security.SignatureVerifier; +import org.apache.seata.common.security.VerificationResult; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Servlet filter that authenticates every inbound HTTP request against a shared secret and + * a per-caller permission set. + * + * <p>Life-cycle of a single request: + * <ol> + * <li>Path is compared against {@link SecurityProperties#getExcludePaths()}; matches are + * passed straight through (e.g. {@code /naming/v1/health}).</li> + * <li>Extract security headers. If missing: + * <ul> + * <li>{@code WARN} mode → log and pass through.</li> + * <li>{@code ENFORCE} mode → reject with 401.</li> + * </ul> + * </li> + * <li>Look up {@link ClusterIdentity} by {@code X-Seata-Cluster-Id}. Unknown → 401.</li> + * <li>Buffer the request body so both the signature verifier and downstream controllers can + * read it — {@code ServletInputStream} is single-shot by default.</li> + * <li>Run {@link SignatureVerifier} — freshness, nonce, HMAC.</li> + * <li>Run {@link PermissionChecker} — route → permission and allow-list scoping.</li> + * <li>On success, expose the identity in the request attribute {@link #ATTR_IDENTITY} for + * downstream code (e.g. {@code ConsoleRemotingFilter} choosing the target TC) and pass on.</li> + * </ol> + */ +public class SecurityFilter implements Filter { + + /** Request attribute name under which the authenticated identity is exposed. */ + public static final String ATTR_IDENTITY = "seata.security.identity"; + + /** Response header used to surface the error code for machine consumers. */ + public static final String RESP_HEADER_ERROR = "X-Seata-Auth-Error"; + + private static final Logger LOGGER = LoggerFactory.getLogger(SecurityFilter.class); + + private final SecurityProperties properties; + + private final ClusterIdentityRegistry registry; + + private final SignatureVerifier verifier; + + private final PermissionChecker permissionChecker; + + public SecurityFilter(SecurityProperties properties, + ClusterIdentityRegistry registry, + SignatureVerifier verifier, + PermissionChecker permissionChecker) { + this.properties = Objects.requireNonNull(properties, "properties"); + this.registry = Objects.requireNonNull(registry, "registry"); + this.verifier = Objects.requireNonNull(verifier, "verifier"); + this.permissionChecker = Objects.requireNonNull(permissionChecker, "permissionChecker"); + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) { + chain.doFilter(request, response); + return; + } + HttpServletRequest httpReq = (HttpServletRequest) request; + HttpServletResponse httpResp = (HttpServletResponse) response; + + // Fast-path bypass: health checks and any other whitelisted path. + if (isExcluded(httpReq.getRequestURI())) { + chain.doFilter(request, response); + return; + } + + String clusterId = httpReq.getHeader(SecurityConstants.HEADER_CLUSTER_ID); + String timestamp = httpReq.getHeader(SecurityConstants.HEADER_TIMESTAMP); + String nonce = httpReq.getHeader(SecurityConstants.HEADER_NONCE); + String algName = httpReq.getHeader(SecurityConstants.HEADER_SIGN_ALG); + String signature = httpReq.getHeader(SecurityConstants.HEADER_SIGNATURE); + + // ---- Missing headers ---- + if (isBlank(clusterId) || isBlank(timestamp) || isBlank(nonce) || isBlank(signature)) { + handleFailure(httpReq, httpResp, chain, + SecurityConstants.ErrorCode.MISSING_SIGNATURE, + "one or more required security headers are missing", null); + return; + } + + // ---- Parse timestamp / algorithm ---- + long ts; + try { + ts = Long.parseLong(timestamp); + } catch (NumberFormatException e) { + handleFailure(httpReq, httpResp, chain, SecurityConstants.ErrorCode.BAD_REQUEST, + "timestamp header is not a valid long", null); + return; + } + SignatureAlgorithm algorithm; + try { + algorithm = algName == null ? SignatureAlgorithm.HMAC_SHA256 + : SignatureAlgorithm.fromWireName(algName); + } catch (IllegalArgumentException e) { + handleFailure(httpReq, httpResp, chain, SecurityConstants.ErrorCode.UNSUPPORTED_ALG, + e.getMessage(), null); + return; + } + + // ---- Identity lookup ---- + Optional<ClusterIdentity> identityOpt = registry.find(clusterId); + if (!identityOpt.isPresent()) { + handleFailure(httpReq, httpResp, chain, SecurityConstants.ErrorCode.UNKNOWN_CLUSTER_ID, + "no identity registered for cluster-id: " + clusterId, null); + return; + } + ClusterIdentity identity = identityOpt.get(); + + // ---- Buffer body so the verifier and the downstream controller can both read it. ---- + CachedBodyRequestWrapper wrapper = new CachedBodyRequestWrapper(httpReq); + byte[] body = wrapper.getCachedBody(); + + // ---- Verify signature ---- + CanonicalRequest canonical = CanonicalRequest.builder() + .method(httpReq.getMethod()) + .path(httpReq.getRequestURI()) + .queryParams(parseQuery(httpReq)) + .clusterId(clusterId) + .timestampMillis(ts) + .nonce(nonce) + .algorithm(algorithm) + .body(body) + .build(); + + VerificationResult result = verifier.verify(canonical, identity.getSecret(), signature); + if (!result.isSuccess()) { + handleFailure(wrapper, httpResp, chain, result.getErrorCode(), result.getMessage(), identity); + return; + } + + // ---- Permission check ---- + String namespace = httpReq.getParameter("namespace"); + String cluster = httpReq.getParameter("clusterName"); + String vgroup = httpReq.getParameter("vGroup"); + boolean allowed = permissionChecker.check(identity, httpReq.getMethod(), + httpReq.getRequestURI(), namespace, cluster, vgroup); + if (!allowed) { + handleFailure(wrapper, httpResp, chain, SecurityConstants.ErrorCode.FORBIDDEN, + "caller " + clusterId + " lacks permission for " + httpReq.getMethod() + + " " + httpReq.getRequestURI(), identity); + return; + } + + // ---- Success: hand off to the rest of the chain ---- + wrapper.setAttribute(ATTR_IDENTITY, identity); + chain.doFilter(wrapper, httpResp); + } + + private boolean isExcluded(String uri) { + List<String> excluded = properties.getExcludePaths(); + if (excluded == null || excluded.isEmpty()) { + return false; + } + for (String pattern : excluded) { + if (matches(pattern, uri)) { + return true; + } + } + return false; + } + + /** Minimal Ant-style matcher: supports trailing {@code /**} and exact match. */ + static boolean matches(String pattern, String uri) { + if (pattern == null || uri == null) { + return false; + } + if (pattern.endsWith("/**")) { + String prefix = pattern.substring(0, pattern.length() - 3); + return uri.startsWith(prefix); + } + return pattern.equals(uri); + } Review Comment: SecurityFilter.matches("/x/**", uri) uses uri.startsWith("/x"), which also matches "/xY" and can over-exclude routes. This can accidentally bypass auth if an exclude-path like "/actuator/**" is configured and a route starts with the same prefix but isn't a descendant. ########## server/src/main/java/org/apache/seata/server/security/SeataServerAuthFilter.java: ########## @@ -0,0 +1,344 @@ +/* + * 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.seata.server.security; + +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletInputStream; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletRequestWrapper; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.seata.common.security.CanonicalRequest; +import org.apache.seata.common.security.SecurityConstants; +import org.apache.seata.common.security.SignatureAlgorithm; +import org.apache.seata.common.security.SignatureVerifier; +import org.apache.seata.common.security.VerificationResult; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.BufferedReader; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; +import java.util.Enumeration; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +/** + * TC-side inbound filter that authenticates requests coming from NamingServer. + * + * <p>Structure mirrors {@code namingserver.security.SecurityFilter} — same pipeline: + * exclude → parse headers → identity lookup → buffer body → verify signature → authorise route. + * The two live in separate modules because they have different dependency footprints, but a + * mismatch in behaviour will show up in the shared integration tests. + * + * <p>The filter registers at {@link org.springframework.core.Ordered#HIGHEST_PRECEDENCE} in + * the auto-configuration so it runs before any of the existing TC filters + * (e.g. {@code XSSHttpRequestFilter}, {@code RaftRequestFilter}). + */ +public class SeataServerAuthFilter implements Filter { + + /** Exposes the authenticated caller on the request attributes. */ + public static final String ATTR_CALLER = "seata.server.security.caller"; + + /** Machine-readable error code on the response, mirroring the NamingServer side. */ + public static final String RESP_HEADER_ERROR = "X-Seata-Auth-Error"; + + private static final Logger LOGGER = LoggerFactory.getLogger(SeataServerAuthFilter.class); + + private final ServerSecurityProperties properties; + + private final AllowedCallerRegistry registry; + + private final SignatureVerifier verifier; + + private final RouteAuthorizer routeAuthorizer; + + public SeataServerAuthFilter( + ServerSecurityProperties properties, + AllowedCallerRegistry registry, + SignatureVerifier verifier, + RouteAuthorizer routeAuthorizer) { + this.properties = Objects.requireNonNull(properties, "properties"); + this.registry = Objects.requireNonNull(registry, "registry"); + this.verifier = Objects.requireNonNull(verifier, "verifier"); + this.routeAuthorizer = Objects.requireNonNull(routeAuthorizer, "routeAuthorizer"); + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) { + chain.doFilter(request, response); + return; + } + HttpServletRequest httpReq = (HttpServletRequest) request; + HttpServletResponse httpResp = (HttpServletResponse) response; + + String path = httpReq.getRequestURI(); + String method = httpReq.getMethod(); + + // 1. Route filter: only intercept paths TC deems sensitive. If the route is not + // on the protected list, pass through untouched — TC has other filters for the + // Netty side, static assets, etc. + CallerPermission required = routeAuthorizer.requiredPermission(method, path); + if (required == null || isExcluded(path)) { + chain.doFilter(request, response); + return; + } + + // 2. Extract security headers. + String clusterId = httpReq.getHeader(SecurityConstants.HEADER_CLUSTER_ID); + String tsStr = httpReq.getHeader(SecurityConstants.HEADER_TIMESTAMP); + String nonce = httpReq.getHeader(SecurityConstants.HEADER_NONCE); + String algName = httpReq.getHeader(SecurityConstants.HEADER_SIGN_ALG); + String sig = httpReq.getHeader(SecurityConstants.HEADER_SIGNATURE); + + if (isBlank(clusterId) || isBlank(tsStr) || isBlank(nonce) || isBlank(sig)) { + reject( + httpReq, + httpResp, + chain, + SecurityConstants.ErrorCode.MISSING_SIGNATURE, + "one or more required security headers are missing", + null); + return; + } + + long ts; + try { + ts = Long.parseLong(tsStr); + } catch (NumberFormatException e) { + reject( + httpReq, + httpResp, + chain, + SecurityConstants.ErrorCode.BAD_REQUEST, + "timestamp header is not a valid long", + null); + return; + } + SignatureAlgorithm alg; + try { + alg = algName == null ? SignatureAlgorithm.HMAC_SHA256 : SignatureAlgorithm.fromWireName(algName); + } catch (IllegalArgumentException e) { + reject(httpReq, httpResp, chain, SecurityConstants.ErrorCode.UNSUPPORTED_ALG, e.getMessage(), null); + return; + } + + // 3. Identity lookup. + Optional<AllowedCaller> callerOpt = registry.find(clusterId); + if (!callerOpt.isPresent()) { + reject( + httpReq, + httpResp, + chain, + SecurityConstants.ErrorCode.UNKNOWN_CLUSTER_ID, + "cluster-id not in allowed-callers: " + clusterId, + null); + return; + } + AllowedCaller caller = callerOpt.get(); + + // 4. Buffer body — needed so the verifier + controller can both read it. + CachedBodyRequestWrapper wrapper = new CachedBodyRequestWrapper(httpReq); + + // 5. Verify signature. + CanonicalRequest canonical = CanonicalRequest.builder() + .method(method) + .path(path) + .queryParams(collectQuery(httpReq)) + .clusterId(clusterId) + .timestampMillis(ts) + .nonce(nonce) + .algorithm(alg) + .body(wrapper.getCachedBody()) + .build(); + + VerificationResult verdict = verifier.verify(canonical, caller.getSecret(), sig); + if (!verdict.isSuccess()) { + reject(wrapper, httpResp, chain, verdict.getErrorCode(), verdict.getMessage(), caller); + return; + } + + // 6. Route-level permission — signature verified, but does the caller hold the right? + if (!caller.hasPermission(required)) { + reject( + wrapper, + httpResp, + chain, + SecurityConstants.ErrorCode.FORBIDDEN, + "caller " + clusterId + " lacks permission " + required, + caller); + return; + } + + // 7. Success. + wrapper.setAttribute(ATTR_CALLER, caller); + chain.doFilter(wrapper, httpResp); + } + + private boolean isExcluded(String path) { + List<String> excluded = properties.getExcludePaths(); + if (excluded == null || excluded.isEmpty()) { + return false; + } + for (String pattern : excluded) { + if (matches(pattern, path)) { + return true; + } + } + return false; + } + + static boolean matches(String pattern, String path) { + if (pattern == null || path == null) { + return false; + } + if (pattern.endsWith("/**")) { + return path.startsWith(pattern.substring(0, pattern.length() - 3)); + } + return pattern.equals(path); + } Review Comment: matches("/x/**", path) currently uses startsWith("/x") which also matches "/xY..." (e.g. "/actuatorX"), so excludePaths can unintentionally bypass auth for non-descendant routes. For Ant-style "/prefix/**" it should match exactly "/prefix" or "/prefix/...". ########## server/pom.xml: ########## @@ -135,6 +135,12 @@ <groupId>javax.servlet</groupId> <artifactId>javax.servlet-api</artifactId> </dependency> + <!-- Jakarta Servlet API required by the security auth filter, aligned with Spring Boot 4.x. --> Review Comment: server/pom.xml declares both javax.servlet-api (compile scope) and jakarta.servlet-api (provided). With Spring Boot 4.x targeting Jakarta namespaces, keeping javax.servlet-api can introduce unnecessary/legacy dependencies and confusion; if nothing in server uses javax.servlet.*, it should be removed. ########## common/src/main/java/org/apache/seata/common/security/NonceCache.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.seata.common.security; + +import java.util.Iterator; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicLong; + +/** + * A per-JVM anti-replay cache. It stores nonces recently seen inside the replay window and + * refuses to re-admit any of them, so an attacker cannot capture a signed request and re-send it. + * + * <h3>Design notes</h3> + * <ul> + * <li>Backed by {@link ConcurrentHashMap} of {@code cluster-id + '|' + nonce → insertion millis} + * — namespacing by cluster-id prevents cross-tenant nonce collisions.</li> + * <li>Lazy eviction: entries are removed on access when older than the TTL. A tiny amount of + * cross-check work amortises against every {@link #putIfAbsent(String, String)} call, + * so we do not need a background thread.</li> + * <li>The single-node scope is intentional — this class only defends replay against the + * exact same NamingServer node. To defend across nodes when the caller cycles hosts, + * bind the target node id into the signature (see {@code CanonicalRequest}). A future + * Redis-backed impl of this interface would close the multi-node gap.</li> + * </ul> + */ +public interface NonceCache { + + /** + * Attempt to register a nonce for a given cluster identity. + * + * @param clusterId caller identity ({@code X-Seata-Cluster-Id} header value) + * @param nonce the received nonce ({@code X-Seata-Nonce} header value) + * @return {@code true} if the nonce was fresh and has been recorded; + * {@code false} if the same nonce was already seen inside the TTL + */ + boolean putIfAbsent(String clusterId, String nonce); + + /** Current cache size — exposed for metrics / tests. */ + int size(); + + /** Force TTL eviction now — normally not needed; provided for tests. */ + void evictExpired(); + + /** + * Default in-memory implementation. Thread-safe, lock-free on the fast path. + */ + final class InMemory implements NonceCache { + + private final ConcurrentHashMap<String, Long> entries = new ConcurrentHashMap<>(); + private final long ttlMillis; + private final AtomicLong opsSinceLastSweep = new AtomicLong(); + private final long sweepEveryOps; + private final Clock clock; + + /** + * @param ttlMillis how long a nonce is remembered + * @param clock time source (injectable for deterministic tests) + */ + public InMemory(long ttlMillis, Clock clock) { + this(ttlMillis, clock, 1024L); + } + + /** + * @param ttlMillis nonce TTL + * @param clock time source + * @param sweepEveryOps run a lazy TTL sweep once every N operations + */ + public InMemory(long ttlMillis, Clock clock, long sweepEveryOps) { + if (ttlMillis <= 0) { + throw new IllegalArgumentException("ttlMillis must be > 0"); + } + this.ttlMillis = ttlMillis; + this.clock = clock == null ? System::currentTimeMillis : clock; + this.sweepEveryOps = Math.max(1L, sweepEveryOps); + } + + @Override + public boolean putIfAbsent(String clusterId, String nonce) { + if (clusterId == null || nonce == null) { + throw new IllegalArgumentException("clusterId and nonce must not be null"); + } + long now = clock.now(); + String key = key(clusterId, nonce); + Long prev = entries.putIfAbsent(key, now); + if (prev != null) { + // If the existing entry has itself expired, treat this as a fresh insert. + if (now - prev > ttlMillis) { + // Replace only if nobody has moved the value in the meantime. + if (entries.replace(key, prev, now)) { + maybeSweep(now); + return true; + } + // Someone else already registered it after expiry — treat as replay. + return false; + } + return false; + } + maybeSweep(now); + return true; + } + + @Override + public int size() { + return entries.size(); + } + + @Override + public void evictExpired() { + long now = clock.now(); + Iterator<Map.Entry<String, Long>> it = entries.entrySet().iterator(); + while (it.hasNext()) { + Map.Entry<String, Long> e = it.next(); + if (now - e.getValue() > ttlMillis) { + it.remove(); + } + } + } + + private void maybeSweep(long now) { + long ops = opsSinceLastSweep.incrementAndGet(); + if (ops % sweepEveryOps == 0) { + evictExpired(); + } + } + + private static String key(String clusterId, String nonce) { + // Deliberately a plain concatenation with a delimiter that cannot appear inside + // a UUID nonce or a cluster-id. Avoids the overhead of building a composite key object. + return clusterId + '|' + nonce; + } Review Comment: NonceCache.InMemory builds the replay key by concatenating clusterId + '|' + nonce, but both values are attacker-controlled headers. If either contains '|', different (clusterId, nonce) pairs can collide, causing cross-tenant nonce collisions and denial-of-service via forced REPLAY_DETECTED. ########## common/src/main/java/org/apache/seata/common/security/HmacSigner.java: ########## @@ -0,0 +1,138 @@ +/* + * 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.seata.common.security; + +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; +import java.nio.charset.StandardCharsets; +import java.security.InvalidKeyException; +import java.security.NoSuchAlgorithmException; +import java.util.Base64; +import java.util.Objects; + +/** + * Core HMAC sign / verify logic. Deliberately kept small and dependency-free so it can be + * shared between {@code common}, {@code namingserver}, {@code seata-server} and clients. + * + * <h3>Signing</h3> + * {@link #sign(CanonicalRequest, byte[])} produces the Base64-encoded MAC that goes into the + * {@link SecurityConstants#HEADER_SIGNATURE} header. + * + * <h3>Verifying</h3> + * {@link #verify(CanonicalRequest, byte[], String)} recomputes the MAC and compares it with the + * received value using a <b>constant-time</b> comparison. That defends against timing side + * channels that would otherwise let an attacker guess the signature one byte at a time. + * + * <p>The class is stateless and thread-safe: each call creates a fresh {@link Mac} instance. + * {@code Mac} itself is <em>not</em> thread-safe if reused across threads, but constructing one is + * cheap compared to the network round trip we are protecting. + */ +public final class HmacSigner { + + /** Reject secrets shorter than this to prevent trivial brute force. 256 bit minimum. */ + public static final int MIN_KEY_LENGTH_BYTES = 32; + + private HmacSigner() { + // utility + } + + /** + * Sign a canonical request. + * + * @param request the request being signed + * @param secret raw shared secret bytes (≥ {@link #MIN_KEY_LENGTH_BYTES}) + * @return Base64-encoded MAC + * @throws IllegalArgumentException if the secret is too short + * @throws IllegalStateException if the JCA provider is missing the algorithm + */ + public static String sign(CanonicalRequest request, byte[] secret) { + Objects.requireNonNull(request, "request"); + validateSecret(secret); + byte[] mac = mac( + request.getAlgorithm(), + secret, + SignatureCanonicalizer.canonicalize(request).getBytes(StandardCharsets.UTF_8)); + return Base64.getEncoder().encodeToString(mac); + } + + /** + * Verify a signature. Returns {@code true} iff the recomputed signature exactly matches the + * received value, in constant time relative to the signature length. + * + * @param request canonical request as the receiver reconstructed it + * @param secret the shared secret for the caller identity + * @param receivedSignature Base64 string from {@link SecurityConstants#HEADER_SIGNATURE} + * @return {@code true} iff the signature is valid + */ + public static boolean verify(CanonicalRequest request, byte[] secret, String receivedSignature) { + if (receivedSignature == null || receivedSignature.isEmpty()) { + return false; + } + byte[] received; + try { + received = Base64.getDecoder().decode(receivedSignature); + } catch (IllegalArgumentException badBase64) { + // Not valid Base64 → cannot possibly match. Do NOT surface the reason + // to the caller: it would leak information about the failure mode. + return false; + } + byte[] expected = mac( + request.getAlgorithm(), + secret, + SignatureCanonicalizer.canonicalize(request).getBytes(StandardCharsets.UTF_8)); + return constantTimeEquals(expected, received); + } + + /** + * Length-safe constant-time byte array comparison. Both inputs are walked to + * their full lengths before returning, so timing does not depend on which byte + * first differs. + */ + static boolean constantTimeEquals(byte[] a, byte[] b) { + if (a == null || b == null) { + return false; + } + // XOR the length difference into the accumulator too: mismatched lengths must + // never fast-fail (that would leak length via timing). + int diff = a.length ^ b.length; + int len = Math.min(a.length, b.length); + for (int i = 0; i < len; i++) { + diff |= (a[i] ^ b[i]); + } + return diff == 0; + } Review Comment: The constantTimeEquals() Javadoc says both inputs are walked to their full lengths, but the implementation only iterates to min(a.length,b.length). This makes timing dependent on attacker-controlled length and contradicts the stated side-channel properties. ########## server/src/main/java/org/apache/seata/server/security/SeataServerAuthFilter.java: ########## @@ -0,0 +1,344 @@ +/* + * 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.seata.server.security; + +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletInputStream; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletRequestWrapper; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.seata.common.security.CanonicalRequest; +import org.apache.seata.common.security.SecurityConstants; +import org.apache.seata.common.security.SignatureAlgorithm; +import org.apache.seata.common.security.SignatureVerifier; +import org.apache.seata.common.security.VerificationResult; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.BufferedReader; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; +import java.util.Enumeration; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +/** + * TC-side inbound filter that authenticates requests coming from NamingServer. + * + * <p>Structure mirrors {@code namingserver.security.SecurityFilter} — same pipeline: + * exclude → parse headers → identity lookup → buffer body → verify signature → authorise route. + * The two live in separate modules because they have different dependency footprints, but a + * mismatch in behaviour will show up in the shared integration tests. + * + * <p>The filter registers at {@link org.springframework.core.Ordered#HIGHEST_PRECEDENCE} in + * the auto-configuration so it runs before any of the existing TC filters + * (e.g. {@code XSSHttpRequestFilter}, {@code RaftRequestFilter}). + */ +public class SeataServerAuthFilter implements Filter { + + /** Exposes the authenticated caller on the request attributes. */ + public static final String ATTR_CALLER = "seata.server.security.caller"; + + /** Machine-readable error code on the response, mirroring the NamingServer side. */ + public static final String RESP_HEADER_ERROR = "X-Seata-Auth-Error"; + + private static final Logger LOGGER = LoggerFactory.getLogger(SeataServerAuthFilter.class); + + private final ServerSecurityProperties properties; + + private final AllowedCallerRegistry registry; + + private final SignatureVerifier verifier; + + private final RouteAuthorizer routeAuthorizer; + + public SeataServerAuthFilter( + ServerSecurityProperties properties, + AllowedCallerRegistry registry, + SignatureVerifier verifier, + RouteAuthorizer routeAuthorizer) { + this.properties = Objects.requireNonNull(properties, "properties"); + this.registry = Objects.requireNonNull(registry, "registry"); + this.verifier = Objects.requireNonNull(verifier, "verifier"); + this.routeAuthorizer = Objects.requireNonNull(routeAuthorizer, "routeAuthorizer"); + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) { + chain.doFilter(request, response); + return; + } + HttpServletRequest httpReq = (HttpServletRequest) request; + HttpServletResponse httpResp = (HttpServletResponse) response; + + String path = httpReq.getRequestURI(); + String method = httpReq.getMethod(); + + // 1. Route filter: only intercept paths TC deems sensitive. If the route is not + // on the protected list, pass through untouched — TC has other filters for the + // Netty side, static assets, etc. + CallerPermission required = routeAuthorizer.requiredPermission(method, path); + if (required == null || isExcluded(path)) { + chain.doFilter(request, response); + return; + } + + // 2. Extract security headers. + String clusterId = httpReq.getHeader(SecurityConstants.HEADER_CLUSTER_ID); + String tsStr = httpReq.getHeader(SecurityConstants.HEADER_TIMESTAMP); + String nonce = httpReq.getHeader(SecurityConstants.HEADER_NONCE); + String algName = httpReq.getHeader(SecurityConstants.HEADER_SIGN_ALG); + String sig = httpReq.getHeader(SecurityConstants.HEADER_SIGNATURE); + + if (isBlank(clusterId) || isBlank(tsStr) || isBlank(nonce) || isBlank(sig)) { + reject( + httpReq, + httpResp, + chain, + SecurityConstants.ErrorCode.MISSING_SIGNATURE, + "one or more required security headers are missing", + null); + return; + } + + long ts; + try { + ts = Long.parseLong(tsStr); + } catch (NumberFormatException e) { + reject( + httpReq, + httpResp, + chain, + SecurityConstants.ErrorCode.BAD_REQUEST, + "timestamp header is not a valid long", + null); + return; + } + SignatureAlgorithm alg; + try { + alg = algName == null ? SignatureAlgorithm.HMAC_SHA256 : SignatureAlgorithm.fromWireName(algName); + } catch (IllegalArgumentException e) { + reject(httpReq, httpResp, chain, SecurityConstants.ErrorCode.UNSUPPORTED_ALG, e.getMessage(), null); + return; + } + + // 3. Identity lookup. + Optional<AllowedCaller> callerOpt = registry.find(clusterId); + if (!callerOpt.isPresent()) { + reject( + httpReq, + httpResp, + chain, + SecurityConstants.ErrorCode.UNKNOWN_CLUSTER_ID, + "cluster-id not in allowed-callers: " + clusterId, + null); + return; + } + AllowedCaller caller = callerOpt.get(); + + // 4. Buffer body — needed so the verifier + controller can both read it. + CachedBodyRequestWrapper wrapper = new CachedBodyRequestWrapper(httpReq); + + // 5. Verify signature. + CanonicalRequest canonical = CanonicalRequest.builder() + .method(method) + .path(path) + .queryParams(collectQuery(httpReq)) + .clusterId(clusterId) + .timestampMillis(ts) + .nonce(nonce) + .algorithm(alg) + .body(wrapper.getCachedBody()) + .build(); + + VerificationResult verdict = verifier.verify(canonical, caller.getSecret(), sig); + if (!verdict.isSuccess()) { + reject(wrapper, httpResp, chain, verdict.getErrorCode(), verdict.getMessage(), caller); + return; + } + + // 6. Route-level permission — signature verified, but does the caller hold the right? + if (!caller.hasPermission(required)) { + reject( + wrapper, + httpResp, + chain, + SecurityConstants.ErrorCode.FORBIDDEN, + "caller " + clusterId + " lacks permission " + required, + caller); + return; + } + + // 7. Success. + wrapper.setAttribute(ATTR_CALLER, caller); + chain.doFilter(wrapper, httpResp); + } + + private boolean isExcluded(String path) { + List<String> excluded = properties.getExcludePaths(); + if (excluded == null || excluded.isEmpty()) { + return false; + } + for (String pattern : excluded) { + if (matches(pattern, path)) { + return true; + } + } + return false; + } + + static boolean matches(String pattern, String path) { + if (pattern == null || path == null) { + return false; + } + if (pattern.endsWith("/**")) { + return path.startsWith(pattern.substring(0, pattern.length() - 3)); + } + return pattern.equals(path); + } + + private void reject( + HttpServletRequest req, + HttpServletResponse resp, + FilterChain chain, + SecurityConstants.ErrorCode code, + String message, + AllowedCaller caller) + throws IOException, ServletException { + ServerSecurityProperties.Mode mode = properties.getMode(); + if (mode == ServerSecurityProperties.Mode.WARN) { + LOGGER.warn( + "[seata-server][security][WARN] {} {} rejected code={} msg={} caller={}", + req.getMethod(), + req.getRequestURI(), + code, + message, + caller == null ? "unknown" : caller.getId()); + if (caller != null) { + req.setAttribute(ATTR_CALLER, caller); + } + chain.doFilter(req, resp); + return; + } + int status = (code == SecurityConstants.ErrorCode.FORBIDDEN) + ? HttpServletResponse.SC_FORBIDDEN + : HttpServletResponse.SC_UNAUTHORIZED; + LOGGER.warn( + "[seata-server][security][ENFORCE] {} {} status={} code={} msg={} caller={}", + req.getMethod(), + req.getRequestURI(), + status, + code, + message, + caller == null ? "unknown" : caller.getId()); + resp.setHeader(RESP_HEADER_ERROR, code.name()); + resp.setContentType("application/json;charset=UTF-8"); + resp.setStatus(status); + resp.getWriter().write("{\"code\":\"" + code.name() + "\",\"message\":\"" + escapeJson(message) + "\"}"); + } + + private static Map<String, String> collectQuery(HttpServletRequest req) { + Map<String, String> params = new LinkedHashMap<>(); + Enumeration<String> names = req.getParameterNames(); + while (names.hasMoreElements()) { + String name = names.nextElement(); + params.put(name, req.getParameter(name)); + } + return params; + } + + private static boolean isBlank(String s) { + return s == null || s.isEmpty(); + } + + private static String escapeJson(String v) { + if (v == null) { + return ""; + } + return v.replace("\\", "\\\\").replace("\"", "\\\""); + } Review Comment: escapeJson() only escapes backslash and quotes, so error messages containing control characters (e.g. newlines/tabs) will emit invalid JSON in the rejection body. Since parts of the message can include untrusted header values, JSON escaping should handle standard control characters at minimum. ########## namingserver/src/main/java/org/apache/seata/namingserver/security/SecurityFilter.java: ########## @@ -0,0 +1,323 @@ +/* + * 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.seata.namingserver.security; + +import jakarta.servlet.Filter; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRequest; +import jakarta.servlet.ServletResponse; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.BufferedReader; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStreamReader; +import java.nio.charset.StandardCharsets; +import java.util.Enumeration; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +import jakarta.servlet.ReadListener; +import jakarta.servlet.ServletInputStream; +import jakarta.servlet.http.HttpServletRequestWrapper; +import org.apache.seata.common.security.CanonicalRequest; +import org.apache.seata.common.security.SecurityConstants; +import org.apache.seata.common.security.SignatureAlgorithm; +import org.apache.seata.common.security.SignatureVerifier; +import org.apache.seata.common.security.VerificationResult; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Servlet filter that authenticates every inbound HTTP request against a shared secret and + * a per-caller permission set. + * + * <p>Life-cycle of a single request: + * <ol> + * <li>Path is compared against {@link SecurityProperties#getExcludePaths()}; matches are + * passed straight through (e.g. {@code /naming/v1/health}).</li> + * <li>Extract security headers. If missing: + * <ul> + * <li>{@code WARN} mode → log and pass through.</li> + * <li>{@code ENFORCE} mode → reject with 401.</li> + * </ul> + * </li> + * <li>Look up {@link ClusterIdentity} by {@code X-Seata-Cluster-Id}. Unknown → 401.</li> + * <li>Buffer the request body so both the signature verifier and downstream controllers can + * read it — {@code ServletInputStream} is single-shot by default.</li> + * <li>Run {@link SignatureVerifier} — freshness, nonce, HMAC.</li> + * <li>Run {@link PermissionChecker} — route → permission and allow-list scoping.</li> + * <li>On success, expose the identity in the request attribute {@link #ATTR_IDENTITY} for + * downstream code (e.g. {@code ConsoleRemotingFilter} choosing the target TC) and pass on.</li> + * </ol> + */ +public class SecurityFilter implements Filter { + + /** Request attribute name under which the authenticated identity is exposed. */ + public static final String ATTR_IDENTITY = "seata.security.identity"; + + /** Response header used to surface the error code for machine consumers. */ + public static final String RESP_HEADER_ERROR = "X-Seata-Auth-Error"; + + private static final Logger LOGGER = LoggerFactory.getLogger(SecurityFilter.class); + + private final SecurityProperties properties; + + private final ClusterIdentityRegistry registry; + + private final SignatureVerifier verifier; + + private final PermissionChecker permissionChecker; + + public SecurityFilter(SecurityProperties properties, + ClusterIdentityRegistry registry, + SignatureVerifier verifier, + PermissionChecker permissionChecker) { + this.properties = Objects.requireNonNull(properties, "properties"); + this.registry = Objects.requireNonNull(registry, "registry"); + this.verifier = Objects.requireNonNull(verifier, "verifier"); + this.permissionChecker = Objects.requireNonNull(permissionChecker, "permissionChecker"); + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) { + chain.doFilter(request, response); + return; + } + HttpServletRequest httpReq = (HttpServletRequest) request; + HttpServletResponse httpResp = (HttpServletResponse) response; + + // Fast-path bypass: health checks and any other whitelisted path. + if (isExcluded(httpReq.getRequestURI())) { + chain.doFilter(request, response); + return; + } + + String clusterId = httpReq.getHeader(SecurityConstants.HEADER_CLUSTER_ID); + String timestamp = httpReq.getHeader(SecurityConstants.HEADER_TIMESTAMP); + String nonce = httpReq.getHeader(SecurityConstants.HEADER_NONCE); + String algName = httpReq.getHeader(SecurityConstants.HEADER_SIGN_ALG); + String signature = httpReq.getHeader(SecurityConstants.HEADER_SIGNATURE); + + // ---- Missing headers ---- + if (isBlank(clusterId) || isBlank(timestamp) || isBlank(nonce) || isBlank(signature)) { + handleFailure(httpReq, httpResp, chain, + SecurityConstants.ErrorCode.MISSING_SIGNATURE, + "one or more required security headers are missing", null); + return; + } + + // ---- Parse timestamp / algorithm ---- + long ts; + try { + ts = Long.parseLong(timestamp); + } catch (NumberFormatException e) { + handleFailure(httpReq, httpResp, chain, SecurityConstants.ErrorCode.BAD_REQUEST, + "timestamp header is not a valid long", null); + return; + } + SignatureAlgorithm algorithm; + try { + algorithm = algName == null ? SignatureAlgorithm.HMAC_SHA256 + : SignatureAlgorithm.fromWireName(algName); + } catch (IllegalArgumentException e) { + handleFailure(httpReq, httpResp, chain, SecurityConstants.ErrorCode.UNSUPPORTED_ALG, + e.getMessage(), null); + return; + } + + // ---- Identity lookup ---- + Optional<ClusterIdentity> identityOpt = registry.find(clusterId); + if (!identityOpt.isPresent()) { + handleFailure(httpReq, httpResp, chain, SecurityConstants.ErrorCode.UNKNOWN_CLUSTER_ID, + "no identity registered for cluster-id: " + clusterId, null); + return; + } + ClusterIdentity identity = identityOpt.get(); + + // ---- Buffer body so the verifier and the downstream controller can both read it. ---- + CachedBodyRequestWrapper wrapper = new CachedBodyRequestWrapper(httpReq); + byte[] body = wrapper.getCachedBody(); + + // ---- Verify signature ---- + CanonicalRequest canonical = CanonicalRequest.builder() + .method(httpReq.getMethod()) + .path(httpReq.getRequestURI()) + .queryParams(parseQuery(httpReq)) + .clusterId(clusterId) + .timestampMillis(ts) + .nonce(nonce) + .algorithm(algorithm) + .body(body) + .build(); + + VerificationResult result = verifier.verify(canonical, identity.getSecret(), signature); + if (!result.isSuccess()) { + handleFailure(wrapper, httpResp, chain, result.getErrorCode(), result.getMessage(), identity); + return; + } + + // ---- Permission check ---- + String namespace = httpReq.getParameter("namespace"); + String cluster = httpReq.getParameter("clusterName"); + String vgroup = httpReq.getParameter("vGroup"); + boolean allowed = permissionChecker.check(identity, httpReq.getMethod(), + httpReq.getRequestURI(), namespace, cluster, vgroup); + if (!allowed) { + handleFailure(wrapper, httpResp, chain, SecurityConstants.ErrorCode.FORBIDDEN, + "caller " + clusterId + " lacks permission for " + httpReq.getMethod() + + " " + httpReq.getRequestURI(), identity); + return; + } + + // ---- Success: hand off to the rest of the chain ---- + wrapper.setAttribute(ATTR_IDENTITY, identity); + chain.doFilter(wrapper, httpResp); + } + + private boolean isExcluded(String uri) { + List<String> excluded = properties.getExcludePaths(); + if (excluded == null || excluded.isEmpty()) { + return false; + } + for (String pattern : excluded) { + if (matches(pattern, uri)) { + return true; + } + } + return false; + } + + /** Minimal Ant-style matcher: supports trailing {@code /**} and exact match. */ + static boolean matches(String pattern, String uri) { + if (pattern == null || uri == null) { + return false; + } + if (pattern.endsWith("/**")) { + String prefix = pattern.substring(0, pattern.length() - 3); + return uri.startsWith(prefix); + } + return pattern.equals(uri); + } + + private Map<String, String> parseQuery(HttpServletRequest req) { + Map<String, String> params = new LinkedHashMap<>(); + Enumeration<String> names = req.getParameterNames(); + while (names.hasMoreElements()) { + String name = names.nextElement(); + params.put(name, req.getParameter(name)); + } + return params; + } + + private void handleFailure(HttpServletRequest req, + HttpServletResponse resp, + FilterChain chain, + SecurityConstants.ErrorCode code, + String message, + ClusterIdentity identity) throws IOException, ServletException { + SecurityProperties.Mode mode = properties.getMode(); + if (mode == SecurityProperties.Mode.WARN) { + LOGGER.warn("[security][WARN] {} {} rejected: code={} msg={} caller={}", + req.getMethod(), req.getRequestURI(), code, message, + identity == null ? "unknown" : identity.getId()); + if (identity != null) { + req.setAttribute(ATTR_IDENTITY, identity); + } + chain.doFilter(req, resp); + return; + } + // ENFORCE + int status = (code == SecurityConstants.ErrorCode.FORBIDDEN) + ? HttpServletResponse.SC_FORBIDDEN + : HttpServletResponse.SC_UNAUTHORIZED; + LOGGER.warn("[security][ENFORCE] {} {} rejected status={} code={} msg={} caller={}", + req.getMethod(), req.getRequestURI(), status, code, message, + identity == null ? "unknown" : identity.getId()); + resp.setHeader(RESP_HEADER_ERROR, code.name()); + resp.setContentType("application/json;charset=UTF-8"); + resp.setStatus(status); + resp.getWriter().write("{\"code\":\"" + code.name() + "\",\"message\":\"" + + escapeJson(message) + "\"}"); + } + + private static String escapeJson(String v) { + if (v == null) { + return ""; + } + return v.replace("\\", "\\\\").replace("\"", "\\\""); + } Review Comment: escapeJson() only escapes backslash and quotes, so control characters in the message can break the JSON response body. Because the message can include untrusted header values (e.g. cluster-id), it should also escape common control characters (\n/\r/\t and other <0x20). ########## common/src/test/java/org/apache/seata/common/security/SignatureCanonicalizerTest.java: ########## @@ -0,0 +1,121 @@ +/* + * 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.seata.common.security; + +import org.junit.jupiter.api.Test; + +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class SignatureCanonicalizerTest { + + @Test + void canonicalize_produces_stable_string_regardless_of_query_order() { + Map<String, String> orderA = new LinkedHashMap<>(); + orderA.put("beta", "2"); + orderA.put("alpha", "1"); + + Map<String, String> orderB = new LinkedHashMap<>(); + orderB.put("alpha", "1"); + orderB.put("beta", "2"); + + CanonicalRequest reqA = baseRequest().queryParams(orderA).build(); + CanonicalRequest reqB = baseRequest().queryParams(orderB).build(); + + String a = SignatureCanonicalizer.canonicalize(reqA); + String b = SignatureCanonicalizer.canonicalize(reqB); + + assertEquals(a, b, "same query set must produce identical canonical strings"); + assertTrue(a.contains("alpha=1&beta=2"), "keys must appear in ascending order in the canonical query"); + } + + @Test + void canonicalize_body_change_changes_the_string() { + CanonicalRequest a = baseRequest().body("hello".getBytes()).build(); + CanonicalRequest b = baseRequest().body("hellО".getBytes()).build(); + assertNotEquals( + SignatureCanonicalizer.canonicalize(a), + SignatureCanonicalizer.canonicalize(b), + "any byte change in body must flip the SHA-256 tail"); + } + + @Test + void canonicalize_method_case_is_normalized_upper() { + CanonicalRequest lower = baseRequest().method("post").build(); + CanonicalRequest upper = baseRequest().method("POST").build(); + assertEquals(SignatureCanonicalizer.canonicalize(upper), SignatureCanonicalizer.canonicalize(lower)); + } + + @Test + void canonicalize_empty_query_and_body_still_produces_valid_string() { + CanonicalRequest req = + baseRequest().body(new byte[0]).queryParams(new HashMap<>()).build(); + String s = SignatureCanonicalizer.canonicalize(req); + // 7 fixed segments split by \n. Body digest for empty input is a well-known constant. + assertTrue( + s.endsWith("e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"), + "empty body must hash to the well-known SHA-256 of empty input"); + } + + @Test + void canonical_query_url_encodes_special_characters() { + Map<String, String> params = new HashMap<>(); + params.put("k", "a b/c=d&e"); + String canonical = SignatureCanonicalizer.canonicalQuery(params); + // spaces become %20 (not '+'), slash '/' stays literal per URLEncoder, + // '=' inside value becomes %3D, '&' becomes %26. Review Comment: The comment says the slash '/' stays literal, but the expected assertion encodes it as %2F. This is misleading when troubleshooting canonicalization behavior. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
