Include project code into project authentication token

[Glandos]

Fix #780

The token used for shared links (aka authentication) includes the project password (aka code).
This code is checked when reading the token.
If the code is changed, the token is invalid. Test is included for that.

Also, reset token are now using URLSafeTimedSerializer instead of the deprecated JWS provided by itsdangerous.
The main change is that age is checked when verifying. The coolest effect is that warnings disappeared from tests.

[zorun]

Wow, good work, it's more involved than I thought!

It looks good, I just have two comments:

• serializing the password does not look like a good idea, and we don't need it. Why not just use the password as an input to the serializer, just like the secret key?
• you are using plain assert in the test, is it intended?

[zorun]

To explain more: the serializer only signs the data, it does not provide any confidentiality. So it should be fairly easy to extract the payload from a token. If you include the private code in the payload, it reveals it.

Using the private code as salt could work, but it's not the intended use.

I would simply concatenate the global secret key and the project's private code, and use that as the "secret key" passed to the serializer. The private code probably needs conversion to bytes or something.

[Glandos]

@zorun commented on 14 juil. 2021 à 19:01 UTC+2:

• serializing the password does not look like a good idea, and we don't need it. Why not just use the password as an input to the serializer, just like the secret key?

OK, you're right, I think I can do a much better job here. I was focused on how to do it, and forgot about what data I should use to feed the serializer.

You should remember that the code is hashed, so you have only a string like 'pbkdf2:sha256:260000$twIFvN4IQyWHNRhg$9035393f09173a3071339a17a163d6f340c452611fe3f6b165ef7d253dd966a9'. So a concatenation is possible, but using it as a salt can do the job too.

• you are using plain assert in the test, is it intended?

Totally. pytest allows plain assert. It is sometimes even more readable.

[zorun]

Ah right, this explains why the token size grows so much. Another reason not to include the (hashed) private code in the payload :)

[Glandos]

@zorun commented on 14 juil. 2021 à 19:11 UTC+2:

I would simply concatenate the global secret key and the project's private code, and use that as the "secret key" passed to the serializer. The private code probably needs conversion to bytes or something.

Well, it is possible, but using loads_unsafe to get the project id, load the project from the database to get the password, and then re-instanciate a new serializer with the right secret key.

    def generate_token(self, token_type="auth"):
        """Generate a timed and serialized JsonWebToken

        :param token_type: Either "auth" for authentication (invalidated when project code changed),
                        or "reset" for password reset (invalidated after expiration)
        """

        if token_type == "reset":
            serializer = URLSafeTimedSerializer(
                current_app.config["SECRET_KEY"], salt=token_type
            )
            token = serializer.dumps({"project_id": self.id})
        else:
            serializer = URLSafeSerializer(
                current_app.config["SECRET_KEY"] + self.password, salt=token_type
            )
            token = serializer.dumps({"project_id": self.id, "password": self.password})

        return token

    @staticmethod
    def verify_token(token, token_type="auth", max_age=3600):
        """Return the project id associated to the provided token,
        None if the provided token is expired or not valid.

        :param token: Serialized TimedJsonWebToken
        :param token_type: Either "auth" for authentication (invalidated when project code changed),
                        or "reset" for password reset (invalidated after expiration)
        :param max_age: Token expiration time (in seconds). Only used with token_type "reset"
        """
        loads_kwargs = {}
        if token_type == "reset":
            serializer = URLSafeTimedSerializer(
                current_app.config["SECRET_KEY"], salt=token_type
            )
            loads_kwargs["max_age"] = max_age
        else:
            serializer = URLSafeSerializer(
                current_app.config["SECRET_KEY"], salt=token_type
            )
            (valid, data) = serializer.loads_unsafe(token)
            if not valid and data is not None:
                project = Project.query.get(data.get("project_id", None))
                if project is not None:
                    # Recreate serializer with correct key
                    serializer = URLSafeSerializer(
                        current_app.config["SECRET_KEY"] + project.password,
                        salt=token_type,
                    )
        try:
            data = serializer.loads(token, **loads_kwargs)
        except SignatureExpired:
            return None
        except BadSignature:
            return None

        return data.get("project_id")

Tests are passing with this code. But honestly, I don't know if loading unsafe data and passing it to SQLAlchemy is dangerous or not. It's possible to run extra check to ensure that the unsafely loaded project id can be a valid project id, using a regexp or something like this.

[zorun]

Yeah, this is something I didn't think about :( I also don't really like loading untrusted data this way...

[Glandos]

OK, now that every tests are OK, I would like to add a last change: reduce the token payload from {"project_id": self.id} to [self.id]. Token will then be shortened by a few characters, which seems a good thing. Of course, the signature part of the token will be the largest.

Example:

• Old token: http://127.0.0.1:5000/authenticate?project_id=demo&token=eyJwcm9qZWN0X2lkIjoiZGVtbyJ9._UfUurb8_LpSiK4X4D7fAF_ET9Q
• New token: http://127.0.0.1:5000/authenticate?project_id=demo&token=WyJkZW1vIl0.6Nq2V32OdYuE4mW2ZlpzDmmSZbQ

In any case, old tokens from v4 are invalidated, so…

[zorun]

Thanks, I have added a few comments.

Don't forget to update the documentation, we have something about authenticating with a token in there.

And yes, I'm OK to shorten the payload. Actually, do we need the payload at all now?

[Glandos]

And yes, I'm OK to shorten the payload. Actually, do we need the payload at all now?

I think so: the project name itself. Otherwise, it is an empty string valid for every project with the same password. OK, since password is hashed+salted this shouldn't happen, but...

[almet]

Oh, that's interesting, thanks :-) I like the idea of changing the token length. Also, I feel that we could use shorter and more readable tokens by using another URL for them. What about something like http://127.0.0.1:5000/demo/WyJkZW1vIl0.6Nq2V32OdYuE4mW2ZlpzDmmSZbQ rather than using QueryStrings?

[Glandos]

Rebased for invitation templates. Ready for merging?

[Glandos]

@almet commented on 1 sept. 2021 à 20:49 UTC+2:

Oh, that's interesting, thanks :-) I like the idea of changing the token length. Also, I feel that we could use shorter and more readable tokens by using another URL for them. What about something like http://127.0.0.1:5000/demo/WyJkZW1vIl0.6Nq2V32OdYuE4mW2ZlpzDmmSZbQ rather than using QueryStrings?

Arg, I missed that interesting comment.

Using /project/token is a bit difficult, and confusing. What about /authenticate/<project_id>/<token>?

[almet]

I don't get why it's a problem? Why not, but the main idea was to shorten the URL to be friendlier, so adding /authenticate defeats the purpose from my POV.

[Glandos]

OK, here's my tries:

• Original: http://127.0.0.1:5000/authenticate?project_id=demo&token=WyJkZW1vIl0.JA_croSWJsz-hvyps7pv7lsAvyg
• With my proposal: http://127.0.0.1:5000/authenticate/demo/WyJkZW1vIl0.JA_croSWJsz-hvyps7pv7lsAvyg
• With @almet proposal: http://127.0.0.1:5000/demo/WyJkZW1vIl0.JA_croSWJsz-hvyps7pv7lsAvyg

They all work, so indeed, it was not a problem to make it shorter.
The only issue is that one behavior is different. I used "/<project_id>/<string:token>" and project_id is checked by the blueprint to be valid. So if the project doesn't exist, the user is redirected to project creation page. With the old page, it displayed the error page saying that project or token is invalid.
Do you have an opinion here? We are about to have breaking changes here, so it's time to break things if they make sense. I think it's possible to fallback to the old behavior, by using a different string parameter, but I didn't try it yet.

[Glandos]

By the way: using a different route for token will simplify the code. Now, /authenticate mixes token and project/code, which is less convenient to read.

[almet]

Ah, since the token contains the project id, we might just want to use an URL like /join/{token}. That seems to work and reduces the size of the URL! What do you think?

Also : what do you think about persisting some shorter tokens in the database, matching the project ID, to shorten the size of the URL?

[Glandos]

I am merging this PR as is, and open a new one for new path. It's another (even if desirable) goal.