Skip to content

Fix auth config oidc scope regex #20483

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

Merged
merged 3 commits into from
Oct 24, 2024

Conversation

rlacko58
Copy link
Contributor

@rlacko58 rlacko58 commented May 25, 2024

Comprehensive Summary of your change

This PR fixes the OIDC Auth scopes validation on the auth config page to comply with RFC standards.

Example scopes that are not compatible currently with Harbor's validation:

RFC:
https://datatracker.ietf.org/doc/html/rfc6749#appendix-A.4
scope = scope-token ( SP scope-token )
scope-token = 1
( %x21 / %x23-5B / %x5D-7E )

A scope is composed of one or more scope-tokens separated by spaces, and each scope-token must consist of one or more characters defined by NQCHAR.

https://datatracker.ietf.org/doc/html/rfc6749#appendix-A
NQCHAR = %x21 / %x23-5B / %x5D-7E
NQCHAR includes all printable ASCII characters except
double quote ("), backslash (), and space ( ).

Breakdown of changes:

  • Previously: [\w.]
  • New regex: ((?!["\\ ])[ -~])
    [ -~] - Match all ASCII characters between space ( ) and tilde (~)
    (?!["\\ ]) - Negative lookahead ignore double quote ("), backslash (), and space ( )

Tests of this regex: https://regex101.com/r/IrNM6D/1

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.

Copy link

codecov bot commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.09%. Comparing base (c8c11b4) to head (3c4e8ad).
Report is 304 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main   #20483       +/-   ##
===========================================
+ Coverage   45.36%   66.09%   +20.72%     
===========================================
  Files         244     1049      +805     
  Lines       13333   114649   +101316     
  Branches     2719     2867      +148     
===========================================
+ Hits         6049    75776    +69727     
- Misses       6983    34730    +27747     
- Partials      301     4143     +3842     
Flag Coverage Δ
unittests 66.09% <ø> (+20.72%) ⬆️

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

see 1288 files with indirect coverage changes

Copy link
Contributor

@stonezdj stonezdj left a comment

Choose a reason for hiding this comment

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

LGTM

@Vad1mo
Copy link
Member

Vad1mo commented Jun 8, 2024

@rlacko58 @stonezdj, before merging and accepting, can you think of any potential backward incompatibilities?

@rlacko58
Copy link
Contributor Author

rlacko58 commented Jun 8, 2024

It's compatible with the previous regex. This just further extends the list of allowed characters. @Vad1mo

Previously we had \w. Which represents a-zA-Z0-9_

In the new one we extend this list with all ascii characters between space ( ) and tilde (~). Only exception is double quote ("), backslash (), and space ( ).

You can see the tests here: https://regex101.com/r/IrNM6D/1

@Vad1mo
Copy link
Member

Vad1mo commented Jun 11, 2024

love the test comparison here https://regex101.com/r/IrNM6D/1, attaching it here for history purposes.

image

@rlacko58
Copy link
Contributor Author

I see the Prettier test failed. I'll update the PR later today/tomorrow. @Vad1mo

@Vad1mo
Copy link
Member

Vad1mo commented Jun 11, 2024

@rlacko58 can you take a look into the issues? there are some linter errors.

Copy link

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.

@reasonerjt
Copy link
Contributor

@rlacko58
Thanks for the contribution!
Do you plan to continue working on this PR? If the answer's yes, could you please rebase and fix the CI errors?

@rlacko58 rlacko58 force-pushed the fix-config-auth-oidcscope-regex branch 2 times, most recently from aef8455 to 63dd026 Compare August 13, 2024 09:21
@rlacko58
Copy link
Contributor Author

@reasonerjt Thanks for the reminder. It should be good to go now. I've put the change to a separate commit to make the review easier

@reasonerjt
Copy link
Contributor

@rlacko58 Thanks for the update. There're still some lint errors. Could you check?

https://datatracker.ietf.org/doc/html/rfc6749#appendix-A.4
     scope       = scope-token *( SP scope-token )
     scope-token = 1*( %x21 / %x23-5B / %x5D-7E )

A scope is composed of one or more scope-tokens separated by spaces,
and each scope-token must consist of one or more characters
defined by NQCHAR.

https://datatracker.ietf.org/doc/html/rfc6749#appendix-A
NQCHAR     = %x21 / %x23-5B / %x5D-7E
NQCHAR includes all printable ASCII characters except
double quote ("), backslash (\), and space ( ).

Signed-off-by: Laszlo Rafael <[email protected]>
@rlacko58 rlacko58 force-pushed the fix-config-auth-oidcscope-regex branch from 63dd026 to 3eeffa0 Compare August 15, 2024 12:13
@rlacko58
Copy link
Contributor Author

@reasonerjt Now it should be finally good to go

Copy link

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 github-actions bot added the Stale label Oct 15, 2024
@OrlinVasilev OrlinVasilev enabled auto-merge (squash) October 15, 2024 12:15
@OrlinVasilev
Copy link
Member

Did someone tested that @goharbor/all-maintainers ? I think we can merge that

@wy65701436 wy65701436 added target/2.13.0 issues that are targeting v2.13.0 and removed candidate/2.13.0 labels Oct 24, 2024
@OrlinVasilev OrlinVasilev merged commit 8254c02 into goharbor:main Oct 24, 2024
14 checks passed
ianseyer pushed a commit to ianseyer/harbor that referenced this pull request Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc env/oidc/azure-ad release-note/update Update or Fix target/2.13.0 issues that are targeting v2.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants