Makefile,cmd/tidb-server: add tidb-server FIPS build target #47949

tiancaiamao · 2023-10-24T08:54:33Z

What problem does this PR solve?

Issue Number: close #47948

Problem Summary:

What is changed and how it works?

After googling, this blog post is what I found the most useful information on FIPS support of golang application.
According to it, if we use the boringssl way, there are only two changes needed:

build with GOEXPERIMENT=boringcrypto
configue the cipher of the TLS, choose FIPS compliance ones

For the configuration part, we can just import _ "crypto/tls/fipsonly" and use conditional build tag.

FYI: how envoy implement FIPS is another good material and worth reading.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

ENABLE_FIPS=1 make server
go tool nm bin/tidb-server |grep boring

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

 1d0bd60 t local.crypto/internal/boring._Cfunc__goboringcrypto_ECDSA_sign.abi0
 1d0bec0 t local.crypto/internal/boring._Cfunc__goboringcrypto_ECDSA_size.abi0
 1d0bf40 t local.crypto/internal/boring._Cfunc__goboringcrypto_ECDSA_verify.abi0
 1d1d6c0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_GROUP_free
 1d0c0a0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_GROUP_free.abi0
 1d0c120 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_GROUP_new_by_curve_name.abi0
 1d1d700 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_free
 1d0c1c0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_free.abi0
 1d0c240 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_generate_key_fips.abi0
 1d0c2c0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_get0_group.abi0
 1d0c340 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_get0_private_key.abi0
 1d0c3c0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_get0_public_key.abi0
 1d0c440 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_new_by_curve_name.abi0
 1d0c4e0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_set_private_key.abi0
 1d0c5a0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_KEY_set_public_key.abi0
 1d1d740 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_free
 1d0c660 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_free.abi0
 1d0c6e0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_get_affine_coordinates_GFp.abi0
 1d0c7e0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_mul.abi0
 1d0c900 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_new.abi0
 1d0c980 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_oct2point.abi0
 1d0caa0 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_point2oct.abi0
 1d0cc00 t local.crypto/internal/boring._Cfunc__goboringcrypto_EC_POINT_set_affine_coordinates_GFp.abi0
 1d0cd00 t local.crypto/internal/boring._Cfunc__goboringcrypto_EVP_AEAD_CTX_cleanup.abi0

I also add "-fips" to the tidb server version information:

./bin/tidb-server -V
Release Version: v7.6.0-alpha-4-g31ea0f1009-dirty-fips
...

No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

tiprow · 2023-10-24T08:55:06Z

Hi @tiancaiamao. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-10-24T09:03:50Z

Codecov Report

Merging #47949 (8c95e2c) into master (44f9430) will increase coverage by 2.0745%.
Report is 18 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #47949        +/-   ##
================================================
+ Coverage   71.3984%   73.4729%   +2.0745%     
================================================
  Files          1403       1430        +27     
  Lines        406971     420028     +13057     
================================================
+ Hits         290571     308607     +18036     
+ Misses        96445      92575      -3870     
+ Partials      19955      18846      -1109

Flag	Coverage Δ
integration	`43.8496% <ø> (?)`
unit	`72.0969% <ø> (+0.6984%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`53.9874% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`48.8021% <ø> (-4.2891%)`	⬇️

wuhuizuo · 2023-10-26T09:54:56Z

@tiancaiamao does it need some special steps to build or test in make tasks which will run in CI jobs?

tiancaiamao · 2023-10-26T10:06:48Z

@tiancaiamao does it need some special steps to build or test in make tasks which will run in CI jobs?

No, currently CI is not affected. If TLS is not used, this binary can be used just as a normal tidb binary.

But if TLS connection is used, or we want to add test cases in that situration, we need to adjust the CI pipeline. @wuhuizuo

CabinfeverB · 2023-10-26T12:45:33Z

Do we need to add the release note?

coderplay · 2023-10-27T00:09:30Z

cmd/tidb-server/fips.go

+import "github.com/pingcap/tidb/pkg/parser/mysql"
+
+func init() {
+	mysql.TiDBReleaseVersion += "-fips"


where is this variable used?

When output TiDB information, for example, use './bin/tidb-server -V '

./bin/tidb-server -V
Release Version: v7.6.0-alpha-4-g31ea0f1009-dirty-fips
...
@CabinfeverB

https://github.com/tiancaiamao/tidb/blob/server-fips/cmd/tidb-server/main.go#L877C18-L877C18
Used by './bin/tidb-server -V' to print the version information @coderplay

overvenus · 2023-11-10T08:27:49Z

Makefile

@@ -163,6 +163,14 @@ else
 	CGO_ENABLED=1 $(GOBUILD) $(RACE_FLAG) -ldflags '$(LDFLAGS) $(CHECK_FLAG)' -o '$(TARGET)' ./cmd/tidb-server
 endif

+server_fips:


How about enable FIPS build by an env variable? So that we do not need to add new targes "server_fips", "br_fips", "build_tools_fips", "build_dumpling_fips", etc.

ifeq ($(ENABLE_FIPS),1) GOBUILD := GOEXPERIMENT=boringcrypto CGO_ENABLED=1 $(GO) build $(BUILD_FLAG) -tags codes else GOBUILD := $(GO) build $(BUILD_FLAG) -tags codes endif

➜ make server CGO_ENABLED=1 GO111MODULE=on go build -tags codes -ldflags '-X "github.com/pingcap/tidb/parser/mysql.TiDBReleaseVersion=v6.5.5-dirty" -X "github.com/pingcap/tidb/util/versioninfo.TiDBBuildTS=2023-11-10 08:24:28" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitHash=71bcc44f77a37cfb0a6dc3660e092c78c1e46acb" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitBranch=HEAD" -X "github.com/pingcap/tidb/util/versioninfo.TiDBEdition=Community" ' -o bin/tidb-server ./tidb-server ➜ make server ENABLE_FIPS=1 CGO_ENABLED=1 GOEXPERIMENT=boringcrypto CGO_ENABLED=1 GO111MODULE=on go build -tags codes -ldflags '-X "github.com/pingcap/tidb/parser/mysql.TiDBReleaseVersion=v6.5.5-dirty" -X "github.com/pingcap/tidb/util/versioninfo.TiDBBuildTS=2023-11-10 08:24:47" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitHash=71bcc44f77a37cfb0a6dc3660e092c78c1e46acb" -X "github.com/pingcap/tidb/util/versioninfo.TiDBGitBranch=HEAD" -X "github.com/pingcap/tidb/util/versioninfo.TiDBEdition=Community" ' -o bin/tidb-server ./tidb-server

overvenus

LGTM

ti-chi-bot · 2023-11-15T06:57:36Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [overvenus]

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 · 2023-11-15T06:57:40Z

[LGTM Timeline notifier]

Timeline:

2023-11-10 13:27:53.586772168 +0000 UTC m=+3823671.173882308: ☑️ agreed by overvenus.
2023-11-15 06:57:39.117395727 +0000 UTC m=+4232256.704505873: ☑️ agreed by CabinfeverB.

ti-chi-bot · 2023-11-22T07:23:02Z

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

Signed-off-by: ti-chi-bot <[email protected]>

…48782) close #47948

Makefile,cmd/tidb-server: add tidb-server FIPS build target

f6d9e14

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 24, 2023

tiancaiamao requested a review from bb7133 October 24, 2023 08:54

add license header

12b971c

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 26, 2023

lidezhu mentioned this pull request Oct 26, 2023

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

Merged

coderplay reviewed Oct 27, 2023

View reviewed changes

Leavrth mentioned this pull request Nov 8, 2023

br: make br support FIPS #48421

Merged

13 tasks

overvenus reviewed Nov 10, 2023

View reviewed changes

tiancaiamao added 3 commits November 10, 2023 19:17

Merge branch 'master' into server-fips

108a724

instead of introducing server-fips target, use ENABLE_FIPS=1

3b124df

remove useless server_fips

8c95e2c

overvenus approved these changes Nov 10, 2023

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 10, 2023

CabinfeverB approved these changes Nov 15, 2023

View reviewed changes

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

ti-chi-bot bot merged commit 1bb8b6f into pingcap:master Nov 15, 2023
14 of 16 checks passed

tiancaiamao deleted the server-fips branch November 15, 2023 08:05

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 mentioned this pull request Nov 22, 2023

Makefile,cmd/tidb-server: add tidb-server FIPS build target (#47949) | tidb-test=pr/2255 #48782

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Nov 22, 2023

This is an automated cherry-pick of pingcap#47949

b1ffb3b

Signed-off-by: ti-chi-bot <[email protected]>

This was referenced Nov 22, 2023

Makefile(ticdc): support build cdc in fips mode (#9961) pingcap/tiflow#10131

Merged

br: make br support FIPS (#48421) | tidb-test=pr/2255 #48786

Merged

mornyx mentioned this pull request Nov 23, 2023

*: add fips build support pingcap/ng-monitoring#201

Merged

overvenus mentioned this pull request Dec 11, 2023

*: add fips build support (#201) pingcap/ng-monitoring#216

Merged

ti-chi-bot bot pushed a commit that referenced this pull request Dec 13, 2023

Makefile,cmd/tidb-server: add tidb-server FIPS build target (#47949) (#…

bfbcedf

…48782) close #47948

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile,cmd/tidb-server: add tidb-server FIPS build target #47949

Makefile,cmd/tidb-server: add tidb-server FIPS build target #47949

tiancaiamao commented Oct 24, 2023 •

edited

Loading

tiprow bot commented Oct 24, 2023

codecov bot commented Oct 24, 2023 •

edited

Loading

wuhuizuo commented Oct 26, 2023 •

edited

Loading

tiancaiamao commented Oct 26, 2023

CabinfeverB commented Oct 26, 2023

coderplay Oct 27, 2023

CabinfeverB Oct 27, 2023

tiancaiamao Oct 31, 2023

tiancaiamao Oct 31, 2023

overvenus Nov 10, 2023

overvenus left a comment

ti-chi-bot bot commented Nov 15, 2023

ti-chi-bot bot commented Nov 15, 2023

ti-chi-bot commented Nov 22, 2023

Makefile,cmd/tidb-server: add tidb-server FIPS build target #47949

Makefile,cmd/tidb-server: add tidb-server FIPS build target #47949

Conversation

tiancaiamao commented Oct 24, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

tiprow bot commented Oct 24, 2023

codecov bot commented Oct 24, 2023 • edited Loading

Codecov Report

wuhuizuo commented Oct 26, 2023 • edited Loading

tiancaiamao commented Oct 26, 2023

CabinfeverB commented Oct 26, 2023

coderplay Oct 27, 2023

Choose a reason for hiding this comment

CabinfeverB Oct 27, 2023

Choose a reason for hiding this comment

tiancaiamao Oct 31, 2023

Choose a reason for hiding this comment

tiancaiamao Oct 31, 2023

Choose a reason for hiding this comment

overvenus Nov 10, 2023

Choose a reason for hiding this comment

overvenus left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 15, 2023

ti-chi-bot bot commented Nov 15, 2023

[LGTM Timeline notifier]

ti-chi-bot commented Nov 22, 2023

tiancaiamao commented Oct 24, 2023 •

edited

Loading

codecov bot commented Oct 24, 2023 •

edited

Loading

wuhuizuo commented Oct 26, 2023 •

edited

Loading