Updated signed_video_set_private_key by mithydolphin · Pull Request #402 · AxisCommunications/signed-video-framework

mithydolphin · 2025-02-27T14:48:28Z

Updated signed_video_set_private_key to work with ONVIF. Added getter
functions for the private key and certificate chain. Included a test
to verify the signing functionality.

Describe your changes

Please include a summary of the change, a relevant motivation and context.

Issue ticket number and link

Fixes #(issue)

Checklist before requesting a review

I have performed a self-review of my own code
I have verified that the code builds perfectly fine on my local system
I have added tests that prove my fix is effective or that my feature works
I have commented my code, particularly in hard-to-understand areas
I have verified that my code follows the style already available in the repository
I have made corresponding changes to the documentation

Updated signed_video_set_private_key to work with ONVIF. Added getter functions for the private key and certificate chain. Included a test to verify the signing functionality.

bjornvolcker

Some initial comments

lib/src/sv_sign.c

lib/vendors/axis-communications/sv_vendor_axis_communications.c

tests/check/check_signed_video_sign.c

Change-Id: If5540f944d12920de0daf890cd79427aa8eb45be

Change-Id: I3145e9824128a31f63e5c8a47f692855bbc56695

bjornvolcker

There are still a bunch of unanswered review questions.

Also, have you verified that it works to use the reconstructed key on a device?

lib/src/sv_sign.c

lib/vendors/axis-communications/sv_vendor_axis_communications.c

lib/vendors/axis-communications/sv_vendor_axis_communications_internal.h

tests/check/check_signed_video_sign.c

Change-Id: Ib332af2a44b5136a0cda361aa9da7ccaf7c53e70

bjornvolcker

Still a few old review comments to address. And a few new ones.

lib/src/sv_sign.c

lib/vendors/axis-communications/sv_vendor_axis_communications.c

lib/vendors/axis-communications/sv_vendor_axis_communications_internal.h

tests/check/check_signed_video_sign.c

lib/vendors/axis-communications/sv_vendor_axis_communications.c

lib/src/sv_common.c

lib/vendors/axis-communications/sv_vendor_axis_communications.c

lib/src/sv_sign.c

lib/src/sv_openssl.c

lib/src/sv_internal.h

lib/src/sv_common.c

bjornvolcker · 2025-03-05T05:59:38Z

This PR is quite large. Is it possible to split it into a series of 5-10 separate PRs. It will make reviewing much easier.

lusikamalo2

No extra comments from my side so far.

lib/src/sv_sign.c

lib/src/sv_openssl.c

lib/src/sv_common.c

lib/src/sv_openssl.c

lib/vendors/axis-communications/sv_vendor_axis_communications.c

lib/src/sv_internal.h

lib/src/sv_sign.c

lib/vendors/axis-communications/sv_vendor_axis_communications.c

lib/src/sv_sign.c

tests/check/check_signed_video_sign.c

Change-Id: I6dc497e6681719abeef1477cec9c416a5439f777

bjornvolcker

It still doesn't work. How are you testing the various cases?

order of operations
number of signing plugins
fallback to signed video upon failure

I still suggest that splitting this PR into smaller pieces will help you test and solve these.

lib/vendors/axis-communications/sv_vendor_axis_communications.c

lib/src/sv_sign.c

bjornvolcker · 2025-03-06T05:51:41Z

lib/src/sv_internal.h

 * retrieving the private key and certificate chain from the signed video object,
- * and setting the ONVIF signing key pair.
+ * and setting the ONVIF signing key pair. The function will free the ONVIF object
+ * on failure, allowing the framework to continue with the signed video.


Yes, this is the desired behavior, but is that really what happens with your implementation?
Have you tested/verified that?

bjornvolcker · 2025-03-07T05:32:43Z

lib/src/sv_sign.c

+    // Try to initialize ONVIF; if it is not available or cannot be initialized,
+    // fallback to Signed Video signing plugin.
+    initialize_onvif(self);
+    if (!self->onvif) {


Now when ONVIF signing plugin is called through the Axis signing plugin (see other PR) you need to remove the if-statement

bjornvolcker · 2025-03-07T05:34:58Z

lib/src/sv_sign.c

-
-  return status;
+  self->private_key = NULL;
+  if (status != SV_OK && self->onvif != NULL) assert(true);


Well, you should also check the opposite. I only gave you an example. And also shouldn't it be false in assert?

assert((status == SV_OK && !self->onvif) || (status != SV_OK && self->onvif));

bjornvolcker · 2025-03-07T05:35:37Z

lib/src/sv_sign.c

  // Sanity check ONVIF object.
-  if(!self->onvif) {
-    return SV_INVALID_PARAMETER;
+  if (!self->onvif) {


You should also check if private_key exists

bjornvolcker · 2025-03-07T05:36:04Z

lib/src/sv_sign.c

    // Temporally turn the PEM |private_key| into an EVP_PKEY and allocate memory for signatures.
    SV_THROW(openssl_private_key_malloc(self->sign_data, private_key, private_key_size));
    SV_THROW(openssl_read_pubkey_from_private_key(self->sign_data, &self->pem_public_key));
+    self->private_key = private_key;


You forgot to set private_key_size

bjornvolcker · 2025-03-07T05:36:47Z

lib/vendors/axis-communications/sv_vendor_axis_communications.c

-    if (status == SV_OK) {
-      return status;
-    }
+  if (sv->sign_data && sv->sign_data->key && sv->onvif) {


No need for if-statement. The check should be inside initilaize_onvif()

bjornvolcker · 2025-03-07T05:38:25Z

lib/src/sv_internal.h

  // For signing plugin
  void *plugin_handle;
  sign_or_verify_data_t *sign_data;  // Pointer to all necessary information to sign in a plugin.
+  const char *private_key;


Add comment what this is used for. A reader might assume that this is the signing key used during the session, but it is not.

bjornvolcker · 2025-03-07T05:41:21Z

lib/src/sv_sign.c

+    self->private_key = private_key;
+
+    // Try to initialize ONVIF; if it is not available or cannot be initialized,
+    // fallback to Signed Video signing plugin.


The comment about "fallback" should be inside initialize_onvif since you do not take any actions regarding that out here.

bjornvolcker · 2025-03-07T05:41:58Z

lib/vendors/axis-communications/sv_vendor_axis_communications.c

+
+  sv_vendor_axis_communications_t *self = (sv_vendor_axis_communications_t *)handle;
+
+  // Return the certificate chain


Redundant comment. I think the reader is clever enough to understand that from the line below.

Updated signed_video_set_private_key

e2af86d

Updated signed_video_set_private_key to work with ONVIF. Added getter functions for the private key and certificate chain. Included a test to verify the signing functionality.

mithydolphin requested review from Caroline-Holmstrom, bjornvolcker and lusikamalo2 as code owners February 27, 2025 14:48

patch 2

ec16d03

bjornvolcker requested changes Feb 27, 2025

View reviewed changes

mithydolphin added 2 commits March 3, 2025 12:17

Patch 2

64f4097

Change-Id: If5540f944d12920de0daf890cd79427aa8eb45be

PATCH 4

d5f2e04

Change-Id: I3145e9824128a31f63e5c8a47f692855bbc56695

bjornvolcker requested changes Mar 3, 2025

View reviewed changes

Patch 5

c80ea0e

Change-Id: Ib332af2a44b5136a0cda361aa9da7ccaf7c53e70

bjornvolcker requested changes Mar 4, 2025

View reviewed changes

mithydolphin added 4 commits March 4, 2025 15:25

patch 6

d2712da

Merge branch 'master' into update-set_private_key-for-onvif

031de7d

patch 7

18dbc0c

patch 8

e8a620c

bjornvolcker requested changes Mar 4, 2025

View reviewed changes

lusikamalo2 reviewed Mar 5, 2025

View reviewed changes

patch 9

75bed39

bjornvolcker requested changes Mar 5, 2025

View reviewed changes

lib/src/sv_sign.c Outdated Show resolved Hide resolved

lib/src/sv_sign.c Outdated Show resolved Hide resolved

lib/src/sv_openssl.c Outdated Show resolved Hide resolved

lib/src/sv_openssl.c Outdated Show resolved Hide resolved

lib/src/sv_common.c Outdated Show resolved Hide resolved

patch 10

45ff6d7

bjornvolcker requested changes Mar 5, 2025

View reviewed changes

Patch 11

44e9ba8

Change-Id: I6dc497e6681719abeef1477cec9c416a5439f777

bjornvolcker requested changes Mar 6, 2025

View reviewed changes

patch 12

b3800d5

bjornvolcker requested changes Mar 7, 2025

View reviewed changes

bjornvolcker closed this Mar 7, 2025

mithydolphin deleted the update-set_private_key-for-onvif branch March 18, 2025 14:43


		sv_vendor_axis_communications_t self = (sv_vendor_axis_communications_t )handle;

		// Return the certificate chain

Conversation

mithydolphin commented Feb 27, 2025

Describe your changes

Issue ticket number and link

Checklist before requesting a review

Uh oh!

bjornvolcker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bjornvolcker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bjornvolcker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bjornvolcker commented Mar 5, 2025

Uh oh!

lusikamalo2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bjornvolcker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bjornvolcker left a comment •

edited

Loading