-
Notifications
You must be signed in to change notification settings - Fork 345
Migration to go-oidc from upstream v1 #407
Migration to go-oidc from upstream v1 #407
Conversation
This change aims to make use of coreos/go-oidc. The generic adapter aims to act as a sidecar, provide a way to SkipClientID is out of scope for now. That does not mean that we cannot revisit this in the near future.
Why github.com/coreos/go-oidc v1 and not v2? v1 seems to be a deprecated branch. |
@@ -27,6 +27,18 @@ | |||
packages = ["."] | |||
revision = "144418e1475d8bf7abbdc48583500f1a20c62ea7" | |||
|
|||
[[projects]] | |||
branch = "v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should probably use v2
if possible. Otherwise LGTM.
@jangaraj @slaskawi I understand your concern and it's a fair argument. Although, v1 and v2 are completely different APIs and there's a huge break of API compatibility. For example:
And they are completely different APIs, which requires a considerable amount of time migrating the codebase. Actually, we already have been working on it here: https://github.com/gambol99/keycloak-proxy/commits/develop. This proposal to target |
@abstractj Ok, sounds perfectly reasonable. BTW, I already approved this PR. |
Hello, Something changed since the merge, I've been using latest docker image with Azure AD Oauth2, it was working fine till 2 days ago. It looks like the new merged oidc makes a request with no version, and receives a v1 Azure AD response (sts.windows.net/xxxxxxx) which is not expected. I've tried to move to an old docker image, but I have the same problem. which I did'nt understand !!! Thanks for your help. |
@magnock I'm sorry but I'm not sure how this pull request relates with what you mentioned. Nothing was merged into master yet. |
hi @magnock ... perhaps something has changed upstream in Azure as I can't see anything that would have effected this between this and the last version here .. At the error is being thrown in the go-oidc lib which hasn't changed on master ... I recall their used to be an exception in go-oidc for google .. which used to do something similar, i.e. being issued a token outside of the issuer you called ... i did see coreos/go-oidc#121 .. I've not read it fully .. |
BTW: master build is failing - https://travis-ci.org/gambol99/keycloak-proxy/builds/420095894?utm_source=github_status&utm_medium=notification |
@jangaraj thanks for reporting this. I will take a look at what's going on. |
@magnock Did you figure out, where is a problem? There is a problem with endpoint url:
and
Like you said, seems that endpoint/api version is ignored |
@lukasmrtvy would you mind to file a Jira (https://issues.jboss.org/projects/KEYCLOAK) with a proper description, steps to reproduce and affected version? We moved from GH issues, and important things like this can be easily missed if commented on closed PRs. Thanks in advance. |
Apologies for resurfacing an old issue, however I'm getting the same error as @magnock @lukasmrtvy above with Azure AD:
Despite a v2 endpoint, a v1 token seems to be returned. Did anyone find a solution? All I can find is this closed issue: https://issues.redhat.com/browse/KEYCLOAK-8728 . I can create a new issue if feel that would be useful. Thanks. |
This change aims to make use of coreos/go-oidc. The generic adapter
aims to act as a sidecar, provide a way to SkipClientID is out of scope
for now.
That does not mean that we cannot revisit this in the near future.