Add some basic headers to OSIN provided pages #17010

simo5 · 2017-10-23T21:54:12Z

Use restrictive defaults for basic security hygiene.

simo5 · 2017-10-23T21:54:21Z

enj

Needs review from @openshift/api-review and @jwforres to make sure this won't break anything.

enj · 2017-10-24T03:41:06Z

pkg/auth/server/headers/headers.go

+
+// We cannot set HSTS by default, it has too many drawbacks in environments
+// that use self-signed certs
+const standardHeaders map[string]string{


I don't know what half of these mean.

It means I should commit -a --amend it with the code I had locally, before sending half baked code from when I drafted the first write. sigh.

enj · 2017-10-24T03:41:35Z

pkg/auth/server/headers/headers.go

+
+func SetStandardHeaders(w http.ResponseWriter) {
+	for key, val := range standardHeaders {
+		w.Header.Add(key, val)


Set seems more appropriate than add.

enj · 2017-10-24T03:46:34Z

cc @openshift/sig-security

enj · 2017-10-24T03:49:30Z

@simo5 maps cannot be consts.

++ Building go targets for linux/amd64: cmd/openshift cmd/oc cmd/kubefed cmd/template-service-broker pkg/network/sdn-cni-plugin vendor/github.com/containernetworking/cni/plugins/ipam/host-local vendor/github.com/containernetworking/cni/plugins/main/loopback
# github.com/openshift/origin/pkg/auth/server/headers
pkg/auth/server/headers/headers.go:9: syntax error: unexpected { after top level declaration
pkg/auth/server/headers/headers.go:9: const declaration cannot have type without expression
pkg/auth/server/headers/headers.go:9: missing value in const declaration
[ERROR] PID 105: hack/lib/build/binaries.sh:228: `GOOS=${platform%/*} GOARCH=${platform##*/} go install -pkgdir "${OS_OUTPUT_PKGDIR}/${platform}" -tags "${OS_GOFLAGS_TAGS-} ${!platform_gotags_envvar:-}" -ldflags="${local_ldflags}" "${goflags[@]:+${goflags[@]}}" "${nonstatics[@]}"` exited with status 2.

tiran · 2017-10-24T07:31:25Z

pkg/auth/server/headers/headers.go

+	// Do not allow embedding as that can lead to clickjacking attacks
+	"X-Frame-Options": "DENY",
+	// Add other basic scurity hygiene headers
+	"X-Content-Type-Options": "nosniff",


According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Content-Type-Options nosniff could be troublesome if applied to image resources. Are default header only applied to HTML sources?

Note: nosniff only applies to "script" and "style" types. Also applying nosniff to images turned out to be incompatible with existing web sites.

We do not send images in OSIN afaik, but also that page says:
"Note: nosniff only applies to "script" and "style" types." so I don't think we have a problem.

tiran · 2017-10-24T07:38:33Z

pkg/auth/server/headers/headers.go

+	"X-Content-Type-Options": "nosniff",
+	"X-DNS-Prefetch-Control": "off",
+	"X-XSS-Protection": "1; mode=block",
+}


What about CSP headers, https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP ?

We might want to set script src and style src.

Just like for HSTS I am not sure we can set a static CSP because branding may add stuff to pages I think.
But I'll let @enj comment on that

Yeah customer branding would definitely be an issue here, you would have to make the CSP configurable. Seems outside the scope of this particular PR.

Indeed @jwforres , I am going to make an Issue and we can resolve that in 3.8
CSP sounds like something we may want to share with Web console as far as using a common option when customer needs changes, perhaps.

php-coder · 2017-10-24T11:07:36Z

Maybe it's better to re-use this code also for #17002 or it's impossible?

enj · 2017-10-24T12:46:12Z

We should be able to wrap the entire OAuth mux with a handler that adds these headers so we only have to do it in one place.

simo5 · 2017-10-24T12:59:02Z

@php-coder we are adding more headers (namely those to prevent caching) which cannot be added by default to web console because they have a lot of static assets (images/css) that should be cached.

Perhaps we could have acommon set and then wrap it, but OSIN and Webconsole may have differeing opinions and I do not see that much value in sharing a for loop over a constant that may need to differ.

jwforres · 2017-10-24T13:58:49Z

Yeah I tend to agree with @simo5 on having a separate handler from the console. If we shared I would worry about headers getting added that weren't appropriate in one case or the other. Plus we still intend to split out the console from the master at some point.

simo5 · 2017-10-24T14:17:53Z

@eparis I am goign to add kind/bug to this PR and push it to master as the lack of headers is aguably a bug, as we should always have sent more restrictive ones. However it is a tiny change in behavior so I want your belssing.

handled

simo5 · 2017-10-24T14:26:36Z

/test cmd

eparis · 2017-10-24T14:27:23Z

please open a BZ so QA looks specifically in this area, then feel free

simo5 · 2017-10-24T16:07:28Z

@eparis we have a BZ, thanks

simo5 · 2017-10-24T16:08:32Z

/test extended_conformance_gce

simo5 · 2017-10-24T16:08:47Z

@cheimes CSP tracked here #17021

This code is not wired anywhere. Signed-off-by: Simo Sorce <simo@redhat.com>

enj

Just some minor nits.

enj · 2017-10-25T14:33:29Z

test/integration/oauth_server_headers_test.go

+	}
+
+	// Hit the login URL
+	loginURL := &url.URL{}


loginURL := *baseURL is the normal way to do this.

enj · 2017-10-25T14:34:41Z

test/integration/oauth_server_headers_test.go

+
+	// Hit the grant URL
+	grantURL := &url.URL{}
+	*grantURL = *baseURL


Same comment.

enj · 2017-10-25T14:35:21Z

test/integration/oauth_server_headers_test.go

+	for key, val := range checkImportantHeaders {
+		header := resp.Header.Get(key)
+		if header != val {
+			t.Fatalf("While probing %s expected header %s: %s, got {%v}", check_url, key, val, header)


t.Errorf seems more appropriate so that you do not quit the test early.

enj · 2017-10-25T14:36:32Z

@liggitt PTAL at the headers and dead code that was removed.

Use restrictive defaults for basic security hygiene. Signed-off-by: Simo Sorce <simo@redhat.com>

addressed

liggitt · 2017-10-25T17:47:20Z

/approve

enj · 2017-10-25T17:49:44Z

/lgtm

openshift-merge-robot · 2017-10-25T17:49:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, liggitt, simo5

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/auth/OWNERS~~ [enj,liggitt]
~~test/integration/OWNERS~~ [enj,liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-10-25T21:01:09Z

Automatic merge from submit-queue (batch tested with PRs 17020, 17026, 17000, 17010).

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 23, 2017

openshift-merge-robot assigned mrogers950 and legionus Oct 23, 2017

enj reviewed Oct 24, 2017

View reviewed changes

tiran previously requested changes Oct 24, 2017

View reviewed changes

simo5 force-pushed the authheaders branch 2 times, most recently from 2820ab0 to 10a7b84 Compare October 24, 2017 12:50

simo5 added the kind/bug Categorizes issue or PR as related to a bug. label Oct 24, 2017

Remove dead code

c0f2665

This code is not wired anywhere. Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 force-pushed the authheaders branch from 10a7b84 to 2962c86 Compare October 25, 2017 13:30

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 25, 2017

enj previously requested changes Oct 25, 2017

View reviewed changes

Add some basic headers to OSIN provided pages

24083ea

Use restrictive defaults for basic security hygiene. Signed-off-by: Simo Sorce <simo@redhat.com>

simo5 force-pushed the authheaders branch from 2962c86 to 24083ea Compare October 25, 2017 15:06

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2017

openshift-ci-robot assigned enj Oct 25, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017

openshift-merge-robot merged commit 87e24b0 into openshift:master Oct 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some basic headers to OSIN provided pages #17010

Add some basic headers to OSIN provided pages #17010

simo5 commented Oct 23, 2017

simo5 commented Oct 23, 2017

enj left a comment

enj Oct 24, 2017

simo5 Oct 24, 2017

enj Oct 24, 2017

simo5 Oct 24, 2017

enj commented Oct 24, 2017

enj commented Oct 24, 2017

tiran Oct 24, 2017

simo5 Oct 24, 2017

tiran Oct 24, 2017

simo5 Oct 24, 2017

jwforres Oct 24, 2017

simo5 Oct 24, 2017

php-coder commented Oct 24, 2017

enj commented Oct 24, 2017

simo5 commented Oct 24, 2017

jwforres commented Oct 24, 2017

simo5 commented Oct 24, 2017

simo5 commented Oct 24, 2017

eparis commented Oct 24, 2017

simo5 commented Oct 24, 2017

simo5 commented Oct 24, 2017

simo5 commented Oct 24, 2017

enj left a comment

enj Oct 25, 2017

enj Oct 25, 2017

enj Oct 25, 2017

enj commented Oct 25, 2017

liggitt commented Oct 25, 2017

enj commented Oct 25, 2017

openshift-merge-robot commented Oct 25, 2017

openshift-merge-robot commented Oct 25, 2017

Add some basic headers to OSIN provided pages #17010

Add some basic headers to OSIN provided pages #17010

Conversation

simo5 commented Oct 23, 2017

simo5 commented Oct 23, 2017

enj 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

enj commented Oct 24, 2017

enj commented Oct 24, 2017

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

php-coder commented Oct 24, 2017

enj commented Oct 24, 2017

simo5 commented Oct 24, 2017

jwforres commented Oct 24, 2017

simo5 commented Oct 24, 2017

simo5 commented Oct 24, 2017

eparis commented Oct 24, 2017

simo5 commented Oct 24, 2017

simo5 commented Oct 24, 2017

simo5 commented Oct 24, 2017

enj 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

enj commented Oct 25, 2017

liggitt commented Oct 25, 2017

enj commented Oct 25, 2017

openshift-merge-robot commented Oct 25, 2017

openshift-merge-robot commented Oct 25, 2017