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

DS-3542: Stateless sessions authentication #1873

Merged
merged 43 commits into from Dec 2, 2017

Conversation

@tomdesair
Copy link
Contributor

tomdesair commented Oct 26, 2017

This PR illustrates the concept of using "stateless sessions" in DSpace 7 (kudos to @Frederic-Atmire). This allows the DSpace 7 REST webapp to scale horizontally (just add more nodes instead of upgrading the hardware) without additional configuration. You can test this with the following scenarios:

Test clustered setup with curl

  1. Build and deploy this PR to two different Tomcat instances. In my example this will be localhost:8080 and localhost:8280.

  2. All Tomcats in the same cluster have to share the same server key-part. You can pass this secret as a command line option or by using export JAVA_OPTS="-Djwt.token.secret=serverS3cret" when starting both Tomcats.

  3. curl -v -X POST --data "user=edward.maaier@mailinator.com&password=mypass" "http://localhost:8080/dspace7-rest/api/authn/login"

HTTP/1.1 200 OK
Authorization: Bearer eyJhbG...COdbo

  1. curl -v "http://localhost:8080/dspace7-rest/api/authn/status" -H "Authorization: Bearer eyJhbG...COdbo"

HTTP/1.1 200 OK

{
  "okay" : true,
  "authenticated" : true,
  "type" : "status",
  "_links" : {
    "eperson" : {
      "href" : "http://localhost:8080/dspace7-rest/api/eperson/epersons/2245f2c5-1bed-414b-a313-3fd2d2ec89d6"
    },
    "self" : {
      "href" : "http://localhost:8080/dspace7-rest/api/statuses"
    }
  },
  "_embedded" : {
    "eperson" : {
      "uuid" : "2245f2c5-1bed-414b-a313-3fd2d2ec89d6",
      "email" : "atmirenv@gmail.com",
      ...
      }
    }
  }
}
  1. curl -v "http://localhost:8280/dspace7-rest/api/authn/status" -H "Authorization: Bearer eyJhbG...COdbo"

HTTP/1.1 200 OK

{
  "okay" : true,
  "authenticated" : true,
  "type" : "status",
  "_links" : {
    "eperson" : {
      "href" : "http://localhost:8280/dspace7-rest/api/eperson/epersons/2245f2c5-1bed-414b-a313-3fd2d2ec89d6"
    },
    "self" : {
      "href" : "http://localhost:8280/dspace7-rest/api/statuses"
    }
  },
  "_embedded" : {
    "eperson" : {
      "uuid" : "2245f2c5-1bed-414b-a313-3fd2d2ec89d6",
      "email" : "atmirenv@gmail.com",
      ...
      }
    }
  }
}
  1. curl -v "http://localhost:8280/dspace7-rest/api/authn/logout" -H "Authorization: Bearer eyJhbG...COdbo"

HTTP/1.1 200 OK

  1. curl -v "http://localhost:8080/dspace7-rest/api/authn/status" -H "Authorization: Bearer eyJhbG...COdbo"

HTTP/1.1 200 OK
{"okay":true,"authenticated":false,...}

Test using the HAL Browser

We've added a "Login" link in the top menu of the HAL Browser. You can also reach this page by visiting http://localhost:8080/dspace7-rest/login.html. This page will allow you to login using a POST request (so that credentials are not logged in access log files). On successful login, the HAL Browser will store the token and use it for all future requests. Note that automatic token renewal has not been implemented in the HAL Browser.

Implementation Details

To implement the stateless sessions, we're using JSON Web Tokens (JWT). These tokens are signed with a secret key that is unique per EPerson (session) and which also take into account the ip-address of the user (to prevent session hijacking). This also allows the backend to invalidate tokens. Tokens currently expire after 30 minutes but this is adjustable through configuration.

There are still a lot of TODOs, but we think the code is ready enough illustrate this concept to the community. This is what we still plan to do if we are to finalise this implementation:

  • More reviewing and testing (e.g. ldap, shibboleth) => https://jira.duraspace.org/browse/DS-3764
  • Check if we should move from cookies to HTTP headers
  • Double check the signing algorithm and settings
  • Enable token compression
  • Support authentication on different devices at the same time
  • Better exception handling
  • Make sure the default is to generate a random server key part when no static key is defined

UPDATE: We addressed all TODO's. I've also updated the testing procedure. This PR can be considered final now.

While writing tests and doing some more testing, we ran into the Flyway issue mentioned in https://jira.duraspace.org/browse/DS-3745. On a new machine, the Flyway migration was indeed not executed. To make sure Flyway is check on Spring Boot startup, we added a FlywayDatabaseMigrationInitializer to the Application config.

@@ -139,11 +140,8 @@ public String endRequest(Exception failure) {
private void endRequest(String requestId, Exception failure) {
if (requestId != null) {
Session session = null;

This comment has been minimized.

Copy link
@abollini

abollini Oct 31, 2017

Member

this can be removed

@@ -102,7 +103,7 @@ private String startRequest(Request req) {
for (RequestInterceptor requestInterceptor : interceptors) {
if (requestInterceptor != null) {
try {
requestInterceptor.onStart(req.getRequestId(), req.getSession());
requestInterceptor.onStart(req.getRequestId(), null);

This comment has been minimized.

Copy link
@abollini

abollini Oct 31, 2017

Member

as the concept of session has been completely withdrawn (the SessionService interface was deleted) it should be better to change the method signature


private SessionRequestServiceImpl sessionRequestService;
private StatelessRequestServiceImpl sessionRequestService;

This comment has been minimized.

Copy link
@abollini

abollini Oct 31, 2017

Member

please rename the attribute

{
private boolean okay;
private boolean authenticated;
private String email;

This comment has been minimized.

Copy link
@abollini

abollini Oct 31, 2017

Member

this should be replaced with an EPersonRest object

@abollini

This comment has been minimized.

Copy link
Member

abollini commented Oct 31, 2017

I did a first walk trough the code and it looks good. I don't like to have the shibboleth endpoint defined (hardcoded) in the controller, it should be plugged in by the ShibbolethProvider but I need to double check this aspect. In the next days I will do some concrete test

Copy link
Member

tdonohue left a comment

👍 The code here looks good. I do wish there were some unit tests provided, but I'm OK with merging as long as we get some testers

@tomdesair

This comment has been minimized.

Copy link
Contributor Author

tomdesair commented Nov 2, 2017

Hi @abollini and @tdonohue, thanks for reviewing!

I've read your comments and I fully agree. There are still some other TODOs in the code (and in the description above) that need to be fixed before we can merge this.

But I'm holding on with fixing these until the DSpace community agrees that we'll go with stateless authentication for now (see this discussion). Just to prevent wasted development effort.

@tomdesair tomdesair force-pushed the atmire:POC_stateless_sessions branch from e97cb29 to 6249f3f Nov 6, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 6, 2017

Coverage Status

Coverage decreased (-0.04%) to 20.777% when pulling c5d4f9d on atmire:POC_stateless_sessions into 716912f on DSpace:master.

@tomdesair

This comment has been minimized.

Copy link
Contributor Author

tomdesair commented Nov 13, 2017

I've updated the comment above to reflect the latest implementation changes.

We are aware that we lack unit and integration tests. That's still on our TODO list.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 15, 2017

Coverage Status

Coverage decreased (-0.05%) to 20.763% when pulling bf8d63c on atmire:POC_stateless_sessions into 716912f on DSpace:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage increased (+0.5%) to 21.542% when pulling 044e243 on atmire:POC_stateless_sessions into 5d507ed on DSpace:master.

@tomdesair tomdesair force-pushed the atmire:POC_stateless_sessions branch from e79da5e to 5b0c101 Nov 23, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 23, 2017

Coverage Status

Coverage increased (+0.5%) to 21.542% when pulling 5b0c101 on atmire:POC_stateless_sessions into 5d507ed on DSpace:master.

@tomdesair

This comment has been minimized.

Copy link
Contributor Author

tomdesair commented Nov 23, 2017

@abollini and @tdonohue This PR can be considered final now. I've update the description above with some testing and implementation details ☝️

Once this is merged, I'll transfer the description to the DSpace Wiki as documentation.

Copy link
Member

abollini left a comment

Tested it works as expected. Integration Tests are provided to validate normal workflow and attempts to violate the security.
The only potential issue is that the login method allows use of the GET method where the documentation talks about use of the POST to avoid logging. I'm going to create an issue for that

@abollini abollini merged commit 8311ae2 into DSpace:master Dec 2, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.5%) to 21.542%
Details
@abollini

This comment has been minimized.

Copy link
Member

abollini commented Dec 2, 2017

Hi @tomdesair
please move the documentation in the REST contract https://jira.duraspace.org/browse/DS-3776

I have also created a ticket for the use of the GET method in the login endpoint

@tomdesair tomdesair deleted the atmire:POC_stateless_sessions branch Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.