security: support multiple CN for TLS connections #8518

lhy1024 · 2024-08-12T16:01:25Z

What problem does this PR solve?

Issue Number: Close #5134

What is changed and how does it work?

It can be reviewed after #8441 is merged.

Check List

Tests

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

test in tiup with multi cn

without this PR
with this PR

Release note

security: support multiple CN for TLS connections

ti-chi-bot · 2024-08-12T16:01:28Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

lhy1024 · 2024-08-12T16:01:56Z

server/config/config.go

 	cfg.ClientTLSInfo.ClientCertAuth = len(c.Security.CAPath) != 0
 	cfg.ClientTLSInfo.TrustedCAFile = c.Security.CAPath
 	cfg.ClientTLSInfo.CertFile = c.Security.CertPath
 	cfg.ClientTLSInfo.KeyFile = c.Security.KeyPath
-	// Client no need to set the CN. (cfg.ClientTLSInfo.AllowedCN = allowedCN)
+	// Keep compatibility with https://github.com/tikv/pd/pull/2305


Is it necessary?

codecov · 2024-08-12T18:15:44Z

Codecov Report

Attention: Patch coverage is 68.75000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 77.64%. Comparing base (571c59b) to head (26a86dc).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8518      +/-   ##
==========================================
- Coverage   77.66%   77.64%   -0.02%     
==========================================
  Files         474      474              
  Lines       61884    61851      -33     
==========================================
- Hits        48061    48026      -35     
- Misses      10277    10301      +24     
+ Partials     3546     3524      -22

Flag	Coverage Δ
unittests	`77.64% <68.75%> (-0.02%)`	⬇️

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

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 · 2024-08-29T05:15:08Z

Manual test

script

tiup cluster deploy test nightly /home/lhy1024/t.yml
tiup cluster start test --init
tiup cluster patch test /home/lhy1024/pd/bin/pd.tar.gz -R pd --overwrite
tiup cluster tls test enable --wait-timeout 3600
tiup cluster edit-config test # add multi cn
tiup cluster reload test
tiup cluster scale-out test scale-out.yaml
tiup cluster display test
mysql --comments --host 192.168.8.88 --port 4000 -u root --ssl-verify-server-cert=false -p
sysbench oltp_insert --mysql-host=192.168.8.88  --mysql-port=4000 --mysql-user=root  --mysql-password="" --tables=32 --report-interval=1 --events=0 --time=0 --threads=32 prepare
tiup ctl:v8.2.0 pd -u https://192.168.8.88:2379 --cacert="/home/tidb/.tiup/storage/cluster/clusters/test/tls/ca.crt" --key="/home/tidb/.tiup/storage/cluster/clusters/test/tls/client.pem" --cert="/home/tidb/.tiup/storage/cluster/clusters/test/tls/client.crt" member

t.yml

global:
  user: "tidb"
  ssh_port: 22
  deploy_dir: "/tidb-deploy"
  data_dir: "/tidb-data"
  arch: "amd64"
monitored:
  node_exporter_port: 9100
  blackbox_exporter_port: 9115
pd_servers:
  - host: 192.168.8.88
tidb_servers:
  - host: 192.168.8.88
    port: 4000
    status_port: 10080
    deploy_dir: "/tidb-deploy/tidb-4000"
    log_dir: "/tidb-deploy/tidb-4000/log"
tikv_servers:
  - host: 192.168.8.88
    port: 20160
    status_port: 20180
    deploy_dir: "/data1/tidb-deploy/tikv-20160"
    data_dir: "/data1/tidb-data/tikv-20160"
    log_dir: "/data1/tidb-deploy/tikv-20160/log"
monitoring_servers:
  - host: 192.168.8.88
grafana_servers:
  - host: 192.168.8.88
alertmanager_servers:
  - host: 192.168.8.88

scale-out.yml

pd_servers:
  - host: 192.168.8.88
    ssh_port: 22
    name: pd-1
    client_port: 3379
    peer_port: 3380
    deploy_dir: /tidb-deploy/pd-3379
    data_dir: /tidb-data/pd-3379
    log_dir: /tidb-deploy/pd-3379/log
  - host: 192.168.8.88
    ssh_port: 22
    name: pd-2
    client_port: 4379
    peer_port: 4380
    deploy_dir: /tidb-deploy/pd-4379
    data_dir: /tidb-data/pd-4379
    log_dir: /tidb-deploy/pd-4379/log

config file

Note: we need add test-client because of https://github.com/pingcap/tiup/blob/a16d04fccb22106a5c8b2d7d074b7ab94a1dd677/pkg/cluster/manager/cacert.go#L65

result

tiup display:

sysbench test:

pd ctl:

client/tlsutil/tlsconfig.go

Signed-off-by: lhy1024 <admin@liudos.us>

HuSharp · 2024-09-03T07:07:40Z

client/tlsutil/tlsconfig.go

-	// CertAllowedCN is a CN which must be provided by a client
-	CertAllowedCN []string `toml:"cert-allowed-cn" json:"cert-allowed-cn"`
+	// CertAllowedCNs is the list of CN which must be provided by a client
+	CertAllowedCNs []string `toml:"cert-allowed-cn" json:"cert-allowed-cn"`


This parameter doesn't be used. It seems like we can remove this parameter directly, maybe we can discuss it in another issue.

HuSharp

plz help to update doc as well :)

ti-chi-bot · 2024-09-04T01:59:21Z

[LGTM Timeline notifier]

Timeline:

2024-09-03 04:20:49.800742816 +0000 UTC m=+331774.318795738: ☑️ agreed by rleungx.
2024-09-04 01:59:20.49493746 +0000 UTC m=+409685.012990385: ☑️ agreed by HuSharp.

ti-chi-bot · 2024-09-04T02:32:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HuSharp, niubell, rleungx

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~~ [HuSharp,rleungx]
~~client/tlsutil/OWNERS~~ [niubell]
~~server/config/OWNERS~~ [niubell]

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

lhy1024 · 2024-10-09T14:07:40Z

Manual test

script

t.yml

global:
  user: "tidb"
  ssh_port: 22
  deploy_dir: "/tidb-deploy"
  data_dir: "/tidb-data"
  arch: "amd64"
server_configs:
  pd:
    replication.enable-placement-rules: true
    replication.location-labels:
      - host
monitored:
  node_exporter_port: 9100
  blackbox_exporter_port: 9115
pd_servers:
  - host: 192.168.8.88
tidb_servers:
  - host: 192.168.8.88
tikv_servers:
  - host: 192.168.8.88
    port: 20160
    status_port: 20180
    config:
      server.labels: { host: "local" }
  - host: 192.168.8.88
    port: 20161
    status_port: 20181
    config:
      server.labels: { host: "local" }
  - host: 192.168.8.88
    port: 20162
    status_port: 20182
    config:
      server.labels: { host: "local" }
tiflash_servers:
  - host: 192.168.8.88
    tcp_port: 9000
    http_port: 8123
    flash_service_port: 3930
    flash_proxy_port: 20170
    flash_proxy_status_port: 20292
    metrics_port: 8234
    config:
      server.labels: { host: "local" }
  - host: 192.168.8.88
    tcp_port: 9001
    http_port: 8124
    flash_service_port: 3931
    flash_proxy_port: 20171
    flash_proxy_status_port: 20293
    metrics_port: 8235
    config:
      server.labels: { host: "local" }
monitoring_servers:
  - host: 192.168.8.88
grafana_servers:
  - host: 192.168.8.88
alertmanager_servers:
  - host: 192.168.8.88

config file

Note: we need add test-client because of https://github.com/pingcap/tiup/blob/a16d04fccb22106a5c8b2d7d074b7ab94a1dd677/pkg/cluster/manager/cacert.go#L65

result

tiup display:

sysbench test:

tlflash test:

grafana:

lhy1024 commented Aug 12, 2024

View reviewed changes

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2024

lhy1024 changed the title ~~security: support multiple CN support for TLS connections~~ security: support multiple CN for TLS connections Aug 12, 2024

lhy1024 force-pushed the multi-cn branch from 50ad74c to 026049f Compare August 12, 2024 17:59

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2024

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2024

lhy1024 added 2 commits August 15, 2024 13:59

security: support multiple CN support for TLS connections

ec25c00

Signed-off-by: lhy1024 <admin@liudos.us>

refactor test

44c0b7e

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 force-pushed the multi-cn branch from 026049f to 44c0b7e Compare August 15, 2024 05:59

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2024

lhy1024 marked this pull request as ready for review August 15, 2024 05:59

This was referenced Aug 16, 2024

After configuring multi cn, tidb shows down pingcap/tidb#55372

Closed

Multiple CN support for TLS connections #5134

Closed

lhy1024 force-pushed the multi-cn branch from b002475 to 8a5bc25 Compare August 16, 2024 04:27

fix some comments

b346fa4

Signed-off-by: lhy1024 <admin@liudos.us>

lhy1024 force-pushed the multi-cn branch from 8a5bc25 to b346fa4 Compare August 16, 2024 04:28

rleungx reviewed Sep 3, 2024

View reviewed changes

client/tlsutil/tlsconfig.go Outdated Show resolved Hide resolved

lhy1024 added 2 commits September 3, 2024 11:49

address comments

b20cdfa

Signed-off-by: lhy1024 <admin@liudos.us>

unexport tlsInfo

30dd994

Signed-off-by: lhy1024 <admin@liudos.us>

ti-chi-bot bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 3, 2024

ti-chi-bot bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 3, 2024

rleungx approved these changes Sep 3, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Sep 3, 2024

HuSharp reviewed Sep 3, 2024

View reviewed changes

HuSharp approved these changes Sep 4, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 4, 2024

Merge branch 'master' into multi-cn

26a86dc

niubell approved these changes Sep 4, 2024

View reviewed changes

ti-chi-bot bot added approved 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 Sep 4, 2024

ti-chi-bot bot merged commit 52a53c9 into tikv:master Sep 4, 2024
23 of 25 checks passed

lhy1024 deleted the multi-cn branch October 9, 2024 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security: support multiple CN for TLS connections #8518

security: support multiple CN for TLS connections #8518

lhy1024 commented Aug 12, 2024 •

edited

Loading

ti-chi-bot bot commented Aug 12, 2024

lhy1024 Aug 12, 2024

codecov bot commented Aug 12, 2024 •

edited

Loading

lhy1024 commented Aug 29, 2024 •

edited

Loading

HuSharp Sep 3, 2024

HuSharp left a comment

ti-chi-bot bot commented Sep 4, 2024

ti-chi-bot bot commented Sep 4, 2024

lhy1024 commented Oct 9, 2024

security: support multiple CN for TLS connections #8518

security: support multiple CN for TLS connections #8518

Conversation

lhy1024 commented Aug 12, 2024 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Aug 12, 2024

lhy1024 Aug 12, 2024

Choose a reason for hiding this comment

codecov bot commented Aug 12, 2024 • edited Loading

Codecov Report

lhy1024 commented Aug 29, 2024 • edited Loading

script

config file

result

HuSharp Sep 3, 2024

Choose a reason for hiding this comment

HuSharp left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 4, 2024

[LGTM Timeline notifier]

ti-chi-bot bot commented Sep 4, 2024

lhy1024 commented Oct 9, 2024

script

config file

result

lhy1024 commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

lhy1024 commented Aug 29, 2024 •

edited

Loading