Copilot commented on code in PR #13708:
URL: https://github.com/apache/trafficserver/pull/13708#discussion_r4266135911
##########
tests/gold_tests/pluginTest/access_control/access_control.test.py:
##########
@@ -59,16 +59,27 @@ def setupTS(self):
self.ts.Setup.Copy("etc/hmac_keys.txt")
self.ts.Disk.remap_config.AddLines(
- {
+ [
+ f'''map /scoped/
https://127.0.0.1:{self.server.Variables.https_port}/scoped/ \
+ @plugin=access_control.so \
+ @pparam=--symmetric-keys-map={Test.RunDirectory}/hmac_keys.txt \
+ @pparam=--check-cookie=TokenCookie \
+ @pparam=--reject-invalid-token-requests=true \
+ @pparam=--enforce-scope=true \
+ @pparam=--invalid-scope-status-code=403 \
+ @pparam=--extract-subject-to-header=TokenSubject \
+ @pparam=--extract-tokenid-to-header=@TokenId \
+ @pparam=--extract-status-to-header=@TokenStatus \
+ @pparam=--token-response-header=TokenRespHdr''',
f'''map /
https://127.0.0.1:{self.server.Variables.https_port}/ \
@plugin=access_control.so \
@pparam=--symmetric-keys-map={Test.RunDirectory}/hmac_keys.txt \
@pparam=--check-cookie=TokenCookie \
- @pparam=--extract-subject-to-header=@TokenSubject \
+ @pparam=--extract-subject-to-header=TokenSubject \
@pparam=--extract-tokenid-to-header=@TokenId \
@pparam=--extract-status-to-header=@TokenStatus \
- @pparam=--token-response-header=TokenRespHdr'''
- })
+ @pparam=--token-response-header=TokenRespHdr''',
+ ])
Review Comment:
Gold session #5 in `access_control.replay.yaml` expects an out-of-scope
request to *suppress* `TokenSubject` on the `/` remap, but this remap does not
enable `--enforce-scope` (default is false). As written, scope will be ignored
on `/`, and the subject header will still be extracted for valid tokens. To
make the test and configuration consistent, enable `--enforce-scope=true` on
the `/` remap as well (while keeping `--reject-invalid-token-requests`
unset/false to preserve the non-rejecting behavior).
##########
plugins/experimental/access_control/plugin.cc:
##########
@@ -529,12 +529,31 @@ enforceAccessControl(TSHttpTxn txnp, TSRemapRequestInfo
*rri, AccessControlConfi
remapStatus =
handleInvalidToken(txnp, data, reject,
accessTokenStateToHttpStatus(data->_vaState, config), data->_vaState);
} else {
- /* Valid token, if configured extract the token subject to a header,
- * only if we can trust it - token is valid to prevent using it by
mistake */
- if (!config->_extrSubHdrName.empty()) {
- String sub(token->getSubject());
- setHeader(rri->requestBufp, rri->requestHdrp,
config->_extrSubHdrName.c_str(), config->_extrSubHdrName.size(),
- sub.c_str(), sub.size());
+ bool inScope = true;
+ if (config->_enforceScope) {
+ int pathLen = 0;
+ const char *path = TSUrlPathGet(rri->requestBufp,
rri->requestUrl, &pathLen);
+ StringView reqPath(path ? path : "", pathLen);
+ ScopeValidationResult scopeResult = validateScopeDetailed(reqPath,
token->getScope());
+ if (scopeResult != ScopeValidationResult::IN_SCOPE) {
+ inScope = false;
+ if (scopeResult == ScopeValidationResult::INVALID_SCOPE) {
+ data->_vaState = INVALID_SCOPE;
+ } else {
+ data->_vaState = OUT_OF_SCOPE;
+ }
+ remapStatus =
+ handleInvalidToken(txnp, data, reject,
accessTokenStateToHttpStatus(data->_vaState, config), data->_vaState);
+ }
+ }
Review Comment:
`validateScopeDetailed()` distinguishes `INVALID_REQUEST_PATH` (e.g.,
traversal or invalid escapes) from `OUT_OF_SCOPE`, but this code maps
everything except `INVALID_SCOPE` to `OUT_OF_SCOPE`. That collapses potentially
malicious/invalid request paths into the same bucket as simple authorization
failures and forces the `--invalid-scope-status-code` path. Consider mapping
`INVALID_REQUEST_PATH` to an existing `AccessTokenStatus` that results in a
400-class response (e.g., `INVALID_SYNTAX` / `_invalidRequest`), or introduce a
dedicated `AccessTokenStatus` for invalid request paths so operators can
distinguish bad input from authorization failures.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -23,12 +23,104 @@
#include <iostream>
#include <string>
+#include <charconv>
#include "access_control.h"
size_t calcMessageDigest(const StringView hf, const char *secret, const char
*message, size_t messageLen, char *buffer, size_t len);
const char *getSecretMap(const StringMap &map, const StringView &key, size_t
&secretSize);
+static bool
+normalizePath(StringView path, String &normalized)
+{
+ normalized.clear();
+ if (path.empty()) {
+ normalized = "/";
+ return true;
+ }
+
+ normalized.reserve(path.size() + 1);
+ size_t i = 0;
+
+ while (i < path.size()) {
+ while (i < path.size() && path[i] == '/') {
+ ++i;
+ }
+ if (i >= path.size()) {
+ break;
+ }
+
+ size_t seg_start = normalized.size();
+ normalized.push_back('/');
+
+ while (i < path.size() && path[i] != '/') {
+ if (path[i] == '\\') {
+ return false;
+ }
+ if (path[i] == '%') {
+ unsigned int val = 0;
+ if (i + 2 >= path.size()) {
+ return false;
+ }
+ auto [end, ec] = std::from_chars(path.data() + i + 1, path.data() + i
+ 3, val, 16);
+ if (end == path.data() + i + 3 && ec == std::errc{} && val <= 0xFF) {
Review Comment:
`std::errc` is specified in `<system_error>`. Relying on it being
transitively included via `<charconv>` is not guaranteed across standard
library implementations. Please add an explicit `#include <system_error>` (or
adjust the code to avoid referencing `std::errc` directly) to improve
portability.
##########
plugins/experimental/access_control/plugin.cc:
##########
@@ -529,12 +529,31 @@ enforceAccessControl(TSHttpTxn txnp, TSRemapRequestInfo
*rri, AccessControlConfi
remapStatus =
handleInvalidToken(txnp, data, reject,
accessTokenStateToHttpStatus(data->_vaState, config), data->_vaState);
} else {
- /* Valid token, if configured extract the token subject to a header,
- * only if we can trust it - token is valid to prevent using it by
mistake */
- if (!config->_extrSubHdrName.empty()) {
- String sub(token->getSubject());
- setHeader(rri->requestBufp, rri->requestHdrp,
config->_extrSubHdrName.c_str(), config->_extrSubHdrName.size(),
- sub.c_str(), sub.size());
+ bool inScope = true;
+ if (config->_enforceScope) {
+ int pathLen = 0;
+ const char *path = TSUrlPathGet(rri->requestBufp,
rri->requestUrl, &pathLen);
+ StringView reqPath(path ? path : "", pathLen);
+ ScopeValidationResult scopeResult = validateScopeDetailed(reqPath,
token->getScope());
Review Comment:
`validateScopeDetailed()` currently normalizes (and percent-decodes) both
the request path and the token scope, which means the scope normalization work
is repeated on every transaction even though the token scope is constant. If
scope enforcement is expected to be commonly enabled, consider
normalizing/caching the scope once at token parse time (e.g., storing a
normalized scope string in the token instance) and only normalizing the request
path per-request. This reduces per-request allocations and CPU for long-lived
tokens.
--
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]
