Skip to content
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

Add support for secure upgrade #2698

Merged
merged 19 commits into from
Jun 26, 2023

Conversation

ycoheNvidia
Copy link
Contributor

What I did
Added support for secure upgrade

How I did it
It includes image signing during build (in sonic buildimage repo) and verification during image install (in sonic-utilities).
HLD can be found in the following PR: sonic-net/SONiC#1024

How to verify it
Feature is used to allow image was not modified since built from vendor. During installation, image can be verified with a signature attached to it.
In order for image verification - image must be signed - need to provide signing key and certificate (paths in SECURE_UPGRADE_DEV_SIGNING_KEY and SECURE_UPGRADE_DEV_SIGNING_CERT in rules/config) during build , and during image install, need to enable secure boot flag in bios, and signing_certificate should be available in bios.

Feature dependencies
In order for this feature to work smoothly, need to have secure boot feature implemented as well.
The Secure boot feature will be merged in the near future.

sonic-buildimage PR: sonic-net/sonic-buildimage#11862

@Yarden-Z
Copy link

@qiluo-msft Can you please review and approve this PR?
Containing the original secure upgrade item and a fix for the bug found in aboot.

@liat-grozovik
Copy link
Collaborator

@qiluo-msft , @Yarden-Z @xumia please help to review this PR. this one must go into 202211 as well .

@zhangyanzhao
Copy link
Collaborator

moved to 202305 release

@liat-grozovik
Copy link
Collaborator

@qiluo-msft kindly reminder to review and provide feedback

DavidZagury added a commit to DavidZagury/sonic-utilities that referenced this pull request May 23, 2023
@@ -75,6 +75,10 @@ def supports_package_migration(self, image):
"""tells if the image supports package migration"""
return True

def verify_image_sign(self, image_path):
Copy link
Contributor

@qiluo-msft qiluo-msft May 30, 2023

Choose a reason for hiding this comment

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

verify_image_sign

A unsecure normal image is considered not signed image, and should return False. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to verify image signature only of secure boot flag is enabled. This flow was already merged in previous PR #2337 (that was reverted even though it had a fix ready in a day) and described and approved in the HLD as well (sonic-net/SONiC#1024)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qiluo-msft please add your comments/approval

Copy link
Contributor

@qiluo-msft qiluo-msft Jun 5, 2023

Choose a reason for hiding this comment

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

After checking HLD, my comment is still applicable.
I understand "We want to verify image signature only of secure boot flag is enabled." This is the base class implementation, if subclass does not override, it will confuse user that an unsecure normal image is considered "verify_image_sign"-ed.

Copy link
Contributor Author

@ycoheNvidia ycoheNvidia Jun 5, 2023

Choose a reason for hiding this comment

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

What do you suggest to do? @qiluo-msft Does the user read the python code? If we move the prints around verify_image_sign call in main.py inside grub bootloader will it be enough? I Can also add print inside base bootloader ("image verification not implemented, continue without verification"). Returning False here will make other vendor's CI fail, as the previous PR did, and I don't think implementing this method for each bootloader will be positive

Copy link
Contributor

Choose a reason for hiding this comment

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

If you mean "image verification not implemented", you should either throw NotImplmented exception or return a reasonable value to indicate the error. Silently "continue without verification" does not make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified the code as suggested, please review @qiluo-msft

qiluo-msft
qiluo-msft previously approved these changes Jun 21, 2023
@ycoheNvidia
Copy link
Contributor Author

/azp run Azure.sonic-buildimage

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2698 in repo sonic-net/sonic-utilities

@ycoheNvidia
Copy link
Contributor Author

@qiluo-msft @liat-grozovik fixed minor print and issues - CI tests pass

@liat-grozovik liat-grozovik merged commit 316b14c into sonic-net:master Jun 26, 2023
6 checks passed
@liat-grozovik liat-grozovik changed the title Secure upgrade v2 Add support for secure upgrade Jun 26, 2023
@StormLiangMS
Copy link
Contributor

@DavidZagury still got some question.

  1. for device which support this secure upgrade
    a. disabled, to boot unsecure image would be ok.
    b. enabled, unsecure image will be failed to boot.
  2. for device which doesn't support this secure upgrade feature.
    a. unsecure image would be ok to boot/install.

@DavidZagury
Copy link
Contributor

DavidZagury commented Jun 30, 2023

@DavidZagury still got some question.

  1. for device which support this secure upgrade
    a. disabled, to boot unsecure image would be ok.
    b. enabled, unsecure image will be failed to boot.
  2. for device which doesn't support this secure upgrade feature.
    a. unsecure image would be ok to boot/install.
  1. Yes, if Secure Boot is enabled, only Secured OS can be installed and booted. Unsecured images will fail to be installed, and if were already pre-installed, will fail to boot.
  2. Devices that don't support Secure Boot will not check anything related to the security of the image, and any it will be possible to install and boot any image, just as it were in the past.

StormLiangMS pushed a commit that referenced this pull request Jun 30, 2023
- What I did
Added support for secure upgrade

- How I did it
It includes image signing during build (in sonic buildimage repo) and verification during image install (in sonic-utilities).
HLD can be found in the following PR: sonic-net/SONiC#1024

- How to verify it
Feature is used to allow image was not modified since built from vendor. During installation, image can be verified with a signature attached to it.
In order for image verification - image must be signed - need to provide signing key and certificate (paths in SECURE_UPGRADE_DEV_SIGNING_KEY and SECURE_UPGRADE_DEV_SIGNING_CERT in rules/config) during build , and during image install, need to enable secure boot flag in bios, and signing_certificate should be available in bios.

- Feature dependencies
In order for this feature to work smoothly, need to have secure boot feature implemented as well.
The Secure boot feature will be merged in the near future.
StormLiangMS pushed a commit that referenced this pull request Jun 30, 2023
- What I did
Added support for secure upgrade

- How I did it
It includes image signing during build (in sonic buildimage repo) and verification during image install (in sonic-utilities).
HLD can be found in the following PR: sonic-net/SONiC#1024

- How to verify it
Feature is used to allow image was not modified since built from vendor. During installation, image can be verified with a signature attached to it.
In order for image verification - image must be signed - need to provide signing key and certificate (paths in SECURE_UPGRADE_DEV_SIGNING_KEY and SECURE_UPGRADE_DEV_SIGNING_CERT in rules/config) during build , and during image install, need to enable secure boot flag in bios, and signing_certificate should be available in bios.

- Feature dependencies
In order for this feature to work smoothly, need to have secure boot feature implemented as well.
The Secure boot feature will be merged in the near future.
dgsudharsan added a commit to dgsudharsan/sonic-buildimage that referenced this pull request Jul 11, 2023
Update sonic-utilities submodule pointer to include the following:
* ff380e04 [hash]: Implement GH frontend ([sonic-net#2580](sonic-net/sonic-utilities#2580))
* 61bad064 [db_migrator] Set correct CURRENT_VERSION, extend UT ([sonic-net#2895](sonic-net/sonic-utilities#2895))
* 6b8ee47c [CLI][Show][BGP] Show BGP Change for no neighbor scenario ([sonic-net#2885](sonic-net/sonic-utilities#2885))
* 73d8d633 [doc] Update Command-Reference.md, change show bgp peer command to show bfd peer ([sonic-net#2750](sonic-net/sonic-utilities#2750))
* 7bc08c28 [db_migrator] Remove hardcoded config and migrate config from minigraph ([sonic-net#2887](sonic-net/sonic-utilities#2887))
* b1aa9426 [generate_dump]: Enhance show techsupport for Marvell platform ([sonic-net#2676](sonic-net/sonic-utilities#2676))
* 316b14c0 Add support for secure upgrade ([sonic-net#2698](sonic-net/sonic-utilities#2698))
* dc2945bc [dns] Implement config and show commands for static DNS. ([sonic-net#2737](sonic-net/sonic-utilities#2737))
* 8414a709 [chassis][multi asic] change acl_loader to use tcp socket for db communication ([sonic-net#2525](sonic-net/sonic-utilities#2525))
* 0b629ba1 Revert [chassis][voq] Clear fabric counters queue/port (2789) ([sonic-net#2882](sonic-net/sonic-utilities#2882))
* 3ba8241a [db_migtrator] Add migration of FLEX_COUNTER_DELAY_STATUS during 1911->master upgrade + fast-reboot. Add UT. ([sonic-net#2839](sonic-net/sonic-utilities#2839))
* fceef2ed [chassis][voq] Clear fabric counters queue/port ([sonic-net#2789](sonic-net/sonic-utilities#2789))

Signed-off-by: dgsudharsan <sudharsand@nvidia.com>
liat-grozovik pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Jul 11, 2023
Update sonic-utilities submodule pointer to include the following:
* ff380e04 [hash]: Implement GH frontend ([#2580](sonic-net/sonic-utilities#2580))
* 61bad064 [db_migrator] Set correct CURRENT_VERSION, extend UT ([#2895](sonic-net/sonic-utilities#2895))
* 6b8ee47c [CLI][Show][BGP] Show BGP Change for no neighbor scenario ([#2885](sonic-net/sonic-utilities#2885))
* 73d8d633 [doc] Update Command-Reference.md, change show bgp peer command to show bfd peer ([#2750](sonic-net/sonic-utilities#2750))
* 7bc08c28 [db_migrator] Remove hardcoded config and migrate config from minigraph ([#2887](sonic-net/sonic-utilities#2887))
* b1aa9426 [generate_dump]: Enhance show techsupport for Marvell platform ([#2676](sonic-net/sonic-utilities#2676))
* 316b14c0 Add support for secure upgrade ([#2698](sonic-net/sonic-utilities#2698))
* dc2945bc [dns] Implement config and show commands for static DNS. ([#2737](sonic-net/sonic-utilities#2737))
* 8414a709 [chassis][multi asic] change acl_loader to use tcp socket for db communication ([#2525](sonic-net/sonic-utilities#2525))
* 0b629ba1 Revert [chassis][voq] Clear fabric counters queue/port (2789) ([#2882](sonic-net/sonic-utilities#2882))
* 3ba8241a [db_migtrator] Add migration of FLEX_COUNTER_DELAY_STATUS during 1911->master upgrade + fast-reboot. Add UT. ([#2839](sonic-net/sonic-utilities#2839))
* fceef2ed [chassis][voq] Clear fabric counters queue/port ([#2789](sonic-net/sonic-utilities#2789))

Signed-off-by: dgsudharsan <sudharsand@nvidia.com>
pdhruv-marvell pushed a commit to pdhruv-marvell/sonic-utilities that referenced this pull request Aug 23, 2023
- What I did
Added support for secure upgrade

- How I did it
It includes image signing during build (in sonic buildimage repo) and verification during image install (in sonic-utilities).
HLD can be found in the following PR: sonic-net/SONiC#1024

- How to verify it
Feature is used to allow image was not modified since built from vendor. During installation, image can be verified with a signature attached to it.
In order for image verification - image must be signed - need to provide signing key and certificate (paths in SECURE_UPGRADE_DEV_SIGNING_KEY and SECURE_UPGRADE_DEV_SIGNING_CERT in rules/config) during build , and during image install, need to enable secure boot flag in bios, and signing_certificate should be available in bios.

- Feature dependencies
In order for this feature to work smoothly, need to have secure boot feature implemented as well.
The Secure boot feature will be merged in the near future.
sonic-otn pushed a commit to sonic-otn/sonic-buildimage that referenced this pull request Sep 20, 2023
Update sonic-utilities submodule pointer to include the following:
* ff380e04 [hash]: Implement GH frontend ([sonic-net#2580](sonic-net/sonic-utilities#2580))
* 61bad064 [db_migrator] Set correct CURRENT_VERSION, extend UT ([sonic-net#2895](sonic-net/sonic-utilities#2895))
* 6b8ee47c [CLI][Show][BGP] Show BGP Change for no neighbor scenario ([sonic-net#2885](sonic-net/sonic-utilities#2885))
* 73d8d633 [doc] Update Command-Reference.md, change show bgp peer command to show bfd peer ([sonic-net#2750](sonic-net/sonic-utilities#2750))
* 7bc08c28 [db_migrator] Remove hardcoded config and migrate config from minigraph ([sonic-net#2887](sonic-net/sonic-utilities#2887))
* b1aa9426 [generate_dump]: Enhance show techsupport for Marvell platform ([sonic-net#2676](sonic-net/sonic-utilities#2676))
* 316b14c0 Add support for secure upgrade ([sonic-net#2698](sonic-net/sonic-utilities#2698))
* dc2945bc [dns] Implement config and show commands for static DNS. ([sonic-net#2737](sonic-net/sonic-utilities#2737))
* 8414a709 [chassis][multi asic] change acl_loader to use tcp socket for db communication ([sonic-net#2525](sonic-net/sonic-utilities#2525))
* 0b629ba1 Revert [chassis][voq] Clear fabric counters queue/port (2789) ([sonic-net#2882](sonic-net/sonic-utilities#2882))
* 3ba8241a [db_migtrator] Add migration of FLEX_COUNTER_DELAY_STATUS during 1911->master upgrade + fast-reboot. Add UT. ([sonic-net#2839](sonic-net/sonic-utilities#2839))
* fceef2ed [chassis][voq] Clear fabric counters queue/port ([sonic-net#2789](sonic-net/sonic-utilities#2789))

Signed-off-by: dgsudharsan <sudharsand@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants