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

update goth to add pkce support #21426

Closed
wants to merge 8 commits into from

Conversation

techknowlogick
Copy link
Member

@techknowlogick techknowlogick commented Oct 12, 2022

fix #21376

credit to @zeripath for the PR to goth

@techknowlogick techknowlogick added the type/enhancement An improvement of existing functionality label Oct 12, 2022
@techknowlogick techknowlogick added this to the 1.18.0 milestone Oct 12, 2022
@zeripath
Copy link
Contributor

I think we need to add a few more things to our openconnectid to hook it in

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 12, 2022
@mhkarimi1383
Copy link

CI is failing...
from the logs:

Please run 'make tidy' and commit the result

@Gusted
Copy link
Contributor

Gusted commented Oct 14, 2022

CI failure still related, likely due to the changes in assets/go-licenses.json

@mhkarimi1383
Copy link

Any update on this?

@lunny
Copy link
Member

lunny commented Oct 21, 2022

I think we need to add a few more things to our openconnectid to hook it in

What did you mean? Should we have more changes in this PR?

@lunny lunny modified the milestones: 1.18.0, 1.19.0 Oct 24, 2022
@silverwind
Copy link
Member

Might want to update to v1.75.1.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 7, 2022
@lafriks lafriks added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Nov 7, 2022
@zeripath
Copy link
Contributor

zeripath commented Nov 7, 2022

I think we need to add a few more things to our openconnectid to hook it in

What did you mean? Should we have more changes in this PR?

Yes, AFAIU we will need some more things in the openconnectid code to ensure that the PKCE stuff is hooked in.

@mhkarimi1383
Copy link

I think we need to add a few more things to our openconnectid to hook it in

What did you mean? Should we have more changes in this PR?

Yes, AFAIU we will need some more things in the openconnectid code to ensure that the PKCE stuff is hooked in.

If you can give me a test docker image I can test it for git.karimi.dev (I have LogTo and gitea)

@wolfogre
Copy link
Member

wolfogre commented Nov 9, 2022

I think we need to add a few more things to our openconnectid to hook it in

What did you mean? Should we have more changes in this PR?

Yes, AFAIU we will need some more things in the openconnectid code to ensure that the PKCE stuff is hooked in.

If you can give me a test docker image I can test it for git.karimi.dev (I have LogTo and gitea)

@mhkarimi1383

You could try this image docker.io/wolfogre/gitea-dev:pr21426, please note that it's for testing only.

@mhkarimi1383
Copy link

I think we need to add a few more things to our openconnectid to hook it in

What did you mean? Should we have more changes in this PR?

Yes, AFAIU we will need some more things in the openconnectid code to ensure that the PKCE stuff is hooked in.

If you can give me a test docker image I can test it for git.karimi.dev (I have LogTo and gitea)

@mhkarimi1383

You could try this image docker.io/wolfogre/gitea-dev:pr21426, please note that it's for testing only.

@wolfogre

the problem persist

gitea       | 2022/11/09 21:08:56 [636ba668] router: completed GET /user/oauth2/Logto for 51.89.252.106:0, 307 Temporary Redirect in 3.6ms @ auth/oauth.go:801(auth.SignInOAuth)
gitea       | 2022/11/09 21:08:56 ...rs/web/auth/oauth.go:860:SignInOAuthCallback() [I] [636ba668-2] Failed OAuth callback: (invalid_request) Authorization Server policy requires PKCE to be used for this request
gitea       | 2022/11/09 21:08:56 [636ba668-2] router: completed GET /user/oauth2/Logto/callback?error=invalid_request&error_description=Authorization%20Server%20policy%20requires%20PKCE%20to%20be%20used%20for%20this%20request&state=39f9aeb6-70d4-4724-aa4b-7674620d762c&iss=https%3A%2F%2Fauth.karimi.dev%2Foidc for 51.89.252.106:0, 303 See Other in 2.6ms @ auth/oauth.go:835(auth.SignInOAuthCallback)
gitea       | 2022/11/09 21:08:57 [636ba669] router: completed GET /user/login for 51.89.252.106:0, 200 OK in 4.5ms @ auth/auth.go:152(auth.SignIn)

@mhkarimi1383
Copy link

Any updates?

@zeripath
Copy link
Contributor

conflicts resolved

@zeripath
Copy link
Contributor

Any updates?

I haven't had any time to do the work to add the PKCE code in.

@lunny
Copy link
Member

lunny commented Jan 27, 2023

conflicts resolved

Looks like it's a blank PR now

@mhkarimi1383
Copy link

But issue for PKCE Persist

@jolheiser
Copy link
Member

jolheiser commented Jan 27, 2023

This (module update) has been superseded by #22410

Note that, as @zeripath said, more work needs to be done for the actual PKCE part, but that is for another PR as I understand it.

(If that is not the case, feel free to re-open and add commits)

@jolheiser jolheiser closed this Jan 27, 2023
@techknowlogick techknowlogick deleted the update-goth branch January 27, 2023 15:32
@lunny lunny removed this from the 1.19.0 milestone Jan 28, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement PKCE for OpenID Connect - Unable to login with LogTo
10 participants