CERT-Polska / mwdb-core

Malware repository component for samples & static configuration with REST API interface.
https://mwdb.readthedocs.io/
Other
320 stars 73 forks source link

Proposed changes in JWT tokens used for authorization #530

Closed psrok1 closed 2 years ago

psrok1 commented 2 years ago

Feature Category

Describe the problem

After short review, I found that there are few issues with currently used JWT tokens:

  1. Currently session token can be reused as set password token, because JWT doesn't explicitly limit the scope. Fortunately, that's the only case I found. The required JWT payload fields usually differ, but in set password token, required part is a subset of fields set in session token payload.

    JWTs are used in various ways in authorization process, so it would be nice to define a scope field that declares what action is authorized by JWT. Possible values are:

    • session
    • api_key
    • set_password
    • download_file Scope should be checked before action is authorized.
  2. The payload part is non-standard and doesn't contain necessary parts that are usually required in JWT. RFC 7519 declares these registered claims as optional, but it would be nice to follow the specification

Good example is JWT used as session token:

image

Standard claims are:

See also https://datatracker.ietf.org/doc/html/rfc7519#section-4.1

  1. itsdangerous JOSE functionality is deprecated and documentation recommends to use authlib for JWT generation (https://itsdangerous.palletsprojects.com/en/2.0.x/jws/). We already use it for OIDC integration, so it should not be a problem to move to the another library!

Describe the solution you'd like

Move to the authlib implementation and implement JWT in a way that comply with the standards.

Note on backwards compatibility

As always, we should be aware to not make any service disruption after upgrade to the new version. Session, set password and file download tokens are pretty volatile and recoverable. API keys are not and can be difficult to change.

That's why PR https://github.com/CERT-Polska/mwdb-core/pull/424 still waits for a good time to be merged. In this case we should also keep API keys backwards compatible, but mark them as legacy in the UI with proper information that they should be regenerated in the near future.

KWMORALE commented 2 years ago

Notes:

It would be good to consider on the list that could invalidate tokens or some system that determines the validity of tokens after their use.

Remember about password_ver and identity_ver values during implementation of new JWT tokens system in mwdb-core.

The following library looks better than authlib for this task: https://pyjwt.readthedocs.io/en/stable/usage.html