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

Add OIDC access token validation #107

Merged
merged 27 commits into from
Dec 9, 2021
Merged

Conversation

fredx30
Copy link
Contributor

@fredx30 fredx30 commented Nov 8, 2021

Requires merge of

Summary

Added a slew of options for setting OIDC parameters for JWT token verification. Upon setting enable this will check will be enforced for requests sent to the api such that all requests not carrying a valid bearer token will fail.

Motivation

This works in coalition with the coresponding frontend implementation iver-wharf/wharf-web#70. Both PRs make up all the needed code to get basics for OIDC working.

@fredx30 fredx30 self-assigned this Nov 8, 2021
@fredx30 fredx30 added this to In progress in Backlog via automation Nov 8, 2021
@fredx30 fredx30 changed the title Add OIDC token checking Add OIDC access token validation Nov 8, 2021
@fredx30 fredx30 changed the base branch from master to feature/cors-credentials-allow-origin-specific November 8, 2021 20:54
@fredx30 fredx30 requested review from applejag and Alexamakans and removed request for applejag November 8, 2021 21:00
@fredx30 fredx30 added the enhancement New feature or request label Nov 8, 2021
@fredx30 fredx30 force-pushed the feature/cors-credentials-allow-origin-specific branch 2 times, most recently from ed95856 to 25e4e8b Compare November 10, 2021 09:50
Base automatically changed from feature/cors-credentials-allow-origin-specific to master November 10, 2021 10:00
@fredx30 fredx30 force-pushed the feature/add-oidc-token-checking branch from f538bbf to 14c4de2 Compare November 10, 2021 10:04
@fredx30 fredx30 marked this pull request as ready for review November 10, 2021 10:04
@fredx30 fredx30 linked an issue Nov 10, 2021 that may be closed by this pull request
oidc_middleware.go Outdated Show resolved Hide resolved
Copy link
Member

@Alexamakans Alexamakans left a comment

Choose a reason for hiding this comment

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

Logic looks good as far as I can tell 👍🏻

Eventually we will want to change the error handling in VerifyTokenMiddleware to use problem.Response I think, but for now I think this is good.

With some of the suggestions there will be other changes required (mainly *map -> map), but your IDE should tell you about those, and it should be fairly straightforward I think. Don't hesitate to ask if weird stuff happens 👍🏻

Suggest doing a replace all for OICD -> OIDC as well.

oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
Backlog automation moved this from In progress to Review in progress Nov 10, 2021
config.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
@fredx30 fredx30 force-pushed the feature/add-oidc-token-checking branch from ea6be92 to b0c5e6b Compare November 11, 2021 13:11
config.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@fredx30
Copy link
Contributor Author

fredx30 commented Nov 11, 2021

Regarding the pointers im moving the discussion to teams to try to move it quick will summarise here.

oidc_middleware.go Outdated Show resolved Hide resolved
@fredx30 fredx30 force-pushed the feature/add-oidc-token-checking branch from b0c5e6b to 32a9791 Compare November 15, 2021 16:11
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
@fredx30
Copy link
Contributor Author

fredx30 commented Nov 24, 2021

My apologies here after rebasing onto the latest changes i found the comments have been detached from the code references. I belive this is due to the change set of go imports -w . as sugested here #107 (comment).

This formatting is applied here now aswell though!

Fredrik Dyrvold and others added 4 commits December 3, 2021 11:02
@fredx30 fredx30 force-pushed the feature/add-oidc-token-checking branch from 4546dd9 to 768f39f Compare December 3, 2021 10:03
@applejag
Copy link
Contributor

applejag commented Dec 7, 2021

Now that #124 has been merged, if you merge/rebase from master then we can deploy this to our staging environment to try this out if you want.

utils_test.go Outdated Show resolved Hide resolved
Copy link
Member

@Alexamakans Alexamakans left a comment

Choose a reason for hiding this comment

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

Would like to see the changes implemented, but approving to speed up the process.
Nice work 👍🏻

config.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
oidc_middleware.go Outdated Show resolved Hide resolved
}

// SubscribeToKeyURLUpdates ensures new keys are fetched as necessary.
// As a standard OIDC login provider keys should be checked for updates ever 1 day 1 hour.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// As a standard OIDC login provider keys should be checked for updates ever 1 day 1 hour.
// As a standard OIDC login provider keys should be checked for updates every 25 hours.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the connection is easier to make towards daily keyrotation if the text mentions day so i would skip this fix.

oidc_middleware.go Show resolved Hide resolved
oidc_middleware.go Show resolved Hide resolved
Backlog automation moved this from Review in progress to Reviewer approved Dec 8, 2021
fredx30 and others added 4 commits December 8, 2021 17:59
Co-authored-by: Alexamakans <79503481+Alexamakans@users.noreply.github.com>
Co-authored-by: Alexamakans <79503481+Alexamakans@users.noreply.github.com>
Co-authored-by: Alexamakans <79503481+Alexamakans@users.noreply.github.com>
@fredx30 fredx30 merged commit 31ee8d0 into master Dec 9, 2021
Backlog automation moved this from Reviewer approved to Done Dec 9, 2021
@fredx30 fredx30 deleted the feature/add-oidc-token-checking branch January 7, 2022 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Authentication, first iteration
3 participants