cmcfarlen commented on code in PR #13708:
URL: https://github.com/apache/trafficserver/pull/13708#discussion_r4251250345
##########
tests/gold_tests/pluginTest/access_control/replays/access_control.replay.yaml:
##########
@@ -76,3 +76,52 @@ sessions:
headers:
fields:
- [ set-cookie, { value: Expires=, not: contains } ]
+
+ # 3: In-scope request with signed scoped token
+ # ```
+ #
payload='sub=user1&exp=4073587200&nbf=1514764800&iat=1514160000&tid=1234567890&scope=/scoped/in-scope/&kid=key1&st=HMAC-SHA-256&md='
+ # signature=$(echo -n ${payload} | openssl dgst -sha256 -hmac "PEIFtmunx9" |
sed -e 's/^.* //')
+ # token="${payload}${signature}"
+ # cookie=$(echo -n "${token}" | base64 -w0 | tr '+/' '-_' | tr -d '=')
+ # ```
+ - client-request:
+ method: "GET"
+ version: "1.1"
+ url: /scoped/in-scope/resource
+ headers:
+ fields:
+ - [ uuid, 3 ]
+ - [ Host, example.com ]
+ - [ Cookie,
"TokenCookie=c3ViPXVzZXIxJmV4cD00MDczNTg3MjAwJm5iZj0xNTE0NzY0ODAwJmlhdD0xNTE0MTYwMDAwJnRpZD0xMjM0NTY3ODkwJnNjb3BlPS9zY29wZWQvaW4tc2NvcGUvJmtpZD1rZXkxJnN0PUhNQUMtU0hBLTI1NiZtZD0xOWViZDdkZGVjNzM3N2FjMDNkY2FmYmMwMDRhN2Q0ZDZkZjdjMTEwZDE0ZTQxMWQ5OGNjYWNkZGVlYTAyMjQ3"
]
+
+ proxy-request:
+ headers:
+ fields:
+ - [ @TokenSubject, { value: user1, as: equal } ]
Review Comment:
Two problems with this check:
- `@` can't start a plain YAML scalar, so `[ @TokenSubject, ... ]` won't
load. It needs quoting: `"@TokenSubject"`. This may be the AuTest 0of4 failure.
- Even quoted, it can't pass. ATS doesn't send `@`-prefixed headers to the
origin (`MIME.cc`, "Don't print names that begin with an '@'"), so the verifier
server never sees `@TokenSubject`. The `/scoped/` remap needs a plain header
name for `--extract-subject-to-header` for this to be observable on the server
side.
##########
tests/gold_tests/pluginTest/access_control/replays/access_control.replay.yaml:
##########
@@ -76,3 +76,52 @@ sessions:
headers:
fields:
- [ set-cookie, { value: Expires=, not: contains } ]
+
+ # 3: In-scope request with signed scoped token
+ # ```
+ #
payload='sub=user1&exp=4073587200&nbf=1514764800&iat=1514160000&tid=1234567890&scope=/scoped/in-scope/&kid=key1&st=HMAC-SHA-256&md='
+ # signature=$(echo -n ${payload} | openssl dgst -sha256 -hmac "PEIFtmunx9" |
sed -e 's/^.* //')
+ # token="${payload}${signature}"
+ # cookie=$(echo -n "${token}" | base64 -w0 | tr '+/' '-_' | tr -d '=')
+ # ```
+ - client-request:
+ method: "GET"
+ version: "1.1"
+ url: /scoped/in-scope/resource
+ headers:
+ fields:
+ - [ uuid, 3 ]
+ - [ Host, example.com ]
+ - [ Cookie,
"TokenCookie=c3ViPXVzZXIxJmV4cD00MDczNTg3MjAwJm5iZj0xNTE0NzY0ODAwJmlhdD0xNTE0MTYwMDAwJnRpZD0xMjM0NTY3ODkwJnNjb3BlPS9zY29wZWQvaW4tc2NvcGUvJmtpZD1rZXkxJnN0PUhNQUMtU0hBLTI1NiZtZD0xOWViZDdkZGVjNzM3N2FjMDNkY2FmYmMwMDRhN2Q0ZDZkZjdjMTEwZDE0ZTQxMWQ5OGNjYWNkZGVlYTAyMjQ3"
]
+
+ proxy-request:
+ headers:
+ fields:
+ - [ @TokenSubject, { value: user1, as: equal } ]
+
+ server-response:
+ status: 200
+ headers:
+ fields:
+ - [ Content-Length, 10 ]
+
+ proxy-response:
+ status: 200
+
+ # 4: Out-of-scope request with signed scoped token
+ - client-request:
+ method: "GET"
+ version: "1.1"
+ url: /scoped/out-of-scope/resource
+ headers:
+ fields:
+ - [ uuid, 4 ]
+ - [ Host, example.com ]
+ - [ Cookie,
"TokenCookie=c3ViPXVzZXIxJmV4cD00MDczNTg3MjAwJm5iZj0xNTE0NzY0ODAwJmlhdD0xNTE0MTYwMDAwJnRpZD0xMjM0NTY3ODkwJnNjb3BlPS9zY29wZWQvaW4tc2NvcGUvJmtpZD1rZXkxJnN0PUhNQUMtU0hBLTI1NiZtZD0xOWViZDdkZGVjNzM3N2FjMDNkY2FmYmMwMDRhN2Q0ZDZkZjdjMTEwZDE0ZTQxMWQ5OGNjYWNkZGVlYTAyMjQ3"
]
+
+ proxy-response:
+ status: 403
+ headers:
+ fields:
+ - [ @TokenSubject, { as: absent } ]
Review Comment:
`@TokenSubject` is only ever set on the request to the origin, never on the
response to the client, so this `absent` check passes whatever the plugin does.
To cover header suppression for out-of-scope requests, check the proxy request
with a non-`@` header name, as above.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -23,12 +23,123 @@
#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, bool isScope = false)
+{
+ normalized.clear();
+ if (path.empty()) {
+ normalized = "/";
+ return true;
+ }
+
+ normalized.reserve(path.size() + 1);
+ std::vector<size_t> segment_starts;
+ 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) {
+ if (val == '/' || val == '\\' || val < 0x20 || val == 0x7F) {
+ return false;
+ }
+ normalized.push_back(static_cast<char>(val));
+ i += 3;
+ } else {
+ return false;
+ }
+ } else {
+ normalized.push_back(path[i]);
+ ++i;
+ }
+ }
+
+ StringView seg(normalized.data() + seg_start + 1, normalized.size() -
seg_start - 1);
+
+ if (seg == ".") {
+ normalized.resize(seg_start);
+ } else if (seg == "..") {
Review Comment:
The scope check runs on a decoded, `.`/`..`-resolved copy of the path, but
ATS forwards the raw path unchanged. So the result depends on how the origin
interprets that raw path:
- `/reports/..;/hr/payroll`: `..;` isn't `..` here, so it passes a
`/reports` scope. Tomcat/Spring treat `..;` as `..` and serve `/hr/payroll`.
- `/reports/%252e%252e/hr`: one decode gives a literal `%2e%2e` segment,
which passes. An origin that decodes again walks out of the scope.
- Going the other way, `/reports/../hr` is accepted for an `/hr` scope, but
the origin receives `/reports/../hr`.
For an authorization check I'd reject request paths with dot segments (raw
or encoded) rather than resolving them. Then the path that's checked is the
path that's forwarded. Resolving is only safe if the forwarded URL is
canonicalized the same way.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -23,12 +23,123 @@
#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, bool isScope = false)
+{
+ normalized.clear();
+ if (path.empty()) {
+ normalized = "/";
+ return true;
+ }
+
+ normalized.reserve(path.size() + 1);
+ std::vector<size_t> segment_starts;
+ 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) {
+ if (val == '/' || val == '\\' || val < 0x20 || val == 0x7F) {
+ return false;
+ }
+ normalized.push_back(static_cast<char>(val));
+ i += 3;
+ } else {
+ return false;
+ }
+ } else {
+ normalized.push_back(path[i]);
+ ++i;
+ }
+ }
+
+ StringView seg(normalized.data() + seg_start + 1, normalized.size() -
seg_start - 1);
+
+ if (seg == ".") {
+ normalized.resize(seg_start);
+ } else if (seg == "..") {
+ if (isScope) {
+ return false;
+ }
+
+ normalized.resize(seg_start);
+
+ if (!segment_starts.empty()) {
+ normalized.resize(segment_starts.back());
+ segment_starts.pop_back();
+ }
+ } else {
+ segment_starts.push_back(seg_start);
+ }
+ }
+ if (normalized.empty()) {
+ normalized = "/";
+ }
+ return true;
+}
+
+ScopeValidationResult
+validateScopeDetailed(StringView requestPath, StringView scope)
+{
+ if (scope.empty()) {
+ return ScopeValidationResult::IN_SCOPE;
+ }
+
+ String normScope;
+ if (!normalizePath(scope, normScope, /* isScope = */ true)) {
+ return ScopeValidationResult::INVALID_SCOPE;
+ }
+
+ String normRequestPath;
+ if (!normalizePath(requestPath, normRequestPath, /* isScope = */ false)) {
Review Comment:
The request path is normalized, and can be rejected, before the `normScope
== "/"` shortcut below. So a token scoped to `/`, which the docs say is
unrestricted, still fails on `/files/a%2Fb` or `/discount/100%`. Checking for a
`/` scope before normalizing the request path fixes it.
##########
plugins/experimental/access_control/access_control.cc:
##########
@@ -23,12 +23,123 @@
#include <iostream>
#include <string>
+#include <charconv>
Review Comment:
Nit: `std::vector` is used in `normalizePath` but `<vector>` isn't included.
It only builds because another header pulls it in.
##########
plugins/experimental/access_control/plugin.cc:
##########
@@ -529,12 +529,28 @@ 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());
+ 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) {
+ if (scopeResult == ScopeValidationResult::INVALID_SCOPE) {
+ data->_vaState = INVALID_SCOPE;
+ } else if (scopeResult ==
ScopeValidationResult::INVALID_REQUEST_PATH) {
+ data->_vaState = INVALID_SYNTAX;
Review Comment:
`INVALID_REQUEST_PATH` maps to `INVALID_SYNTAX` here. A valid, signed token
on a request with an unacceptable path therefore gets
`--invalid-syntax-status-code` and an invalid-syntax status header, as if the
token were malformed. The docs say out-of-scope requests use
`--invalid-scope-status-code`. `OUT_OF_SCOPE` seems the better fit, or a
distinct state if you want to tell the two apart.
##########
doc/admin-guide/plugins/access_control.en.rst:
##########
@@ -237,7 +237,14 @@ Query-Param-Style Named Claim format
* ``iat`` for `issued at time`_, `optional`
* ``tid`` for `token id`_, `optional`
* ``ver`` for `version`_, `optional`, defaults to ``ver=1`` if not
specified.
- * ``scope`` for `scope`_, `optional`, ignored by the current version of the
plugin, still not finalized (more applications and their use cases need to be
studied to finalize the format)
+ * ``scope`` for `scope`_, `optional`
Review Comment:
Upgrade concern: master documents `scope` as "ignored by the current version
of the plugin, still not finalized". A deployment issuing tokens with a
non-path scope such as `scope=read` was unrestricted before. With this change
`read` becomes the path prefix `/read`, and with
`--reject-invalid-token-requests` every request gets a 403. Even for an
experimental plugin, this needs an opt-in flag (e.g. `--enforce-scope`) or at
least an entry in the upgrading notes.
--
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]
