Skip to content

Storage Configuration - Renewal of cloud storage credentials #423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 17 commits into
base: development
Choose a base branch
from

Conversation

jmelancongen
Copy link
Contributor

With the current specification, a cloud provider must continuously renew the credentials assigned to a device using the SetStorageConfiguration API. This means that a cloud provider must keep track of all devices and attempt to refresh this configuration, generally over Uplink, regularly to ensure that there is no loss of recording.

Instead of a manual procedure by the cloud provider, we propose that the device manage the lifecycle of its credentials on its own, by accepting an endpoint to a simple API that provides credentials to the device on-demand. This will allow the device to refresh credentials much faster in case of outages, where the device comes back online after a while and wants to resume recording as quickly as possible.

@jmelancongen jmelancongen requested a review from ubkr May 21, 2024 18:41
@jmelancongen
Copy link
Contributor Author

To be Added: An OpenAPI document defining the schema of the response that device should expect from the API

@jmelancongen jmelancongen changed the title Proposal: Storage Configuration - Renewal of cloud storage credentials Storage Configuration - Renewal of cloud storage credentials Jun 7, 2024
Copy link
Contributor

@bsriramprasad bsriramprasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added feedback

@jmelancongen
Copy link
Contributor Author

Updated based on feedback from F2F:

  • Removed the content-type completely. So application/json is just assumed, but has no impact.
  • Removed LocalPath & Type fields from the renewal response. It doesn't make sense to change those during a renewal.
  • Clarified that null values are expected to clear the corresponding optional parameter
  • Clarified that the storage configuration shall be updated with the values from the renewal. So that further GetStorageConfigurations will see the current values.

@bsriramprasad
Copy link
Contributor

bsriramprasad commented Oct 17, 2024

@jmelancongen Once PR #481 is approved, you may want to update the CertPathValidationPolicyID used to validate the renewal endpoint server certificate. requirement to reflect proposed changes in #481 ?

Copy link
Contributor

@kieran242 kieran242 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to approve. Only 1 suggested correction.

@ubkr ubkr self-requested a review April 15, 2025 09:56
<title>Configuration Renewal</title>
<para>
The configuration allows for a renewal endpoint to be set. If the device supports this feature, it shall automatically renew the credentials
when they are about to expire.
Copy link
Contributor

@bsriramprasad bsriramprasad May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
when they are about to expire.
when they are about to expire OR after the device reboots.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ones feels like a backward requirement. If the device had some credentials, reboots, and remembers them, and they are still valid, is there a reason it shall renew anyway?
I understand that this is a valid implementation to do so, but it is not the mandatory implementation, right?

meantime to avoid service disruption.
</para>
<para>
Once the device has called the renewal endpoint, the corresponding <literal>StorageConfiguration</literal> shall be updated with the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The payload from renew response is not the usual configuration that client wants to read back. (e.g. confirm a SET operation by doing a GET/Read) more so when these values will not survive a reboot.

Even if the client calls GetStorageConfiguration, due to security reasons device shall NOT return the current configured credentials and hence we do not really see the need to store the credentials from renew back into StorageConfiguration. Hence requirement to store into the StorageConfiguration would need to be relaxed.

Whether the configuration is stored in persistent or non-persistent memory as discussed before can be left to the device.

Proposal

Suggested change
Once the device has called the renewal endpoint, the corresponding <literal>StorageConfiguration</literal> shall be updated with the
Once the device has called the renewal endpoint, the response received shall be stored by the device for further usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence feels a bit meaningless now though. Also, the initial intent was that if the renew endpoint provides a different storage account/S3 bucket, then that is not a secret value, and should be observable by callers of the GetStorageConfiguration.
Not doing so means that there is no way for an observer to know where the device is currently recording. My primary interest there was for debug purposes, so that might not be that bad to remove. But then I'm wondering if we want to simply enforce that all fields are cleared if the renew configuration is provided, to ensure we never get conflicting information?

Copy link
Contributor

@bsriramprasad bsriramprasad May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we are kind of putting static (rarely changed) vs dynamic (frequently changed, frequency may vary) and expecting all to be stored for every renew - especially when device has no control on update frequency, we are bringing out this conversation

Static (frequency of update is quite less!)

  • Storage type
  • Storage region

Dynamic (We do not expect Dynamic data will survive a reboot)

  • Storage URI (would like this in static too, but practically this can be considered dynamic)
  • user name
  • password
  • token
  • token expiry

'if renewal configuration' is available during device operation, it should take precedence over initial/stored configuration.

Meanwhile, Say if device reboots and since the renewal configuration 'may not be persistent', device can give out a try with initial configuration OR if renewal configuration is available, device will reach out to renewal endpoint for fresh credentials.

We want to decouple configuration data (can survive reboot) with the runtime data (may or may not survive reboot!) and do not want to update persistent data too often to conserve flash wear. That's the whole baseline of our concern :-)

},
"expiresAt": "<ISO 8601 date-time>"
}]]></programlisting>
Any null value in the response shall clear the corresponding optional parameter in the <literal>StorageConfiguration</literal>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the cases that I see ambiguity

  • Renewal Response leaves out a field from payload
  • RenewalResponse includes a field with a null string/value

Both are treated the same?

In either case, for the same argument as in https://github.com/onvif/specs/pull/423/files#r2077654596, response from renewal should not modify StorageConfiguration but the 'local/temporary/non-persistent' data that device maintains.

Any updates to StorageConfiguration should only happen via SetStorageConfiguration (e.g. over Uplink) to avoid any race conditions (or nondeterministic) for updating same data from 2 different paths.

Suggested change
Any null value in the response shall clear the corresponding optional parameter in the <literal>StorageConfiguration</literal>.
Any null value in the response shall clear the corresponding optional parameter stored in the device.</literal>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants