tediousjs / tedious

Node TDS module for connecting to SQL Server databases.
http://tediousjs.github.io/tedious/
MIT License
1.56k stars 443 forks source link

azure-active-directory-access-token REFRESH conn automatically on expiring #1144

Open panbhatt opened 3 years ago

panbhatt commented 3 years ago

I am working on application, where we are connecting to the SQL Server database and trying to create the connection pool (with TYPEORM and Sequelize). As per the org policies our token expires every 24 hours, that means all the connections in the pool are getting expired, and we have to restart the app to make sure every object /pool is created again.

Do we have an internal functionality to refresh the token automatically when it expires. It seems so obvious that the solution to restart the application wont' fly. Is there any way in the app we can do it, so that we can save restarts. Any help would be appreciated.

IanChokS commented 3 years ago

Hi @panbhatt I'll take a look and see if there's a way for tedious to do that!

panbhatt commented 3 years ago

Thanks @IanChokS : What i think can be done is that besides taking the token details, we can ask for a function (or a object having a function) that can be called once the duration of the token got expired. However, i would leave it up to the community to decide what best can be done. The other way to achieve this to to hack the ORM solutions and call their connection manager (which is bypass) and i dont think its a good way to go.

cameronstubber commented 3 years ago

I think a good solution could be that we have a boolean flag in the configuration object used at the start to signal if we want to auto refresh or not.

I'm sure there is somebody out there who is happy with the session expiring.

panbhatt commented 3 years ago

Any update on this, team.

arthurschreiber commented 3 years ago

@panbhatt I think that's not really an issue in tedious, but an issue in the connection pool library you're using? If the connection pool library would allow specifying a factory function to "build" the connection (or connection configuration), you could use that to refresh the token ever so often.

panbhatt commented 3 years ago

the only caveat with this issue is all the ORM's has to change their structure just for this. i.e. they have to implement this functionality, which i think its not required, as the problem is present at TEDIOUS JS level, if in the dialect options they take a function (since they are the only who is providing the functionality for token validation etc) and refresh their connection details, so that whenever a new connection is required, it always used the fresh token. i do not understand why we need to change 10's of other libraries, when this can be done at one central place.

arthurschreiber commented 3 years ago

@panbhatt I understand. How do you feel about allowing to pass in an async function as token (something like () => Promise<string> | string) for the azure-active-directory-access-token, and we then call this function to get the token when establishing a connection?

Would you be willing to open a PR that implements this?

panbhatt commented 3 years ago

i can try working on that, however it seems like it would be hard for me to test it without an availability of azure sql server and azure auth. let me know, if can be provided an env, i can try it.

cameronstubber commented 3 years ago

After playing with this a bit in the recent week, I actually now also agree that this is not up to Tedious to deal with. Mainly, since the token is already created before you even start with Tedious. You are just passing the token through. It should be up to the application or the library doing the authentication in the first place to ensure it has an always valid token.

cameronstubber commented 3 years ago

Actually it could be nice to allow us to provide tedious with a callback if it throws an error due to an expired token. That way, we can feed it the a function which will get a new token for it, if it encountered an expiry. Then tedious doesnt have to actually do any token authentication work, but it can help to provide a seamless experience controlled by the application.

panbhatt commented 3 years ago

just wanted to know will it be a good practice:

  1. Pass an async function as a dialect option and in tedious we can invoke him always prior to creating a new connection.
  2. There is an idle time out for the connection (which i assume should always be low then that the timeperiod of the token), so i am assuming if a connection is not used it would timeout automatically.
  3. prior to giving the connection back to the pool we can check if the connection is Active and Valid), if it is not then recreate the connection. Just wondering it would have to have an impact on the connection pool, since once the conn is in the pool, we in TEDIOUSJS has no idea when it is going to be expired.
arthurschreiber commented 3 years ago

3. prior to giving the connection back to the pool we can check if the connection is Active and Valid), if it is not then recreate the connection. Just wondering it would have to have an impact on the connection pool, since once the conn is in the pool, we in TEDIOUSJS has no idea when it is going to be expired.

Maybe I'm wrong here, but the token is only checked once, when you open the connection. As long as the connection is kept open, the token is not re-checked. Only when the token is used when a new connection is established by the connection pool after the token has expired, the authentication fails. This could e.g. happen due to the idle time out of the connection pool you mentioned.

This is why I suggested we allow the token option to be a (async) function - it'd be invoked by tedious only when a connection is established, and the function would be responsible for returning a token that is valid at that moment. The function could decide itself how it handles token caching and expiration, and the connection pool does not have to know about any of this. Idle timeout of the connection pool would also not be an issue.

panbhatt commented 3 years ago

I tried to work on this and need your feedback on this as just trying to make sure (without actually testing this), what we are proposing is right. Changes are as follows:

extra: {
          authentication: {
            type: "azure-active-directory-access-token",
            options: {
              encrypt: true,
              token: "",
              tokenGenerator,
            },
          },
}

async function tokenGenerator(): Promise<String> {
 // This function will get the token remotely and will use some function E.g. await getPresentToken()
  return "abc";
}

in connection.ts (line no 235)

interface AzureActiveDirectoryAccessTokenAuthentication {
  type: 'azure-active-directory-access-token';
  options: {
    /**
     * A user need to provide `token` which they retrieved else where
     * to forming the connection.
     */
    token: string;
    tokenGenerator() : Promise<String>; 
  };
}

in line no 2531: in function sendLogin7Packet()

case 'azure-active-directory-access-token':
        
        payload.fedAuth = {
          type: 'SECURITYTOKEN',
          echo: this.fedAuthRequired,
          fedAuthToken: await authentication.options.tokenGenerator()
        };
        break;

the problem is this function sendLogin7Packet is not async() so we can't use await here. If we have to then we need to make sure async is replicated back to the top level. Just wanted to know is this is enough for this. Adding async might be a problem.

panbhatt commented 3 years ago

Another way to achieve this can be since ASYNC would force a lot of things to change is:

  1. Pass the token generator as a function to the extra options.
  2. Also pass a timeout interval (this interval would always be less then the token timeout )E.g. if the token timeout is 60 minutes, it can be 50 minutes. we will do a setInterval call in the TEDIUOUS package to update the TOKEN variable continously via running a function that will call the tokenGenerator function in an inline Async block. E.g
    setInterval(5000, ()=> {
    (async()=> { await tokenGenerator() } )(); 
    }

    this will make sure whenever the token is about to expire a new token is generated prior to it.

we can look in the expiry of individual connection too so that we dont have any pending connections that are expired.

mathew-pigram commented 3 years ago

@panbhatt - did you get this to work? I'm facing the same problem with tokens expiring. Any guidance you could provide would be much appreciated.

panbhatt commented 3 years ago

Hey @mathew-pigram : There is no way you can achieve it as of now via using this driver. I have talked to the Microsoft guys and got an answer to use another method which uses Service Principal assigned to the App Service. That's how i had overcome this issue. This issue is not on the focus of MS team as of now.

mathew-pigram commented 3 years ago

@panbhatt Thanks for the quick reply.

I assume your connection now looks similar to this?

authentication: { type: 'azure-active-directory-msi-app-service', }

Did you encounter any issues when the app is not executing inside an App Service - e.g. when developing on your PC?

panbhatt commented 3 years ago

Yes, that's correct. I used that one. In our organization we are not being allowed to test this on our PC, we are working against SQL Server on our local PC, but in order to test this we deploy our APP to Azure and use the kudu console to make the changes. but this indeed worked.

mathew-pigram commented 3 years ago

@panbhatt Thanks for your help, much appreciated.

funkydev commented 3 years ago

Regarding all discussions, I'm on a way to implement token generator functionality. It would allow removing deprecated @azure/ms-rest-nodeauth package from tedious (that would be a breaking change, mentioned in https://github.com/tediousjs/tedious/pull/939).

But instead of implementing @azure/identity dependency, users would be allowed to implement retrieving tokens as they want, without modifications to the tedious library. They would be free to choose what library they want to use.

That would make tedious code a little bit cleaner, without unnecessary dependencies, that could be deprecated anytime.

I'll create 3 pull requests:

  1. PR with a small refactor that will clean up today's authentication types. (https://github.com/tediousjs/tedious/pull/1242)
  2. PR with the implementation of token generator feature (in progress)
  3. PR that removes usage of @azure/ms-rest-nodeauth – BREAKING CHANGE (todo)

So keep your fingers crossed and just wait for updates.

elliot-huffman commented 1 month ago

I made a PR that should help with this issue:

1624

This PR will allow you to configure your auth system the way you need and just pass the credential object. This should remove the requirement to get a new access token manually as the credential object can now generate them via the standards maintained by MS at @Azure/Identity