diff --git a/src/mgmt/rpc/handlers/common/RecordsUtils.cc b/src/mgmt/rpc/handlers/common/RecordsUtils.cc index 0d2291c0b4a..fb8811f861c 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 b94e66dc07d..5be131b8d48 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 04c8716574c..9f901bbb62e 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 resp; - // we need the type and the update type for now. - using LookupContext = std::tuple; + // 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; 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(data); + auto &[dataType, checkType, pattern, updateType, accessType, is_config] = *static_cast(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 00000000000..b03a548d05a --- /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)