feat: ActiveDirectoryPassword and sqlpassword authentication support #707

Hadis-Knj · 2018-02-08T21:40:53Z

squash merged of #661, please see details and previous discussion there.

Hadis-Knj · 2018-02-08T21:51:47Z

noticed the latency in generating token in adal-node lib, follow up on AzureAD/azure-activedirectory-library-for-nodejs#192

src/connection.js

arthurschreiber

Hey @Hadis-Fard,

what's the current state of this PR? @David-Engel let me know that there's a bunch of customers waiting for it's integration, so I'm trying to figure out what needs to be done to get it into a shippable state.

I left a few comments for things that jumped out to me, but I also wanted to check with how you feel about these changes. 🙇

arthurschreiber · 2018-05-23T19:33:07Z

src/token/fedauth-info-parser.js

+
+      const countOfInfoIDs = data.readUInt32LE(infoData.offset);
+      infoData.offset += 4;
+      async.times(countOfInfoIDs, () => {


I don't think async.times usage is required here. The full token data was already loaded into data, and can be processed synchronously here.

arthurschreiber · 2018-05-24T06:08:42Z

src/token/fedauth-info-parser.js

@@ -0,0 +1,61 @@
+const async = require('async');
+
+function readFedAuthInfoOpt(data, infoData, FEDAUTHINFOID, callback) {


It'd be great if this function could be refactored a bit for better readability. It can be a regular non-async method, and I think it should return a { stsurl: '...', spn: '...' } object.

Having it take and modify the infoData object in place is a bit confusing. 🤔

Also, FEDAUTHINFOID could be a top-level constant, instead of being passed in as well.

arthurschreiber · 2018-05-24T06:15:50Z

src/token/feature-ext-parser.js

@@ -0,0 +1,27 @@
+
+module.exports = function featureExtAckParser(parser, colMetadata, options, callback) {


I don't think this parser is quite right. 🤔

According to the MS-TDS spec, the FEATUREEXTACK token can return multiple different features, while this implementation only ever returns the latest feature that was read.

Also, it only returns the feature data length, not the actual data itself.

What do you think about changing the token to return an object with the following structure:

{ name: 'FEATUREEXTACK', event: 'featureExtAck', features: [ { id: number, data: Buffer, }, .... ] }

arthurschreiber · 2018-05-24T06:21:44Z

Oh, and another thing that we'll need before this can be merged is a SQL Server in Azure that we can use for CI testing.

@David-Engel @tediousjs/microsoft-contributors Is there some way to get an instance for testing?

Hadis-Knj · 2018-05-24T20:16:39Z

Thanks @arthurschreiber for reviewing this PR . It contains the AAD foundation and support of two auth methods - sqlpassword and AD password, and their unit tests. there are two other auth methods -token based auth and AD integrated auth - to be implemented, but those can be added incrementally in separate PRs after the foundation is merged, which is the same approach we took in implementing AAD in other client drivers.
I will go through your review comments shortly once i'm done with the tasks in hand🙇‍♀️ , about the ci we can go with the idea discussed in #624, @v-suhame could you help in giving the context or ideas you have 🙏 ?

Suraiya-Hameed · 2018-05-25T00:28:01Z

I have set environment variables in Travis (AZURE_PASS, AZURE_SERVER, AZURE_USERNAME) that has all the credentials needed for CI testing against Azure DB. We can add it as a stage after lint so, we end up testing with Azure only if every other test passes.

    - stage: azure
      node_js: 8
      before_install:
        - mkdir ~/.tedious
        - >
          echo '{
            "config": {
              "server": "'${AZURE_SERVER}'",
              "userName": "'${AZURE_USERNAME}'",
              "password": "'${AZURE_PASS}'",
              "options": {
                "port": 1433,
                "database": "test",
                "encrypt": "true"
              }
            }
          }' > ~/.tedious/test-connection.json
      before_script: skip
      script: npm run test-integration

How does that sound? Also, is it enough to just test with the latest version of node? 🤔

Suraiya-Hameed · 2018-05-25T00:37:30Z

We should also fix the test that fails intermittently (test/integration/pause-resume-test.js), it fails every time with remote server 😅
@arthurschreiber Does @chdh's suggestion in #518 (comment) sounds good or did you have anything else in mind?

arthurschreiber · 2018-05-25T17:13:48Z

@v-suhame That looks great! Can we add this change to the travis configuration in this pull request here?

Suraiya-Hameed · 2018-05-25T18:08:29Z

I forgot AAD had 4 auth mechanisms 😅 The initial credentials I had set was for SQL Server Authentication, I have added 2 more variables for Active Directory - Password authentication (AZURE_AD_USER and AZURE_AD_PASS). For now I can add 2 stages, one to test sqlpassword and other for AD password, we can always update it when we find a better alternative.

arthurschreiber · 2018-11-05T13:14:01Z

@Hadis-Fard This should be almost ready to merge now. I had to update some tests to better handle missing permissions and will extract these changes out of this PR, but I think this should be ready otherwise. Would you mind taking another look over the refactorings I made? 🙇

Hadis-Knj · 2018-11-05T22:41:05Z

src/connection.js

@@ -744,7 +743,7 @@ class Connection extends EventEmitter {
        switch (this.fedAuthInfo.fedAuthLibrary) {
          case FEDAUTH_OPTIONS.LIBRARY_ADAL:
            if (0 !== fedAuthAck.length) {
-              this.loginError = ConnectionError(`Active Directory authentication acknowledgment for ${this.fedAuthInfo.method} authentication method includes extra data`);


We should keep the method property, azure-active-directory should be a basket for various methods of AAD auth, for example security token, msi, and aad integrated auth.

I'd rather have individual auth types, like azure-active-directory-integrated. I was not really able to find a lot of documentation on the other two methods, but for those we could also have separate authentication types? 🤔

Hadis-Knj · 2018-11-05T22:55:58Z

src/connection.js

@@ -739,17 +738,14 @@ class Connection extends EventEmitter {
        if (!this.fedAuthInfo.fedAuthInfoRequested) {
          this.loginError = ConnectionError('Did not request Active Directory authentication, but received the acknowledgment');
          this.loggedIn = false;
-        }
-        switch (this.fedAuthInfo.fedAuthLibrary) {
-          case FEDAUTH_OPTIONS.LIBRARY_ADAL:


we should keep adal library check and FEDAUTH_OPTIONS and ADAL LIBRARY as we'll expand the feature to other auth types

Here as well, I'd rather have this as separate authentication types, instead of mixing this all together into one type with a large set of options.

Hadis-Knj · 2018-11-06T02:01:37Z

@arthurschreiber in addition to 'azure-active-directory' user should set the method of authentication which could beActiveDirectoryIntegrated, ActiveDirectoryPassword https://docs.microsoft.com/en-us/sql/connect/jdbc/connecting-using-azure-active-directory-authentication?view=sql-server-2017 and also MSI token, each with different underlying implementation, for example, adal-library is only valid for activedirectorypassword, and token based auth with use another lib, see 2.2.6.4 Login7 FedAuth row.

so in the config in addition to azure-active-password we should have another field to set the method as ones of the above,

new Connection({
  "server": "ADServer",
  "authentication": {
    "type": "azure-active-directory",
    "method" : "ACTIVEDIRECOTRYPASSWORD",
    "options": {
      "userName": "ADUser",
      "password": "foobar"
    }
  }
});

and have the FedAuthEnum back in code, check the inclusion and check tds version

 if (this.config.authentication.type === 'azure-active-directory') {      
      if(!Object.values(this.fedAuthInfo.ValidFedAuthEnum).includes(config.authentication.toUpperCase())) {          
        throw new Error(`${config.authentication} is not a valid authentication method`);
       }
 if (this.config.options.tdsVersion < '7_4') {
         throw new Error(`Azure Active Directory authentication is not supported in the TDS version ${this.config.options.tdsVersion}`);
        }

arthurschreiber · 2018-11-06T10:07:59Z

noticed the latency in generating token in adal-node lib, follow up on AzureAD/azure-activedirectory-library-for-nodejs#192

@Hadis-Fard The latency in getting a token was due to the way execution was delayed via recursive setImmediate calls. This was basically filling up the event loop and delaying the token response handling. I made a few refactorings that got rid of this waiting, and this sped up the whole token request process.

arthurschreiber · 2018-11-06T14:32:25Z

@Hadis-Fard We could also rename the authentication type to azure-active-directory-password, and have the integrated type be called azure-active-directory-integrated.

Hadis-Knj · 2018-11-06T23:07:39Z

👍 to have separate type for each kind of AAD auth. However all other drivers are using the same keyword for AAD methods . I suggest to rename azure-active-directory-password to ActiveDirectoryPassword and so on.
❤️ fixing the latency issue ❤️

Hadis-Knj · 2018-11-08T01:22:24Z

@arthurschreiber thanks for working on this 👌, we need to make adjustments and then AAD should be ready to merge:

update the AAD Keyword as discussed earlier.
"options" in json file after "authentication" entry won't get set.

  "server": "yourServer",
    "authentication": {
      "type": "azure-active-directory",
      "options": {
        "userName": "user",
        "password": "pwd",
    },
    "options": {
      "encrypt": true
    },
  }

in this case encrypt is not set to true and authentication fails with the message that server requires encrypt to be true

…auth

arthurschreiber · 2018-11-18T20:54:03Z

"options" in json file after "authentication" entry won't get set.

@Hadis-Fard there's two options values now, one at the top level configuration and one specific to the authentication that is being used. As can be seen in the Travis CI build, this works as expected. 👍

arthurschreiber · 2018-11-21T10:10:47Z

🎉 This PR is included in version 4.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Hadis-Knj · 2018-11-22T01:18:07Z

@arthurschreiber setting encrypt in the options object after authentication would raise Server requires encryption, set \'encrypt\' config option to true.' error.

var connectionADnew = {
    server: "ADServer.windows.net",
    authentication: {
      type: "azure-active-directory-password",
      options: {
        userName: "ADuser",
        password: "pwd",
    },
    options: {
      encrypt: true,
    }
  }
};

arthurschreiber · 2018-11-24T12:29:16Z

@Hadis-Fard I've trouble reproducing this error? This is exactly how the Travis tests are configured, and how I locally test against Azure DB.

tedious/.travis.yml

Lines 83 to 98 in da45fef

    
                   echo '{ 
        
                     "config": { 
        
                       "server": "'${AZURE_SERVER}'", 
        
                       "authentication": { 
        
                         "type": "azure-active-directory-password", 
        
                         "options": { 
        
                           "userName": "'${AZURE_AD_USER}'", 
        
                           "password": "'${AZURE_AD_PASS}'" 
        
                         } 
        
                       }, 
        
                       "options": { 
        
                         "database": "test", 
        
                         "encrypt": true 
        
                       } 
        
                     } 
        
                   }' > ~/.tedious/test-connection.json

arthurschreiber · 2018-11-24T18:19:04Z

@Hadis-Fard I only get the error you mentioned if I don't set encrypt at all (or set it to false). This is expected until #694 is merged (which I hope to do sometime next week).

Hadis-Knj · 2018-11-27T21:43:22Z

@arthurschreiber 😕 we don't need to worry about this error that much since as you said it'll gonna get fixed by merging #694

Hadis-Fard added 2 commits February 8, 2018 13:26

feat: aad authentication support

6e2d073

chore: add linebreak

8c971f8

Hadis-Knj mentioned this pull request Feb 8, 2018

ActiveDirectoryPassword and sqlpassword authentication #661

Closed

chore: lint fix

b4441fd

Suraiya-Hameed reviewed Feb 13, 2018

View reviewed changes

src/connection.js Outdated Show resolved Hide resolved

Hadis-Fard added 2 commits February 13, 2018 11:08

chore: fix else block indentations

6cfa3cd

refactor: use error instead of assert

4d91c4f

Hadis-Knj requested a review from arthurschreiber February 13, 2018 21:52

Hadis-Fard added 13 commits March 1, 2018 14:07

chore: merge branch master

511338d

refactor: remove authentication from connection class configs

1cad62c

test: add feature ext parser test

9f691c9

refactor: export login7paylod by name

de5e3b8

refactor: pass parsed data to callback

8a9b876

fix: add featureextack event to catch errors

dd2bbc9

test: add error cases for invalid aad auth

4dd2b1f

test: add unit test for fedauth info parser

fc24867

chore: removed required flow

12a902f

chore: rename Pos to offset

fe0122b

test: writeuint32atoffset unittest

ce3eceb

chore: rename writeuin32leatpos to offset

ce13bdc

chore: edit the error messages

c233c74

debben mentioned this pull request May 21, 2018

Support for AccessToken? #745

Closed

arthurschreiber reviewed May 24, 2018

View reviewed changes

arthurschreiber added 10 commits November 3, 2018 21:27

test: update feature ext parser tests

9c9ffa5

test: update login7 payload tests

cd388dc

test: skip TVP test cases when missing permissions

fffa06a

fix: use the global ADAL cache

a10055f

test: skip TVP tests on older TDS versions

e7b1bb5

test: fix skipping of tests

d39dfc1

test: use temporary procedures

2acfa02

test: use temporary procedure

231962e

test: handle setup failures more gracefully

2837053

test: increase timeouts

19be9f6

Hadis-Knj commented Nov 6, 2018

View reviewed changes

arthurschreiber added 2 commits November 18, 2018 21:38

fix: rename azure-active-directory to active-directory-password

4face04

Merge branch 'master' of github.com:tediousjs/tedious into federated-…

e3a5a89

…auth

arthurschreiber added 2 commits November 18, 2018 22:01

text: update tests for authentication type rename

8a597f9

fix: rename config option back

01a36f8

arthurschreiber mentioned this pull request Nov 20, 2018

update tests to be slightly more resilient #827

Merged

Merge branch 'master' into federated-auth

572e97b

arthurschreiber merged commit 83bf922 into tediousjs:master Nov 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ActiveDirectoryPassword and sqlpassword authentication support #707

feat: ActiveDirectoryPassword and sqlpassword authentication support #707

Hadis-Knj commented Feb 8, 2018

Hadis-Knj commented Feb 8, 2018

arthurschreiber left a comment

arthurschreiber May 23, 2018

arthurschreiber May 24, 2018

arthurschreiber May 24, 2018

arthurschreiber May 24, 2018

arthurschreiber commented May 24, 2018

Hadis-Knj commented May 24, 2018 •

edited

Loading

Suraiya-Hameed commented May 25, 2018 •

edited

Loading

Suraiya-Hameed commented May 25, 2018

arthurschreiber commented May 25, 2018

Suraiya-Hameed commented May 25, 2018

arthurschreiber commented Nov 5, 2018

Hadis-Knj Nov 5, 2018

arthurschreiber Nov 6, 2018

Hadis-Knj Nov 5, 2018

arthurschreiber Nov 6, 2018

Hadis-Knj commented Nov 6, 2018 •

edited

Loading

arthurschreiber commented Nov 6, 2018 •

edited

Loading

arthurschreiber commented Nov 6, 2018

Hadis-Knj commented Nov 6, 2018

Hadis-Knj commented Nov 8, 2018

arthurschreiber commented Nov 18, 2018

arthurschreiber commented Nov 21, 2018

Hadis-Knj commented Nov 22, 2018

arthurschreiber commented Nov 24, 2018

arthurschreiber commented Nov 24, 2018

Hadis-Knj commented Nov 27, 2018

		@@ -0,0 +1,61 @@
		const async = require('async');

		function readFedAuthInfoOpt(data, infoData, FEDAUTHINFOID, callback) {

		@@ -0,0 +1,27 @@

		module.exports = function featureExtAckParser(parser, colMetadata, options, callback) {

feat: ActiveDirectoryPassword and sqlpassword authentication support #707

feat: ActiveDirectoryPassword and sqlpassword authentication support #707

Conversation

Hadis-Knj commented Feb 8, 2018

Hadis-Knj commented Feb 8, 2018

arthurschreiber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurschreiber commented May 24, 2018

Hadis-Knj commented May 24, 2018 • edited Loading

Suraiya-Hameed commented May 25, 2018 • edited Loading

Suraiya-Hameed commented May 25, 2018

arthurschreiber commented May 25, 2018

Suraiya-Hameed commented May 25, 2018

arthurschreiber commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hadis-Knj commented Nov 6, 2018 • edited Loading

arthurschreiber commented Nov 6, 2018 • edited Loading

arthurschreiber commented Nov 6, 2018

Hadis-Knj commented Nov 6, 2018

Hadis-Knj commented Nov 8, 2018

arthurschreiber commented Nov 18, 2018

arthurschreiber commented Nov 21, 2018

Hadis-Knj commented Nov 22, 2018

arthurschreiber commented Nov 24, 2018

arthurschreiber commented Nov 24, 2018

Hadis-Knj commented Nov 27, 2018

Hadis-Knj commented May 24, 2018 •

edited

Loading

Suraiya-Hameed commented May 25, 2018 •

edited

Loading

Hadis-Knj commented Nov 6, 2018 •

edited

Loading

arthurschreiber commented Nov 6, 2018 •

edited

Loading