Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
benjsto merged 4 commits into from
Nov 30, 2018
Merged

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

benjsto 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.

danbf
danbf reviewed Nov 27, 2018
View reviewed changes
internal/proxy/options.go
Cluster string `envconfig:"CLUSTER"`
Scheme string `envconfig:"SCHEME" default:"https"`
ProviderURLString string `envconfig:"PROVIDER_URL"`
ProviderURLInternalString string `envconfig:"PROVIDER_URL_INTERNAL"`
Copy link
Contributor

@danbf danbf Nov 27, 2018

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:

sso/quickstart/docker-compose.yml

Line 32 in 6db5495

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

Copy link
Contributor

@danbf danbf Nov 27, 2018

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

sso/quickstart/kubernetes/sso-proxy-deployment.yml

Lines 26 to 27 in 6db5495

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

Copy link
Contributor Author

@benjsto benjsto Nov 30, 2018

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.

@danbf danbf mentioned this pull request Nov 29, 2018
Closed
@benjsto benjsto force-pushed the benjsto/internal-proxy-provider-url branch 2 times, most recently from 203aece to 979657b Compare November 30, 2018 01:44
@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 •
edited
Loading

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.

danbf
danbf approved these changes Nov 30, 2018
View reviewed changes
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

Reviewers

@danbf danbf danbf approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@benjsto @danbf @loganmeetsworld