harbor icon indicating copy to clipboard operation
harbor copied to clipboard

feat: Disable CSRF check for "/c/oidc/onboard" API for authenticating and Onboarding a User via API from Custom CLI

Open Rajpratik71 opened this issue 2 years ago • 14 comments

Issue being fixed

Fixes #16966

Please indicate you've done the following:

  • [*] Well Written Title and Summary of the PR
  • [*] Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • [*] Accepted the DCO. Commits without the DCO will delay acceptance.
  • [*] Made sure tests are passing and test coverage is added if needed.
  • [*] Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

Rajpratik71 avatar Jun 09 '22 08:06 Rajpratik71

Codecov Report

Merging #16969 (5476e4d) into main (697f1c7) will decrease coverage by 22.76%. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #16969       +/-   ##
===========================================
- Coverage   67.39%   44.64%   -22.76%     
===========================================
  Files         984      235      -749     
  Lines      106980    13089    -93891     
  Branches     2670     2670               
===========================================
- Hits        72096     5843    -66253     
+ Misses      31004     6951    -24053     
+ Partials     3880      295     -3585     
Flag Coverage Δ
unittests 44.64% <ø> (-22.76%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

see 753 files with indirect coverage changes

codecov[bot] avatar Jun 09 '22 09:06 codecov[bot]

@reasonerjt requesting review

Rajpratik71 avatar Jun 10 '22 15:06 Rajpratik71

It would be great if it can be merged sooner, our solution is depending on this fix. Meanwhile, is there any workaround to onboard OIDC user through API ? Thanks in advance.

maheshsmartcow avatar Jun 20 '22 14:06 maheshsmartcow

@zyyw @reasonerjt requesting review

Rajpratik71 avatar Jun 25 '22 06:06 Rajpratik71

Maybe I am wrong but, as I understand it from @Rajpratik71 and other similar issues, is that they want to manage the user before he actually logs in to Harbor the first time. Examples are add the user to the different projects and grant them the right permissions.

Am I right @Rajpratik71 ?

Vad1mo avatar Jul 06 '22 07:07 Vad1mo

Hi @reasonerjt @Vad1mo

This is required to do:

  1. authentication with OIDC provider and
  2. user onboarding

via custom-made cli or curl request using API.

As mentioned in issue, In browser flow CSRF token got added in request, i.e. why it is happening fine but when calling direct API , csrf check is blocking.

i.e. exception was added in "csrfSkipper" function to skip csrf check for "/c/oidc/onboard" API.

Rajpratik71 avatar Jul 11 '22 19:07 Rajpratik71

@ywk253100 @wy65701436 @stonezdj @zyyw @daixiang0 @heww requesting review

Rajpratik71 avatar Jul 25 '22 18:07 Rajpratik71

I also need the same behavior. For my use case @Vad1mo is correct. If not this then a similar API should exist for backend flow

ashishkumar-07 avatar Aug 30 '22 12:08 ashishkumar-07

why blocked? we need this pr.

fredbjer avatar Sep 03 '22 02:09 fredbjer

Maybe I am wrong but, as I understand it from @Rajpratik71 and other similar issues, is that they want to manage the user before he actually logs in to Harbor the first time. Examples are add the user to the different projects and grant them the right permissions.

@ashishkumar-07 I think this may be a valid requirement but I'm not convinced this PR is the right way to solve the problem. To use this endpoint you need the ID token of a specific user, but admin can't do that b/c he doesn't know the credentials of this user. If we wanna support this case, we need to find a way to allow admin to onboard users without ID token, and for that purpose, I don't think we should use the current /c/onboard endpoint.

reasonerjt avatar Sep 07 '22 15:09 reasonerjt

@reasonerjt I think there is some confusion in understanding the scenario.

Here, we don't want this for admin user and don't needs to be done by admin user.

This step is required for self-service i.e signup to harbor using configured Oauth.

This is required for first time signing in of a User in Harbor with configured Oauth.

While from UI, First time signing in of a User in Harbor with configured Oauth got success because in request UI passes the CSRF Token after getting the token from configured Auth/Oauth server then using the token fires the "/c/oidc/onboard" API to get it onboarded in Harbor as a User.

From next time it is not required as user is already there in Harbor.

On the other hand

While from CLI/API , First time signing in of a User in Harbor with configured Oauth got failed because of missing CSRF Token after getting the token from configured Auth/Oauth server then using the token fires the "/c/oidc/onboard" API to get it onboarded in Harbor as a User.

But as CLI/API request doesn't add CSRF Header in requests i.e. the check is failing

Rajpratik71 avatar Sep 07 '22 18:09 Rajpratik71

Further , missed the today community meeting due to time zone confusion, was thinking to discuss this.

@reasonerjt and Harbor Team can we schedule a meeting to discuss this over call ?

Rajpratik71 avatar Sep 07 '22 18:09 Rajpratik71

@Rajpratik71 I would love to, I'll reach out to you on slack.

reasonerjt avatar Sep 08 '22 11:09 reasonerjt

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

github-actions[bot] avatar Nov 08 '22 09:11 github-actions[bot]

This PR was closed because it has been stalled for 30 days with no activity. If this PR is still relevant, please re-open a new PR against main.

github-actions[bot] avatar Dec 09 '22 09:12 github-actions[bot]

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

github-actions[bot] avatar Feb 09 '23 09:02 github-actions[bot]

Any updates? We also need this pr.

WeiXie96 avatar Mar 13 '23 09:03 WeiXie96

Is it still possible that it can be merged sooner or later? We want to onboard OIDC users through CLI but not GUI, and this fix can solve our problem.

WeiXie96 avatar Mar 14 '23 10:03 WeiXie96

Sorry for asking: can it still be merged into harbor's main branch? And will there be a release that includes this change then? Otherwise we need to build harbor again from the source code.

WeiXie96 avatar Mar 17 '23 09:03 WeiXie96

I build images from this commit with CSRF disabled I try to create a user and get an error

curl -X 'POST' 'https://harbor-registry.xxxx/c/oidc/onboard' -H 'Content-Type: application/json' -H 'Authorization: Basic YWRtaW46SGFyYm9yMTIzU=' -d '{"username": "testuser" }'

{"errors":[{"code":"BAD_REQUEST","message":"Failed to get OIDC user info from session"}]}

At the same time, everything onboard works through the GUI. I use keycloack

what else is needed?

in harbor logs

[DEBUG] [/server/middleware/security/basic_auth.go:79][requestID="266741f532898a29f1e2ecf1083a980f"]: a basic auth security context generated for request POST /c/oidc/onboard [DEBUG] [/lib/http/error.go:61]: {"errors":[{"code":"BAD_REQUEST","message":"Failed to get OIDC user info from session"}]}

Vovanys avatar Mar 29 '23 14:03 Vovanys

@goharbor/all-maintainers can you check please

OrlinVasilev avatar Mar 29 '23 16:03 OrlinVasilev

My error message is still {"errors":[{"code":"FORBIDDEN","message":"CSRF token invalid"}]}, any updates on this issue??

WeiXie96 avatar Apr 03 '23 07:04 WeiXie96

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

github-actions[bot] avatar Jun 05 '23 09:06 github-actions[bot]

This PR was closed because it has been stalled for 30 days with no activity. If this PR is still relevant, please re-open a new PR against main.

github-actions[bot] avatar Jul 05 '23 09:07 github-actions[bot]