Multiple API Key support per account

[maria-farooq]

Need the ability to create and manage multiple API keys / tokens for a given account so that a user can give out different tokens to different applications as needed.

[maria-farooq]

Hi @scottbarstow @deruelle @gvagenas @abdulazizali77 ,

Arslan is interested to contribute on this task. We will watch it over to address in case he needs any clarifications.

Thanks

[arslan70]

Hi guys. There are 2 ways to go about this.

1. To expose the database wrapped in a microservice instead of direct JDBC connectivity. This approach will certainly cater scalability and decouple this logic but I can see there is a lot of refactoring required.

However if the projected traffic don't need to be distributed over different servers then this approach might be an overkill. If we are going to take this road. There are 2 potential options to choose a) Akka clustering b) Spring rest service with cloud features. You get out of box support of service discovery with netflix's Eureka if number of services grow.

2. Change 1-1 mapping of auth token to many to many. This can be started with a schema change, change in the DAO layer and analysis of which methods will be affected.

[scottbarstow]

@arslan70 Have you gotten Restcomm Connect working yet on your machine and tested the APIs?

[arslan70]

Hi @scottbarstow , yes my local fork is running fine. The account API seems to be working fine

arslan@arslan:/opt/Restcomm-JBoss-AS7-7.6.0/bin/restcomm$ curl -X GET http://ACae6e420f425248d6a26948c17a9e2acf:63838e426de7a1ff61647222dc71334@192.168.10.100:8080/restcomm/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf

ACae6e420f425248d6a26948c17a9e2acf Default Administrator Account administrator@company.com active Administrator Full Tue, 24 Apr 2012 00:00:00 +0500 Mon, 22 May 2017 22:11:55 +0500 63838e426de7a1ff61647222dc713340 /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/AvailablePhoneNumbers /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Calls /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Conferences /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/IncomingPhoneNumbers /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Notifications /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/OutgoingCallerIds /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Recordings /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Sandbox /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/SMS/Messages /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Transcriptions The account auth token has a 1:1 relationship. Right now there is no relationship with APIs. To have multiple api keys at the schema level, a resolution table say account_apis like below can work. account_sid | api_id | auth_token ACae6e420f425248d6a26948c17a9e2acf | Geolocation_api_id | xyz I'm still analyzing the impact on the code which is doing authentication. What do you think about it?

[scottbarstow]

@arslan70 the task is not to have API-specific keys. It is to support multiple API tokens for the account. Change what is now 1:1 to 1:many

[arslan70]

I see, that is relatively simpler to implement. I'll work on a patch straight away. I don't think it requires elaborate planning.

[arslan70]

I managed to change the schema and mybatis DAO layer to accommodate multiple auth tokens per user. Have added test cases to handle auth token addition and deletion. I'm a little lost at tying up all together including the UI and authentication. Right now the default token is used as the token at zero index. But that's just to verify the existing functionality is not broken. How to go about taking this further?

[deruelle]

@arslan70 can you also make sure in your tests that accessing the API via any of the tokens created actually work ? I think we need to create a separate github issue for UI as it will need to involve @ipsilantide for how this would look like from a UX perspective.

[scottbarstow]

@arslan70 A good place to start is where the tokens are actually being used in the various APIs.

It would also be good to create a PR so that we can review the work to date and see how far along you've gotten and give you clear direction for finishing things off. As Jean stated we can tackle the UI elements with help from the UX designer once the plumbing is all done.

I'm a bit concerned about your comment of there being a "default" in the array. That should not be necessary, so I'd like to review that in particular.

Sounds like you're making great progress, so if you want to create a PR we can begin to work to completion on the task.

Thanks

On Tue, May 30, 2017 at 6:14 AM, Jean Deruelle notifications@github.com wrote:

@arslan70 https://github.com/arslan70 can you also make sure in your tests that accessing the API via any of the tokens created actually work ? I think we need to create a separate github issue for UI as it will need to involve @ipsilantide https://github.com/ipsilantide for how this would look like from a UX perspective.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RestComm/Restcomm-Connect/issues/2164#issuecomment-304835200, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtjozMAKtWL2t53SxbqVrUDRmSCTIiJks5r--wRgaJpZM4NhPxy .

-- scottbarstow.com @scottbarstow +1-919-349-9473 (m)

[arslan70]

Hi @deruelle @scottbarstow let me give you a quick review of the changes made.

1. Removed auth_token from restcomm_accounts table
2. Added a new table restcomm_accounts_auth_tokens account_sid | auth_token | status
3. Changed mybatis layer to accommodate above changes
4. Changed the code which authenticates tokens to match from a list instead of single string value.
5. Changed the restcomm.script which inserts hard coded tokens in database as there is no implementation of adding tokens from UI.

Now I am able to invoke the API using both tokens in the table. token - 1: 88f8c12cc7b8f8423e5c38b035249166 curl -X GET http://ACae6e420f425248d6a26948c17a9e2acf:88f8c12cc7b8f8423e5c38b035249166@192.168.10.100:8080/restcomm/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf

ACae6e420f425248d6a26948c17a9e2acf Default Administrator Account administrator@company.com active Administrator ...... Token - 2: 77f8c12cc7b8f8423e5c38b035249166 curl -X GET http://ACae6e420f425248d6a26948c17a9e2acf:77f8c12cc7b8f8423e5c38b035249166@192.168.10.100:8080/restcomm/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf ACae6e420f425248d6a26948c17a9e2acf Default Administrator Account administrator@company.com active Administrator Full Tue, 24 Apr 2012 00:00:00 +0500 Tue, 30 May 2017 23:26:45 +0500 77f8c12cc7b8f8423e5c38b035249166 /2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf @scottbarstow by default I meant the token at index zero of tokens list. That will be displayed in the UI(Screenshot).  Untill we change it to show a list. I'll work on it but need to pick up angular first. Don't have much experience with angular. @scottbarstow I'm sorry I'm not sure what is a PR in this context.

[deruelle]

good idea @arslan70 to change it to list as bare minimum change to UI to reflect addition or removal of keys to the Account.

Did you cover changes to the API to add or remove a token ? For removal, do you cover the fact that we need at least one token ?

PR means Pull Request, feel free to read the Open Source Playbook

[scottbarstow]

@arslan70 these changes look good. Could you also please add a new varchar field to the new table for "description" that is say 64 chars so that users can add a descriptor of what the token is used for? This should be nullable i.e. not required. I also think status could likely be a bool instead of a "status" field, i.e. "isActive". It's only ever active or not.

accountsid, token, description, isActive sid1, token1, "", true sid1, token2, "Used for XYZ Chatbot", true sid1, token3, "Ipitimi app token", false

I go back and forth on whether we need the isActive field. A part of me says that if it's in the table, it's active and if the user doesn't want it used any more it should be deleted to prevent unwanted usage down the road.

@deruelle do you have an opinion here?

On Wed, May 31, 2017 at 3:29 AM, Jean Deruelle notifications@github.com wrote:

good idea @arslan70 https://github.com/arslan70 to change it to list as bare minimum change to UI to reflect addition or removal of keys to the Account.

Did you cover changes to the API to add or remove a token ? For removal, do you cover the fact that we need at least one token ?

PR means Pull Request, feel free to read the Open Source Playbook https://docs.google.com/document/d/1RZz2nd2ivCK_rg1vKX9ansgNF6NpK_PZl81GxZ2MSnM/edit#heading=h.mcjitemt6ng1

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RestComm/Restcomm-Connect/issues/2164#issuecomment-305108598, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtjo6LSgGnqVXKN5rVOye6-kvJp3NmNks5r_RbCgaJpZM4NhPxy .

-- scottbarstow.com @scottbarstow +1-919-349-9473 (m)

[arslan70]

@deruelle >Did you cover changes to the API to add or remove a token ? Right now only the DAO layer has this. The functionality needs to be exposed through http endpoints(to-do).

@deruelle >For removal, do you cover the fact that we need at least one token ? Yes, delete method in DAO layer fetches the existing tokens and return false if there is only one token in database. I think we need to have a custom Exception to handle in a better way. The exception will be propagated back to the UI. What do you think?

@scottbarstow Description field makes sense, I'll add it.

For the fact about isActive field, I think we should have this field. Consider this scenario, I'm your client and buy an API key to use. I don't pay you for sometime and you have to revoke my token. Now if we delete the token itself to stop using me the api. When I pay you back you will have to give me a new token. I'll have to update my application as well.

[deruelle]

Thanks @arslan70

The functionality needs to be exposed through http endpoints(to-do).

Ok let's cover this as well.

The exception will be propagated back to the UI

Let's return a proper HTTP error code back from the API. the UI will use the API and based on HTTP Status code will display the right message.

isActive Field

@arslan70 if the customer stops paying, there is more to it than just the token, There is clients, DIDs, etc. The account can be suspended (already supported today ie nothing will be removed) until payment is done. I would tend to go with not having a isActive field to avoid bloating the DB and just remove if not needed.

[arslan70]

Hi @deruelle and @scottbarstow thanks for the previous feedback. Auth token Http APIs are implemented.

1. AddAuthToken() Method: Post Relative URL: /restcomm/2012-04-24/Accounts/addAccountAuthToken Parameters:

• AuthToken (String)
• Description (String

Response: Sucess: 200 ok Failure: 503 Internal server error

2. GetAuthTokens() Method: Get Relative URL: /restcomm/2012-04-24/Accounts/getAuthTokens Parameters: none

Response: Sucess: 200 ok

100382761f0479ab3e31fe835d7f5583 77f8c12cc7b8f8423e5c38b035249166

3. DeleteAuthToken() Method: Post Relative URL: /restcomm/2012-04-24/Accounts/deleteAccountAuthToken Parameters:

• AuthToken (String)

Response: Sucess: 200 ok Failure: 404 Not found

I'm able to make API calls using tokens added by HTTP api. I'm going to prepare the code for a merge now if this seems alright.

[scottbarstow]

@arslan70 yes please create a pull request and check the code in.

We also probably don't want to be able to create new tokens by having a token. This needs to be under tighter control, as what you're exposing is a huge security hole. If I know an API token, I can then create unlimited other API tokens. We'll need to put this under tighter security controls.

also, I'm not in love with the API methods in the above examples, as they're not RESTful. The url construct should be something like .../Accounts/{sid}/tokens and then support the GET / POST / PUT / DELETE HTTP methods rather than having method names in the url.

@maria-farooq Can you weigh in here on the possible security options to restrict API token creation?

[deruelle]

@scottbarstow this is tied to this issue on securing the Accounts API using password only https://github.com/RestComm/Restcomm-Connect/issues/1496. The same should apply here.

[arslan70]

Hi @scottbarstow

Can we Restify the endpoints in a further issue? Right now I wanted to share the progress through the pull request to validate the changes. I have created a pull request.

As far as security is concerned these endpoints are secured. We can discuss further on this.

[scottbarstow]

Yes it's fine to create the PR and go from there.

On Wed, Jun 7, 2017 at 2:24 AM, arslan70 notifications@github.com wrote:

Hi @scottbarstow https://github.com/scottbarstow

Can we Restify the endpoints in a further issue? Right now I wanted to share the progress through the pull request to validate the changes. I have created a pull request.

As far as security is concerned these endpoints are secured. We can discuss further on this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RestComm/Restcomm-Connect/issues/2164#issuecomment-306698871, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtjowyK1DtXF-saVwRhLPfIGmPJ3DX2ks5sBkIkgaJpZM4NhPxy .

-- scottbarstow.com @scottbarstow +1-919-349-9473 (m)

[marca56]

• the above is a workable start, I think;
• but I would also add the ability to revoke a token from the server;
• and would also want the ability to manage a list of tokens that get deprecated or rotated on some interval

[scottbarstow]

@marca56 Token revocation is already supported. Not sure what you are talking about here with regard to managing a list of tokens that get deprecated or rotated. We purposely decided not to have any sort of status on the token (see earlier comments). Tokens exist or don't.

Please clarify what you're thinking here. Thanks

[arslan70]

@scottbarstow I'm refactoring the endpoints to be URL: restcomm/2012-04-24/Accounts/tokens POST: will add a token DELETE: will delete the token GET: will list the tokens

SID is implicitly extracted from the authenticated account so it is not part of the URL. Does this make sense?

@scottbarstow as far as the migration script go. I'm trying to imagine it will take the existing auth tokens tied to every account and put them in the newly created auth token table.

At what stage the script runs? Do I have to write a runner for such a script? I believe there should be similar scripts in place for previous schema changes, but with a quick scan of the code I couldn't find any. I'm looking into it but need a nudge from you in the right direction.

@scottbarstow Auth token creation method is storing the provided token string and saving as it is. Do we need to apply a hash to the string or some other implementation is needed?

@marca56 Do you mean to keep a track of the deleted tokens? We can do this by moving the tokens into a separate table upon deletion. But in a previous discussion it was decided it wasn't really needed.

[scottbarstow]

@arslan70 in order:

Ok on SID. @maria-farooq can you chime in on the migration script / how we do schema migrations?

On Auth token creation we should not be passing in the value. The system should be responsible for token generation, not the client. Let me know if that makes sense.

On Sat, Jun 10, 2017 at 9:46 AM, Arslan notifications@github.com wrote:

@scottbarstow https://github.com/scottbarstow I'm refactoring the endpoints to be URL: restcomm/2012-04-24/Accounts/tokens POST: will add a token DELETE: will delete the token GET: will list the tokens

SID is implicitly extracted from the authenticated account so it is not part of the URL. Does this make sense?

@scottbarstow https://github.com/scottbarstow as far as the migration script go. I'm trying to imagine it will take the existing auth tokens tied to every account and put them in the newly created auth token table.

At what stage the script runs? Do I have to write a runner for such a script? I believe there should be similar scripts in place for previous schema changes, but with a quick scan of the code I couldn't find any. I'm looking into it but need a nudge from you in the right direction.

@scottbarstow https://github.com/scottbarstow Auth token creation method is storing the provided token string and saving as it is. Do we need to apply a hash to the string or some other implementation is needed?

@marca56 https://github.com/marca56 Do you mean to keep a track of the deleted tokens? We can do this by moving the tokens into a separate table upon deletion. But in a previous discussion it was decided it wasn't really needed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/RestComm/Restcomm-Connect/issues/2164#issuecomment-307566116, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtjoza73sRMkIZbwLQypJH3rHdilC3Oks5sCp5JgaJpZM4NhPxy .

-- scottbarstow.com @scottbarstow +1-919-349-9473 (m)

[arslan70]

The system should be responsible for token generation

I have changed the logic to generate a MD5 hash of a random integer as a token.

I want to discuss an issue. If the token generated from initial password is deleted the user is locked out. Can't sign in again, can't reset the password as well because the account is not in uninitialized state.

One way to fix this is to mark the token generated from password as non deletable. Thoughts on this?

[marca56]

this is for the roadmap because I don't think we have a reset or forgot username or password feature on cloud yet. Is that correct, @scottbarstow ? If we did, I would recommend not setting the token generated as non-deletable and just send the user a forgot username/password dialog and allow them to reset the account (which should generate a new password and token). We should think about 2FA here too.

[deruelle]

Please see guidelines from Twilio to be backward compatible on this https://www.twilio.com/docs/api/rest/keys

[deruelle]

@arslan70 on

I want to discuss an issue. If the token generated from initial password is deleted the user is locked out. Can't sign in again, can't reset the password as well because the account is not in uninitialized state.

One way to fix this is to mark the token generated from password as non deletable. Thoughts on this?

the right thing to do is separate the token from password as the work on https://github.com/RestComm/Restcomm-Connect/issues/1496 is doing. In the meanwhile, I think we can leave the user being locked out. The parent account should be able issue a password reset for the user.

[scottbarstow]

@arslan70 If I understand the issue above, you are saying that the current token is generated based on the user's initial password. Do I have that right?

And, also if I understand, you've created some new algorithm (that is not the same as the current token algo) that generates new tokens?

I don't understand the relationship between username / password above generating tokens. The two should be unrelated. Perhaps easier to jump on a chat to go through the issues rather than trying to work them out through Github and email.

[arslan70]

Hi @deruelle @scottbarstow the endpoint implementation is more or less the same as https://www.twilio.com/docs/api/rest/keys

We don't have the update method for API key(yet).

@scottbarstow

you are saying that the current token is generated based on the user's initial password.

No. Every new token is created by a MD5 hash of a random integer.

Let me try to explain the issue here, we can have a chat on this as well.

Before the schema change the API token was generated by applying a MD5 hash and saved along with SID(1:1). The password itself was not stored. All the validation was done by either applying the hash to the provided string(Http login password) or directly against the hash.

Something like Md5Hash(Password provided by user) == Hash saved against SID

Now with the schema change the generated token goes in a list instead of (1:1). At the start when user updates his password there is a single hash token generated from his password. The user can log in because

Md5Hash(Password provided by user) exists in a list of hash tokens saved against SID

Now we do have a HTTP API to delete the token. I can pass in the token which was generated when I updated my password for the first time. The token gets deleted and I can no longer use restcomm because the authentication will fail when it tries to match the password provided against the list of tokens.

If there is work being done on reset password, it may work because it will put a token to be matched against to make the validation work. But if I'm the super admin then I believe all is lost.

One suggestion is to use a status field against tokens. The token which is generated by the first password update will have a status which will prevent the delete endpoint to delete it.

Or we save the password against an SID and the validation will be like

If (Password provided == password saved in db || Md5(Password provided) exists in list of Hash tokens

Do I make any sense? Please feel free to buzz me on skype or any other channel convenient for discussion.

[scottbarstow]

I get it now. Let's take the discussion out of here to resolve

[scottbarstow]

We should (imo and @deruelle may disagree) completely disconnect these two concepts as is being done in #1496 . You should assume that you won't have to worry about this issue of conflation of password and token. Let's try and get these two things done so that when done we have a list of tokens that are separate and distinct from account pw

[deruelle]

@scottbarstow I agree, this is what I was trying to convey in my earlier message as well https://github.com/RestComm/Restcomm-Connect/issues/2164#issuecomment-308153893

[scottbarstow]

One question I've probably missed in all of this @arslan70 is that we need a new UI for this. I guess once the PR is checked in, we can assign a UI task to our team unless you're interested in taking this part on as well.

[deruelle]

@scottbarstow agreed let's create a separate Github Issue for the UI for this involving @ipsilantide and @ammendonca

[arslan70]

@scottbarstow it will be a good idea for a UI expert to take a lead on this