Kubernetes security header annotations #2460

dtomcej · 2017-11-24T03:42:08Z

This PR implements the secure headers middleware from kubernetes annotations. This PR follows up #2334 and #2146.

ldez · 2017-11-26T12:46:35Z

@dtomcej to fix the "gen pb" could you rebase your branch?

timoreimann

I know the PR isn't ready yet, but I thought it could be worthwhile to give some early feedback, especially if we want this to still make it into 1.5. Hope you don't mind!

timoreimann · 2017-11-26T08:28:27Z

provider/kubernetes/kubernetes.go

+func getSliceAnnotation(meta v1.ObjectMeta, name string) []string {
+	value := []string{}
+	if annotation := meta.Annotations[name]; annotation != "" {
+		for _, subannotation := range strings.Split(annotation, ",") {


Hmm, I'm not sure if the name subannotation is ideal. To me, it gives a fault sense of an annotation (that is, a map) inside an existing annotation. IIRC correctly, however, it's just a list of values.

How about calling the variable value then (and adjust the log message below accordingly?

Sure.

Subannotation was a carry over from the docker where labels get clumped together.

timoreimann · 2017-11-26T08:29:26Z

provider/kubernetes/kubernetes.go

+	if annotation := meta.Annotations[name]; annotation != "" {
+		for _, subannotation := range strings.Split(annotation, ",") {
+			if len(subannotation) == 0 {
+				log.Warnf("Could not load annotation %v, skipping", subannotation)


I think the message should be more specific and about the fact we couldn't find any values for a known annotation.

timoreimann · 2017-11-26T08:29:45Z

provider/kubernetes/kubernetes.go

+		}
+	}
+	if len(value) == 0 {
+		log.Warnf("Could not load %v annotation", name)


The skipping part is missing here.

timoreimann · 2017-11-26T08:50:40Z

provider/kubernetes/kubernetes.go

+	}
+	if len(value) == 0 {
+		log.Warnf("Could not load %v annotation", name)
+		return nil


Do we have to be conscious about security-critical cases where missing header value should lead to the whole Ingress being skipped? I'm thinking of the equivalent of missing basic auth credentials that shouldn't lead to a frontend being accessible without authentication.

Nothing here really prevents code from loading, just appends or changes headers.

timoreimann · 2017-11-26T08:51:28Z

provider/kubernetes/kubernetes.go

+		return nil
+	}
+	return value
+}


The file is starting to get cluttered. How about moving those little helpers into a helpers.go file?

please no helpers/utils.go file. In test ok but not in the prod code

No disagreement in code reviews without an alternative suggestion, they say.

What do you suggest instead?

yes, annotations_parser.go or annotations.go by example.

Will do! 👍

timoreimann · 2017-11-26T09:53:02Z

provider/kubernetes/kubernetes.go

+	value := []string{}
+	if annotation := meta.Annotations[name]; annotation != "" {
+		for _, subannotation := range strings.Split(annotation, ",") {
+			if len(subannotation) == 0 {


Not entirely sure, but doesn't Split on an empty string yield the empty string, i.e., ""? That would still be non-zero in terms of the length.

Wow. Did not know that. Will update accordingly.

Ahhh so in this case, its to catch empty sub-elements (such as x,y,,z). The empty test is on the line above :)

dtomcej · 2017-11-27T15:38:21Z

@ldez Rebased!

ldez · 2017-11-27T16:10:36Z

templates/kubernetes.tmpl

+  [frontends."{{$frontendName}}".headers]
+  SSLRedirect = {{$frontend.Headers.SSLRedirect}}
+  SSLTemporaryRedirect = {{$frontend.Headers.SSLTemporaryRedirect}}
+  SSLHost = "{{$frontend.Headers.SSLHost}}""


small typo: double double quote 😄

timoreimann · 2017-11-28T06:18:14Z

provider/kubernetes/kubernetes.go

@@ -53,7 +73,7 @@ type Provider struct {
 	lastConfiguration      safe.Safe
 }

-func (p *Provider) newK8sClient() (Client, error) {
+func (p Provider) newK8sClient() (Client, error) {


Do we really want a copy of Provider when calling newK8sClient now?

We don't reload the static configuration (eg we don't recreate the client).

NewInClusterClient(endpoint string) and NewExternalClusterClient(endpoint, token, caFilePath string) use string, a pointer as receiver don't change anything.

The use of a pointer as a receiver must be reserved only when a method changes the state of the receiver. (use a pointer as a receiver produce an escape to heap)

Maybe I have miss something?

Late to the game, sorry.

The problem with receivers in Go is that they conflate two aspects: Mutability and performance / copy overhead. The Go Code Review Comments have a paragraph on the subject, but eventually suggest to leans towards pointer receivers, especially when the involved receiver is a non-trivial struct. I certainly think that Provider is rather complex (especially since it embeds another BaseProvider), so a pointer receiver would have been in order for me.

ldez

LGTM

juliens

LGTM

mmatur

LGTM 👏

add anno list add remove forcessl add k8s headers change error to warn log change empty maps to nil woops durp quote strings fix autogen update value/logging lol double quote move parsing template out empty maps ad docs

dtomcej requested a review from a team as a code owner November 24, 2017 03:42

traefiker added area/provider/k8s/ingress size/S status/0-needs-triage labels Nov 24, 2017

dtomcej added size/M WIP and removed size/S labels Nov 24, 2017

ldez added contributor/waiting-for-corrections kind/enhancement a new or improved feature. status/2-needs-review and removed status/0-needs-triage WIP labels Nov 24, 2017

ldez added this to the 1.5 milestone Nov 24, 2017

timoreimann reviewed Nov 26, 2017

View reviewed changes

dtomcej force-pushed the kubernetes-secure-annotations branch from 9a5d445 to cb9c79c Compare November 27, 2017 15:37

dtomcej force-pushed the kubernetes-secure-annotations branch from 09f35c3 to a0315a9 Compare November 27, 2017 16:11

ldez requested changes Nov 27, 2017

View reviewed changes

dtomcej force-pushed the kubernetes-secure-annotations branch 2 times, most recently from dea3558 to ff9947d Compare November 27, 2017 18:43

dtomcej closed this Nov 27, 2017

dtomcej reopened this Nov 27, 2017

ldez removed the contributor/waiting-for-corrections label Nov 27, 2017

ldez force-pushed the kubernetes-secure-annotations branch from 7f67d54 to 1431e26 Compare November 28, 2017 00:19

timoreimann reviewed Nov 28, 2017

View reviewed changes

ldez approved these changes Nov 28, 2017

View reviewed changes

juliens approved these changes Nov 28, 2017

View reviewed changes

mmatur approved these changes Nov 28, 2017

View reviewed changes

mmatur added the status/3-needs-merge label Nov 28, 2017

mmatur removed the status/2-needs-review label Nov 28, 2017

traefiker added the status/4-merge-in-progress label Nov 28, 2017

dtomcej and others added 3 commits November 28, 2017 11:04

initial commit

7508d7e

add anno list add remove forcessl add k8s headers change error to warn log change empty maps to nil woops durp quote strings fix autogen update value/logging lol double quote move parsing template out empty maps ad docs

refactor: reduce code.

e4ff217

doc: format table.

23addba

traefiker force-pushed the kubernetes-secure-annotations branch from 3595be6 to 23addba Compare November 28, 2017 11:04

traefiker added bot/need-human-merge and removed status/4-merge-in-progress labels Nov 28, 2017

traefiker approved these changes Nov 28, 2017

View reviewed changes

ldez removed the bot/need-human-merge label Nov 28, 2017

traefiker merged commit 4cb6241 into traefik:master Nov 28, 2017

traefiker removed the status/3-needs-merge label Nov 28, 2017

ldez mentioned this pull request Dec 4, 2017

Feature Request: Allow request header modifying / appending via configuration #2515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kubernetes security header annotations #2460

Kubernetes security header annotations #2460

dtomcej commented Nov 24, 2017

ldez commented Nov 26, 2017

timoreimann left a comment

timoreimann Nov 26, 2017

dtomcej Nov 27, 2017

timoreimann Nov 26, 2017

dtomcej Nov 27, 2017

timoreimann Nov 26, 2017

timoreimann Nov 26, 2017

dtomcej Nov 27, 2017

timoreimann Nov 26, 2017

ldez Nov 27, 2017

timoreimann Nov 27, 2017

ldez Nov 27, 2017

timoreimann Nov 27, 2017

dtomcej Nov 27, 2017

timoreimann Nov 26, 2017

dtomcej Nov 27, 2017

dtomcej Nov 27, 2017

dtomcej commented Nov 27, 2017

ldez Nov 27, 2017

dtomcej Nov 27, 2017

timoreimann Nov 28, 2017

ldez Nov 28, 2017 •

edited

timoreimann Nov 28, 2017

ldez left a comment

juliens left a comment

mmatur left a comment

Kubernetes security header annotations #2460

Kubernetes security header annotations #2460

Conversation

dtomcej commented Nov 24, 2017

ldez commented Nov 26, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtomcej commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez Nov 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

ldez Nov 28, 2017 •

edited