-
Notifications
You must be signed in to change notification settings - Fork 5k
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 docker security headers via labels #2334
Conversation
provider/docker/docker.go
Outdated
"getRequestHeaders": p.getRequestHeaders, | ||
"hasResponseHeaders": p.hasResponseHeaders, | ||
"getResponseHeaders": p.getResponseHeaders, | ||
"hasAllowedHostsHeaders": p.hasAllowedHostsHeaders, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify your code like that:
"hasAllowedHostsHeaders": p.hasLabel(types.LabelFrontendAllowedHosts),
func (p Provider) hasLabel(label string) func(container dockerData) bool {
return func(container dockerData) bool {
label, err := getLabel(container, label)
return err == nil && len(label) > 0
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good Call!
Was just getting code roughed in. I knew that there is a lot of duplication, but I wanted to see it as a whole first, before condensing :)
In case it wasn't clear, I will be integrating k8s annotations in this PR as well, since the functions will be the same. |
@dtomcej So that your PR is integrated in the 1.5, it is imperative that it is ready by Tuesday 21 November at the latest. 🚀 |
Thank you for the reminder! Will have it ready! |
types/common_label.go
Outdated
@@ -16,6 +16,24 @@ const ( | |||
LabelFrontendEntryPoints = LabelPrefix + "frontend.entryPoints" | |||
LabelFrontendRequestHeader = LabelPrefix + "frontend.headers.customrequestheaders" | |||
LabelFrontendResponseHeader = LabelPrefix + "frontend.headers.customresoponseheaders" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed this typo: customresoponseheaders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already fixed in master
52fea6e
to
e1c388b
Compare
643d9b3
to
c6fa8bb
Compare
"getServicePassHostHeader": p.getServicePassHostHeader, | ||
"getServicePriority": p.getServicePriority, | ||
"getServiceBackend": p.getServiceBackend, | ||
"getServiceRedirect": p.getServiceRedirect, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entry is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added!
3d66e52
to
bdc5f55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtomcej Many thanks for this PR! 👏
Just few suggestions about the documentation.
|
||
| Label | Description | | ||
|-----------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| `traefik.frontend.headers.allowedHosts=EXPR` | Provide a list of allowed hosts that requests will be processed. Format: `Host1,Host2` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provides to be homogeneous with the others lines
|`traefik.frontend.headers.customrequestheaders=EXPR ` | Provides the container with custom request headers that will be appended to each request forwarded to the container. Format: `HEADER:value,HEADER2:value2` | | ||
| `traefik.frontend.headers.customresponseheaders=EXPR` | Appends the headers to each response returned by the container, before forwarding the response to the client. Format: `HEADER:value,HEADER2:value2` | | ||
|`traefik.frontend.headers.hostsProxyHeaders=EXPR ` | Provides a list of headers that the proxied hostname may be stored. Format: `HEADER1,HEADER2` | | ||
| `traefik.frontend.headers.SSLRedirect=true` | Force the frontend to redirect to SSL if a non-SSL request is sent. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forces to be homogeneous with the others lines
| `traefik.frontend.headers.customresponseheaders=EXPR` | Appends the headers to each response returned by the container, before forwarding the response to the client. Format: `HEADER:value,HEADER2:value2` | | ||
|`traefik.frontend.headers.hostsProxyHeaders=EXPR ` | Provides a list of headers that the proxied hostname may be stored. Format: `HEADER1,HEADER2` | | ||
| `traefik.frontend.headers.SSLRedirect=true` | Force the frontend to redirect to SSL if a non-SSL request is sent. | | ||
| `traefik.frontend.headers.SSLTemporaryRedirect=true` | Force the frontend to redirect to SSL if a non-SSL request is sent, but by sending a 302 instead of a 301. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forces to be homogeneous with the others lines
74672f0
to
f9bde55
Compare
provider/docker/docker.go
Outdated
func getBoolHeader(container dockerData, containerType string) bool { | ||
label, err := getLabel(container, containerType) | ||
return err == nil && len(label) > 0 && strings.EqualFold(strings.TrimSpace(label), "true") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could your remove this empty line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup!
f9bde55
to
0a54441
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
add template function list function frameworks add prelim function added template functions add functions to template fix case on template vars fix typo re-add redirect update templates delete autogen add security headers documentation fix missing line chore: add autogen. chore: remove old generated file. fix spelling :) remove missing line
0a54441
to
763a29c
Compare
It doesn't look like this adds support for kubernetes? |
@robholland this PR only concern Docker. |
#2334 (comment) says kubernetes would be included and #2146 was closed in favour of this :( |
@robholland You are correct. My intention was to have both in this PR, but it was decided to keep the two separate due to testing. Will have a new PR for k8s today. Again, apologies for the delay. |
@robholland Discussed with the team, and the new PR will make it into 1.5, so will make the next release, so this delay will not affect the mainline release :) |
Great, thanks! :) |
| Label | Description | | ||
|-----------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| `traefik.frontend.headers.allowedHosts=EXPR` | Provides a list of allowed hosts that requests will be processed. Format: `Host1,Host2` | | ||
|`traefik.frontend.headers.customrequestheaders=EXPR ` | Provides the container with custom request headers that will be appended to each request forwarded to the container. Format: `HEADER:value,HEADER2:value2` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these meant to be lowercase or camel case?
traefik.frontend.headers.customrequestheaders
should be traefik.frontend.headers.customRequestHeaders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vito-c Please open an issue instead of commented an already merged PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vito-c or open a PR. thanks
@@ -16,6 +16,24 @@ const ( | |||
LabelFrontendEntryPoints = LabelPrefix + "frontend.entryPoints" | |||
LabelFrontendRequestHeader = LabelPrefix + "frontend.headers.customrequestheaders" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if camelcase then these should be updated as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vito-c Please open an issue instead of commented an already merged PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vito-c or open a PR. thanks
This PR creates the ability to configure the security headers for frontends via docker labels.
This should provide comprehensive functionality such as per-frontend SSL redirection.
Based on some of the work done in #2146
Fixes #1903