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

Reject TLS secrets with certificate that has no CN or SANs #1965

Closed
erwbgy opened this issue Nov 29, 2019 · 10 comments · Fixed by #1985
Closed

Reject TLS secrets with certificate that has no CN or SANs #1965

erwbgy opened this issue Nov 29, 2019 · 10 comments · Fixed by #1985
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@erwbgy
Copy link
Contributor

erwbgy commented Nov 29, 2019

Envoy crashes when processing a TLS certificate that does not have SubjectAltNames or a CN field in the Subject.

Create a key, certificate and TLS Secret and then an Ingress that uses this Secret:

$ openssl req -new -newkey rsa:2048 -days 365 -nodes -subj="/DC=com/DC=domain/DC=my" -x509 -keyout server.key -out server.crt -sha256
$ kubectl create secret tls tls-secret-bad --cert=server.crt --key=server.key
secret/tls-secret-bad created

$ cat ingress.yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: contour
  name: test
spec:
  rules:
  - host: my.domain.com
    http:
      paths:
      - backend:
          serviceName: test
          servicePort: 8080
        path: /
  tls:
  - hosts:
    - my.domain.com
    secretName: tls-secret-bad

$ kubectl create -f ingress.yaml
ingress.extensions/test created

$ kubectl create deployment test --image=gcr.io/google_containers/explorer:1.0
deployment.apps/test created

$ kubectl expose deployment test --port=8080
service/test exposed

The envoy Pod logs show an assertion failure:

$ kubectl -n projectcontour logs envoy-7ms2h
...
[2019-11-29 09:32:24.547][1][info][upstream] [source/server/lds_api.cc:60] lds: add/update listener 'ingress_https'
[2019-11-29 09:32:24.547][1][info][upstream] [source/server/lds_api.cc:60] lds: add/update listener 'stats-health'
[2019-11-29 09:32:24.549][1][critical][assert] [source/extensions/transport_sockets/tls/context_impl.cc:838] assert failure: cn_index >= 0.
[2019-11-29 09:32:24.549][1][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:81] Caught Aborted, suspect faulting address 0x1
[2019-11-29 09:32:24.549][1][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:69] Backtrace (use tools/stack_decode.py to get line numbers):
[2019-11-29 09:32:24.549][1][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:75] #0: [0x7f492059e420]
[2019-11-29 09:32:24.551][1][critical][backtrace] [bazel-out/k8-opt/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:73] #1: Envoy::Extensions::TransportSockets::Tls::ServerContextImpl::ServerContextImpl() [0xbadb44]

and stay in CrashLoopBackOff until the offending Ingress is removed.

@prateekjainaa
Copy link

I will be interested to know the use case why these fields will be empty.

@erwbgy
Copy link
Contributor Author

erwbgy commented Nov 29, 2019

Clearly there isn't a use case, but it should not be possible to break Contour for the entire cluster just by creating one bad Ingress and Secret.

@davecheney davecheney added this to the 1.1.0 milestone Nov 29, 2019
@davecheney davecheney added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Nov 29, 2019
@jpeach
Copy link
Contributor

jpeach commented Nov 29, 2019 via email

@erwbgy
Copy link
Contributor Author

erwbgy commented Dec 2, 2019

I couldn't find one so I raised envoyproxy/envoy#9182

@jpeach
Copy link
Contributor

jpeach commented Dec 3, 2019

Thanks @erwbgy

@davecheney davecheney self-assigned this Dec 3, 2019
@davecheney
Copy link
Contributor

I'm working on this now. The specific fix is straight forward but opens a lot of other questions about what other kinds of certificates will envoy reject. Obviously we'll fix this specific issue quick smart, but the yakity-sax whack-a-mole song is starting to grow in volume

@jpeach
Copy link
Contributor

jpeach commented Dec 3, 2019 via email

@davecheney
Copy link
Contributor

Are you also going to validate the certificate chain? What about ensuring it the certificate matches the vhost name?

I'm classifying this class of problems into three groups

a. crashes envoy
b. doesn't crash envoy but will block updates if envoy NACKs a discovery response
c. valid configuration with nonsense values; a cert that doesn't match a signing chain.

At the moment, I'm prioritising a and b, the list of possible validation failures feels large enough.

@jpeach
Copy link
Contributor

jpeach commented Dec 3, 2019 via email

davecheney added a commit to davecheney/contour that referenced this issue Dec 5, 2019
Fixes projectcontour#1965

During cache insertion reject certificates which lack a Subject
CommonName (CN) or SubjectAltName extension.

This PR uncovered that, well basically, all our TLS fixtures don't fit
this requirement which is extra sadface and thus this PR contains
replacement fixtures for all the previous ones.

This PR also adds a unit test for dag.isValidSecret as we are likely to
continue to add to this function as further validation edge cases are
uncovered. This necessitated a small addition to the internal/assert
comparator logic to be able to compare error values.

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit to davecheney/contour that referenced this issue Dec 5, 2019
Fixes projectcontour#1965

During cache insertion reject certificates which lack a Subject
CommonName (CN) or SubjectAltName extension.

This PR uncovered that, well basically, all our TLS fixtures don't fit
this requirement which is extra sadface and thus this PR contains
replacement fixtures for all the previous ones.

This PR also adds a unit test for dag.isValidSecret as we are likely to
continue to add to this function as further validation edge cases are
uncovered. This necessitated a small addition to the internal/assert
comparator logic to be able to compare error values.

Signed-off-by: Dave Cheney <[email protected]>
@davecheney
Copy link
Contributor

(B) and (C) make sense to me, but for (A), my preference is to fix envoy. This issue has been there for ever AFAIK, so I don’t think there’s any urgency.

I don't want to have to explain to cluster admins that a user can crash their envoy process by creating a secret without a common name.

davecheney added a commit to davecheney/contour that referenced this issue Dec 6, 2019
Fixes projectcontour#1965

During cache insertion reject certificates which lack a Subject
CommonName (CN) or SubjectAltName extension.

This PR uncovered that, well basically, all our TLS fixtures don't fit
this requirement which is extra sadface and thus this PR contains
replacement fixtures for all the previous ones.

This PR also adds a unit test for dag.isValidSecret as we are likely to
continue to add to this function as further validation edge cases are
uncovered. This necessitated a small addition to the internal/assert
comparator logic to be able to compare error values.

Signed-off-by: Dave Cheney <[email protected]>
davecheney added a commit that referenced this issue Dec 6, 2019
Fixes #1965

During cache insertion reject certificates which lack a Subject
CommonName (CN) or SubjectAltName extension.

This PR uncovered that, well basically, all our TLS fixtures don't fit
this requirement which is extra sadface and thus this PR contains
replacement fixtures for all the previous ones.

This PR also adds a unit test for dag.isValidSecret as we are likely to
continue to add to this function as further validation edge cases are
uncovered. This necessitated a small addition to the internal/assert
comparator logic to be able to compare error values.

Signed-off-by: Dave Cheney <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants