mgmt/rpc: refuse RECA_READ_ONLY/RECA_NO_ACCESS writes

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);
   }

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

src/mgmt/rpc/handlers/config/Configuration.cc

@@ -119,8 +119,9 @@ 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.
+  using LookupContext = std::tuple<RecDataT, RecCheckT, const char *, RecUpdateT, RecAccessT>;
 
   for (auto const &kv : params) {
     SetRecordCmdInfo info;
@@ -132,19 +133,21 @@ set_config_records(std::string_view const & /* id ATS_UNUSED */, YAML::Node cons
     }
 
     LookupContext recordCtx;
+    std::get<RecAccessT>(recordCtx) = RECA_NULL;
 
     // 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] = *static_cast<LookupContext *>(data);
         if (REC_TYPE_IS_CONFIG(record->rec_type)) {
           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 +159,19 @@ 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] = recordCtx;
+
+    // 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.  The management RPC must refuse both, with distinct error
+    // codes 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(std::error_code{errors::Codes::RECORD}).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) {

tests/gold_tests/traffic_ctl/traffic_ctl_set_read_only.test.py

@@ -0,0 +1,105 @@
+'''
+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
+
+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 not-writable error."""
+    if not resp.is_error():
+        return (False, f"set should have failed but returned a result: {resp.result!r}")
+    err = resp.error_as_str()
+    if "Record is read-only" not in err:
+        return (False, f"unexpected error message: {err!r}")
+    return (True, "set was refused with 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)