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

allow an SNI cert to be used to respond for a particular IP #85308

Merged
merged 1 commit into from Jan 7, 2020

Conversation

@deads2k
Copy link
Contributor

deads2k commented Nov 14, 2019

Because the kube-apiserver listens on multiple IPs, it is natural to have different serving certificates for the various IPs (service network, host-network, localhost, etc). SNI doesn't directly support this from the client-side because when an IP is specified it is unambiguous what server name should be used. This means that the serverName in the handshake is not set.

By allowing a direct mapping for the certificate to specify an IP and intercepting the clienthello, we can choose a specific certificate that is signed for the IP and will allow the connection to succeed. This does not introspect all IPs set in the certificate. We could also add that if we wished

/kind bug
/priority important-soon
@kubernetes/sig-api-machinery-misc

If a serving certificates param specifies a name that is an IP for an SNI certificate, it will have priority for replying to server connections.

/hold I've promised to hold for @liggitt to have comments.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 14, 2019

or the common name in the certificate to specify an IP

I didn't think common name was supposed to be used in the presence of DNS/IP SANs

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Nov 14, 2019

I won't be able to get to this until next week at the earliest

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Nov 18, 2019

or the common name in the certificate to specify an IP

I didn't think common name was supposed to be used in the presence of DNS/IP SANs

Used by clients or used by servers for their configuration? I claim that this only uses the field as an intent to key for IPs. Alternatives include:

  1. requiring explicit flags to be set
  2. reading all IPs

I don't have strong preferences, but I do think we have a need to configure these values.

@enj

This comment has been minimized.

Copy link
Member

enj commented Nov 18, 2019

/assign

After KubeCon.

@fedebongio

This comment has been minimized.

Copy link
Contributor

fedebongio commented Nov 26, 2019

/assign @liggitt

@hickeng

This comment has been minimized.

Copy link

hickeng commented Dec 2, 2019

@deads2k I had a similar issue and initially wrote something very similar, selecting certs from the SNI set based on IP. This had to change however due to an environment where access to the apiserver was via a NAT load balancer, routed via the overlay, with the consequence that the destination IP did not allow differentiation between external access and access from pods.
I'm noting this here in case this is a scenario of interest to you.

In the end I ended up with the following: v1.16.2...hickeng:cert-filter

I was planning on switching to a more expressive filter syntax, probably a BPF subset, and submitting a PR but have not yet had the time to do so.

This was written before the dynamic certificate work merged - I'm assuming it's better to show you as is rather than after I find time to forward port, possibly after this PR merges. As such it's a diff against v1.16.2.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 2, 2020

This sets up those rules, but does not introspect all IPs set in the certificate. We could also add that if we wished

If we're going to try to use IP addresses to select certificates, I would expect the IP SANs to be a much stronger signal than the common name. The common name is not even used to verify IP hostnames in RFC-compliant TLS stacks (e.g. https://github.com/golang/go/blob/master/src/crypto/x509/verify.go#L1012-L1021), so I think selecting a certificate based on the common name to answer an IP-based request would break those clients.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 2, 2020

@deads2k I had a similar issue and initially wrote something very similar, selecting certs from the SNI set based on IP. This had to change however due to an environment where access to the apiserver was via a NAT load balancer, routed via the overlay, with the consequence that the destination IP did not allow differentiation between external access and access from pods.
I'm noting this here in case this is a scenario of interest to you.

scenarios like that are concerning to me as well

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Jan 2, 2020

This sets up those rules, but does not introspect all IPs set in the certificate. We could also add that if we wished

If we're going to try to use IP addresses to select certificates, I would expect the IP SANs to be a much stronger signal than the common name. The common name is not even used to verify IP hostnames in RFC-compliant TLS stacks (e.g. https://github.com/golang/go/blob/master/src/crypto/x509/verify.go#L1012-L1021), so I think selecting a certificate based on the common name to answer an IP-based request would break those clients.

As I noted in #85308 (comment), I'm happy to make it an explicit configuration choice. Given the focus of comments here, that seems like a path you'd find acceptable to make the configuration possible even if it isn't easy?

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 2, 2020

As I noted in #85308 (comment), I'm happy to make it an explicit configuration choice. Given the focus of comments here, that seems like a path you'd find acceptable to make the configuration possible even if it isn't easy?

Yeah, letting particular deployments opt-in if they know their network topology exposes accurate IP info to the kube-apiserver seems better (since this is done during the handshake, we don't even have the benefit of X-Forwarded-For-style headers to convey client request info through a proxy)

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 2, 2020

Since we already have the ability to indicate explicit hostnames in --tls-sni-cert-key options, something like --tls-sni-cert-key=foo.crt,foo.key:1.2.3.4 could include 1.2.3.4 in a set of IP addresses explicitly opted into this behavior

@deads2k deads2k force-pushed the deads2k:limited-sni-support branch 2 times, most recently from 95fb9df to 510fbf8 Jan 3, 2020
@deads2k deads2k force-pushed the deads2k:limited-sni-support branch 2 times, most recently from f24e2bd to 0dc6214 Jan 3, 2020
@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Jan 3, 2020

/hold cancel

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Jan 3, 2020

Updated to only match on specifically specified sni IPs.

@deads2k deads2k force-pushed the deads2k:limited-sni-support branch from 0dc6214 to dfafa1a Jan 3, 2020
@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Jan 3, 2020

/retest

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 6, 2020

Update the "--tls-sni-cert-key" flag help to describe this new capability (and the implication that it should only be used when the kube-apiserver has visibility to the IP the client requested)

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 6, 2020

just a couple doc updates, squash, then lgtm

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 6, 2020

In the end I ended up with the following: v1.16.2...hickeng:cert-filter

I was planning on switching to a more expressive filter syntax, probably a BPF subset, and submitting a PR but have not yet had the time to do so.

@deads2k did you consider separating the IP-based certs in a way that would allow selecting them by destination IP or by source IP? It's slightly weird to glom them into the SNI flag when we're not actually using SNI here.

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Jan 6, 2020

@deads2k did you consider separating the IP-based certs in a way that would allow selecting them by destination IP or by source IP? It's slightly weird to glom them into the SNI flag when we're not actually using SNI here.

It felt pretty natural to describe it via SNI for me, sttts, and our other cluster-admins here. When getting accessed via this name, use this serving cert. It's possible to add more wiring, but it doesn't seem more helpful to me.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 6, 2020

It felt pretty natural to describe it via SNI for me, sttts, and our other cluster-admins here.

do you have an idea what you would want to do if you encountered #85308 (comment) and needed to limit IP-cert-matching to requests from specific ranges?

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Jan 6, 2020

It felt pretty natural to describe it via SNI for me, sttts, and our other cluster-admins here.

do you have an idea what you would want to do if you encountered #85308 (comment) and needed to limit IP-cert-matching to requests from specific ranges?

A cluster-admin won't be in worse shape from this today since they simply wouldn't configure the IP listening feature. I think the logical end state for trying to handle source ranges ends up as a filter chain. I don't think I'm inclined to go there in the end.

@deads2k deads2k force-pushed the deads2k:limited-sni-support branch from 01b89d5 to 80a42a1 Jan 6, 2020
@deads2k deads2k force-pushed the deads2k:limited-sni-support branch from 80a42a1 to 2c8639d Jan 6, 2020
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jan 6, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 6, 2020
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 6, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 6, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

3 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 7, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 7, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jan 7, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit a26fcf5 into kubernetes:master Jan 7, 2020
15 checks passed
15 checks passed
cla/linuxfoundation deads2k authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.