dwyl / auth

🚪 🔐 UX-focussed Turnkey Authentication Solution for Web Apps/APIs (Documented, Tested & Maintained)
https://authdemo.fly.dev
GNU General Public License v2.0
130 stars 9 forks source link

Technical Question: should we use a `JWT` for `AUTH_API_KEY`? #268

Closed nelsonic closed 1 year ago

nelsonic commented 1 year ago

At present an AUTH_API_KEY has the format:

88SwQDWYGmiDACVRaBGqQghzLd5jfX7YynefCJ8M/88SwQH8CEDWzWSq7PyWe7ADpjYCpmczr5zyTK4d/authdemo.fly.dev

Note: this is not a valid key. but the format is correct.

The reason I wet with this and still like it is because it's human-readable and immediately obvious which environment (auth instance) it's for; in this case: authdemo.fly.dev So if I see this key in my .env file or environment variable, I know exactly what it's for. 👌 I think this has a pretty major advantage in terms of Developer Experience ... 💭

But equally it has a disadvantage: no embedded info like expiry. We outlined the benefits of JWTs in our mega-popular doc: https://github.com/dwyl/learn-json-web-tokens And we are using them in auth for session tokens:

dwyl-mvp-jwt

https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth_web/controllers/auth_controller.ex#L200

So my question is: should the next version of auth (and auth_plug) use a JWT as the AUTH_API_KEY? Will having a JWT for the AUTH_API_KEY and a different JWT for session token be confusing to devs? 🤷‍♂️ Are we missing something?

Please let me know your thoughts ... 💭 🙏

LuchoTurtle commented 1 year ago

I don't think it makes much sense to have info like expiry in the AUTH_API_KEY. In my opinion, the key should be a unique string that the application uses to guarantee that the application using it is genuine in the "auth ecosystem".

However, the JWT session token that is returned to the browser and to the user should certainly have such kind of metadata. After all, JWT is just a way of encoding claims between two parties into a string. This session token should certainly be JWT but I fail to see the usefulness of having metadata info in a key that is used as env variable - it should be present in the session token JWT.

A similar thing happens when working with OpenID Connect.

If I have a client, a server and an identity provider like Keycloak, for example, the server uses in deployment a key as env variable that the identity provider issues, so it knows the server is legitimate. This key doesn't need to be a JWT.

However, if I were to use an OIDC (OpenID Connect) flow for users to authenticate in the server...

image

the interactions between the client and the server are made through JWT tokens. But the web server securely communicates with the identity provider to check if the issued ID tokens and Access Tokens (part of the OIDC authentication flow) are legitimate. And the only reason the identity provider is the provider for the server, is because it gave the server "a key" so it knows it's a legitimate application to be communicated with. This happens during deployment and does not necessarily mean the key is a JWT.

tldr:

JWT = for communication between client and web servers AUTH_API_KEY = doesn't really need to be JWT, I don't see how it is useful. It's just a way for the auth application to know the server is legitimate.

nelsonic commented 1 year ago

@LuchoTurtle thanks for your insightful reply. 👌 Yeah, I was trying to rack my brain to figure out how using a JWT as the AUTH_API_KEY would be useful. The only thing I can think of is validity checking on the side of the "consuming" app ... A couple of days ago we saw the client_id XYZ is not valid error: https://github.com/dwyl/phoenix-chat-example/issues/150 image

To the person using auth_plug this is not a very helpful error; a potential time-waster... ⏳ 💸 🔥 If the AUTH_API_KEY was a JWT, we could easily add a AuthPlug.verify_jwt/1 function and thus avoid this class of errors completely. Obviously we can (significantly) improve our error handling/reporting in auth ... but if there was a way of checking the AUTH_API_KEY with a function, it would eliminate the need for handling this type of error because the engineer would immediately know if their AUTH_API_KEY is valid and could even test it independently on https://jwt.io/

Hmm ... I'm still not "sold" on this in terms of cost-benefit. 🤔

SimonLab commented 1 year ago

I'm adding a bit more context as it took me a bit of time to understand the question:

To be able to use the auth application you first have to create an application, see https://github.com/dwyl/auth_plug#2-get-your-auth_api_key

The create action in the app_controlller is called: https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth_web/controllers/app_controller.ex#L14-L30

Which calls App.create_app/1: https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth/app.ex#L96-L108

Which then calls Auth.Apikey.create_apikey(app): https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth/apikey.ex#L36-L38

Finally encrypt_encode(id) is defined as: https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth/apikey.ex#L27-L29

~Just spotted this while I was doing this recap and I'm not sure if this is intentional here or a bug: We are passing the full app struct to Auth.Apikey.create_apikey(app). From the definition of the other functions def create_api_key(id) do and def encrypt_encode(plaintext) do it seems that we should pass the app.id instead or its string representation (plaintext?)~ This works because the encrypt function in Fields is defined here https://github.com/dwyl/fields/blob/main/lib/aes.ex#L29-L30 with

  @spec encrypt(any) :: String.t()
  def encrypt(plaintext) do

~the argument can by of any types, but I still think app.id might be what was intended? Let me know if this makes sense.~ Looked at the wrong function, this is the correct one: https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth/apikey.ex#L65-L77

But to come back to the auth api key we see that it is created using Fields and the format is client_id/secret_id`

Now when a user authenticate the auth app is using jwt to encode the person's info https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth_web/controllers/auth_controller.ex#L200

Now to come back to the error describe above https://github.com/dwyl/auth/issues/268#issuecomment-1424470180

client_id: .... is not valid

It is defined here: https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth_web/controllers/auth_controller.ex#L17-L30

By calling client_id_valid?: https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth_web/controllers/auth_controller.ex#L81-L95

I think the error message could be better, something like "the auth_api_key used with auth_plug is not linked to any applications on the auth app"?

~Note also that the decode_decrypt function is defined as:~ https://github.com/dwyl/auth/blob/83c286a24d47bd087ba41925a740d4030cf95e19/lib/auth/apikey.ex#L44-L52

~Is there also a bug with decrypted_key = key |> Base58.decode() |> Fields.AES.decrypt() |> String.to_integer()? I don't understand why String.to_integer() is used here, are we sure that the string returned byField.AES.decrypt` can be converted to an integer, especially if when we first encoded, the key might have represented the app struct and not just the id (see above)~

Looked again and the app.id is correctly encoded, I looked at create_api_key instead of create_apikey. We might want to rename some of the functions to avoid this mistake

But I don't think it makes sense to use jwt for the auth api key. I can't see a use case where we want to define an expired time to be then checked on the application with auth_plug. When an application is created on auth I don't think we want it to be inaccessible after a period of time?

Let me know if all this makes sense.

LuchoTurtle commented 1 year ago

Thanks for the comprehensive explanation @SimonLab , makes much more sense now. I had some run-ins with this error but now knowing how it works helps a lot! I'd open an issue but since auth is in the process of being rebuilt, I wager it's not useful. As @nelsonic said, "it's like re-arranging deck chairs on the Titanic 🚢"

nelsonic commented 1 year ago

Thanks for both your feedback on this. Very good that we're all thinking on similar lines. 💭 Totally agree that this is confusing and potentially error-prone. 😕 Very much going to try and simplify the steps for creating an AUTH_API_KEY. 🔑 So that people using the API or trying to run/build the App on their localhost have only 1 step: copy.

nelsonic commented 1 year ago

@SimonLab thanks for stepping through the existing code to give context. I actually didn't want to get bogged down in the existing code because I've already agreed it's over-complicated. All I wanted to discuss was if AUTH_API_KEY should be a JWT or not. However ... it has prompted me to open a follow-up question: Do we even need an AUTH_API_KEY: https://github.com/dwyl/auth/issues/277

LuchoTurtle commented 1 year ago

Since some of this discussion was migrated to https://github.com/dwyl/auth/issues/277, should this issue be closed? The usage of the very existence of an AUTH_API_KEY was debated in https://github.com/dwyl/auth/issues/277.

Although it's confirmed in https://github.com/dwyl/auth/issues/277#issuecomment-1445010981 that we'll have an AUTH_URL/AUTH_API_KEY, it stated that it will have a auth.domain.com/:person_id/:public_key format, not JWT. 💭

nelsonic commented 1 year ago

Indeed. Not using JWT in AUTH_API_KEY. ✅ Thanks.