038ee16 · typeable/oauth2-proxy

git.iko.soy/typeable/oauth2-proxy

Merge pull request #280 from kamaln7/kamal/whitelist-redirects-with-ports

Allow redirects to whitelisted hosts with ports

commit038ee169bdf417096ecde42afa92d69a36b1eabc authorJoel Speed <Joel.speed@hotmail.co.uk> dateWed 15 Jan 2020, 14:44 (7 years ago) parenteee4b55 11cc396

4 files changed+204−40

compare with master

unifiedsplit

CHANGELOG.md+1 docs/configuration/configuration.md+1−1 oauthproxy.go+56−2 oauthproxy_test.go+146−37

CHANGELOG.md+1

@@ -8,6 +8,7 @@

88- [#347](https://github.com/pusher/oauth2_proxy/pull/347) Update keycloak provider configuration documentation

99- [#325](https://github.com/pusher/oauth2_proxy/pull/325) dist.sh: use sha256sum (@syscll)

1010- [#179](https://github.com/pusher/oauth2_proxy/pull/179) Add Nextcloud provider (@Ramblurr)

11+- [#280](https://github.com/pusher/oauth2_proxy/pull/280) whitelisted redirect domains: add support for whitelisting specific ports or allowing wildcard ports (@kamaln7)

1112- [#351](https://github.com/pusher/oauth2_proxy/pull/351) Add DigitalOcean Auth provider (@kamaln7)

1213

1314# v4.1.0

docs/configuration/configuration.md+1−1

@@ -112,7 +112,7 @@ An example [oauth2_proxy.cfg]({{ site.gitweb }}/contrib/oauth2_proxy.cfg.example

112112| `-version` | n/a | print version string | |

114114

115−Note, when using the `whitelist-domain` option, any domain prefixed with a `.` will allow any subdomain of the specified domain as a valid redirect URL.

115+Note: when using the `whitelist-domain` option, any domain prefixed with a `.` will allow any subdomain of the specified domain as a valid redirect URL. By default, only empty ports are allowed. This translates to allowing the default port of the URL's protocol (80 for HTTP, 443 for HTTPS, etc.) since browsers omit them. To allow only a specific port, add it to the whitelisted domain: `example.com:8080`. To allow any port, use `*`: `example.com:*`.

116116

117117See below for provider specific options

118118

oauthproxy.go+56−2

@@ -514,6 +514,43 @@ func (p *OAuthProxy) GetRedirect(req *http.Request) (redirect string, err error)

514514 return

515515}

516516

517+// splitHostPort separates host and port. If the port is not valid, it returns

518+// the entire input as host, and it doesn't check the validity of the host.

519+// Unlike net.SplitHostPort, but per RFC 3986, it requires ports to be numeric.

520+// *** taken from net/url, modified validOptionalPort() to accept ":*"

521+func splitHostPort(hostport string) (host, port string) {

522+ host = hostport

523+

524+ colon := strings.LastIndexByte(host, ':')

525+ if colon != -1 && validOptionalPort(host[colon:]) {

526+ host, port = host[:colon], host[colon+1:]

527+ }

528+

529+ if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") {

530+ host = host[1 : len(host)-1]

531+ }

532+

533+ return

534+}

535+

536+// validOptionalPort reports whether port is either an empty string

537+// or matches /^:\d*$/

538+// *** taken from net/url, modified to accept ":*"

539+func validOptionalPort(port string) bool {

540+ if port == "" || port == ":*" {

541+ return true

542+ }

543+ if port[0] != ':' {

544+ return false

545+ }

546+ for _, b := range port[1:] {

547+ if b < '0' || b > '9' {

548+ return false

549+ }

550+ }

551+ return true

552+}

553+

517554// IsValidRedirect checks whether the redirect URL is whitelisted

518555func (p *OAuthProxy) IsValidRedirect(redirect string) bool {

519556 switch {

@@ -524,11 +561,28 @@ func (p *OAuthProxy) IsValidRedirect(redirect string) bool {

524561 if err != nil {

525562 return false

526563 }

564+ redirectHostname := redirectURL.Hostname()

565+

527566 for _, domain := range p.whitelistDomains {

528− if (redirectURL.Host == domain) || (strings.HasPrefix(domain, ".") && strings.HasSuffix(redirectURL.Host, domain)) {

529− return true

567+ domainHostname, domainPort := splitHostPort(strings.TrimLeft(domain, "."))

568+ if domainHostname == "" {

569+ continue

570+ }

571+

572+ if (redirectHostname == domainHostname) || (strings.HasPrefix(domain, ".") && strings.HasSuffix(redirectHostname, domainHostname)) {

573+ // the domain names match, now validate the ports

574+ // if the whitelisted domain's port is '*', allow all ports

575+ // if the whitelisted domain contains a specific port, only allow that port

576+ // if the whitelisted domain doesn't contain a port at all, only allow empty redirect ports ie http and https

577+ redirectPort := redirectURL.Port()

578+ if (domainPort == "*") ||

579+ (domainPort == redirectPort) ||

580+ (domainPort == "" && redirectPort == "") {

581+ return true

582+ }

530583 }

531584 }

585+

532586 return false

533587 default:

534588 return false

oauthproxy_test.go+146−37

@@ -182,49 +182,158 @@ func TestIsValidRedirect(t *testing.T) {

182182 opts.ClientSecret = "fgkdsgj"

183183 opts.CookieSecret = "ljgiogbj"

184184 // Should match domains that are exactly foo.bar and any subdomain of bar.foo

185− opts.WhitelistDomains = []string{"foo.bar", ".bar.foo"}

185+ opts.WhitelistDomains = []string{

186+ "foo.bar",

187+ ".bar.foo",

188+ "port.bar:8080",

189+ ".sub.port.bar:8080",

190+ "anyport.bar:*",

191+ ".sub.anyport.bar:*",

192+ }

186193 opts.Validate()

187194

188195 proxy := NewOAuthProxy(opts, func(string) bool { return true })

189196

190− noRD := proxy.IsValidRedirect("")

191− assert.Equal(t, false, noRD)

192−

193− singleSlash := proxy.IsValidRedirect("/redirect")

194− assert.Equal(t, true, singleSlash)

195−

196− doubleSlash := proxy.IsValidRedirect("//redirect")

197− assert.Equal(t, false, doubleSlash)

198−

199− validHTTP := proxy.IsValidRedirect("http://foo.bar/redirect")

200− assert.Equal(t, true, validHTTP)

201−

202− validHTTPS := proxy.IsValidRedirect("https://foo.bar/redirect")

203− assert.Equal(t, true, validHTTPS)

204−

205− invalidHTTPSubdomain := proxy.IsValidRedirect("http://baz.foo.bar/redirect")

206− assert.Equal(t, false, invalidHTTPSubdomain)

207−

208− invalidHTTPSSubdomain := proxy.IsValidRedirect("https://baz.foo.bar/redirect")

209− assert.Equal(t, false, invalidHTTPSSubdomain)

210−

211− validHTTPSubdomain := proxy.IsValidRedirect("http://baz.bar.foo/redirect")

212− assert.Equal(t, true, validHTTPSubdomain)

213−

214− validHTTPSSubdomain := proxy.IsValidRedirect("https://baz.bar.foo/redirect")

215− assert.Equal(t, true, validHTTPSSubdomain)

216−

217− invalidHTTP1 := proxy.IsValidRedirect("http://foo.bar.evil.corp/redirect")

218− assert.Equal(t, false, invalidHTTP1)

219−

220− invalidHTTPS1 := proxy.IsValidRedirect("https://foo.bar.evil.corp/redirect")

221− assert.Equal(t, false, invalidHTTPS1)

197+ testCases := []struct {

198+ Desc, Redirect string

199+ ExpectedResult bool

200+ }{

201+ {

202+ Desc: "noRD",

203+ Redirect: "",

204+ ExpectedResult: false,

205+ },

206+ {

207+ Desc: "singleSlash",

208+ Redirect: "/redirect",

209+ ExpectedResult: true,

210+ },

211+ {

212+ Desc: "doubleSlash",

213+ Redirect: "//redirect",

214+ ExpectedResult: false,

215+ },

216+ {

217+ Desc: "validHTTP",

218+ Redirect: "http://foo.bar/redirect",

219+ ExpectedResult: true,

220+ },

221+ {

222+ Desc: "validHTTPS",

223+ Redirect: "https://foo.bar/redirect",

224+ ExpectedResult: true,

225+ },

226+ {

227+ Desc: "invalidHTTPSubdomain",

228+ Redirect: "http://baz.foo.bar/redirect",

229+ ExpectedResult: false,

230+ },

231+ {

232+ Desc: "invalidHTTPSSubdomain",

233+ Redirect: "https://baz.foo.bar/redirect",

234+ ExpectedResult: false,

235+ },

236+ {

237+ Desc: "validHTTPSubdomain",

238+ Redirect: "http://baz.bar.foo/redirect",

239+ ExpectedResult: true,

240+ },

241+ {

242+ Desc: "validHTTPSSubdomain",

243+ Redirect: "https://baz.bar.foo/redirect",

244+ ExpectedResult: true,

245+ },

246+ {

247+ Desc: "validHTTPDomain",

248+ Redirect: "http://bar.foo/redirect",

249+ ExpectedResult: true,

250+ },

251+ {

252+ Desc: "invalidHTTP1",

253+ Redirect: "http://foo.bar.evil.corp/redirect",

254+ ExpectedResult: false,

255+ },

256+ {

257+ Desc: "invalidHTTPS1",

258+ Redirect: "https://foo.bar.evil.corp/redirect",

259+ ExpectedResult: false,

260+ },

261+ {

262+ Desc: "invalidHTTP2",

263+ Redirect: "http://evil.corp/redirect?rd=foo.bar",

264+ ExpectedResult: false,

265+ },

266+ {

267+ Desc: "invalidHTTPS2",

268+ Redirect: "https://evil.corp/redirect?rd=foo.bar",

269+ ExpectedResult: false,

270+ },

271+ {

272+ Desc: "invalidPort",

273+ Redirect: "https://evil.corp:3838/redirect",

274+ ExpectedResult: false,

275+ },

276+ {

277+ Desc: "invalidEmptyPort",

278+ Redirect: "http://foo.bar:3838/redirect",

279+ ExpectedResult: false,

280+ },

281+ {

282+ Desc: "invalidEmptyPortSubdomain",

283+ Redirect: "http://baz.bar.foo:3838/redirect",

284+ ExpectedResult: false,

285+ },

286+ {

287+ Desc: "validSpecificPort",

288+ Redirect: "http://port.bar:8080/redirect",

289+ ExpectedResult: true,

290+ },

291+ {

292+ Desc: "invalidSpecificPort",

293+ Redirect: "http://port.bar:3838/redirect",

294+ ExpectedResult: false,

295+ },

296+ {

297+ Desc: "validSpecificPortSubdomain",

298+ Redirect: "http://foo.sub.port.bar:8080/redirect",

299+ ExpectedResult: true,

300+ },

301+ {

302+ Desc: "invalidSpecificPortSubdomain",

303+ Redirect: "http://foo.sub.port.bar:3838/redirect",

304+ ExpectedResult: false,

305+ },

306+ {

307+ Desc: "validAnyPort1",

308+ Redirect: "http://anyport.bar:8080/redirect",

309+ ExpectedResult: true,

310+ },

311+ {

312+ Desc: "validAnyPort2",

313+ Redirect: "http://anyport.bar:8081/redirect",

314+ ExpectedResult: true,

315+ },

316+ {

317+ Desc: "validAnyPortSubdomain1",

318+ Redirect: "http://a.sub.anyport.bar:8080/redirect",

319+ ExpectedResult: true,

320+ },

321+ {

322+ Desc: "validAnyPortSubdomain2",

323+ Redirect: "http://a.sub.anyport.bar:8081/redirect",

324+ ExpectedResult: true,

325+ },

326+ }

222327

223− invalidHTTP2 := proxy.IsValidRedirect("http://evil.corp/redirect?rd=foo.bar")

224− assert.Equal(t, false, invalidHTTP2)

328+ for _, tc := range testCases {

329+ t.Run(tc.Desc, func(t *testing.T) {

330+ result := proxy.IsValidRedirect(tc.Redirect)

225331

226− invalidHTTPS2 := proxy.IsValidRedirect("https://evil.corp/redirect?rd=foo.bar")

227− assert.Equal(t, false, invalidHTTPS2)

332+ if result != tc.ExpectedResult {

333+ t.Errorf("expected %t got %t", tc.ExpectedResult, result)

334+ }

335+ })

336+ }

228337}

229338

230339type TestProvider struct {