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

How is this package specific to Auth0? #224

Closed
gnarea opened this issue Jul 11, 2022 · 20 comments
Closed

How is this package specific to Auth0? #224

gnarea opened this issue Jul 11, 2022 · 20 comments
Assignees

Comments

@gnarea
Copy link
Contributor

gnarea commented Jul 11, 2022

❗ UPDATE: this request already has a PR, the plan is described in this comment in the PR. Please follow that plan to complete this change.

I've just checked the code and can't really see how it's specific to Auth0. It just looks like a spec-compliant package so it should work with any spec-compliant id server as far as I can see (the main spec here being RFC7517 or JWK). Right?

I'm considering using this package on an open source API server, and I don't want operators to be tied to Auth0, which is why I wanted to check.

@simoneb
Copy link
Member

simoneb commented Jul 12, 2022

You may very well be right. This package was created a few years back and we probably thought that Auth0 was the only identity provider supporting this protocol, or in any case we weren't using any others so we didn't want to assume it was generic while in fact it would work only with Auth0.

Now the implementation is fairly simple but I think the best way forward to repurpose this is to have a confirmation that it works with other implementations as well. You said you were going to use it against an open source implementation, right? What best opportunity to check that it's working! :)

Would you like to try it out and let us know if it works?

@gnarea
Copy link
Contributor Author

gnarea commented Jul 12, 2022

Great, makes sense! 👍🏾

I'm planning to use it in an open source API server we'll start building in a few weeks, but the actual identity server is likely to be Google Identity-Aware Proxy (IAP) in our case. However, being our server open source and cloud-agnostic, other people could configure it to use Auth0, Cognito, etc.

I haven't tried it yet, but one thing I just noticed might be problematic is the JWK URL: IAP doesn't use the .well-known endpoint, but https://www.gstatic.com/iap/verify/public_key-jwk (same set of keys for all tenants it seems), so the following line wouldn't work with IAP:

const response = await fetch(`${domain}.well-known/jwks.json`, { timeout: 5000 })

Would you accept a PR to make the whole URL customisable?

The .well-known endpoint seems to be a convention rather than something required by a spec, so I guess IAP isn't doing something wrong.

@simoneb
Copy link
Member

simoneb commented Jul 13, 2022

Would you accept a PR to make the whole URL customisable?

absolutely!

@gnarea
Copy link
Contributor Author

gnarea commented Jul 13, 2022

Fantastic! In that case I'll wait until we're ready to integrate IAP and create a single PR, in the very unlikely event that more changes were needed.

@simoneb
Copy link
Member

simoneb commented Jul 28, 2022

@gnarea just checking if you managed to make any progress on this

@gnarea
Copy link
Contributor Author

gnarea commented Jul 28, 2022

Thanks for checking in @simoneb! We haven't started yet as the project isn't 100% confirmed but if it all goes according to plan we should start in a month or so.

@simoneb
Copy link
Member

simoneb commented Jul 28, 2022

No problem, keep us posted!

@gnarea
Copy link
Contributor Author

gnarea commented Apr 7, 2023

I finally got round to looking into this, and got a proof of concept working. I've tested it with mock-oauth2-server.

It's all very pretty straightforward but there's a couple of design decisions we should make before I propose a PR with tests:

Domain vs URL

We're currently taking a domain and producing the URL ${domain}.well-known/jwks.json. However, now we have to capture the full URL, which raises the question: should we capture this URL in a backwardly-compatible way (for example, capture the URL suffix separately and default to .well-known/jwks.json), or as a breaking change (for example, replacing domain with url)?

I think the latter is worth considering if we're also dropping "auth0" from the name of this package.

Certificate vs public key

The current implementation is passing a PEM-encoded X509 certificate as a "secret" to @fastify/jwt (key.x5c[0]). However, the field x5c is optional, and neither Google or mock-oauth2-server set it.

Whilst it's certainly valid to pass the certificate as it contains the public key, I don't know why the original implementation didn't just convert the key from JWK to PEM. It's very unlikely, but maybe this is a workaround that was needed at some point -- and if it was indeed needed at some point, maybe old Auth0 tenants may be behind a feature flag that preserves the old (bogus) behaviour.

All of this is extremely unlikely and I think we can just convert the key from JWK to PEM unconditionally. However, if we want to be extra cautious, we could do it only when the certificates are missing.

@toomuchdesign
Copy link
Collaborator

Hi all,
so I’m going to try and wrap up the efforts spent here. I have very little technical context about the subject, so I’m trying to behave as a facilitator.

How to release

Since we’re going to make fastify-auth0-verify more generic, I’d be inclined to fork to a new repo and deprecate fastify-auth0-verify once we feel comfortable with it.

We might work on a PR here and when good enough we could check it out on a new repo. In this regard we should find a good new name for it:

  • fastily-jwt-verify
  • @fastify/jwt-veridy

This would be a good moment to introduce the few expected breaking changes.

auth0 support off the shelf

Since we’re considering deprecating fastify-auth0-verify and I would expect the new plugin to be mostly used with auth0, I’d suggest to find a good DX solution to provide 0auth support as a zero-configuration setup. @gnarea How the proposed branch behaves in this regard?

Back to the questions

Domain vs URL

As I said it's a good opportunity to introduce breaking changes which DX could benefit from. I would suggest designing APIs supporting auth0 with a minimal effort.

Certificate vs public key

Trying to get more context:

If there is a more standard way to do so which works with auth0 we might consider taking this direction and leaving old Auth0 tenants use the current fastify-auth0-verify plugin.

Thanks again for raising this issue. I'm happy to discuss with you all any of the mentioned points.

@gnarea
Copy link
Contributor Author

gnarea commented Apr 17, 2023

Thanks for looking into this @toomuchdesign! To answer your questions:

How to release

Agreed it'd be good to fork the project.

Re: name, I think JWKS is the key word here. So maybe something like fastify-jwks-verify or simply fastify-jwks.

Re: breaking changes, I think it'd all be around the domain parameter:

  • Today: It's either the domain or URL to an Auth0 tenant (without the .well-known/jwks.json bit).
  • Future: Full URL to the JWKS endpoint. So in this case, the name domain would have to be replaced with something like jwksEndpointUrl or jwksEndpoint.

auth0 support off the shelf

Without any further changes to accommodate Auth0 tenants, they'd just have to specify the full URL, including the .well-known/jwks.json suffix. For example, https://tenant-name.auth0.com/.well-known/jwks.json or https://tenant-name.com/.well-known/jwks.json (if using their own domain name). See: https://auth0.com/docs/secure/tokens/json-web-tokens/json-web-key-sets

Auth0 tenants won't have to change anything else if they were to migrate to the new package.

Certificate vs public key

I'm not sure, it looks like the use of the certificate has always been around since the first commit: https://github.com/nearform/fastify-auth0-verify/blame/b6fc1006135b28dbcce5c3312e215ae728a16af2/index.js#L93


If there is a more standard way to do so which works with auth0 we might consider taking this direction and leaving old Auth0 tenants use the current fastify-auth0-verify plugin.

Makes sense, and I think the approach I'm proposing would meet those requirements.

@toomuchdesign
Copy link
Collaborator

Thanks for the quick reply! Sounds good to me 👍 I was thinking that as soon as we decide the new name, we might create a new repo and move the operations there.

I've just realised that the domain prop is only used once to create the WKS endpoint, so it sounds fine to me replacing it with an absolute URL. If documenting the auth0 setup wasn't enough we might maybe consider exposing a sort of auth0 config preset (nothing I would consider for this first iteration).

Beside the public authenticate method I see that fastify-auth0-verify decorates the Fastify instance with a few more methods/props:

  • auth0Verify
  • auth0VerifySecretsCache

I understand they are private attributes, therefore we could safely rename those to something more generic, right?

@gnarea
Copy link
Contributor Author

gnarea commented Apr 17, 2023

Thanks @toomuchdesign! Agreed 👌🏾

Re: auth0Verify and auth0VerifySecretsCache, they don't appear to be documented, but they are exposed in the index.d.ts, so there's a chance some people may be using them. I can't think of any reason why anyone would need those, other than perhaps to use it for debugging purposes. So I think we could remove them and, if anyone ever asks for those, bring them back under generic names. Wdyt?

@toomuchdesign
Copy link
Collaborator

Both auth0Verify and auth0VerifySecretsCache seem to be used here:

function getSecret(request, reply, cb) {

Are they going to be still relevant in the new vendor agnostic implementation?

@gnarea
Copy link
Contributor Author

gnarea commented Apr 17, 2023

I meant people using this plugin shouldn't need to use those values directly, but we'll still need them inside the plugin -- although we don't need them to be decorated on the Fastify instance or request.

@simoneb
Copy link
Member

simoneb commented Apr 17, 2023

Hey folks, thanks for looking into this. I think we have two options on the table:

  1. create a new repo by "forking this one". This will imply:
  • writing a notice in the readme to suggest people to migrate to the new package
  • archiving this repo as all you could do with this package can be done with the new one
  1. rename this repo. This will imply:
  • deprecating the existing package on npm
  • writing a notice in the repo to let people know of the rename and that they should switch to the new package, with migration instructions

I don't have strong opinions but whatever we do, we should decommission the old one to avoid having to maintain 2 codebases and 2 packages. My personal preference nonetheless would be for option 2) nonetheless.

In any case and regardless of the decision, we should first make sure we have a service to test this against, to make sure that any chances we do are working. @gnarea do you have any recommendation about a spec-compliant service other than auth0 that we can use to test that whatever changes we make, this keeps working?

@toomuchdesign
Copy link
Collaborator

toomuchdesign commented Apr 20, 2023

So we have now integration tests against auth0!

@gnarea would you mind opening a PR with your work so that we work together there? (we might need to rebase over the just updated master, happy to land a hand if the case)

Also is there any JWT based auth provider you would recommend to add integration tests for?

In the meanwhile I'm considering adding integration tests against https://www.npmjs.com/package/oauth2-mock-server

@toomuchdesign
Copy link
Collaborator

toomuchdesign commented Apr 26, 2023

Hi @gnarea! I'm taking a look at this oauth2 Fastify plugin:
https://github.com/fastify/fastify-oauth2

Is it able to fulfil your use case or it has something missing?

UPDATE: I understand fastify-oauth2 handles jwt retrieval but doesn't assume anything about jwt verification. Did I get it right?

@gnarea
Copy link
Contributor Author

gnarea commented Apr 26, 2023

Hey folks! I'm terribly sorry for going quiet suddenly. I've been dealing with a couple of things that require my full attention, but things are going back to normal soon so I should be able to follow up on this by Monday at the latest.

@simoneb
Copy link
Member

simoneb commented May 19, 2023

We can now close this because we have:

  1. created https://github.com/nearform/fastify-jwt-jwks, which is a vendor-agnostic way to do what this package was doing
  2. we changed this package's implementation to rely on the new package above, thereby removing much of the code and keeping this as a thin layer on top of the other package

@simoneb simoneb closed this as completed May 19, 2023
@gnarea
Copy link
Contributor Author

gnarea commented May 19, 2023

Hey everybody. That's fantastic, thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants