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

Add support for fetching k8s Ingress TLS data from secrets #2439

Merged
merged 22 commits into from
Jan 7, 2018
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
3892fb7
Add tls support for k8s ingress ressource
gopenguin Nov 20, 2017
228362c
Merge branch 'master' into k8s-ingress-tls
gopenguin Nov 21, 2017
765fba4
Add docs for the k8s tls ingress feature
gopenguin Nov 21, 2017
1f96b88
Fix import prefix and errorhandling
gopenguin Nov 21, 2017
bf648b8
Fix typo
gopenguin Nov 21, 2017
4c0ba9e
Fix naming convention
gopenguin Nov 21, 2017
18c5f9e
Merge branch 'master' into k8s-ingress-tls
gopenguin Nov 22, 2017
cc0b448
Refactor k8s ingress tls extraction and add tests
gopenguin Nov 23, 2017
1c60f29
Merge branch 'master' into k8s-ingress-tls
gopenguin Nov 23, 2017
b0a7899
Remove cert validation and fix some other issues
gopenguin Dec 4, 2017
741163e
Merge branch 'master' into k8s-ingress-tls
gopenguin Dec 5, 2017
c13b21a
Implement the builder pattern for TLS types
gopenguin Dec 5, 2017
bce97e9
Improve error messages and doc
gopenguin Dec 6, 2017
383b9e8
Pass annotation 'traefik.frontend.entryPoints' to cert
gopenguin Dec 6, 2017
74616a4
Merge branch 'master' into k8s-ingress-tls
gopenguin Dec 11, 2017
e082818
Add tls config to k8s template and extend test
gopenguin Dec 11, 2017
3ecf770
Fix template file
nmengin Dec 19, 2017
d0a5c2f
Update documentation
gopenguin Dec 20, 2017
4d156ec
Abort ingress if error in TLS and add docs
gopenguin Jan 6, 2018
d00111a
Merge branch 'master' into k8s-ingress-tls
gopenguin Jan 6, 2018
6ab8f21
Fix tests
gopenguin Jan 6, 2018
1d2e121
Add an invalid ingress to TestTLSSecretLoad to cover error case
gopenguin Jan 6, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions autogen/gentemplates/gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 42 additions & 0 deletions docs/user-guide/kubernetes.md
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,48 @@ echo "$(minikube ip) traefik-ui.minikube" | sudo tee -a /etc/hosts

We should now be able to visit [traefik-ui.minikube](http://traefik-ui.minikube) in the browser and view the Træfik Web UI.

### Add a TLS Certificate to the Ingress

!!! note
For this example to work you need a TLS entrypoint. You don't have to provide a TLS certificate at this point. For more details see [here](/configuration/entrypoints/).

To setup a HTTPS protected ingress, you can leverage the TLS feature of the ingress resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/a HTTPS protected/an HTTPS-protected/


```yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
name: traefik-web-ui
namespace: kube-system
annotations:
kubernetes.io/ingress.class: traefik
spec:
rules:
- host: traefik-ui.minikube
http:
paths:
- backend:
serviceName: traefik-web-ui
servicePort: 80
tls:
- hosts:
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't see us parsing the hosts list in our code explicitly. Am I missing something?

- traefik-ui.minikube
secretName: traefik-ui-tls-cert
```

In addition to the modified ingress you need to provide the TLS certificate via a kubernetes secret in the same namespace as the ingress. The following two commands will generate a new certificate and create a secret containing the key and cert files.

```shell
openssl req -x509 -nodes -days 365 -newkey rsa:2048 -keyout /tmp/tls.key -out /tmp/tls.crt -subj "/CN=traefik-ui.minikube"
kubectl -n kube-system create secret tls traefik-ui-tls-cert --key=/tmp/tls.key --cert=/tmp/tls.crt
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop the /tmp directory part as it may not exist on every system. Saving the key and cert in the current working directory seems good enough for me.

```

!!! note
The secret must have two entries named `tls.key`and `tls.crt`. See the [kubernetes documentation](https://kubernetes.io/docs/concepts/services-networking/ingress/#tls) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent extension to the guide 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had no idea how to do something like this but I'm glad it is OK


Copy link
Contributor

@nmengin nmengin Dec 19, 2017

Choose a reason for hiding this comment

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

Can you please add a note to explain that the certificates contained into the K8s secrets will be added to all the TLS entryPoints linked to the services which use them (if the label traefik.frontend.entryPoints is defined) or to all the TLS entryPoints defined into DefaultEntryPoints by default ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would the following ok with you?

The TLS certificates will be added to all entrypoints defined by the ingress annotation traefik.frontend.entryPoints. If no such annotation is provided, the TLS certificates will be added to all TLS enabled defaultEntryPoints.

Did you mean with 'note' the note formatting or just an addition to the documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gopenguin

Yes the sentence LGTM 👍
I mean note formatting at the end of the block instructions 😉

!!! note
The TLS certificates will be added to all entrypoints defined by the ingress annotation `traefik.frontend.entryPoints`. If no such annotation is provided, the TLS certificates will be added to all TLS enabled `defaultEntryPoints`.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/to all TLS enabled/to all TLS-enabled/


## Basic Authentication

It's possible to add additional authentication annotations in the Ingress rule.
Expand Down
49 changes: 49 additions & 0 deletions provider/kubernetes/builder_configuration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package kubernetes
import (
"testing"

"github.com/containous/traefik/tls"
"github.com/containous/traefik/types"
"github.com/stretchr/testify/assert"
)
Expand Down Expand Up @@ -195,6 +196,39 @@ func route(name string, rule string) func(*types.Route) string {
}
}

func tlsConfigurations(opts ...func(*tls.Configuration)) func(*types.Configuration) {
return func(c *types.Configuration) {
for _, opt := range opts {
tlsConf := &tls.Configuration{}
opt(tlsConf)
c.TLSConfiguration = append(c.TLSConfiguration, tlsConf)
}
}
}

func tlsConfiguration(opts ...func(*tls.Configuration)) func(*tls.Configuration) {
return func(c *tls.Configuration) {
for _, opt := range opts {
opt(c)
}
}
}

func tlsEntryPoints(entryPoints ...string) func(*tls.Configuration) {
return func(c *tls.Configuration) {
c.EntryPoints = entryPoints
}
}

func certificate(cert string, key string) func(*tls.Configuration) {
return func(c *tls.Configuration) {
c.Certificate = &tls.Certificate{
CertFile: tls.FileOrContent(cert),
KeyFile: tls.FileOrContent(key),
}
}
}

// Test

func TestBuildConfiguration(t *testing.T) {
Expand Down Expand Up @@ -241,6 +275,12 @@ func TestBuildConfiguration(t *testing.T) {
),
),
),
tlsConfigurations(
tlsConfiguration(
tlsEntryPoints("https"),
certificate("certificate", "key"),
),
),
)

assert.EqualValues(t, sampleConfiguration(), actual)
Expand Down Expand Up @@ -329,5 +369,14 @@ func sampleConfiguration() *types.Configuration {
},
},
},
TLSConfiguration: []*tls.Configuration{
{
EntryPoints: []string{"https"},
Certificate: &tls.Certificate{
CertFile: tls.FileOrContent("certificate"),
KeyFile: tls.FileOrContent("key"),
},
},
},
}
}
29 changes: 28 additions & 1 deletion provider/kubernetes/builder_ingress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,23 @@ func iBackend(name string, port intstr.IntOrString) func(*v1beta1.HTTPIngressPat
}
}

func iTLSs(opts ...func(*v1beta1.IngressTLS)) func(*v1beta1.Ingress) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this to iTLSes? Makes it slightly more readable (and grammatically correct) IMHO.

return func(i *v1beta1.Ingress) {
for _, opt := range opts {
iTLS := v1beta1.IngressTLS{}
opt(&iTLS)
i.Spec.TLS = append(i.Spec.TLS, iTLS)
}
}
}

func iTLS(secret string, hosts ...string) func(*v1beta1.IngressTLS) {
return func(i *v1beta1.IngressTLS) {
i.SecretName = secret
i.Hosts = hosts
}
}

// Test

func TestBuildIngress(t *testing.T) {
Expand All @@ -107,7 +124,11 @@ func TestBuildIngress(t *testing.T) {
onePath(iBackend("service2", intstr.FromInt(802))),
),
),
))
),
iTLSs(
iTLS("tls-secret", "foo"),
),
)

assert.EqualValues(t, sampleIngress(), i)
}
Expand Down Expand Up @@ -164,6 +185,12 @@ func sampleIngress() *v1beta1.Ingress {
},
},
},
TLS: []v1beta1.IngressTLS{
{
Hosts: []string{"foo"},
SecretName: "tls-secret",
},
},
},
}
}
50 changes: 50 additions & 0 deletions provider/kubernetes/kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/containous/traefik/provider"
"github.com/containous/traefik/provider/label"
"github.com/containous/traefik/safe"
"github.com/containous/traefik/tls"
"github.com/containous/traefik/types"
"k8s.io/client-go/pkg/api/v1"
"k8s.io/client-go/pkg/apis/extensions/v1beta1"
Expand Down Expand Up @@ -354,6 +355,13 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)
}
}
}

tlsConfigs, err := getTLSConfigurations(i, k8sClient)
if err != nil {
log.Errorf("Error configuring TLS for ingress %s/%s: %v", i.Namespace, i.Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this express the intended behavior as summarized by @dtomcej here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out, if an entry point is TLS-enabled at this point as the default configuration (passing nil) is handled further down the road and override entry points are only available as strings without any context. Therefore, I can't check if an entry point is TLS-enabled or not.

But I can't imagine a way to solve this problem properly as either the internals need to know about a TLS configuration error and abort in the case of an HTTPS-only entry point or the provider needs to know about the entry points (default and override).

I'm sorry for not pointing this out earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing the research.

If we can't figure out whether it's safe to accept an illegal definition or not, then my opinion is that we should err on the side of safety and skip it. Better be safe than sorry.

What do you (and @dtomcej) think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Staying on the safe side should be our first choice as this is security critical. I'd also add a note to the docs regarding this and also the deviation from the kubernetes spec (host names are ignored and therefore the wildcard behaviour cannot be implemented).

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you refresh my mind again: what's the reason we cannot comply with the Kubernetes spec? Is there a potential path (possibly in the future) we could improve on this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In a kubernetes ingress resource multiple TLSes per ingress are allowed. Each consists of two parts: a list of host names and a reference to a kubernetes secret. There are two cases in which a TLS configuration can be:

  1. A list of host names is provided: Only the defined host names in the list are used for this TLS certificate, not the ones defined in the certificate
  2. The list of host names is empty (wildcard behavior): In this case not a general wildcard is used. Instead, only the host names in the ingress are used as the wildcard. A wildcard TLS may only be used without any other TLS definitions in this ingress.

I think there is a path to implement this feature in the future as this behavior is a special case of the already implemented one in the traefik internals. (https://github.com/containous/traefik/blob/c446c291d9dc49895e69c91e51a054778c71f21a/tls/certificate.go#L150-L154 would be the default behavior, but it can be overwritten by the provider)
I have no clue what so ever how to pass this information from a provider to the internals.

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly, after doing a bit of research, there does not seem to be any implementation outside of ingress controller annotations to handle the failures of any of the components of an ingress. It appears that it is all-or-nothing, where the tls section of the spec is parsed in parallel with the rules section of the spec, leaving the annotations to handle the relationship between http/rules and tls sections.

Therefore, I would now be more inclined to just fail/skip loading the entire ingress on an error loading any portion of the tls section of the spec, since there is no way (without going into all the middlewares) to know how important the separate tls sections are.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gopenguin thanks for the clarification with regards to the wildcard matter. As long as we clearly document the constraints, I'm fine with it.

In the light of @dtomcej's latest comment: could you please change the logic so that we skip illegal Ingress definitions? AFAICS, that is the last point that keeps us from merging this PR.

} else {
templateObjects.TLSConfiguration = append(templateObjects.TLSConfiguration, tlsConfigs...)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put the TLS logic into a dedicated function?

}
return &templateObjects, nil
}
Expand Down Expand Up @@ -441,6 +449,48 @@ func loadAuthCredentials(namespace, secretName string, k8sClient Client) ([]stri
return creds, nil
}

func getTLSConfigurations(ingress *v1beta1.Ingress, k8sClient Client) ([]*tls.Configuration, error) {
var tlsConfigs []*tls.Configuration

for _, t := range ingress.Spec.TLS {
tlsSecret, exists, err := k8sClient.GetSecret(ingress.Namespace, t.SecretName)
if err != nil {
return nil, fmt.Errorf("failed to fetch secret %s/%s: %v", ingress.Namespace, t.SecretName, err)
}
if !exists {
return nil, fmt.Errorf("secret %s/%s does not exist", ingress.Namespace, t.SecretName)
}

tlsCrtData, tlsCrtExists := tlsSecret.Data["tls.crt"]
tlsKeyData, tlsKeyExists := tlsSecret.Data["tls.key"]

var missingEntries []string
if !tlsCrtExists {
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT about testing if both tlsCrtExists and tlsKeyExists are false and generate a gloabl error message to allow users to correct the two erros in one time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds reasonable. Would be secret %s/%s must have two entries named 'tls.crt' and 'tls.key': both entries are missing ok with you?

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be "secret %s/%s is missing the following TLS data entries: %s", namespace, name, strings.Join(", ", missingEntries).

missingEntries = append(missingEntries, "tls.crt")
}
if !tlsKeyExists {
missingEntries = append(missingEntries, "tls.key")
}
if len(missingEntries) > 0 {
return nil, fmt.Errorf("secret %s/%s is missing the following TLS data entries: %s", ingress.Namespace, t.SecretName, strings.Join(missingEntries, ", "))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this a lot 👍


entryPoints := label.GetSliceStringValue(ingress.Annotations, label.TraefikFrontendEntryPoints)

tlsConfig := &tls.Configuration{
EntryPoints: entryPoints,
Certificate: &tls.Certificate{
CertFile: tls.FileOrContent(tlsCrtData),
KeyFile: tls.FileOrContent(tlsKeyData),
},
}

tlsConfigs = append(tlsConfigs, tlsConfig)
}

return tlsConfigs, nil
}

func endpointPortNumber(servicePort v1.ServicePort, endpointPorts []v1.EndpointPort) int {
if len(endpointPorts) > 0 {
//name is optional if there is only one port
Expand Down