Generate escaped regexes for cors config #16204

liggitt · 2017-09-07T15:18:12Z

this ensures the generated config matches hosts exactly instead of treating . like a "match any character" rule

liggitt · 2017-09-07T15:19:14Z

cc @openshift/sig-security
cc @spadgett

enj

Comment about func. Otherwise LGTM.

enj · 2017-09-07T18:19:41Z

pkg/cmd/server/start/master_args.go

@@ -149,7 +150,15 @@ func (args MasterArgs) BuildSerializeableMasterConfig() (*configapi.MasterConfig
 	// always include localhost as an allowed CORS origin
 	// always include master public address as an allowed CORS origin
 	corsAllowedOrigins := sets.NewString(args.CORSAllowedOrigins...)
-	corsAllowedOrigins.Insert(assetPublicAddr.Host, masterPublicAddr.Host, "localhost", "127.0.0.1")
+	matchLiteralHost := func(s string) string {


Why an inline function?

no reason, fixed

spadgett · 2017-09-08T11:07:36Z

How is this list used by the API server?

It looks to me like the Access-Control-Allow-Origin header itself doesn't allow regular expression. It's either a list of domains or a single wildcard *

https://www.w3.org/TR/cors/#access-control-allow-origin-response-header

liggitt · 2017-09-08T14:29:07Z

How is this list used by the API server?

It tries to match the incoming Origin header against one of these regexes, and if one matches, echoes the Origin header back as Access-Control-Allow-Origin:

origin/vendor/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/server/filters/cors.go

Lines 42 to 51 in 72f5bc5

    
           origin := req.Header.Get("Origin") 
        
           if origin != "" { 
        
           	allowed := false 
        
           	for _, re := range allowedOriginPatternsREs { 
        
           		if allowed = re.MatchString(origin); allowed { 
        
           			break 
        
           		} 
        
           	} 
        
           	if allowed { 
        
           		w.Header().Set("Access-Control-Allow-Origin", origin)

spadgett · 2017-09-08T16:11:07Z

pkg/cmd/server/start/master_args.go

+		makeExactMatchRegex(assetPublicAddr.Host),
+		makeExactMatchRegex(masterPublicAddr.Host),
+		makeExactMatchRegex("localhost"),
+		makeExactMatchRegex("127.0.0.1"),


It looks like this will now block origins like https://localhost:9000? If so, I wonder if it's still worth settings these as defaults.

If we trust localhost, I assume we trust it on any port and protocol?

Do assetPublicAddres.Host and masterPublicAddres.Host include scheme and port as well?

hmm, yeah... need to rethink the scheme/port bit

ok, pinned to the beginning of the host with // and the end of the host with (:|$) or $, depending on whether host already contained a port.

This results in output like this:

corsAllowedOrigins: - //127\.0\.0\.1(:|$) - //192\.168\.1\.101:8443$ - //localhost(:|$)

@spadgett, are you able to check if that works across browsers for the cross-domain case?

liggitt · 2017-09-12T20:45:06Z

/retest

spadgett · 2017-09-13T11:26:38Z

@liggitt Built and tested the cross-domain case with Chrome, Firefox, Safari, and IE 11. It works in all browsers. I also checked using curl that substituting another character for . does not add the CORS headers.

/lgtm

openshift-merge-robot · 2017-09-13T11:26:40Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/cmd/OWNERS~~ [liggitt,spadgett]

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

openshift-bot · 2017-09-13T19:44:32Z

/retest