Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sso-proxy: internal host should apply to redeem, /refresh, /validate, /profile #123

Merged
merged 4 commits into from
Nov 30, 2018

Conversation

benjsto
Copy link
Contributor

@benjsto benjsto commented Nov 26, 2018

Problem

Dan's PR added the ability to have an optional 'internal' host defined that proxy will use to connect to auth. However, that internal host was only applied on /redeem requests. Other cases where proxy makes a request to auth, like /refresh, /validate, or /profile should also use this internal host if it is provided.

Solution

If the optional PROVIDER_URL_INTERNAL is set, use that host to connect to auth for /redeem, /refresh, /validate, and /profile.

Cluster string `envconfig:"CLUSTER"`
Scheme string `envconfig:"SCHEME" default:"https"`
ProviderURLString string `envconfig:"PROVIDER_URL"`
ProviderURLInternalString string `envconfig:"PROVIDER_URL_INTERNAL"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not make a breaking change here in the env-var name if possible even if we do it internally to make things clearer. if we do change things, let's make sure we update the things like the quickstart:

- PROXY_PROVIDER_URL=http://host.docker.internal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we try to re-jigger the kubernetes quick start here

- name: PROVIDER_URL
value: https://sso-auth.mydomain.com
to enable the internal connection between sso-proxy->sso-auth ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, good idea - added.

@danbf
Copy link
Contributor

danbf commented Nov 27, 2018

@benjsto thanks much for finding these. i like your solution in here neater then the one i pushed earlier.

@benjsto benjsto force-pushed the benjsto/internal-proxy-provider-url branch from 979657b to ec6cd33 Compare November 30, 2018 01:49
@benjsto
Copy link
Contributor Author

benjsto commented Nov 30, 2018

Thanks @danbf for catching that the Host header on requests to sso-auth still need to use the non-internal host, as that service specifically looks only for that Host header and will 404 without.

Copy link
Contributor

@danbf danbf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@benjsto benjsto merged commit c0ce81d into buzzfeed:master Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants