nrkno / sofie-core

Sofie Core: A Part of the Sofie TV Studio Automation System
https://github.com/nrkno/Sofie-TV-automation/
MIT License
131 stars 40 forks source link

RFC: Configuring gateway 'root' properties from blueprints #1297

Open Julusian opened 1 month ago

Julusian commented 1 month ago

About Me

This RFC is posted on behalf of the BBC

Use Case

We want the blueprints to own the configuration of the system.

For a MOS gateway, the ID of the gateway is defined at the root level of the gateway, which blueprints are not able to configure. For playout gateway, there are various boolean options at the root level which control some behaviour of the gateway, these have suboptimal defaults that cannot be changed by the blueprints.

Proposal

I do not have a proposed solution, I am looking for ideas on how this should be solved.

Process

The Sofie Team will evaluate this RFC and open up a discussion about it, usually within a week.

jstarpl commented 1 month ago

Could this potentially be a task for system blueprints? Maybe they could provide callbacks to return defaults for a first connection of a PeripheralDevice? Or even every connection, and "onPeripheralDeviceConnected" callback - if it were async, one could imagine a sort-of DHCP setup, where the System Blueprint could query a central command server for what these settings should be, and then that system could make the decision. Perhaps it would then make sense to expose some environment variables to the System Blueprint to make identifying instances easier.

nytamin commented 4 weeks ago

I've got a potential proposal that might solve this:

I believe the root problem lies with that the current PeripheralDevice type (see definition) contains some properties that are owned and set by the device itself (such as category, type, status, versions, token), some properties owned and set by Sofie Core (studioId, settings, connected) as well as one whose ownership is a ambiguous (name).

I propose that we rework the PeripheralDevice to be a pure "status carrier" instead, so that it'll contain only statuses and identifier info (such as token, status, connected, category, versions, nrcsName etc).

So we would: 1) move the settings to be a setting on the Studio, similar to how we've done it with the sub-devices settings. 2) move the studioId to a new collection (PeripheralDeviceStudioMap). This is to make the lookup for Device -> Studio simple and not require a deep lookup of the Studio. 3) Add a blueprint method (to the system blueprints) to be able to add a PeripheralDeviceStudioMap entry. 4) Add a blueprint method (to the studio blueprints ) to be able to set the settings for a PeripheralDevice (similar to the sub-devices).

scriptorian commented 3 weeks ago

Please also consider the configuration API impact. I can imagine that there may be some blueprint defaults for PeripheralDevice settings that we can define in a config-preset and provide a more limited set of settings to the API to offer simpler client implementation and allow safer parameter validation.

Julusian commented 3 weeks ago

@nytamin I'm not entirely opposed to that plan, but have some thoughts

1 & 4) I think this would work. I think it would fit into the existing applyConfig method without needing a new one. I think the question then becomes, is the current strategy of having the subdevices separated from the parent device ok?

2 & 3) I am not sure I like this. Adding a collection which contains just a pair of ids feels very SQL and not very noSQL.
And will we be able to give that blueprint method enough info to make an informed decision about which studio a device should be part of?

What if the device provided the studioId as part of its init? From a quick skim, I don't see not persisting this as making anything harder, in various places we are finding devices by studioId, but we can simply use the object on the studio document. That moves the determination of the id to be defined in the environment, which I think makes sense. Your deployment kind of needs to know this relationship anyway so that you dont restart/delete/update the wrong one when doing maintenance. So I think it better to rely on that instead of relying on the blueprints to figure out and match this.
Of course, for installations which are a single studio this will be trivial as everything can default to studio0 unless something else is specified


I know I've pitched before that I would like to see the parent and child devices combined into one object at some point, might be worth considering doing that at the same time as the rest of this if it feels like not much more work.

nytamin commented 3 weeks ago

What if the device provided the studioId as part of its init?

I don't think that's a good idea, because the action of "adding a device to a studio" is essentally giving that device access to that studio, so that is something that should be done by an authorized user / internally/ blueprints. Otherwise that would be a potential attack vector by a malicious actor.

Instead of having the StudioId-DeviceId-pair as a separate collection, it could be a property on the Studio? I guess if we add a db-index for that property as well, doing a lookup on Studios, to figure out which studio a Device has should be fine.

Julusian commented 3 weeks ago

I don't think that's a good idea, because the action of "adding a device to a studio" is essentally giving that device access to that studio, so that is something that should be done by an authorized user / internally/ blueprints. Otherwise that would be a potential attack vector by a malicious actor.

I think I've said this before, but I would also really like to change the registration flow to be that the deviceId and token has to be added in Sofie by the user before the device is allowed to connect.
What I have proposed here would be leaning into that direction, but not going all the way there yet.

This got me curious, and I can now say that we don't have enough functional security for the current system to do anything. Any ddp client (including a malicious client) can after calling init for the first time, issue a core.ddp.ddpClient.call('/peripheralDevices/update', [{ _id: 'ExampleDevice' }, { $set: { studioId: 'studio0' } }, {}, ]) to add themselves to a studio. That is simply replicating what the ui does when attaching a device to the studio.

After #1287, if running with the new security mode enabled then that won't be possible. But it is also expected that nginx/whatever should be performing the authentication and only allowing connection to sofie if the peripheraldevice has valid credentials. Meaning the token wouldn't really do anything. And if the deviceId/studioId were specified via header instead of part of the init payload, then the proxy could enforce certain values to tighten it up even more. The result of this being that the auto-add and attach behaviour would actually be fine and desirable.

Julusian commented 1 week ago

jumping back a little bit here, it sounds like @nytamin and I agree on 1 & 4, so I shall start working on those. 2 & 3 are not in the scope of what we need as part of this RFC, so I propose that we ignore those for this RFC (unless it becomes problematic while implementing) and this can be revisited later