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

Use bookworm macsec container for 2205. #443

Merged
merged 4 commits into from
Sep 10, 2024
Merged

Conversation

wumiaont
Copy link

@wumiaont wumiaont commented Aug 29, 2024

Why I did it

It's required that sonic 202205 to use bookworm macsec container. The reason is to utilize 202405 macsec container which uses symcrypt with provider + engine support for openssl so when 202405 macsec (hardware + macsec control plane) is FIPS certified we can declare that chassis deploying sonic 202205 is FIPS complaint with its macsec solution.

Another requirement is to have macsec container runs in FIPS mode always.

Work item tracking
  • Microsoft ADO (number only):

How I did it

  1. Have macsec bookworm container available on public docker repo: publicmirror.azurecr.io. This bookworm container was taking from 202405 daily build repository.
  2. Override /etc/fips/fips_enable to have value 1 during docker image build.
  3. It's found that the cli plugins in bookworm container has slight issue to work under 202205. For example, sonic-clear macsec does not work. Solution is to copy all cli plugin scripts to docker using the 202205 cli scripts.

How to verify it

Few tests have been run to verify it works.

  1. Sonic OC macsec test suite has been run against the modified image. All tests passed.
  2. Static macsec test (chassis ---- axia ) with 400G port with PN unchanged. Test run overnight. No issue.
  3. Dynamic macsec test (chassis ---- chassis) with 400G port. Test run overnight. No issue.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

Tested branch (Please provide the tested image version)

FROM publicmirror.azurecr.io/docker-macsec:202405
RUN mkdir /etc/fips && echo 1 > /etc/fips/fips_enable
COPY ["cli", "/cli/"]
ENTRYPOINT ["/usr/local/bin/supervisord"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

@wumiaont I see you have put an if else with the "use_bookworm_macsec_container" set to true. shouldn't we copy the other files like start.sh,, critical_processes, wpa_supplicant.conf etc - this is done today with bullseye ?

Copy link
Author

Choose a reason for hiding this comment

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

The docker-macsec container (bookworm) already contains those files. There's no difference between 202205 and 202405 for those files so I did not try to copy and overwrite them. For example: in critical_processes file, it has these "program:macsecmgrd". Same between 2205 and 2405.

Copy link
Author

@wumiaont wumiaont Sep 5, 2024

Choose a reason for hiding this comment

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

wpa_supplicant.conf is same between 2205 and 2405 also. Other files such as start.sh has a very slight change. In 2205 it's only " #!/usr/bin/env bash" in 2405 it adds "
TZ=$(cat /etc/timezone)
rm -rf /etc/localtime
ln -sf /usr/share/zoneinfo/$TZ /etc/localtime". This is trying to resolve timezone issue with container found and resolved in 2405. Better to have this fix as fix is harmless.

Copy link
Author

Choose a reason for hiding this comment

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

Only files need to modify inside bookworm container is/cli/clear/plugins/clear_macsec_counter.py. Only one line change in this file between 2205 and 2405. This line in 2405 is "clicommon.run_command(['show', 'macsec', '--dump-file'])" and in 2205 is clicommon.run_command("show macsec --dump-file"). API format changed. CLI plugins are taken out of container and put into host. This makes original plugin inside bookworm CLI plugin(2405) not work in 2205 host environment because of above API format change. That's why I decide to copy all cli plugins (2205, 3 files) into bookworm container to replace 2405 cli plugins. CLI has been verified during testing. Also changes are very little for macsec CLI (very slight code change for bugs found)

@judyjoseph
Copy link
Collaborator

@xumia , @saiarcot895 could you review this change too

@wumiaont
Copy link
Author

wumiaont commented Sep 5, 2024

docker-bookworm container already contains all needed files so there's no need to copy them (which will be 202205 implementation). Any application running inside container should be 202405. Any application runs outside of container (on host which is 202205) should use 202205 implementations. That's why this PR only overwrite cli plugin files as those files are pulled out of container and run on host environment. It's found that original CLI files within bookworm container image (2405) has at least one issue as explained above. Thus overwrite CLI files to have 202205 implementation is the correct approach.

@saiarcot895
Copy link
Contributor

Is the 202405 macsec container the first one that can be FIPS certified and always run in FIPS-only mode? Can the 202311 macsec container do this?

If yes, I would recommend using the 202311 container, mainly so that there's a common Bullseye base.

@wumiaont
Copy link
Author

wumiaont commented Sep 6, 2024

Is the 202405 macsec container the first one that can be FIPS certified and always run in FIPS-only mode? Can the 202311 macsec container do this?

If yes, I would recommend using the 202311 container, mainly so that there's a common Bullseye base.

No. 202305 is openssl 1.1.1 which only supports symcrypt engine. Symcrypt engine does not provide support for CMAC. Without CMAC support macsec will not be fips complaint. 202405 uses openssl 3.0.11 to support symcrypt provider which does have CMAC support. 202405 will be the first version to apply FIPS certification for macsec. 202305 will have FIPS certification but it's without macsec.

Copy link
Contributor

@xumia xumia left a comment

Choose a reason for hiding this comment

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

LGTM

@BYGX-wcr
Copy link
Collaborator

BYGX-wcr commented Sep 8, 2024

Hi @lguohan , can you please merge this PR? We want it in our next T2 image.

@BYGX-wcr BYGX-wcr merged commit c8bc5c3 into sonic-net:202205 Sep 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants