Authorization for Downloads of restricted Bitstreams: Short lived token endpoint

[KevinVdV]

References

Add references/links to any related tickets or PRs. These may include:

• Link to REST Contract

Description

This PR adds support for short lived token generation. The JWTTokenHandler class used to generate our session tokens has been split up into 2 parts:

• SessionJWTTokenHandler
• ShortLivedJWTTokenHandler

The difference being when that they have different configuration properties. If you have existing configuration that differs from the default ones you will need to look at the authentication.cfg file changes to see the new properties. Furthermore the "expiration" configuration property has been changed to use milliseconds instead of minutes.

Instructions for Reviewers

If you want to test this PR locally alter the jwt.shortLived.token.expiration in the local.cfg & put it to something like 30 seconds (as 2 seconds is very short if you want to test this).

To test this pr:

• Request a short lived token
• Provide the token to a private bitstream using the "token" parameter
• Wait until the configured time expires & try to download the file again, it should fail now

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
• My PR passes Checkstyle validation based on the Code Style Guide.
• My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
• My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
• If my PR includes new, third-party dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• If my PR modifies the REST API, I've linked to the REST Contract page (or open PR) related to this change.

[benbosman]

@KevinVdV I tried reviewing this PR, but the first thing I noticed is that my main authentication header is broken after creating a token.
This may indicate the eperson's session_salt is adjusted when the token is created, which should only happen for the main authentication header

Can you create an IT which:

• creates an authentication header using /server/api/authn/login
• creates a short-lived token using /server/api/authn/shortlivedtokens
• verifies the user is still authentication with the first authentication header using /server/api/authn/status

[abollini]

hi @KevinVdV thanks for the PR it looks good overall.
I have made some small change request inline, the "bigger" one is related to the use of the word "session" to discriminate between short lived tokens and "normal" one. I suggest to name it just loginToken but it would be useful to get a feedback from @tdonohue on that to avoid unnecessary change if he disagree with my suggestion or multiple changes if he has a better name suggestion.

[KevinVdV]

@abollini @benbosman All your feedback has been processed, let me know if anything is still missing (or if you find new things).

[benbosman]

I have not reviewed this code yet, but I've extensively tested it, and this branch creates security risks.

Using a token it's possible to:

• Create another token (infinitely increasing the duration of the token by requesting a new one every second based on the previous token)
• Create another security header (creating a long-lived header based on a short-lived token)

I've tested this using:

token=$(curl --silent -X POST 'http://localhost:8080/server/api/authn/shortlivedtokens' -H "Authorization: $authorization2" | jq --raw-output '.token'); echo "$token"; curl -v "http://localhost:8080/server/api/authn/shortlivedtokens?token=$token" -X POST
token=$(curl --silent -X POST 'http://localhost:8080/server/api/authn/shortlivedtokens' -H "Authorization: $authorization2" | jq --raw-output '.token'); echo "$token"; curl -v "http://localhost:8080/server/api/authn/login?token=$token" -X POST

I would recommend to explicitly refuse /server/api/authn/login and /server/api/authn/shortlivedtokens to accept a token, and add ITs that verify it's not allowed (even with a valid token)

[abollini]

I have flagged as resolved my previous feedback were processed. I agree with the @benbosman finding but I'm also ok to postpone this security improvements to beta4. Indeed, in the Rest Contract discussion we have not identified the need to prevent the use of short lived token for the renew endpoint.

[benbosman]

I've reviewed the code, and it seems all issues have already been solved, including the last 2 @abollini noted and @jonas-atmire responded to.
I also appreciate the large amount of tests.

Since I prefer not to create new security risks, I'd prefer to have that issue solved first. But if not realistic, we can perhaps postpone and create a critical ticket

[tdonohue]

@KevinVdV : would it be possible to resolve the security issues noted by Ben before merging this? I'd rather not merge something with a known security issue. Just an FYI, we are looking to release beta3 as soon as this Thursday. So if this PR (and others dependent on it) is going to be in beta3, ideally we'd get this PR ready to merge by tomorrow (if that's at all possible).

[tdonohue]

@peter-atmire : It looks like your most recent commit here causes some unexpected behavior in the EPerson endpoints (based on the Travis CI results). These tests are now throwing the wrong exception (a 403 instead of a 401) & therefore failing:

[ERROR]   EPersonRestRepositoryIT.patchReplacePasswordWithOtherUserTokenFail:1964 Status expected:<401> but was:<403>
[ERROR]   EPersonRestRepositoryIT.patchReplacePasswordWithRandomTokenPatchFail:1916 Status expected:<401> but was:<403>
[ERROR]   EPersonRestRepositoryIT.registerNewAccountPatchUpdatePasswordRandomUserUuidFail:2053 Status expected:<401> but was:<403>

I suspect your change may be too low-level. Maybe we should be rejecting the token param at the AuthenticationRestController level, rather than letting it through until it hits the JWTTokenHandler

[benbosman]

@tdonohue Because the Travis impact didn't match that commit, I've checked it out in more detail. The problem seems to be in the REST Contract, and not in the commit.

• Peter seems to have changed the AuthenticationRestControllerIT, which succeeds: https://travis-ci.com/github/DSpace/DSpace/builds/173680203#L5682
• The error https://travis-ci.com/github/DSpace/DSpace/builds/173680203#L5725 was quite odd, when I searched for that code in his branch, I couldn't find it. I think Travis merges this PR with master, and https://github.com/DSpace/DSpace/blob/master/dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java#L1891 is failing
• The reason for this to fail can be found in the REST Contract. The https://github.com/DSpace/Rest7Contract/blob/master/epersons.md#create-new-eperson-based-on-registration-token uses a token parameter, and this PR also uses a token parameter. For the eperson, this implies the token is interpreted as an authentication token and as an eperson registration token. Because it's not a valid authentication token, the test are requested.
• This is a situation where the ITs have identified a conflict, we can't use the same token parameter for both the eperson registration and the general authentication. If Peter didn't commit this last change, Travis wouldn't have verified the merge of this PR with master, and the issue would have occurred in master. The problem will also occur with the code prior to Peter's last commit. I will create a REST Contract PR to adjust this

[tdonohue]

@benbosman : Thanks for looking into this. I agree with your approach to simply rename one of these params. We don't want two different things named token.

[benbosman]

Thanks for the changes
I can see https://github.com/DSpace/DSpace/pull/2783/files/f9257dad12c97d1aa3daaf9beca35bed76ba98b0..9044daf50eb1b4044b6b4d80ce12b32e0a399712 solves the overlap of the tokens
I can also see d364ac6 solves the security risk

I realize Travis is broken for all PRs today, but I've received the build output, and there are no issues

This has resolved all my feedback

[tdonohue]

👍 This looks good to me. I've reviewed the code, and don't have any additional questions/comments. I've also verified that all of @abollini 's prior feedback was addressed (he had asked for more tests which have been added). I've tested this in conjunction with the Angular PR DSpace/dspace-angular#716 and they work together as described

[tdonohue]

Merging, as this is now at +2 and I've verified that all of @abollini 's prior feedback was addressed. Thanks @KevinVdV and @peter-atmire !