Add support for a list of Whitelisted domains #464

JoelSpeed · 2017-10-02T09:56:28Z

I wrote this on Friday to fix #399

Tidied it up this morning and went to create PR noticing #461 had been created.

I think this better fixes #399 as it looks for domain suffixes rather than explicit domains which I think was the point of the discussion.

Take from this what you will

ploxiln · 2017-12-01T19:12:54Z

main.go

@@ -43,6 +44,7 @@ func main() {
 	flagSet.Bool("ssl-insecure-skip-verify", false, "skip validation of certificates presented when using HTTPS")

 	flagSet.Var(&emailDomains, "email-domain", "authenticate emails with the specified domain (may be given multiple times). Use * to authenticate any email")
+	flagSet.Var(&whitelistDomains, "whitelist-domains", "allowed domains for redirection after authentication")


the variable is plural, but the option name string should be singular "whitelist-domain" because it will be used like --whitelist-domain=.first.com --whitelist-domain=.second.com

ploxiln · 2017-12-01T19:14:43Z

oauthproxy.go

+		redirect = strings.TrimPrefix(redirect, "https://")
+		redirect = strings.Split(redirect, "/")[0]
+		for _, domain := range p.whitelistDomains {
+			if strings.HasSuffix(redirect, domain) {


This logic is repetitive with the case above. You could consider using url.Parse()

You could also consider protecting users from a silly mistake by only doing a suffix match if the whitelisted domain starts with ., otherwise doing an exact match.

JoelSpeed · 2017-12-11T09:37:18Z

@ploxiln I have addressed your feedback and rebased onto the current master branch. Thanks for your comments

ploxiln · 2017-12-11T18:16:46Z

This looks like a reasonable implementation to me. I'm not a maintainer though, I just try to help out. I think we'll have to wait for @hlhendy's judgement.

The test failures appear to not be related, they're in TestValidatorOverwriteEmailListVia...

One other thing that is typically asked of PRs in this project is that you squash down your commits into one.

bouk · 2017-12-22T15:20:28Z

It would be nice if you could add support for setting it though the env also

JoelSpeed · 2018-01-02T09:28:20Z

@bouk Happy to add environment support, looks to me like I just need to add an env struct tag for the new flag, something like OAUTH2_PROXY_WHITELIST_DOMAINS?

However it looks at the moment as if the environment loader can only load strings where this needs to load a list of strings, unless I've missed something?

oauth2_proxy/env_options.go

Lines 31 to 33 in d75f626

    
           if v != "" { 
        
           	cfg[cfgName] = v 
        
           }

bouk · 2018-01-08T08:56:32Z

The underlying library has support for string slices, so adding the appropriate tag should just work

Quentin-M · 2018-01-15T02:01:06Z

main.go

@@ -43,6 +44,7 @@ func main() {
 	flagSet.Bool("ssl-insecure-skip-verify", false, "skip validation of certificates presented when using HTTPS")

 	flagSet.Var(&emailDomains, "email-domain", "authenticate emails with the specified domain (may be given multiple times). Use * to authenticate any email")
+	flagSet.Var(&whitelistDomains, "whitelist-domain", "allowed domains for redirection after authentication")


s/whitelist-domain/whitelist-domains/?

See "email-domain" just above. It's singular because you specify one at a time: --whitelist-domain=first.com --whitelist-domain=second.com

Quentin-M · 2018-01-15T02:15:24Z

options.go

@@ -32,6 +32,7 @@ type Options struct {
 	AuthenticatedEmailsFile  string   `flag:"authenticated-emails-file" cfg:"authenticated_emails_file"`
 	AzureTenant              string   `flag:"azure-tenant" cfg:"azure_tenant"`
 	EmailDomains             []string `flag:"email-domain" cfg:"email_domains"`
+	WhitelistDomains         []string `flag:"whitelist-domains" cfg:"whitelist_domains"`


If https://github.com/bitly/oauth2_proxy/pull/464/files#r161420105, then s/flag:"whitelist-domains/flag:"whitelist-domain/

oauth2_proxy -whitelist-domain=localhost 2018/01/15 02:13:20 options.go:74: ERROR: flag "whitelist-domains" does not exist panic: ERROR: flag "whitelist-domains" does not exist goroutine 1 [running]: log.Panicf(0x90f438, 0x1d, 0xc420035ca0, 0x1, 0x1) /usr/local/go/src/log/log.go:333 +0xda github.com/mreiferson/go-options.Resolve(0x86e180, 0xc420086e00, 0xc4200f2360, 0xc42006f200) /go/src/github.com/mreiferson/go-options/options.go:74 +0xb11 main.main() /go/src/github.com/bitly/oauth2_proxy/main.go:103 +0x146b

JoelSpeed · 2018-01-18T10:24:39Z

I've pushed a commit that fixes the panic identified by @Quentin-M and adds the ability to set the whitelist domains from the environment with OAUTH2_PROXY_WHITELIST_DOMAINS as suggested by @bouk

bouk

looks great!

smitchelus · 2018-01-28T18:37:17Z

What's the plan for this PR? It solves the exact issue we've been fighting when attempting to use the oauth2_proxy as part of our k8s ingress infrastructure.

Quentin-M · 2018-01-28T18:53:31Z

@smitchelus Pretty sure it'll get merged. In the meantime: https://hub.docker.com/r/qmachu/oauth2_proxy/

JoelSpeed · 2018-01-28T20:15:09Z

@smitchelus I'm hoping this PR will get merged soon and I've also raised another PR that might interest you given your running K8s. #534 Adds support for the Authorization: Bearer <JWT> headers that the K8s dashboard expects.

The two PRs combined make the proxy perfect for authentication across multiple K8s clusters

smitchelus · 2018-01-28T21:52:15Z

Thanks for the update @JoelSpeed, @Quentin-M, and thanks for the image link as well. I've built it locally to confirm it worked for our scenario, but obviously look forward to getting it into an official release soon.

smitchelus · 2018-02-06T14:23:47Z

@Quentin-M, just now getting back to this. I tried pulling your image qmachu/oauth2_proxy, but it is telling me flag provided but not defined: -whitelist-domains. It doesn't appear that there's a particular tag I should be pulling. Thoughts on what I'm doing wrong?

JoelSpeed · 2018-02-06T14:28:01Z

@smitchelus try -whitelist-domain (singular) instead? It was changed between opening the PR and that image being built I believe

smitchelus · 2018-02-06T14:35:27Z

Good thought, but no dice: flag provided but not defined: -whitelist-domain

JoelSpeed · 2018-02-06T14:51:30Z

@smitchelus I've just built an image and tested the flag from this PR, try this image quay.io/joelspeed/oauth2_proxy:whitelist-domains-1

smitchelus · 2018-02-06T15:33:06Z

Thanks @JoelSpeed! I appreciate the help.

Quentin-M · 2018-02-06T15:49:41Z

Here is what I use fwiw:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: oauth2-proxy
  namespace: kube-system
  labels:
    k8s-app: oauth2-proxy
spec:
  replicas: 2
  selector:
    matchLabels:
      k8s-app: oauth2-proxy
  template:
    metadata:
      labels:
        k8s-app: oauth2-proxy
    spec:
      containers:
      - name: oauth2-proxy
        image: qmachu/oauth2_proxy:kubernetes
        args:
        - -provider=oidc
        - -client-id=oauth2_proxy
        - -oidc-issuer-url=https://ingress.{{ .domain }}/identity
        - -redirect-url=http://ingress.{{ .domain }}/oauth2/callback
        - -ssl-insecure-skip-verify=true
        - -whitelist-domain=ingress.{{ .domain }}
        - -email-domain=*
        - -cookie-name=_oauth2_{{ .name }}
        - -cookie-expire=24h
        #- -cookie-refresh=1h (https://github.com/bitly/oauth2_proxy/issues/523)
        - -cookie-secure=true
        - -cookie-domain=ingress.{{ .domain }}
        - -tls-cert=/etc/oauth2/tls/tls.crt
        - -tls-key=/etc/oauth2/tls/tls.key
        - -skip-provider-button=true
        - -footer=-
        env:
        - name: OAUTH2_PROXY_CLIENT_SECRET
          valueFrom:
            secretKeyRef:
              name: oauth2-proxy
              key: client.secret
        - name: OAUTH2_PROXY_COOKIE_SECRET
          valueFrom:
            secretKeyRef:
              name: oauth2-proxy
              key: cookie.secret
        volumeMounts:
        - name: tls
          mountPath: /etc/oauth2/tls
        ports:
        - name: https
          containerPort: 443
        livenessProbe:
          failureThreshold: 3
          httpGet:
            path: /ping
            port: 443
            scheme: HTTPS
          initialDelaySeconds: 10
          periodSeconds: 10
          successThreshold: 1
          timeoutSeconds: 1
      tolerations:
      - effect: NoSchedule
        key: node-role.kubernetes.io/master
      - key: CriticalAddonsOnly
        operator: Exists
      affinity:
        podAntiAffinity:
          preferredDuringSchedulingIgnoredDuringExecution:
          - weight: 100
            podAffinityTerm:
              labelSelector:
                matchExpressions:
                - key: k8s-app
                  operator: In
                  values:
                  - oauth2-proxy
              topologyKey: kubernetes.io/hostname
      volumes:
      - name: tls
        secret:
          secretName: oauth2-proxy

simonvanderveldt · 2018-02-26T15:40:19Z

First of all thanks for this PR! Would really like to just have a single oauth2 proxy for several internal applications :)
I've just tried out this PR, in this case on oauth2proxy.somedomain.com, that should handle signin for multiple applications.
For the internal application I'm using someapp.localhost as a testcase, so I added whitelist_domains = [ ".localhost" ] to my oauth2_proxy.cfg.

I'm using nginx as described in the readme to handle authentication using the auth_request directive with the following changes:

Replaced proxy_pass http://127.0.0.1:4180; with proxy_pass oauth2proxy.somedomain.com;
Changed changed error_page 401 = /oauth2/sign_in; to error_page 401 = /oauth2/start?rd=http://$host$request_uri;

The whole flow through my OAuth provider (GitHub) works and the redirect url is being passed correctly in the state parameter.
Unfortunately once I'm redirected to oauth2proxy.somedomain.com/oauth2/callback I'm presented with a 403 and the message http: named cookie not present even though the _oauth2_proxy cookie is present for that domain.

Am I doing something wrong? Could it be because of the different domain name?
Or is there still something missing on the oauth2_proxy side?

JoelSpeed · 2018-02-26T15:58:27Z

@simonvanderveldt What values do you have for cookie_domain, cookie_name and cookie_secure? By my reckoning the cookie_domain=.somedomain.com and cookie_name is presently the default? You might need to set cookie_secure as well if your oauth2_proxy is behind https

Migration from Bitly to Pusher

Release 3.0.0

JoelSpeed · 2019-01-15T13:28:06Z

Now that @pusher have taken over the repository maintenance, I am closing this in favour of pusher#15

madmod mentioned this pull request Dec 1, 2017

External redirect urls #461

Open

ploxiln reviewed Dec 1, 2017

View reviewed changes

madmod mentioned this pull request Dec 1, 2017

/signin does not pass through any redirect URL #400

Open

JoelSpeed force-pushed the whitelist-domains branch from e9c9e0a to 2652c23 Compare December 11, 2017 09:35

bouk approved these changes Dec 22, 2017

View reviewed changes

Quentin-M reviewed Jan 15, 2018

View reviewed changes

bouk approved these changes Jan 18, 2018

View reviewed changes

ploxiln mentioned this pull request Feb 10, 2018

Added allowed-url option for secure allowance of custom redirection URLs #544

Open

icchy mentioned this pull request Feb 14, 2018

rd param allows other domain #547

Closed

simonvanderveldt mentioned this pull request Feb 16, 2018

Is it possible to use wildcard subdomains in combination with GitHub as OAuth provider? #550

Open

JoelSpeed added 25 commits December 20, 2018 11:06

Add Dockerfile

7fa913e

Add Issue and Pull Request templates

8564ab6

Add CONTRIBUTING guide

3253bef

Add docker image note to README

52f27f7

Fix Quay link

39d11b4

Add CODEOWNERS file

381e878

Remove Go v1.8.x from Travis CI

9096c70

Introduce Makefile

372ecd0

Fix CI after make introduction

eded761

Move dep to GoPath in CI setup

1dddd81

Disable make parallelism

85d76be

Remove .env file

2eb2754

Fix repo link

f80ce24

Merge pull request #7 from pusher/migration

e1f45dd

Migration from Bitly to Pusher

Release v3.0.0

d472cf1

Update release target

d085944

Fix changelog PR link

9e9b1f9

Merge pull request #13 from pusher/release-3.0.0

7887272

Release 3.0.0

Add whitelist domains flag

fb49e68

Test IsValidRedirect method

d1ad95d

Make option name singular

c8bcd9b

Remove duplicated logic

a925187

Test explicit subdomain whitelisting

ddf1ded

Add env option

e74d43c

Add note on subdomain behaviour

d795222

JoelSpeed force-pushed the whitelist-domains branch from ce2c25b to d795222 Compare January 15, 2019 11:57

JoelSpeed mentioned this pull request Jan 15, 2019

Whitelist domains oauth2-proxy/oauth2-proxy#15

Merged

3 tasks

JoelSpeed closed this Jan 15, 2019

Miouge1 mentioned this pull request Jan 17, 2019

[stable/oauth2-proxy] Switch to pusher/oauth2_proxy helm/charts#10740

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for a list of Whitelisted domains #464

Add support for a list of Whitelisted domains #464

JoelSpeed commented Oct 2, 2017

ploxiln Dec 1, 2017

ploxiln Dec 1, 2017

JoelSpeed commented Dec 11, 2017

ploxiln commented Dec 11, 2017

bouk commented Dec 22, 2017

JoelSpeed commented Jan 2, 2018

bouk commented Jan 8, 2018 •

edited

Loading

Quentin-M Jan 15, 2018

ploxiln Jan 15, 2018

Quentin-M Jan 15, 2018 •

edited

Loading

JoelSpeed commented Jan 18, 2018

bouk left a comment

smitchelus commented Jan 28, 2018

Quentin-M commented Jan 28, 2018

JoelSpeed commented Jan 28, 2018 •

edited

Loading

smitchelus commented Jan 28, 2018

smitchelus commented Feb 6, 2018

JoelSpeed commented Feb 6, 2018

smitchelus commented Feb 6, 2018

JoelSpeed commented Feb 6, 2018

smitchelus commented Feb 6, 2018

Quentin-M commented Feb 6, 2018

simonvanderveldt commented Feb 26, 2018 •

edited

Loading

JoelSpeed commented Feb 26, 2018

JoelSpeed commented Jan 15, 2019

Add support for a list of Whitelisted domains #464

Add support for a list of Whitelisted domains #464

Conversation

JoelSpeed commented Oct 2, 2017

ploxiln Dec 1, 2017

Choose a reason for hiding this comment

ploxiln Dec 1, 2017

Choose a reason for hiding this comment

JoelSpeed commented Dec 11, 2017

ploxiln commented Dec 11, 2017

bouk commented Dec 22, 2017

JoelSpeed commented Jan 2, 2018

bouk commented Jan 8, 2018 • edited Loading

Quentin-M Jan 15, 2018

Choose a reason for hiding this comment

ploxiln Jan 15, 2018

Choose a reason for hiding this comment

Quentin-M Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

JoelSpeed commented Jan 18, 2018

bouk left a comment

Choose a reason for hiding this comment

smitchelus commented Jan 28, 2018

Quentin-M commented Jan 28, 2018

JoelSpeed commented Jan 28, 2018 • edited Loading

smitchelus commented Jan 28, 2018

smitchelus commented Feb 6, 2018

JoelSpeed commented Feb 6, 2018

smitchelus commented Feb 6, 2018

JoelSpeed commented Feb 6, 2018

smitchelus commented Feb 6, 2018

Quentin-M commented Feb 6, 2018

simonvanderveldt commented Feb 26, 2018 • edited Loading

JoelSpeed commented Feb 26, 2018

JoelSpeed commented Jan 15, 2019

bouk commented Jan 8, 2018 •

edited

Loading

Quentin-M Jan 15, 2018 •

edited

Loading

JoelSpeed commented Jan 28, 2018 •

edited

Loading

simonvanderveldt commented Feb 26, 2018 •

edited

Loading