spec: expose tedge configs to external clients - #4241
Conversation
| We choose **opt-in**, for *safe by default*. | ||
| With a denylist, a newly added setting is exposed the moment it is introduced, | ||
| and a new secret leaks unless someone remembers to deny it — a silent failure. | ||
| With an allowlist, a new setting is invisible until a maintainer marks it exposable, | ||
| so the worst case is a missing value (loud, easy to fix), not a leaked secret. | ||
| Marking exposability where each setting is defined also keeps the decision | ||
| reviewable alongside the value. |
There was a problem hiding this comment.
I'm pretty sure we use a special type for secrets, e.g. "Redacted" (or something like that) which could be used to disable automatic exposure via this interface. That would make a more convenient/automatic mapping of new values rather than having to explicitly opting in to each new config key
There was a problem hiding this comment.
The answer is also dependent on how many config keys we are exposing; whether we go for a "expose everything but secrets" or not.
There was a problem hiding this comment.
I was thinking that we'll only expose a very limited number of configs, that we feel would be relevant to the user. For example, I don't see any reason to expose configs like device.key_path, device.cert_path, or even entire config trees like agent.*, apt.* etc that an external client would have no business with.
There was a problem hiding this comment.
I'm pretty sure we use a special type for secrets, e.g. "Redacted" (or something like that) which could be used to disable automatic exposure via this interface.
I'm fairly sure we don't have this at the moment as we store a reference to a secret (e.g. key path or password file) rather than the secret itself, so there isn't any sensitive information to redact. Though since those are just file paths, they aren't relevant for a public API as they're meaningless once they're not on the main device.
I guess with the hsm configurations we do have secrets that we store (key_pin, for example), but we don't actually have any mechanism in place to redact this info as far as I'm aware.
There was a problem hiding this comment.
I guess with the hsm configurations we do have secrets that we store (key_pin, for example), but we don't actually have any mechanism in place to redact this info as far as I'm aware.
AFAIK secrets like device.key_pin are indeed loaded into config as regular string types, but are converted into a SecretString wrapper on the boundary with the HSM module, so they're not accidentally logger after that point.
The optimal solution would be to not have any secrets in the config (indeed putting HSM PIN there was a mistake I regret making), but since the milk is already spilt, and we can potentially have secret data in the config, it seems to me the most complete way to proceed would be to:
- add a mechanism to load the PIN from an external source (e.g. file so it doesn't appear in config directly)
- encourage users to use this new method by logging a warning when usage of the less secure secret-embedded-in-config setting is detected
- provide a way to disable the warning for users who don't want to change anything (probably by adding another setting)
- somehow cope with some users using the older setting which embeds secret in the config - either by introducing the notion of a secret field to
tedge-configand having to handle it differently, or by not doing that and assuming that if the user silenced the warning, they must know what they're doing or don't care (fwiw I don't think adding this secret wrapper to the config would be that much work so we should still try it and only abandon the idea if we encounter unknown problems).
But indeed the proposal only covers step 4 and we don't have to do 1-3 for the proposed feature to work, but I just wanted to mention for completeness. I remember us talking about the secret management more generally, since there were also some other secret values that use another mechanism entirely, but if I remember correctly we haven't agreed on any general solution to that yet?
| The topic adds a `config` channel with the configuration key as its final segment: | ||
|
|
||
| ``` | ||
| te/device/<device>/service/<service>/config/<key> |
There was a problem hiding this comment.
Do we know how many settings would be potentially published here? Or since we're using opt-in, then we only expose "useful" config keys, so the number would be under 10?
There was a problem hiding this comment.
Ok, let's go with the current proposal, however after the implementation if the /config/<key> topics are too spammy, then we might just go with a single /config topic which has the entire list of keys and avoid the individual per key topics. Having the HTTP endpoints are fine, but the MQTT topics per key will most likely just pollute the retained messages
There was a problem hiding this comment.
I'm starting to rethink this decision to publish individual config values to separate topics. While it provides the convenience of getting a simple config value even to pure MQTT clients, without having to parse the entire config as a JSON document, it came with some complexity as well as follows:
- If an exposed value that was set before was unset and the service is restarted, the old value will still remain with the broker. To handle this case, we have introduced a reconciliation logic where explicitly send clear messages for all "unset but exposed" values as well on every startup. While this works, if there was a single config topic with the entire configuration, we could simply replace that with the new one.
- Since there is an arbitrary duration for all retained config messages of a service to be delivered to the agent, the agent effectively exposes partial config of that entity from the moment the first retained messages is delivered until the last. If there was a single
configmessage, then it's always all or nothing.
So, which one matters? The convenience of single config key subscribes or the simplicity of all configs at a single topic?
There was a problem hiding this comment.
Yeah I think publishing individual config keys to individual topic is not required...though maybe we only need to implement the http endpoint (which can do both all the config or individual keys), and just have a flow that calls the api and then publishes a single retained message with all of the keys in one message.
If users want to publish to multiple topics, then they can also do this by just modifying the flow, or creating their own.
There was a problem hiding this comment.
We get the MQTT API for free, as the mappers/components are using the MQTT interface to "share their configs" with the tegde-agent. So, the choice was between a single te/<entity-topic-id>/config topic vs multiple per-key: te/<entity-topic-id>/config/<config.key> topics. So, we go with the single topic solution.
There was a problem hiding this comment.
Switched to a single config document per service in 419d007
Bravo555
left a comment
There was a problem hiding this comment.
Initially I've been bummed out that because of secrets like key_pin we can't just dump the entire config as a payload to a single topic and not have to think about it again, but I see that out of 156 config options we only expose 43 of them, so I'm glad that it doesn't seem to be inhibiting the feature much. Indeed not exposing options which are considered to be, if not outright secrets, then at least considered somewhat private, is maybe better than exposing everything.
rina23q
left a comment
There was a problem hiding this comment.
I think we can move to implementation, so I would approve.
| ### Allowlist marking is a per-field macro attribute, not a group-level or file-level flag | ||
|
|
||
| `#[tedge_config(exposable)]` goes directly on each setting in `define_tedge_config!`, | ||
| generating a `ReadableKey::is_exposable()` accessor — the same pattern already used for |
There was a problem hiding this comment.
This has the potential to cause some slight conflicts with the config migration, but it should be fine as I'm anticipating the facet migration will take longer to complete than this will, and the old API will still be in use until the new one is complete.
This has also made me go and check how we use facet at the moment for these sorts of things, and I've realised we can lean even more heavily on facet than the prototype implementation did. Edit: now implemented
| The publishing itself (topic construction, retain flag, the actor) is one small shared actor used by | ||
| the agent and all three mappers, instead of three near-duplicate implementations. | ||
|
|
||
| Caveat: some values are overridden at launch by a CLI flag or env var not written back to |
There was a problem hiding this comment.
This should only be true for CLI flags, not for environment variables (which are taken into account when reading the config). I wonder whether it's worth adding in API to populate the readable tedge configuration with CLI overrides (to the facet-powered config refactoring).
| at all. | ||
| See "Unexposed and non-existent keys are indistinguishable" below. | ||
|
|
||
| ### The shared publisher subscribes to its own `config/+` and reconciles every message it sees |
There was a problem hiding this comment.
It feels like this section is possibly slightly lacking detail on how it retrieves the information about what's currently set, i.e. "how do we work out when we've received all the retain messages we're subscribed to?" as knowing when you've received all the retain messages is somewhat nontrivial.
There was a problem hiding this comment.
This shared publisher is not concerned about the end of the retained messages delivery as it is equipped to do reconciliation dynamically at any point in time, as tampering attempts can also happen anytime.
There was a problem hiding this comment.
This isn't a concern anymore as we have switched to a single config document per service model in 419d007
| It collects every service's config purely by subscribing, including its own, so it can never know more | ||
| than "did a retained message arrive for this key." | ||
|
|
||
| ### Deprecated aliases are never exposed; ownership decides who publishes |
There was a problem hiding this comment.
I'm completely confused as to how deprecated keys are related to ownership?
There was a problem hiding this comment.
It's actually about a very specific case of the currently exposed agent.entity_store.auto_register which replaced the old deprecated c8y.entity_store.auto_register setting. The only thing that I wanted to highlight with this case was that the agent's setting being exposed does not transitively expose the mapper's value as well as both configs are now owned by two different components. I'll update that text as I realise that 2 different concepts (general handling of deprecated keys vs this specific case about cross component deprecation) got conflated into this single section.
| absent | ||
| - **THEN** it SHALL take no action, so that a clear cannot echo into another clear | ||
|
|
||
| ### Requirement: The agent serves exposed configuration over HTTP |
There was a problem hiding this comment.
Is this specifically retained messages only (i.e. only at startup), or does it include updates when services publish those (e.g. if the mapper starts up shortly after the agent)?
There was a problem hiding this comment.
The config values by other services can be published at any time, as they can be started any time. The agent will then start building that service's config map in its own entity store. So, once all the config/+ retained messages for that external service is delivered to the agent, then it its config map is also complete and can be queried over HTTP. Unfortunately, there is no "readiness" cue here in terms of when all the configs of a given service is available, as that is completely dependent on the time taken for all those retained messages to be delivered and processed by the agent. So, the config endpoint becomes "available" from the moment the first config/+ message is received by the agent.
c25e17a to
0a804be
Compare
Robot Results
|
I approved when only the spec document was written. So my approval is no longer appropriate.
jarhodes314
left a comment
There was a problem hiding this comment.
I think the proposed structuring for the proposal is nice and helps frame the problem correctly. I've suggested some improvements to the config in #4729 (as a PR seemed to be the easiest way to share it along with context by way of what the resulting artifacts look like after following those config suggestions). The broad scope of those changes is to focus more on the motivation for why we want our specs to be a certain way and less on just telling the agent to write fewer words.
| See [thin-edge/thin-edge.io#4235](https://github.com/thin-edge/thin-edge.io/issues/4235) for the full | ||
| requirement and the motivating use cases. | ||
|
|
||
| ## What you can do |
There was a problem hiding this comment.
I don't love this as a heading. It's not immediately clear (or frankly even after reading the section) what it's trying to convey. I think part of this is the "you" aspect: who is this really addressed to? the person proposing the change? reviewers? ai agents? a combination of those?
I think we could just call this ## Proposed Solution or something
| The problem and the user-facing surface are in the [proposal](./proposal.md): | ||
| an external process can't read even a non-secret value like `device.id` without file access. | ||
|
|
||
| This document covers how that surface is built. |
There was a problem hiding this comment.
I find this usage of the term surface unusual and weird.
| So even a non-secret, widely-needed value like `device.id` is out of reach for anything outside the | ||
| component that owns the config file. | ||
| The problem and the user-facing surface are in the [proposal](./proposal.md): | ||
| an external process can't read even a non-secret value like `device.id` without file access. |
There was a problem hiding this comment.
The wording used in proposal.md is clearer:
| an external process can't read even a non-secret value like `device.id` without file access. | |
| A plugin, or a %%te%% component running in its own container, or a child device, | |
| cannot read any configuration setting from the `tedge.toml` configuration file stored on the main device disk. |
| proposal: | ||
| - After "Why", add a "What you can do" section that introduces the feature from the | ||
| user's point of view with short usage examples — before any internal mechanism. | ||
| - Keep "What Changes" for the mechanism and code impact. Never lead the solution with an | ||
| implementation detail (a macro flag, a struct, a topic-construction rule); those belong | ||
| under "What Changes", after the reader has seen how the feature is used. |
There was a problem hiding this comment.
I'm astonished that you delegate even that to an agenr :-(
There was a problem hiding this comment.
The proposal.md generated by Openspec in this case was heavily solution oriented. So, I was just trying to tune it to our liking so that we don't have to rely on yet another document to understand the "user impact" of the proposed solution before getting into the details of the solution.
| Each entity that owns `tedge_config` settings (e.g. `tedge-agent`, or a cloud mapper such as | ||
| `tedge-mapper-c8y`) publishes any setting explicitly marked exposable as a retained `config` MQTT | ||
| message under its own service topic (see the [MQTT API reference](../../references/mqtt-api.md#configuration)). | ||
| The agent collects these as an ordinary MQTT subscriber and serves them as a read-only view over HTTP. |
There was a problem hiding this comment.
The tone of this text is not appropriate for a user manual. This is more akin to a reference or a design doc.
| The agent collects these as an ordinary MQTT subscriber and serves them as a read-only view over HTTP. | |
| The agent and the mappers expose part of their configuration over MQTT as a retained `config` MQTT | |
| messages. | |
| The agent also gather all these configuration settings and serves them as a read-only view over HTTP. |
| A configuration key that is not marked exposable and a key that does not exist at all are | ||
| indistinguishable here: both return `404 Not Found`, so the response reveals nothing about which | ||
| non-exposed settings exist. |
There was a problem hiding this comment.
Speaking about keys that are not marked exposable might confuse the users asking themselves how to expose a key.
| A configuration key that is not marked exposable and a key that does not exist at all are | |
| indistinguishable here: both return `404 Not Found`, so the response reveals nothing about which | |
| non-exposed settings exist. | |
| The agent and the mappers don't expose all their configuration settings (some are secret values, others are meaningless when used from another box). | |
| A configuration key that is not exposed or does not exist at all are | |
| indistinguishable here: both return `404 Not Found`, so the response reveals nothing about which | |
| non-exposed settings exist. |
| ## Configuration | ||
|
|
||
| A service that owns a `tedge_config` setting (`tedge-agent` for core/device settings, each cloud | ||
| mapper for its own cloud's settings) publishes any setting explicitly marked exposable as a retained |
There was a problem hiding this comment.
Explicitly marked exposable is confusing for the users as they have no mean to mark a setting as exposable. This is a internal concept.
| mapper for its own cloud's settings) publishes any setting explicitly marked exposable as a retained | |
| mapper for its own cloud's settings) publishes a non-secret subset of its setting as a retained |
| An empty payload means the value is unset, or was removed from configuration since the last time the | ||
| owning service started. A setting is only published this way if it was explicitly marked exposable |
There was a problem hiding this comment.
Are setting updates dynamically published? When the file is updated? When tedge config set is used?
What about default values - not explicitly set in tedge.toml? What is the effect of tedge config unset?
There was a problem hiding this comment.
There is no dynamic update. The published config represents the config the service was started with. Any subsequent changes take effect (and published) only on the next restart. Default values are published and unset values are omitted. Since we switched to publishing a single config JSON document message instead of a message-per-key, that published document only contains the set values. I have updated the user facing docs to explicitly state these as well in 911c34d.
| // c8y.url has no default, so with no config it should be unset | ||
| let cloud = exposed_cloud_config(&reader, CloudType::C8y, None).unwrap(); | ||
| let (_, url) = cloud.iter().find(|(k, _)| k == "url").unwrap(); | ||
| assert_eq!(url, &None); |
There was a problem hiding this comment.
This feels like it ought to be broken out into a separate test
| for key in secret_keys { | ||
| let parsed: ReadableKey = key | ||
| .parse() | ||
| .unwrap_or_else(|e| panic!("failed to parse known configuration key '{key}': {e}")); |
There was a problem hiding this comment.
| .unwrap_or_else(|e| panic!("failed to parse known configuration key '{key}': {e}")); | |
| .expect(format!("failed to parse known configuration key '{key}'")); |
should do the same job
Add #[tedge_config(exposable)] and a generated ReadableKey::is_exposable() accessor, following the existing help()/doc-comment pattern. This is the opt-in marker used by the config-exposure feature to curate which settings may be published to external clients. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
…pers Mark the initial exposable settings from design.md's allowlist table with #[tedge_config(exposable)], and add exposed_core_config()/exposed_cloud_config() as pure helpers over TEdgeConfigReader, built on the existing readable_keys()/read_string() machinery. Includes a forward-guarding test that fails CI if a known-secret key is ever marked exposable. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
Add Channel::Config{key}/ChannelFilter::Config, mirroring EntityTwinData,
and a config: BTreeMap<String, String> map on EntityMetadata, parallel to
(not merged with) twin_data. Add entity-store ingestion primitives
(ingest_config_value/clear_config_value/get_config/get_config_value) with
the same key validation as twin fragments, so config values arriving over
MQTT can be recorded without opening an external write API.
Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
New extensions crate publishing a component's own exposable configuration as retained per-key MQTT messages under its own service topic, one message per value, with an empty payload for unset keys. The actor subscribes to its own config/+ wildcard and reconciles every message it sees against its expected state, correcting external tampering (including an empty payload that would wipe an owned value) and clearing stale keys left behind by a renamed, removed, or demoted setting. Shared between tedge-agent (core config) and the cloud mappers (their own cloud's config) instead of duplicating topic construction and reconciliation three times over. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
Compute exposed_core_config() at startup, patching mqtt.topic_root and mqtt.device_topic_id with their CLI-effective values since the pure config helper only sees the file value. Spawn the shared ConfigPublisherBuilder for the agent's own service topic alongside the existing twin-manager actor. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
…tion Extend start_basic_actors, the shared helper that already spawns the health monitor for every mapper, to also spawn the shared ConfigPublisherBuilder. The c8y, az, and aws mappers pass their own profile-aware exposed_cloud_config(); collectd and custom mappers, which own no cloud config in tedge_config, pass an empty set. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
Subscribe the entity store to the config channel filter alongside twin data, ingest config/<key> messages (empty payload = removal), and clear a deregistered entity's retained config topics the same way its twin data is already cleared. This lets the agent collect every service's exposed configuration, including its own, purely by subscribing. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
Add GET /te/v1/entities/<service>/config and .../config/<key>, modeled on the existing twin-data routes: a JSON object for the whole-service view, a plain-text body for a single value, and writes (PUT/PATCH/DELETE) rejected with 405 via the existing unmatched-channel fallback. A non-exposed key and a non-existent key return the same 404, since the agent has no source of truth for a key beyond what has arrived over MQTT. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
Add a Robot Framework suite verifying the agent's own retained config topics and HTTP routes, including that a secret setting (device.key_pin) never appears on either surface, and a second suite covering a bootstrapped c8y mapper's own retained config topics and HTTP routes, verifying the cloud/profile qualifier is stripped and other clouds' settings are absent. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
Add the config channel to the MQTT topic reference (category table, telemetry data table, and a new Configuration section with a worked example), and the two GET /te/v1/entities/<service>/config[/<key>] routes to the entity HTTP API reference, cross-linked with each other. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
Replace per-key retained MQTT topics with a single retained JSON document per service on `.../config`. This simplifies the self-correcting publisher: a stale key just drops out of the republished document instead of needing an explicit per-key clear. The HTTP API is unaffected — both the whole-service and single-key routes still work, the latter now looked up in the parsed document. tasks.md is unchecked for the sections whose implementation no longer matches this design (channel/entity-store/publisher, MQTT ingestion, e2e tests, MQTT docs). Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
…t per service Carries the revised design into code: `Channel::Config` drops its per-key segment, the entity store gains a whole-document `set_config` replacing the old per-key ingest/clear, and the shared publisher in tedge_config_ext publishes and reconciles one retained JSON object per service instead of one message per key. Stale-key clearing is no longer a separate code path, since republishing the whole document already drops keys that fell out of the exposed set. The agent/mapper spawn call sites and the HTTP query side needed no changes, since neither depended on the per-key MQTT shape. Signed-off-by: Albin Suresh <albin.suresh@cumulocity.com>
f2a4f30 to
17fb8b1
Compare
Proposed changes
Proposal to exposed selected tedge/mapper configs to external clients that don't have access to the config files via the same file system.
Types of changes
Paste Link to the issue
#4235
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments