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

Makefile(ticdc): support build cdc in fips mode #9961

Merged
merged 4 commits into from
Nov 15, 2023

Conversation

lidezhu
Copy link
Collaborator

@lidezhu lidezhu commented Oct 26, 2023

What problem does this PR solve?

Issue Number: close #9962

What is changed and how it works?

  1. add support to build FIPS-ready cdc binary following the info shown in Makefile,cmd/tidb-server: add tidb-server FIPS build target tidb#47949
  2. add -fips in the release version of FIPS-ready binary
    image
  3. support sanitize version with -fips suffix when do version check

Check List

Tests

  • Unit test
    Make sure the version sanitize process works with -fips suffix;
  • Manual test (add detailed scripts or steps below)
ENABLE_FIPS=1 make cdc
go tool nm bin/cdc | grep boring

Check the boringcrypto linked via cgo, some function names like:

 6d3aa60 t local.crypto/internal/boring._Cfunc__goboringcrypto_AES_cbc_encrypt.abi0
 6d3abc0 t local.crypto/internal/boring._Cfunc__goboringcrypto_AES_ctr128_encrypt.abi0
 6d3ad20 t local.crypto/internal/boring._Cfunc__goboringcrypto_AES_decrypt.abi0
 6d3ade0 t local.crypto/internal/boring._Cfunc__goboringcrypto_AES_encrypt.abi0
 6d3aea0 t local.crypto/internal/boring._Cfunc__goboringcrypto_AES_set_decrypt_key.abi0
 6d3af80 t local.crypto/internal/boring._Cfunc__goboringcrypto_AES_set_encrypt_key.abi0
 6d3b060 t local.crypto/internal/boring._Cfunc__goboringcrypto_BN_bin2bn.abi0
 6d3b140 t local.crypto/internal/boring._Cfunc__goboringcrypto_BN_bn2bin_padded.abi0
 6d3b220 t local.crypto/internal/boring._Cfunc__goboringcrypto_BN_bn2le_padded.abi0
 6d4cec0 t local.crypto/internal/boring._Cfunc__goboringcrypto_BN_free
 6d3b300 t local.crypto/internal/boring._Cfunc__goboringcrypto_BN_free.abi0
 6d3b380 t local.crypto/internal/boring._Cfunc__goboringcrypto_BN_le2bn.abi0
 6d3b460 t local.crypto/internal/boring._Cfunc__goboringcrypto_BN_new.abi0
 6d3b4c0 t local.crypto/internal/boring._Cfunc__goboringcrypto_BN_num_bytes.abi0
 6d3b540 t local.crypto/internal/boring._Cfunc__goboringcrypto_BORINGSSL_bcm_power_on_self_test.abi0
 6d3b5a0 t local.crypto/internal/boring._Cfunc__goboringcrypto_ECDSA_sign.abi0
 6d3b700 t local.crypto/internal/boring._Cfunc__goboringcrypto_ECDSA_size.abi0
 6d3b780 t local.crypto/internal/boring._Cfunc__goboringcrypto_ECDSA_verify.abi0
 6d4cf00 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_GROUP_free
 6d3b8e0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_GROUP_free.abi0
 6d3b960 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_GROUP_new_by_curve_name.abi0
 6d4cf40 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_free
 6d3ba00 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_free.abi0
 6d3ba80 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_generate_key_fips.abi0
 6d3bb00 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_get0_group.abi0
 6d3bb80 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_get0_private_key.abi0
 6d3bc00 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_get0_public_key.abi0
 6d3bc80 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_new_by_curve_name.abi0
 6d3bd20 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_set_private_key.abi0
 6d3bde0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_set_public_key.abi0
 6d4cf80 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_free
 6d3bea0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_free.abi0

Questions

Will it cause performance regression or break compatibility?

The original binary should not be affected. But the binary build with FIPS support is expected to be slower.

Do you need to update user documentation, design documentation or monitoring documentation?

Need to add contents about the support for FIPS-ready binary and how to download it later;

Release note

None

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 26, 2023
@sre-bot
Copy link

sre-bot commented Oct 26, 2023

CLA assistant check
All committers have signed the CLA.

@ti-chi-bot ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 26, 2023
@lidezhu lidezhu changed the title makefile(ticdc): support build cdc in fips mode Makefile(ticdc): support build cdc in fips mode Oct 26, 2023
@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 26, 2023
@CabinfeverB
Copy link

I found 1000825e0 T crypto/internal/boring/sig.StandardCrypto.abi0 in the go tool nm output? It seems unexpected.

@lidezhu
Copy link
Collaborator Author

lidezhu commented Oct 27, 2023

I found 1000825e0 T crypto/internal/boring/sig.StandardCrypto.abi0 in the go tool nm output? It seems unexpected.

The ouput have been updated. The original output is from mac env(tidb binary build with fips mode have similar outupt on mac env).
The current output is taken from linux. And now it doesn't contain any output like StandardCrypto.abi0.

@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 27, 2023
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 13, 2023
Copy link
Contributor

ti-chi-bot bot commented Nov 15, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: overvenus, sdojjy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 15, 2023
Copy link
Contributor

ti-chi-bot bot commented Nov 15, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-11-13 08:21:55.474359187 +0000 UTC m=+4064513.061469333: ☑️ agreed by overvenus.
  • 2023-11-15 07:21:03.82978814 +0000 UTC m=+4233661.416898286: ☑️ agreed by sdojjy.

@lidezhu
Copy link
Collaborator Author

lidezhu commented Nov 15, 2023

/retest

@ti-chi-bot ti-chi-bot bot merged commit 120815e into pingcap:master Nov 15, 2023
14 checks passed
@lidezhu lidezhu deleted the ldz/support-fips branch November 15, 2023 08:32
@overvenus overvenus added the needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. label Nov 22, 2023
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created to branch release-6.5: #10131.

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Nov 22, 2023
@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. lgtm needs-cherry-pick-release-6.5 Should cherry pick this PR to release-6.5 branch. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support to build FIPS-ready cdc binary
6 participants