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

etcdmain: added peer-client-{client,key}-file parameters for supporting separate client and server certs when communicating between peers #12705

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

astromechza
Copy link
Contributor

@astromechza astromechza commented Feb 20, 2021

Due to some certificate authorities not issuing certificates with both client and server usage modes. We needed a way of providing explicit override certs for the server side.

The api is a little awkard, but aims to not break compatibility. Any alternative suggestions are welcomed.

Copy link
Contributor

@ptabor ptabor left a comment

Choose a reason for hiding this comment

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

Could you, please, add a test that checks the new setup with separate peer & server certs ?

@codecov-io
Copy link

codecov-io commented Feb 28, 2021

Codecov Report

Merging #12705 (3d44f5b) into master (d06d93d) will decrease coverage by 13.81%.
The diff coverage is 52.17%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #12705       +/-   ##
===========================================
- Coverage   72.74%   58.93%   -13.82%     
===========================================
  Files         422      422               
  Lines       33018    33035       +17     
===========================================
- Hits        24019    19469     -4550     
- Misses       7081    11780     +4699     
+ Partials     1918     1786      -132     
Flag Coverage Δ
all 58.93% <52.17%> (-13.82%) ⬇️

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

Impacted Files Coverage Δ
pkg/transport/listener.go 48.73% <35.29%> (-0.16%) ⬇️
server/embed/config.go 67.34% <100.00%> (+0.22%) ⬆️
server/etcdmain/config.go 82.32% <100.00%> (+0.31%) ⬆️
client/v2/cancelreq.go 0.00% <0.00%> (-100.00%) ⬇️
client/v3/ordering/util.go 0.00% <0.00%> (-100.00%) ⬇️
client/v3/naming/endpoints/endpoints.go 0.00% <0.00%> (-100.00%) ⬇️
...ver/proxy/grpcproxy/adapter/lock_client_adapter.go 0.00% <0.00%> (-100.00%) ⬇️
client/v3/leasing/util.go 0.00% <0.00%> (-98.04%) ⬇️
...er/proxy/grpcproxy/adapter/watch_client_adapter.go 7.14% <0.00%> (-92.86%) ⬇️
client/v2/keys.go 0.00% <0.00%> (-92.05%) ⬇️
... and 141 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 d06d93d...3d44f5b. Read the comment docs.

Copy link
Contributor

@ptabor ptabor 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.

Please fix the formatting nit:

'gofmt' started at Sun Feb 28 11:08:07 UTC 2021
integration/cluster.go
diff -u integration/cluster.go.orig integration/cluster.go
--- integration/cluster.go.orig	2021-02-28 11:08:11.164866774 +0000
+++ integration/cluster.go	2021-02-28 11:08:11.164866774 +0000
@@ -90,7 +90,7 @@
 	testTLSInfoWithSpecificUsage = transport.TLSInfo{
 		KeyFile:        "../fixtures/server-serverusage.key.insecure",
 		CertFile:       "../fixtures/server-serverusage.crt",
-		ClientKeyFile: "../fixtures/client-clientusage.key.insecure",
+		ClientKeyFile:  "../fixtures/client-clientusage.key.insecure",
 		ClientCertFile: "../fixtures/client-clientusage.crt",
 		TrustedCAFile:  "../fixtures/ca.crt",
 		ClientCertAuth: true,

./scripts/fix.sh should do the trick.

pkg/transport/listener.go Outdated Show resolved Hide resolved
@astromechza astromechza changed the title etcdmain: added peer-server-{client,key}-file parameters for separating peer client and server certs etcdmain: added peer-client-{client,key}-file parameters for supporting separate client and server certs when communicating between peers Feb 28, 2021
@astromechza
Copy link
Contributor Author

Thanks @ptabor, sorry for the delay here, the e2e tests are a bit flakey on my machine so it took a bit to be confident in the change. Certs are working well now.

…client and server certs when communicating between peers

In some environments, the CA is not able to sign certificates with both
'client auth' and 'server auth' extended usage parameters and so an operator
needs to be able to set a seperate client certificate to use when making
requests which is different to the certificate used for accepting requests.
This applies to both proxy and etcd member mode and is available as both a CLI
 flag and config file field for peer TLS.

Signed-off-by: Ben Meier <ben.meier@oracle.com>
@astromechza
Copy link
Contributor Author

A further adjustment added to make it clear this works for proxy mode as well.

@astromechza
Copy link
Contributor Author

Great. all checks passing. @ptabor can you merge or do you know who else can take a look? @gyuho?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants