a12bae3 · typeable/oauth2-proxy

git.iko.soy/typeable/oauth2-proxy

update port whitelisting rules, refactor IsValidRedirect tests

commita12bae35ca0a19e6571ca4ca4ea5497d6f1c31d5 authorKamal Nasser <hello@kamal.io> dateWed 23 Oct 2019, 16:38 (7 years ago) parentae4e915

2 files changed+193−67

compare with master

unifiedsplit

oauthproxy.go+47−9 oauthproxy_test.go+146−58

oauthproxy.go+47−9

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

494494 return

495495}

496496

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

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

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

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

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

502+ host = hostport

503+

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

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

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

507+ }

508+

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

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

511+ }

512+

513+ return

514+}

515+

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

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

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

519+func validOptionalPort(port string) bool {

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

521+ return true

522+ }

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

524+ return false

525+ }

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

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

528+ return false

529+ }

530+ }

531+ return true

532+}

533+

497534// IsValidRedirect checks whether the redirect URL is whitelisted

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

499536 switch {

@@ -507,19 +544,20 @@ func (p *OAuthProxy) IsValidRedirect(redirect string) bool {

507544 redirectHostname := redirectURL.Hostname()

508545

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

510− domainURL := url.URL{

511− Host: strings.TrimLeft(domain, "."),

512− }

513− domainHostname := domainURL.Hostname()

514− if domainHostname == "" {

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

548+ if err != nil || domainHostname == "" {

515549 continue

516550 }

517551

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

519− // if the domain has a port, only allow that port

520− // otherwise allow any port

521− domainPort := domainURL.Port()

522− if (domainPort == "") || (domainPort == redirectURL.Port()) {

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

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

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

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

557+ redirectPort := redirectURL.Port()

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

559+ (domainPort == redirectPort) ||

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

523561 return true

524562 }

525563 }

oauthproxy_test.go+146−58

@@ -182,70 +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", "port.bar:8080", ".sub.port.bar:8080"}

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)

222−

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

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

225−

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

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

228−

229− invalidPort := proxy.IsValidRedirect("https://evil.corp:3838/redirect")

230− assert.Equal(t, false, invalidPort)

231−

232− validAnyPort := proxy.IsValidRedirect("http://foo.bar:3838/redirect")

233− assert.Equal(t, true, validAnyPort)

234−

235− validAnyPortSubdomain := proxy.IsValidRedirect("http://baz.bar.foo:3838/redirect")

236− assert.Equal(t, true, validAnyPortSubdomain)

237−

238− validSpecificPort := proxy.IsValidRedirect("http://port.bar:8080/redirect")

239− assert.Equal(t, true, validSpecificPort)

240−

241− invalidSpecificPort := proxy.IsValidRedirect("http://port.bar:3838/redirect")

242− assert.Equal(t, false, invalidSpecificPort)

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+ }

243327

244− validSpecificPortSubdomain := proxy.IsValidRedirect("http://foo.sub.port.bar:8080/redirect")

245− assert.Equal(t, true, validSpecificPortSubdomain)

328+ for _, tc := range testCases {

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

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

246331

247− invalidSpecificPortSubdomain := proxy.IsValidRedirect("http://foo.sub.port.bar:3838/redirect")

248− assert.Equal(t, false, invalidSpecificPortSubdomain)

332+ if result != tc.ExpectedResult {

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

334+ }

335+ })

336+ }

249337}

250338

251339type TestProvider struct {