*: disable CommonName auth for gRPC-gateway #10366

hexfusion · 2019-01-02T22:52:07Z

When client-cert-auth is enabled gRPC-gateway proxy request to etcd server will use the CN in client server cert if populated. We should not use this for authentication. In general gRPC-gateway does not support CN authentication and we also should document as such.

The check provided in this PR is based on headers send from gRPC-Gateway

https://github.com/grpc-ecosystem/grpc-gateway/blob/8976e602c518589cb6d40aca52a7dd8aef83706a/runtime/context.go#L183#

/cc @gyuho @mitake

auth/store.go

gyuho · 2019-01-02T23:20:56Z

SerialNumber is unique by CA

It's still possible to set the same serial number by providing the template? https://godoc.org/crypto/x509#Certificate

not want to use the CommonName in this client server certificate

Would this affect peer cert validation? We added to 3.3 that we can validate joining peer certs by its common name when etcd --peer-cert-allowed-cn flag is specified.

gyuho · 2019-01-02T23:21:45Z

auth/store.go

 			continue
 		}
 		ai = &AuthInfo{
-			Username: chains[0].Subject.CommonName,


@mitake I remember we had conversation about this chains[0], but can't remember what the reasoning was :0

@gyuho I'll share the conversation after finding it, it is very subtle so I'm also not fully sure about the reasoning...

hexfusion · 2019-01-02T23:32:40Z

It's still possible to set the same serial number by providing the template? https://godoc.org/crypto/x509#Certificate

I will add testing around this to see if it is possible to alter this on the client side. Might need to use finger print or other option if so.

Would this affect peer cert validation? We added to 3.3 that we can validate joining peer certs by its common name when etcd --peer-cert-allowed-cn flag is specified.

This logic should not effect the peer certs as the check is only on client server certs --key-file --cert-file. But I will add tests around this to verify.

mitake · 2019-01-03T08:31:38Z

@hexfusion this change will let etcd return errors to clients of gateway even if they provides valid certificate. I think it introduces confusing and hard to diagnose situation (e.g. users might think "my clients must have valid certificate but etcd returns errors"). Letting etcd rise errors as fast as possible would be more user friendly. How do you think? @gyuho @xiang90 @jpbetz

Probably returning an error at the handler layer will be helpful like this: 28265e7

hexfusion · 2019-01-03T14:33:38Z

@mitake thank you for the note and I agree my initial approach was too broad. gRPC-gateway should not include CN in request. Although CN can be included from client request there is no way to forward this CN in a safe way. For example we could include it in ctx metadata but this feels poor solution to support. Since we know when GW makes request and this request is code generated with identifiable headers. We can unset it's CN value only then. This solution in my opinion will allow users to continue with etcd usage as expected for etcd server. gRPC requests will not be directly effected. We can then add note that CN auth is not supported for gRPC-gateway. This also allows users to use authentication with gRPC-gateway while keeping client-cert-auth support for gRPC.

I think this along with your idea of allowing gRPC-gateway to be disabled all together will be a good solution for folks. Do you agree?

embed/serve.go

codecov-io · 2019-01-03T18:10:05Z

Codecov Report

Merging #10366 into master will increase coverage by 0.17%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master   #10366      +/-   ##
==========================================
+ Coverage   71.68%   71.85%   +0.17%     
==========================================
  Files         392      392              
  Lines       36421    36664     +243     
==========================================
+ Hits        26107    26345     +238     
+ Misses       8495     8491       -4     
- Partials     1819     1828       +9

Impacted Files	Coverage Δ
auth/store.go	`74.67% <28.57%> (-0.86%)`	⬇️
clientv3/concurrency/election.go	`79.68% <0%> (-2.35%)`	⬇️
etcdserver/api/rafthttp/msgappv2_codec.go	`69.56% <0%> (-1.74%)`	⬇️
etcdserver/api/v3rpc/watch.go	`82.67% <0%> (-1.64%)`	⬇️
etcdctl/ctlv3/command/printer_simple.go	`72.48% <0%> (-1.35%)`	⬇️
clientv3/retry_interceptor.go	`65.46% <0%> (-0.33%)`	⬇️
etcdserver/api/v3rpc/lease.go	`69.31% <0%> (ø)`	⬆️
proxy/grpcproxy/watcher.go	`89.79% <0%> (ø)`	⬆️
etcdserver/server.go	`74.82% <0%> (+0.28%)`	⬆️
etcdserver/v3_server.go	`79.4% <0%> (+0.45%)`	⬆️
... and 15 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 8945fec...5e92ca0. Read the comment docs.

hexfusion · 2019-01-03T18:52:46Z

I will improve the tests and would like to break out auth to its own file for curlv3 tests but this I believe covers the issue for now.

auth/store.go

mitake · 2019-01-05T13:36:25Z

LGTM

hexfusion · 2019-01-07T18:57:36Z

@gyuho PTAL, looks like we should backport this to 3.2 as well?

gyuho · 2019-01-07T21:08:29Z

auth/store.go

+	// header. The proxy uses etcd client server certificate. If the certificate
+	// has a CommonName we should never use this for authentication.
+	if gw := md["grpcgateway-accept"]; len(gw) > 0 {
+		return nil


Add a warning as.lg.Warn as @mitake suggested?

Signed-off-by: Sam Batschelet <[email protected]>

gyuho

lgtm thanks @hexfusion !

hexfusion · 2019-01-08T17:34:05Z

@gyuho @mitake thank you for reviews and input.

nelljerram · 2019-08-06T09:09:23Z

@hexfusion I have just come across this change, in an upgrade from etcd 3.3.10 to 3.3.13.

You wrote above:

This also allows users to use authentication with gRPC-gateway while keeping client-cert-auth support for gRPC

How should a gRPC-gateway client authenticate now, instead of by using the Common Name field? (I tried looking in the etcd docs for this, but could not find it.)

hexfusion · 2019-08-06T12:45:26Z

Example of auth:

https://github.com/etcd-io/etcd/blob/master/Documentation/dev-guide/api_grpc_gateway.md#authentication

nelljerram · 2019-10-30T10:43:43Z

@hexfusion @gyuho I'm sorry for adding again to this merged PR thread, but I do not yet understand what the security issue was that motivated this change, and I would be most grateful if you could help me understand that.

The CVE (https://nvd.nist.gov/vuln/detail/CVE-2018-16886) says:

If an etcd client server TLS certificate contains a Common Name (CN) which matches a valid RBAC username, a remote attacker may authenticate as that user with any valid (trusted) client certificate in a REST API request to the gRPC-gateway.

Why is that a problem? The certificate still has to be signed by a CA that the etcd server trusts. Doesn't that mean that the etcd server can trust the CN ?

In the PR comments above, you write:

gRPC-gateway should not include CN in request.

Please can you explain more? Does this refer to the gRPC request that the gateway builds, and that is equivalent to the received HTTP/JSON request? Why should that not include the CN?

Although CN can be included from client request there is no way to forward this CN in a safe way.

Can you say more about the safety issue here? If I understand correctly, the gateway is in the same process (etcd) as the gRPC server, so there is not even a network hop here; so what could be unsafe?

DifferentialOrange · 2024-06-21T08:02:02Z

Sending an error with warning-like message "CommonName of client sending a request against gateway will be ignored" sounds as the opposite of user friendliness for me as a user. Common Name is not being ignored, the request is fully rejected if I have a Common Name in my certificate. And I don't get why having a Common Name is such a crime. (To be honest, I also don't get why username is not preferred to a Common Name if username/password explicitly provided in a client request.)

hexfusion added the WIP label Jan 2, 2019

gyuho reviewed Jan 2, 2019

View reviewed changes

auth/store.go Outdated Show resolved Hide resolved

gyuho reviewed Jan 2, 2019

View reviewed changes

hexfusion force-pushed the fx_cn branch 2 times, most recently from c0ecbd4 to 8b9c450 Compare January 3, 2019 00:03

hexfusion force-pushed the fx_cn branch from 30e8026 to 9d1885c Compare January 3, 2019 14:18

hexfusion changed the title *: unset client server CommonName if client-cert-auth is enabled *: unset CommonName for gRPC-gateway request Jan 3, 2019

hexfusion force-pushed the fx_cn branch from 9d1885c to 5730d30 Compare January 3, 2019 14:49

mitake reviewed Jan 3, 2019

View reviewed changes

embed/serve.go Outdated Show resolved Hide resolved

hexfusion force-pushed the fx_cn branch from f88690e to 6f4693c Compare January 3, 2019 19:13

gyuho reviewed Jan 4, 2019

View reviewed changes

auth/store.go Outdated Show resolved Hide resolved

hexfusion force-pushed the fx_cn branch from 6f4693c to e59f9df Compare January 4, 2019 22:38

hexfusion changed the title *: unset CommonName for gRPC-gateway request *: disable CommonName auth for gRPC-gateway Jan 4, 2019

mitake approved these changes Jan 5, 2019

View reviewed changes

hexfusion removed the WIP label Jan 7, 2019

hexfusion force-pushed the fx_cn branch from 3ec79d4 to 066ed43 Compare January 7, 2019 19:19

gyuho reviewed Jan 7, 2019

View reviewed changes

hexfusion force-pushed the fx_cn branch from 066ed43 to e97f692 Compare January 7, 2019 22:20

hexfusion added 4 commits January 7, 2019 17:22

auth: disable CommonName auth for gRPC-gateway

0818e61

Signed-off-by: Sam Batschelet <[email protected]>

Documentation: document gRPC-gateway CN authentication support

48d7f00

Signed-off-by: Sam Batschelet <[email protected]>

e2e: add ClientTLSCertAuth coverage for curl v3 auth tests

91924e3

Signed-off-by: Sam Batschelet <[email protected]>

CHANGELOG: add "disable CommonName authentication for gRPC-gateway" PR

5e92ca0

Signed-off-by: Sam Batschelet <[email protected]>

hexfusion force-pushed the fx_cn branch from e97f692 to 5e92ca0 Compare January 7, 2019 22:22

gyuho approved these changes Jan 8, 2019

View reviewed changes

gyuho added Release-Backport/v3.2 backport/v3.3 labels Jan 8, 2019

hexfusion merged commit 83c051b into etcd-io:master Jan 8, 2019

hexfusion deleted the fx_cn branch January 8, 2019 17:31

mitake mentioned this pull request Jan 22, 2019

RFC: use unix domain socket for internal RPC of gateway #10425

Closed

jmhbnz mentioned this pull request Dec 9, 2023

Support for CN based authentication on grpc gateway #17025

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: disable CommonName auth for gRPC-gateway #10366

*: disable CommonName auth for gRPC-gateway #10366

hexfusion commented Jan 2, 2019 •

edited

Loading

gyuho commented Jan 2, 2019

gyuho Jan 2, 2019

mitake Jan 3, 2019

hexfusion commented Jan 2, 2019

mitake commented Jan 3, 2019 •

edited

Loading

hexfusion commented Jan 3, 2019

codecov-io commented Jan 3, 2019 •

edited

Loading

hexfusion commented Jan 3, 2019

mitake commented Jan 5, 2019

hexfusion commented Jan 7, 2019

gyuho Jan 7, 2019

hexfusion Jan 7, 2019

gyuho left a comment

hexfusion commented Jan 8, 2019

nelljerram commented Aug 6, 2019

hexfusion commented Aug 6, 2019 •

edited

Loading

nelljerram commented Oct 30, 2019

DifferentialOrange commented Jun 21, 2024 •

edited

Loading

*: disable CommonName auth for gRPC-gateway #10366

*: disable CommonName auth for gRPC-gateway #10366

Conversation

hexfusion commented Jan 2, 2019 • edited Loading

gyuho commented Jan 2, 2019

gyuho Jan 2, 2019

Choose a reason for hiding this comment

mitake Jan 3, 2019

Choose a reason for hiding this comment

hexfusion commented Jan 2, 2019

mitake commented Jan 3, 2019 • edited Loading

hexfusion commented Jan 3, 2019

codecov-io commented Jan 3, 2019 • edited Loading

Codecov Report

hexfusion commented Jan 3, 2019

mitake commented Jan 5, 2019

hexfusion commented Jan 7, 2019

gyuho Jan 7, 2019

Choose a reason for hiding this comment

hexfusion Jan 7, 2019

Choose a reason for hiding this comment

gyuho left a comment

Choose a reason for hiding this comment

hexfusion commented Jan 8, 2019

nelljerram commented Aug 6, 2019

hexfusion commented Aug 6, 2019 • edited Loading

nelljerram commented Oct 30, 2019

DifferentialOrange commented Jun 21, 2024 • edited Loading

hexfusion commented Jan 2, 2019 •

edited

Loading

mitake commented Jan 3, 2019 •

edited

Loading

codecov-io commented Jan 3, 2019 •

edited

Loading

hexfusion commented Aug 6, 2019 •

edited

Loading

DifferentialOrange commented Jun 21, 2024 •

edited

Loading