-
Notifications
You must be signed in to change notification settings - Fork 326
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
Initial ADFS support #68
Conversation
I very much appreciate the effort and I'm excited to get ADFS support in place. As I think you suspect, we need to clean up your code and bring it a bit closer to doing things a bit more Go like. I suggest taking a look at Marshall/Unmarshall in viper and see how Did you see any of the Azure AD items in the upstream library? I'll look a bit closer at what you've offered over the next couple days and offer thoughts Thanks again Simon! |
Hi! I really hope the code is corrected and cleaned before it’s included in the project. My hope is that you get the necessary information about the ADFS-integration and will be able to get it included without too much hassle. I’ll help with the testing! I’m only doing this for a project that really needs it ASAP and your integration with nginx makes it so much easier than the others I’ve tried. But I’m no developer (got help from a co-worker, but none of us have ever used Golang) and have no illusions about that what I’ve written is bad. I do know ADFS, OIDC, nginx, Kubernetes and everything around that so I’ll try to contribute with what I can. 😁👍 ADFS and Azure AD have some differences, so this will probably not work with AAD. I haven’t tried it, but the OIDC type will most likely work. AAD is much easier to test than ADFS, so if you want it included I’ll set it up and test it (maybe even create another bad PR 😉). By the way, three questions:
As a last note, I’m the one who should say thanks. This project is great and I think it will have potential to be the go-to project instead of others to secure resources in the future. Thanks! 😁 |
@simongottschlag can you please try this branch and see if it works? |
Thanks for sending in the updates.
Can we keep them separate? Or create two new ones? They seem like
distinct features.
And I haven't really looked closely at #71 at this point or thought through
the implications.
…On Thu, Feb 7, 2019, 9:29 PM Simon Gottschlag ***@***.*** wrote:
@bnfinet <https://github.com/bnfinet> I think I've really messed up with
doing two PRs. Can we close this one and only use #71
<#71>? I've tried to do some
magic but seems like I'm not able to get it right in this one.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#68 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABNK6zW2cnW_b_hxKgy6WUS_L0Y9Lg_qks5vLQtUgaJpZM4alX4q>
.
|
@bnfinet I've removed the "cookie fix" and added the two sections (I hope) from the other PR. |
@bnfinet Can you merge the last changes I did here to it? After that, I'll be able to test it. I'm using the following right now and it is working as expected (both with ADFS and idToken sent to upstream): https://github.com/simongottschlag/vouch-proxy/tree/v0.4.9-ADFS-03 |
@simongottschlag I've updated the branch! |
Hi, I branched v0.4.9 and merged your branch with my fork (https://github.com/simongottschlag/vouch-proxy/tree/simongottschlag-adfs_get_username) and created a container in quay.io (quay.io/simongottschlag/vouch-proxy:simongottschlag-adfs_get_username). When using this container, it does seem to work as expected. See the logs below:
The version in the logs are corresponding to the commit in GitHub (9630d9b). I think this is working. :) |
Seems to be working just fine, but should most likely be looked over since I know nothing about Golang.