This is an automated email from the ASF dual-hosted git repository.
brbzull0 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 4b2ee8608b mgmt/rpc: refuse RECA_READ_ONLY/RECA_NO_ACCESS writes
(#13142)
4b2ee8608b is described below
commit 4b2ee8608ba54105bde19aa660e77d70c9ab83a9
Author: Damian Meden <[email protected]>
AuthorDate: Wed May 13 10:18:28 2026 +0200
mgmt/rpc: refuse RECA_READ_ONLY/RECA_NO_ACCESS writes (#13142)
* mgmt/rpc: refuse RECA_READ_ONLY/RECA_NO_ACCESS writes
set_config_records did not consult the registrant's access tier,
so admin_config_set_records (and "traffic_ctl config set") accepted
writes to records the registrant marked as protected. Read the
record's access_type alongside the existing metadata, and refuse
the write with an specific error code (RECORD_READ_ONLY /
RECORD_NO_ACCESS)
---
src/mgmt/rpc/handlers/common/RecordsUtils.cc | 4 +
src/mgmt/rpc/handlers/common/RecordsUtils.h | 4 +-
src/mgmt/rpc/handlers/config/Configuration.cc | 41 +++++++-
.../traffic_ctl/traffic_ctl_set_read_only.test.py | 113 +++++++++++++++++++++
4 files changed, 156 insertions(+), 6 deletions(-)
diff --git a/src/mgmt/rpc/handlers/common/RecordsUtils.cc
b/src/mgmt/rpc/handlers/common/RecordsUtils.cc
index 0d2291c0b4..fb8811f861 100644
--- a/src/mgmt/rpc/handlers/common/RecordsUtils.cc
+++ b/src/mgmt/rpc/handlers/common/RecordsUtils.cc
@@ -63,6 +63,10 @@ RPCRecordErrorCategory::message(int ev) const
return {"Found record does not match the requested type"};
case rpc::handlers::errors::RecordError::INVALID_INCOMING_DATA:
return {"Invalid request data provided"};
+ case rpc::handlers::errors::RecordError::RECORD_READ_ONLY:
+ return {"Record is read-only and cannot be modified through the management
interface."};
+ case rpc::handlers::errors::RecordError::RECORD_NO_ACCESS:
+ return {"Record is not accessible through the management interface."};
default:
return "Record error error " + std::to_string(ev);
}
diff --git a/src/mgmt/rpc/handlers/common/RecordsUtils.h
b/src/mgmt/rpc/handlers/common/RecordsUtils.h
index b94e66dc07..5be131b8d4 100644
--- a/src/mgmt/rpc/handlers/common/RecordsUtils.h
+++ b/src/mgmt/rpc/handlers/common/RecordsUtils.h
@@ -42,7 +42,9 @@ enum class RecordError {
GENERAL_ERROR,
RECORD_WRITE_ERROR,
REQUESTED_TYPE_MISMATCH,
- INVALID_INCOMING_DATA
+ INVALID_INCOMING_DATA,
+ RECORD_READ_ONLY,
+ RECORD_NO_ACCESS
};
std::error_code make_error_code(rpc::handlers::errors::RecordError e);
} // namespace rpc::handlers::errors
diff --git a/src/mgmt/rpc/handlers/config/Configuration.cc
b/src/mgmt/rpc/handlers/config/Configuration.cc
index 04c8716574..9f901bbb62 100644
--- a/src/mgmt/rpc/handlers/config/Configuration.cc
+++ b/src/mgmt/rpc/handlers/config/Configuration.cc
@@ -119,8 +119,12 @@ set_config_records(std::string_view const & /* id
ATS_UNUSED */, YAML::Node cons
{
swoc::Rv<YAML::Node> resp;
- // we need the type and the update type for now.
- using LookupContext = std::tuple<RecDataT, RecCheckT, const char *,
RecUpdateT>;
+ // we need the type and the update type for now, plus the access tier so we
+ // can refuse writes to records the registrant marked as protected, and a
+ // flag tracking whether the lookup actually returned a CONFIG record (the
+ // RPC is for config writes; metric/process/plugin records must be
+ // refused even though RecLookupRecord finds them).
+ using LookupContext = std::tuple<RecDataT, RecCheckT, const char *,
RecUpdateT, RecAccessT, bool>;
for (auto const &kv : params) {
SetRecordCmdInfo info;
@@ -131,20 +135,25 @@ set_config_records(std::string_view const & /* id
ATS_UNUSED */, YAML::Node cons
continue;
}
- LookupContext recordCtx;
+ // Value-initialize the tuple so reads after the lookup are well defined
+ // even when the callback never assigns a field (non-CONFIG record, or
+ // an early failure inside the callback).
+ LookupContext recordCtx{};
// Get record info first. TODO: we may just want to get the full record
and then send it back as a response.
const auto ret = RecLookupRecord(
info.name.c_str(),
[](const RecRecord *record, void *data) {
- auto &[dataType, checkType, pattern, updateType] =
*static_cast<LookupContext *>(data);
+ auto &[dataType, checkType, pattern, updateType, accessType,
is_config] = *static_cast<LookupContext *>(data);
if (REC_TYPE_IS_CONFIG(record->rec_type)) {
+ is_config = true;
dataType = record->data_type;
checkType = record->config_meta.check_type;
if (record->config_meta.check_expr) {
pattern = record->config_meta.check_expr;
}
updateType = record->config_meta.update_type;
+ accessType = record->config_meta.access_type;
}
},
&recordCtx);
@@ -156,7 +165,29 @@ set_config_records(std::string_view const & /* id
ATS_UNUSED */, YAML::Node cons
}
// now set the value.
- auto const &[dataType, checkType, pattern, updateType] = recordCtx;
+ auto const &[dataType, checkType, pattern, updateType, accessType,
is_config] = recordCtx;
+
+ // RecLookupRecord finds metrics, config records, etc.
+ // The set RPC only operates on config records;
+ // refuse anything else with a tier-specific error code so callers can
+ // distinguish "wrong record kind" from "record missing".
+ if (!is_config) {
+ auto const ec = std::error_code{err::RecordError::RECORD_NOT_CONFIG};
+ resp.errata().assign(ec).note("{}", ec);
+ continue;
+ }
+
+ // Honour the registrant's access tier. RECA_READ_ONLY records may only
+ // be changed by in-process code (config file load, environment override,
+ // etc.); RECA_NO_ACCESS records are not exposed to the management plane
+ // at all. Surface the per-tier error code in the JSONRPC error.data
+ // entry so callers can branch on the code instead of parsing messages.
+ if (accessType == RECA_READ_ONLY || accessType == RECA_NO_ACCESS) {
+ auto const ec =
+ std::error_code{accessType == RECA_READ_ONLY ?
err::RecordError::RECORD_READ_ONLY : err::RecordError::RECORD_NO_ACCESS};
+ resp.errata().assign(ec).note("{}", ec);
+ continue;
+ }
// run the check only if we have something to check against it.
if (pattern != nullptr && RecordValidityCheck(info.value.c_str(),
checkType, pattern) == false) {
diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_set_read_only.test.py
b/tests/gold_tests/traffic_ctl/traffic_ctl_set_read_only.test.py
new file mode 100644
index 0000000000..b03a548d05
--- /dev/null
+++ b/tests/gold_tests/traffic_ctl/traffic_ctl_set_read_only.test.py
@@ -0,0 +1,113 @@
+'''
+Verify that the management RPC refuses to write records registered as
+RECA_READ_ONLY.
+'''
+# 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.
+
+from jsonrpc import Request, Response
+
+Test.Summary = '''
+Verify that records registered with RECA_READ_ONLY cannot be modified through
+the management RPC backing "traffic_ctl config set"
+(admin_config_set_records).
+'''
+
+Test.ContinueOnFail = True
+
+# proxy.config.thread.max_heartbeat_mseconds is registered as RECA_READ_ONLY
+# in src/records/RecordsConfig.cc with a default of 60. RECA_READ_ONLY = 2
+# in the RecAccessT enum (see include/records/RecDefs.h).
+READ_ONLY_RECORD = "proxy.config.thread.max_heartbeat_mseconds"
+DEFAULT_VALUE = "60"
+ATTEMPTED_VALUE = "999"
+RECA_READ_ONLY = "2"
+RECT_CONFIG_BIT = "1" # bit value in the rec_types filter
+
+# RecordError::RECORD_READ_ONLY in src/mgmt/rpc/handlers/common/RecordsUtils.h
+# (assigned as Codes::RECORD + offset). This is the per-tier code that the
+# JSONRPC response must surface in error.data[*].code so programmatic
+# clients can branch on the access tier without parsing the message text.
+RECORD_READ_ONLY_CODE = 2009
+
+ts = Test.MakeATSProcess("ts")
+
+
+def lookup_request():
+ """Build a JSONRPC request that fetches the read-only record."""
+ return Request.admin_lookup_records([{"record_name": READ_ONLY_RECORD,
"rec_types": [RECT_CONFIG_BIT]}])
+
+
+def assert_record_at_default(resp: Response):
+ """Validate the looked-up record is RECA_READ_ONLY and at its default
value."""
+ if resp.is_error():
+ return (False, f"unexpected error: {resp.error_as_str()}")
+
+ records = resp.result.get('recordList', [])
+ if len(records) != 1:
+ return (False, f"expected exactly 1 record, got {len(records)}")
+
+ rec = records[0]['record']
+ if rec.get('record_name') != READ_ONLY_RECORD:
+ return (False, f"record_name {rec.get('record_name')!r} !=
{READ_ONLY_RECORD!r}")
+ if rec.get('current_value') != DEFAULT_VALUE:
+ return (False, f"current_value {rec.get('current_value')!r} !=
{DEFAULT_VALUE!r}")
+
+ access = rec.get('config_meta', {}).get('access_type')
+ if str(access) != RECA_READ_ONLY:
+ return (False, f"access_type {access!r} != {RECA_READ_ONLY!r} (record
is not RECA_READ_ONLY)")
+
+ return (True, "record is RECA_READ_ONLY and at the registered default")
+
+
+def assert_set_was_rejected(resp: Response):
+ """Validate the set attempt produced the per-tier not-writable error
code."""
+ if not resp.is_error():
+ return (False, f"set should have failed but returned a result:
{resp.result!r}")
+ # Validate the structured error code rather than the message text so the
+ # test stays meaningful if the error wording is ever rephrased and so
+ # that any regression to the generic Codes::RECORD (2000) is caught.
+ if not resp.contains_nested_error(code=RECORD_READ_ONLY_CODE):
+ return (False, f"expected nested error code {RECORD_READ_ONLY_CODE}
(RECORD_READ_ONLY); got: {resp.error_as_str()}")
+ return (True, f"set was refused with code {RECORD_READ_ONLY_CODE}
(RECORD_READ_ONLY)")
+
+
+# Step 0: confirm the record is registered as RECA_READ_ONLY and starts at
+# its registered default value. This anchors the rest of the test against
+# the registration in RecordsConfig.cc -- if someone reclassifies the record
+# the test fails noisily here instead of silently exercising a permissive
+# write path.
+tr = Test.AddTestRun("Confirm record is RECA_READ_ONLY and at default")
+tr.Processes.Default.StartBefore(ts)
+tr.AddJsonRPCClientRequest(ts, lookup_request())
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(assert_record_at_default)
+
+# Step 1: attempt to write the record via the management RPC. The handler
+# must reject the request with the "Record is not writable" error.
+tr = Test.AddTestRun("Attempt to set the RECA_READ_ONLY record (must be
refused)")
+tr.AddJsonRPCClientRequest(
+ ts, Request.admin_config_set_records([{
+ "record_name": READ_ONLY_RECORD,
+ "record_value": ATTEMPTED_VALUE,
+ }]))
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(assert_set_was_rejected)
+
+# Step 2: re-look-up the record. Even if step 1's response had been
+# misleading, the record must still hold its default value -- this is the
+# assertion that catches the underlying bug at the storage level.
+tr = Test.AddTestRun("Confirm record was not modified")
+tr.AddJsonRPCClientRequest(ts, lookup_request())
+tr.Processes.Default.Streams.stdout =
Testers.CustomJSONRPCResponse(assert_record_at_default)
