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

Add idtoken response #71

Closed

Conversation

simongottschlag
Copy link
Contributor

@simongottschlag simongottschlag commented Feb 7, 2019

Makes it possible to configure the following two headers:

  • idToken
  • accessToken

If they are empty in the configuration, nothing will be added. If configured, they will be added as response headers.

Example:

vouch:
  headers:
    idToken: x-vouch-idtoken

This will make it possible to use it in nginx like this:

auth_request_set $auth_resp_x_vouch_idtoken $upstream_http_x_vouch_idtoken;

And then use the upstream header to, for example, add it to the backend:

proxy_set_header Authorization "Bearer $auth_resp_x_vouch_idtoken";

@simongottschlag
Copy link
Contributor Author

Was required to add resource to the ADFS redirect, or else all claims won't be returned.

log.Info("configuring ADFS OAuth")
OAuthopts = oauth2.SetAuthURLParam("resource", GenOAuth.RedirectURL) // Needed or all claims won't be included
}

Copy link
Member

Choose a reason for hiding this comment

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

I suspect this should be in #68 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should! I'm going to try and move it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnfinet I've been able to add it to #68, but I don't think I'll be able to create a PR for idToken without ADFS support (since that's what I'm using).

@@ -21,8 +21,10 @@ import (

// VouchClaims jwt Claims specific to vouch
type VouchClaims struct {
Username string `json:"username"`
Sites []string `json:"sites"` // tempting to make this a map but the array is fewer characters in the jwt
Username string `json:"username"`
Copy link
Member

Choose a reason for hiding this comment

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

I've been wondering if this should be sub instead of username. Do you have an opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No opinion from my side! :)

@bnfinet
Copy link
Member

bnfinet commented Feb 13, 2019

sorry for the delay in reviewing.

re: headers.idToken and headers.accessToken

could you please add corresponding entries to config/config.yml_example and describe the utility, usage and any security concerns which might be warranted?

@simongottschlag
Copy link
Contributor Author

I've added it to the config.yml_example as well as comments

@bnfinet
Copy link
Member

bnfinet commented Feb 14, 2019

@simongottschlag thanks again for the contribution. I've made some small adjustments, mostly cosmetic. Could you please test this branch:
https://github.com/vouch/vouch-proxy/pull/new/simongottschlag-add_idtoken_response

@simongottschlag
Copy link
Contributor Author

simongottschlag commented Feb 16, 2019

I tried it but didn't work. Haven't figured out why yet.
Edit: My bad, it does work - just tried it and seems fine.

@simongottschlag
Copy link
Contributor Author

simongottschlag commented Feb 16, 2019

By the way, seems like this one should be idpIDToken instead of idpAccessToken: (line 360)

vouch-proxy/pkg/cfg/cfg.go

Lines 359 to 361 in c28fe55

if !viper.IsSet(Branding.LCName + ".headers.idpIDToken") {
Cfg.Headers.IdpAccessToken = ""
}

Tried to change it but didn't make a difference. Need to look at it deeper another day.
Edit: Does work.

@simongottschlag
Copy link
Contributor Author

Hi,

After testing it again today, it does seem to work. Make sure the change with viper is fixed.

bnfinet added a commit that referenced this pull request Feb 18, 2019
@bnfinet
Copy link
Member

bnfinet commented Feb 18, 2019

thanks so much Simon, I've adjusted the branch to remove those config items. Golang by default will initlaze and element of the struct that is not set to its "zero value". For strings, the zero value is an empty string mystring == "". So it should be safe to remove those.

Could you test one more time and then I'll merge?

@simongottschlag
Copy link
Contributor Author

simongottschlag commented Feb 19, 2019

Hi,

I had to add to do the following change to get it to work: #78

user.IDToken = string(tokenRes.IDToken)
user.AccessToken = string(tokenRes.AccessToken)

Please remember that I've only tested with ADFS and someone else needs to verify that it works with other providers.

@bnfinet
Copy link
Member

bnfinet commented Feb 19, 2019

Hey Simon,

I need to look a little closer at how User is getting populated and it's elements moved around. In order to extend this feature to all IdPs it may be best to establish an interface and have each ${IdP}User conform.

I'm traveling this week and next so I can't promise that it will happen in short order but I think this is going to be best for the longer term architecture of the user object.

Thanks much!

@pcaro
Copy link

pcaro commented Apr 22, 2019

+1 for this

@bnfinet
Copy link
Member

bnfinet commented May 22, 2019

some of this functionality was implemented in #104 by @artagel

@simongottschlag I'd love it if you cared to chime in on #111 which is more along the lines of where you were storing the info in the User object

But since the upstream tokens can now being passed in the headers I think it fulfills your original requirement. Do let us know if that's not the case.

@bnfinet bnfinet closed this May 22, 2019
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