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 host names in TLS certificates #948

Merged
merged 2 commits into from
Dec 1, 2020

Conversation

fln
Copy link
Contributor

@fln fln commented Nov 30, 2020

What problem does this PR solve?

This PR updates TLS certificate generator to support issuing certificates for host names, not only IP addresses.

This contributes towards fixing #337.

What is changed and how it works?

If host name is detected field DNSNames of x509 SAN extenstion is used instead of IPAddresses.

Check List

Tests

Integration tests that cover this functionality is available in #950.

Release notes:

NONE

@codecov-io
Copy link

codecov-io commented Nov 30, 2020

Codecov Report

Merging #948 (acc0b47) into master (214441a) will decrease coverage by 3.79%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
- Coverage   55.86%   52.07%   -3.80%     
==========================================
  Files         263      263              
  Lines       19509    19513       +4     
==========================================
- Hits        10899    10161     -738     
- Misses       6882     7711     +829     
+ Partials     1728     1641      -87     
Flag Coverage Δ
cluster 38.18% <100.00%> (-5.19%) ⬇️
dm 24.35% <0.00%> (-0.01%) ⬇️
integrate 46.26% <100.00%> (-3.81%) ⬇️
playground 20.28% <ø> (ø)
tiup 16.80% <ø> (ø)
unittest 23.06% <0.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
pkg/cluster/task/tls.go 65.38% <100.00%> (+2.88%) ⬆️
components/cluster/command/check.go 6.37% <0.00%> (-72.82%) ⬇️
pkg/cluster/task/limits.go 0.00% <0.00%> (-68.75%) ⬇️
pkg/cluster/task/sysctl.go 0.00% <0.00%> (-66.67%) ⬇️
components/cluster/command/audit.go 27.27% <0.00%> (-54.55%) ⬇️
pkg/cluster/operation/check.go 0.00% <0.00%> (-51.95%) ⬇️
pkg/cluster/task/rmdir.go 0.00% <0.00%> (-50.00%) ⬇️
pkg/cluster/operation/operation.go 34.78% <0.00%> (-43.48%) ⬇️
components/cluster/command/rename.go 25.00% <0.00%> (-41.67%) ⬇️
components/cluster/command/stop.go 38.46% <0.00%> (-30.77%) ⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 214441a...acc0b47. Read the comment docs.

@lucklove
Copy link
Member

/lgtm

@ti-chi-bot ti-chi-bot added status/LGT1 Indicates that a PR has LGTM 1. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 30, 2020
@fln fln force-pushed the support_hosts_in_tls_certs branch from c36984d to 998b012 Compare November 30, 2020 12:16
This commit updates TLS certificate generator to detect if IP address or
host name was used as host value. If host name is detected field `DNSNames`
of x509 SAN extenstion is used instead of `IPAddresses`.

* https://en.wikipedia.org/wiki/Subject_Alternative_Name
* https://tools.ietf.org/html/rfc5280#section-4.2.1.6

This contributes towards fixing pingcap#337.
@fln fln force-pushed the support_hosts_in_tls_certs branch from 998b012 to 2006b2e Compare November 30, 2020 21:23
@lucklove
Copy link
Member

lucklove commented Dec 1, 2020

/merge

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 1, 2020
@ti-chi-bot
Copy link
Member

Can merge label has been added.

Git tree hash: acc0b47

@ti-chi-bot ti-chi-bot merged commit 5296e8f into pingcap:master Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time-contributor size/S Denotes a PR that changes 10-29 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants