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.

Sign up for GitHub

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

Jump to bottom

Pinniped Auth Provider #19846

Merged
merged 24 commits into from Oct 13, 2023
Merged

Pinniped Auth Provider #19846

merged 24 commits into from Oct 13, 2023

Conversation

RubenV-dev
Copy link
Contributor

@RubenV-dev RubenV-dev commented Sep 7, 2023 •
edited

Hey, I just made a Pull Request!

Enable the auth-backend plugin to get cluster-scoped ID tokens via an RFC 8693 token exchange using a newly introduced Pinniped Auth Provider

Relates to issue #14011 --strictly an integration with the pinniped supervisor, forming only an incomplete part of an overall pinniped integration.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@github-actions github-actions bot added the auth label Sep 7, 2023
@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Sep 7, 2023 •
edited

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/plugin-auth-backend-module-pinniped-provider plugins/auth-backend-module-pinniped-provider minor v0.0.0
@backstage/plugin-auth-node plugins/auth-node patch v0.4.0-next.2

@RubenV-dev
Copy link
Contributor Author

@jamieklassen would appreciate anymore feedback. Thought i would open this up to see if there were any other places that might need addressing for this auth provider.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023 •
edited

Uffizzi Preview deployment-35479 was deleted.

@jamieklassen
Copy link
Member

jamieklassen commented Sep 7, 2023 •
edited

Worth mentioning that this relates to #14011 -- it is also strictly an integration with the pinniped supervisor, forming only an incomplete part of an overall pinniped integration.

jamieklassen
jamieklassen suggested changes Sep 7, 2023
View reviewed changes
Copy link
Member

@jamieklassen jamieklassen left a comment •
edited

Choose a reason for hiding this comment

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

I scanned and made a bunch of "single comments" and didn't gather them into a review so I'm formally "requesting changes" here. Can we also update plugins/auth-backend/config.d.ts with the schema for this new provider? It's a good place to start documenting the fields like federationDomain, clientId, and clientSecret. At a first glance, it might not be obvious that those config values are required.

@RubenV-dev
Copy link
Contributor Author

Sure thing this all makes sense i will get to working on those changes. I have to take a closer look at those dependencies in my package.json, if i recall correctly the linter was screaming about not having certain dependencies installed and i used the provided cli calls to rectify the issue. Probably in all the refactoring those libraries/dependencies arent even used anymore which makes things look a bit funky.

@RubenV-dev RubenV-dev force-pushed the pinniped-auth branch 5 times, most recently from 2b0648e to 4951646 Compare September 8, 2023 16:16
jamieklassen
jamieklassen suggested changes Sep 12, 2023
View reviewed changes
Copy link
Member

@jamieklassen jamieklassen left a comment

Choose a reason for hiding this comment

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

Thought of one more improvement before I forget, and requested some additional input from @cfryanr who works on Pinniped

plugins/auth-backend-module-pinniped-provider/src/authenticator.ts Outdated Show resolved Hide resolved
plugins/auth-backend-module-pinniped-provider/src/authenticator.ts Show resolved Hide resolved
jamieklassen
jamieklassen suggested changes Sep 12, 2023
View reviewed changes
Copy link
Member

@jamieklassen jamieklassen left a comment

Choose a reason for hiding this comment

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

Looks like this also need a rebase; some conflicts to resolve

@RubenV-dev RubenV-dev force-pushed the pinniped-auth branch 2 times, most recently from 3d55d8e to 77414c0 Compare September 12, 2023 20:12
Jamie Klassen and others added 21 commits October 12, 2023 10:12
@rvallejony
wip: auth-backed performs rfc 8693 token exchange
7b4b8a0 
Just driving with the test at this point, hitting some trouble with
express-session/cookies but I have an idea for an approach when I return to it:

Hopefully it will be enough to enable all the right middlewares in our
express app under test and then use `request.agent` from supertest as in

https://github.com/ladjs/supertest/blob/25920e7a1d246b590123417bfce33221db88e947/README.md?plain=1#L244-L256

which can make an initial request to the `/start` endpoint and persist cookies
to the next request (the interesting one under test) to `/handler/frame`.

Signed-off-by: Jamie Klassen <jklassen@vmware.com>
@rvallejony
implement consent redirect in new PinnipedAuthProvider
07df9ce 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
passing pinniped authprovider #handler responds with Id token test
295dae8 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
refactor: minimal passing implementation
7c26171 
Removed all the code that wasn't impacting a failing test, and removed the
"ID token" test -- we'll start with access tokens since they are more important
for our token-exchange use case.

Signed-off-by: Jamie Klassen <jklassen@vmware.com>
Co-authored-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
refine requirements for start method
c1c062a 
Now the unit tests for the start method should render the '#start' describe
in index.test.ts redundant.

Signed-off-by: Jamie Klassen <jklassen@vmware.com>
Co-authored-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
working integration test
b6f103d 
Some thoughts at this point:
* it would be nice to gather all the fakePinnipedSupervisor setup together
  in the beforeEach rather than spreading it throughout the test body
* we need a real JWK/JWKS endpoint for token signing

Signed-off-by: Jamie Klassen <jklassen@vmware.com>
Co-authored-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
authorization code exchange for a valid access_token
7dc7a38 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
Provider requests scopes
ae05982 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
Add redirect and error methods to the handlers strategy along with un…
07dcd84 
…it tests

Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
introduce audience parameter into start method and pass it into oauth…
9fedd45 
… state, fix integration tests

Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
refactor and add #refresh to pinnipedAuth provider
44483b9 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
add offline_access scope to default scope list in #start
448ff7b 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
Change handler to return scope returned by the tokenset
eb1dac4 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
Add rfc token exchange logic to #handler success
f5be2ec 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
refactor provider unit tests
5ee7c78 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
resolve rebase type/compilation errors
70a3c26 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
clean up tests
8d3e9c7 
and remove tokenSignedAlg option from pinniped, since it's not actually
configurable by end users. This means that all the tests use ID tokens signed
with a real JWK.

Signed-off-by: Jamie Klassen <jklassen@vmware.com>
@rvallejony
WIP: new auth pattern refactor, intro module and authenticator tests,…
501a8b5 
… #start refactor complete

Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
Completed Pinniped Authenticator refactor with passing unit tests
362a5e2 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
PR chores: changeset, api-report, cleaning, add catalog-info entry
ae34255 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@rvallejony
Extract rfc8693 tokenexchange logic to a helper function
b497b6e 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
Co-authored-by: Jamie Klassen <jklassen@vmware.com>
@rvallejony
remove pinniped provider from default providers in auth-backend
2f172ba 
Signed-off-by: Ruben Vallejo <rvallejo@vmware.com>
@github-actions github-actions bot removed the auth label Oct 12, 2023
@jamieklassen jamieklassen merged commit e0b5cd9 into backstage:master Oct 13, 2023
35 checks passed
@github-actions
Copy link
Contributor

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.19.0 release, scheduled for Tue, 17 Oct 2023.

@jamieklassen jamieklassen deleted the pinniped-auth branch October 13, 2023 16:04
@jamieklassen jamieklassen mentioned this pull request Nov 22, 2023
Closed
2 tasks
@freben freben mentioned this pull request Apr 16, 2024
Open
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfryanr cfryanr cfryanr left review comments

@Rugvip Rugvip Rugvip approved these changes

@jamieklassen jamieklassen jamieklassen approved these changes

@backstage-service backstage-service Awaiting requested review from backstage-service backstage-service is a code owner

@tudi2d tudi2d Awaiting requested review from tudi2d tudi2d is a code owner automatically assigned from backstage/reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@RubenV-dev @jamieklassen @Rugvip @cfryanr @rvallejony