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

Multiple oauth configs #345

Closed
wants to merge 19 commits into from

Conversation

chiefbrain
Copy link

I did a quick hack regarding #302:

  • multiple oauth configurations (services) can be provided
  • each domain need to specify its desired service. if no domain is specified, the first service is used.

An old config needs to changed changed from

vouch:
  domains:
    - example1.tld
    - example2.tld
oauth:
  provider: oidc

to the following config

vouch:
  domains:
    - uri: example1.tld
      service: oidc_service
    - uri: example2.tld
      service: oidc_service
oauth:
  services:
    id: oidc_service
    provider: oidc

Domains can be specified using the environment using
VOUCH_DOMAINS=uri=example1.tld;service=oidc_service,uri=example2.tld;service=oidc_service.

I'm not sure what to do with the env configuration for oauth.

  • Allow only one oauth service in env (which updates one service by id or all services)
  • Use syntax like Domains.
  • Loop through env vars manually with the following format
OAUTH_oidcService_Provider= oidc
OAUTH_oidcService_ClientSecret = secret

@bnfinet
Copy link
Member

bnfinet commented Dec 22, 2020

@chiefbrain Thank you for putting time and effort into VP

By my reckoning this PR solves a different problem than #302, which is meant to correlate and discover the proper /auth endpoint for each vouch.domains specified. This PR seems focused on allowing multiple IdPs to service different hosts/domains. Do I have that right?

Why not use multiple Vouch Proxy instances? VP is pretty light. The docker container comes in at 12MB.

This PR changes the usage of the configuration element vouch.domains and the env var $VOUCH_DOMAINS. This will break existing setups. In general it's preferable to "not break user space" without a clear reason (usually security related).

Could you please elaborate on the problem you are having and why this is your preferred solution?

@chiefbrain
Copy link
Author

chiefbrain commented Dec 23, 2020

I'm not sure what to do with the env configuration for oauth.
* Use syntax like Domains.

This is the current implementation.

By my reckoning this PR solves a different problem than #302, which is meant to correlate and discover the proper /auth endpoint for each vouch.domains specified. This PR seems focused on allowing multiple IdPs to service different hosts/domains. Do I have that right?

Yes, you are right, this does not solve #302. Since domains are not a simple list any more, I think a callback_url can be added easily.

Why not use multiple Vouch Proxy instances? VP is pretty light. The docker container comes in at 12MB.
Could you please elaborate on the problem you are having and why this is your preferred solution?

To allow an IdP to differentiate between subdomains, one vouch instance has to setup per subdomain and all nginx hosts have to point to their instance. Since vouch can handle multiple domain, I like the idea of having only one vouch instance and using an nginx template for all domains.
Perhaps this solves also #114 for some people offloading access decision to the IdP.

This PR changes the usage of the configuration element vouch.domains and the env var $VOUCH_DOMAINS. This will break existing setups. In general it's preferable to "not break user space" without a clear reason (usually security related).

I thought the callback_uri change would need a config break, so why not use it ;)
This has to be done in a major release of course.

@bnfinet
Copy link
Member

bnfinet commented Jan 5, 2021

@chiefbrain Happy New Year!

as per #308 (fixes #302) I don't think there is a need for a config change.

I'm going to close this. Running multiple VP instances is the preferred solution here. I just don't feel that its onerous to run multiple VPs on modern hardware.

Can you please review these instructions before opening a PR in the future. I hate to see so much effort not result in moving VP forward.
https://github.com/vouch/vouch-proxy#submitting-a-pull-request-for-a-new-feature

Never the less, please accept my thanks for offering your time and attention to Vouch Proxy.

@bnfinet bnfinet closed this Jan 5, 2021
@chiefbrain
Copy link
Author

@bnfinet Happy new Year

[...] offloading access decision to the IdP.

This is just wrong, but led me to the impression my implementation would allow/deny users based on client_id.
I can consider it as a nice finger exercise.

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.

2 participants