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

Support to enable fips for the command sonic_installer #2154

Merged
merged 6 commits into from
Jul 8, 2022

Conversation

xumia
Copy link
Collaborator

@xumia xumia commented May 7, 2022

What I did

Support to enable fips for the command sonic_installer
See sonic-net/SONiC#997

How I did it

sonic-installer set-fips [--enable-fips|--disable-fips]
sonic-installer get-fips

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@lgtm-com
Copy link

lgtm-com bot commented May 7, 2022

This pull request introduces 3 alerts when merging ce5ef1f into 1143869 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Jun 26, 2022

This pull request introduces 3 alerts when merging d119c2d into f64d280 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Jul 1, 2022

This pull request introduces 3 alerts when merging 691d393 into 3274b0e - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Jul 1, 2022

This pull request introduces 2 alerts when merging 4e2cf1c into b5d6659 - view on LGTM.com

new alerts:

  • 2 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Jul 7, 2022

This pull request introduces 2 alerts when merging 6b92447 into 3600639 - view on LGTM.com

new alerts:

  • 2 for Unused local variable

@xumia xumia marked this pull request as ready for review July 7, 2022 14:30
@xumia xumia force-pushed the support-fips-cli branch from ee8b523 to 9104614 Compare July 7, 2022 14:32
# Set fips for image
@sonic_installer.command('set-fips')
@click.argument('image')
@click.option('--disable-fips', is_flag=True,
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 7, 2022

Choose a reason for hiding this comment

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

disable

prefer positive words, like --enable-fips. And default to true. #Closed

Copy link
Collaborator Author

@xumia xumia Jul 7, 2022

Choose a reason for hiding this comment

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

Changed to use --enable-fips/disable-fips.

def set_fips(image):
""" Set fips for the image """
bootloader = get_bootloader()
if image not in bootloader.get_installed_images():
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 7, 2022

Choose a reason for hiding this comment

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

image

Is it convenient to have a default image? #Closed

Copy link
Collaborator Author

@xumia xumia Jul 8, 2022

Choose a reason for hiding this comment

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

There is no function to retrieve the default image, if the image is not set, then we change the next image.

sys.exit(1)
enable = bootloader.get_fips(image)
if enable:
click.echo("Fips is enabled")
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 7, 2022

Choose a reason for hiding this comment

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

Fips

Fips -> FIPS #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, thanks.

click.echo('Disabling FIPS...')
fips = "0"
cmdline = self._get_image_cmdline(image)
cmdline = re.sub(' sonic_fips=[^\s]', '', cmdline) + " sonic_fips=" + fips
Copy link
Contributor

Choose a reason for hiding this comment

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

re.sub

Suggest parsing it instead of manipulate single command string. You may use shlex.split.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is to remove the old sonic-fips option, and then add the new setting, and we do not want to have impact on the other settings.
For example, the options as below:

rw console=tty0     console=ttyS0,  9600n8    quiet intel_idle.max_cstate=0 sonic_fips=0 og_size=4096

After changed, the options with multiple space characters will not be changed

rw console=tty0     console=ttyS0,  9600n8    quiet intel_idle.max_cstate=0 og_size=4096 sonic_fips=1

fips = "1"
else:
click.echo('Disabling FIPS...')
fips = "0"
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 7, 2022

Choose a reason for hiding this comment

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

Just use "1" if enable else "0" #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, thanks.

@yxieca
Copy link
Contributor

yxieca commented Jul 17, 2022

@xumia this change cannot be cherry-picked cleanly.

@yxieca
Copy link
Contributor

yxieca commented Aug 8, 2022

@xumia can you create separate PR for 202205 branch?

@xumia
Copy link
Collaborator Author

xumia commented Aug 9, 2022

@xumia can you create separate PR for 202205 branch?

@yxieca , thanks for your comment, I sent a PR: #2303 to fix the conflict.

xumia added a commit that referenced this pull request Aug 10, 2022
What I did
Cherry-pick #2154
Support to enable fips for the command sonic_installer
See sonic-net/SONiC#997

How I did it
sonic-installer set-fips  [--enable-fips|--disable-fips]
sonic-installer get-fips
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.

3 participants