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

testing/e2e,grpcproxy: Fix: go test --tags "cluster_proxy" -v ./tests/e2e/... #12273

Merged
merged 5 commits into from
Sep 9, 2020

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Sep 7, 2020

PR contains fixes that makes the e2e tests work through grpc proxy:

In particular:

  • Connection between proxy and etcd-server is using a proper CN-less certificate.
  • Spawned etcd-proxy service expects proper line: return v2p.waitReady("httpproxy: endpoints found")

Among others addresses requests:

Tested with:
go test --tags "cluster_proxy" -v ./tests/e2e/...

integration/fixtures/gencerts.sh:
  - refactored common logic to a helper function
  - added definition for client-nocn certificate
    (used for grpc-proxy -> etcd-server) communication.
Executed:
(cd ./integration/fixtures && ./gencerts.sh)

This in particular cereated a new client-nocn.crt (and key) that can be
used for testing grpc-proxy -> etcd-server connections.
…ert flags.

We have following communication schema:
client --- 1 ---> grpc-proxy --- 2 --- > etcd-server

There are 2 sets of flags/certs in grpc proxy [ https://github.com/etcd-io/etcd/blob/master/etcdmain/grpc_proxy.go#L140 ]:
 A. (cert-file, key-file, trusted-ca-file, auto-tls) this are controlling [1] so client to proxy connection and in particular they are describing proxy public identity.
 B. (cert,key, cacert ) - these are controlling [2] so what's the identity that proxy uses to make connections to the etcd-server.

If 2 (B.) contains certificate with CN and etcd-server is running with --client-cert-auth=true, the CN can be used as identity of 'client' from service perspective. This is permission escalation, that we should forbid.

If 1 (A.) contains certificate with CN - it should be considered perfectly valid. The server can (should) have full identity.

So only --cert flag (and not --cert-file flag) should be validated for empty CN.
Change tests/e2e to use proper (client-nocn.crt) certificate when
running in tags="cluster_proxy" mode.

Thanks to this (and previous in this PR) changes, the following test run
finally succeeds:
  ./build && go test --tags "cluster_proxy" -v ./tests/e2e/...
@ptabor
Copy link
Contributor Author

ptabor commented Sep 7, 2020

@mitake @jingyih @jpbetz - please take a look.

@jpbetz
Copy link
Contributor

jpbetz commented Sep 8, 2020

@wenjiaswe Would you be willing to look at the certs related changes?

Copy link
Contributor

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

One very minor question about a comment then LGTM

@ptabor ptabor force-pushed the 2020-09-07-fix-grpc-proxy-tests branch 2 times, most recently from e98e6aa to b46bb55 Compare September 9, 2020 17:54
@ptabor ptabor force-pushed the 2020-09-07-fix-grpc-proxy-tests branch from b46bb55 to 9d5a840 Compare September 9, 2020 18:04
@ptabor ptabor requested a review from jpbetz September 9, 2020 18:07
@jpbetz jpbetz merged commit 76e769c into etcd-io:master Sep 9, 2020
@ptabor ptabor deleted the 2020-09-07-fix-grpc-proxy-tests branch October 22, 2020 10:56
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.

2 participants