Migrate to openid client

[mstrhakr]

This is merge ready. I updated this to 1.1.21 then I tested it with Authelia (custom setup and with old configs) as well as Azure using the preset. This doesn't interfere with any old configuration and adds a bunch of cool features. I have again published the latest version of this on docker hub (mstrhakr/MeshCentral:dev) and tested that as well.

@Ylianst if you have any questions about this, please let me know. Although I wrote some documentation that you can reference as well.

Fixes #4545 Fixes #5179 Fixes #4531 Fixes #5725

[si458]

Have you verified this works with 1.0.21? Also what about existing people who use oidc? I see you have removed alot of options and added new ones in the schema file which will effect the config.json?

[mstrhakr]

Have you verified this works with 1.0.21?

Yes, this was updated to 1.1.21 and then tested as described above.

Also what about existing people who use oidc?

This should be a drop in replacement for existing users as described in the documentation.

I see you have removed alot of options and added new ones in the schema file which will effect the config.json?

Technically I didn't remove any items, they were just re-arranged and modified. Existing configs will work and are handled correctly with no user intervention required.

[exander77]

Can this be merged? I want to test this with Azure and if necessary build upon this.

[mstrhakr]

Can this be merged? I want to test this with Azure and if necessary build upon this.

Feel free to test this from my repository directly (migrate_to_openid-client branch). There is also a docker available at mstrhakr/meshcentral:dev

[exander77]

@mstrhakr I am already building on top of that, I have Azure groups working by reading them from graph api.

Btw, there is some bad merge/rebase:

+        const strategy = domain.authstrategies[req.user.strategy];
         const groups = { 'enabled': typeof strategy.groups == 'object' }
         if ((req.user != null) && (req.user.sid != null) && (req.user.strategy != null)) {
-            const strategy = domain.authstrategies[req.user.strategy];

Strategy is used before it is defined. I had to move that line.

[exander77]

After this is oked and accepted, I will make pull with for Azure groups.

[mstrhakr]

@mstrhakr I am already building on top of that, I have Azure groups working by reading them from graph api.

I already had that as a feature of this build.

Btw, there is some bad merge/rebase:

Thanks! I somehow missed that.

Correct me if I'm wrong but I think this would make more sense.

-         const groups = { 'enabled': typeof strategy.groups == 'object' }
          if ((req.user != null) && (req.user.sid != null) && (req.user.strategy != null)) {
              const strategy = domain.authstrategies[req.user.strategy];
+             const groups = { 'enabled': typeof strategy.groups == 'object' }

[exander77]

@mstrhakr Sure, that works as well. My issue is just that strategy is used before it is defined.

[exander77]

Otherwise, everything worked for me. For groups in Azure, I basically just appropriately fill user.groups.

[jon-nfc]

+1

@mstrhakr thanks for your time in doing this work. Eagerly awaiting so as to take advantage of being able to specify the groups claim name #5725 .

[exander77]

I am testing it with my Azure changes, and so far everything seems ok.

[mstrhakr]

I am testing it with my Azure changes, and so far everything seems ok.

What changes are you making? I only ask because groups are already supported with this branch.

[exander77]

I am testing it with my Azure changes, and so far everything seems ok.

What changes are you making? I only ask because groups are already supported with this branch.

Not for Azure SSO. You support everything, but you don't actually read groups from Azure AD.

[exander77]

This is the Azure code:

        // Azure
        if ((typeof domain.authstrategies.azure == 'object') && (typeof domain.authstrategies.azure.clientid == 'string') && (typeof domain.authstrategies.azure.clientsecret == 'string')) {
            const AzureOAuth2Strategy = require('passport-azure-oauth2');
            let options = { clientID: domain.authstrategies.azure.clientid, clientSecret: domain.authstrategies.azure.clientsecret, tenant: domain.authstrategies.azure.tenantid };
            if (typeof domain.authstrategies.azure.callbackurl == 'string') { options.callbackURL = domain.authstrategies.azure.callbackurl; } else { options.callbackURL = url + 'auth-azure-callback'; }
            parent.authLog('setupDomainAuthStrategy', 'Adding Azure SSO with options: ' + JSON.stringify(options));
            passport.use('azure-' + domain.id, new AzureOAuth2Strategy(options,
                function (accessToken, refreshtoken, params, profile, done) {
                    var userex = null;
                    try { userex = require('jwt-simple').decode(params.id_token, '', true); } catch (ex) { }
                    parent.authLog('setupDomainAuthStrategy', 'Azure profile: ' + JSON.stringify(userex));
                    var user = null;
                    if (userex != null) {
                        var user = { sid: '~azure:' + userex.unique_name, name: userex.name, strategy: 'azure' };
                        if (typeof userex.email == 'string') { user.email = userex.email; }
                    }
                    return done(null, user);
                }
            ));
            authStrategyFlags |= domainAuthStrategyConsts.azure;
        }

As you can see, there is no groups handling - no groups are read from Azure AD, what is actually needed is: Add Graph resource:

-            let options = { clientID: domain.authstrategies.azure.clientid, clientSecret: domain.authstrategies.azure.clientsecret, tenant: domain.authstrategies.azure.tenantid };
+            let options = { clientID: domain.authstrategies.azure.clientid, clientSecret: domain.authstrategies.azure.clientsecret, tenant: domain.authstrategies.azure.tenantid, resource: 'https://graph.microsoft.com' };

Implement a function that accesses graph API using the acceesToken to get https://graph.microsoft.com/v1.0/me/memberOf:

-                function (accessToken, refreshtoken, params, profile, done) {
+                async function (accessToken, refreshtoken, params, profile, done) {
+                    async function fetchUserGroups(accessToken) {
+                        const url = 'https://graph.microsoft.com/v1.0/me/memberOf';
+                        try {
+                            const response = await require('axios').get(url, {
+                                headers: {
+                                    'Authorization': `Bearer ${accessToken}`,
+                                     'Content-Type': 'application/json'
+                                }
+                            });
+                            const groups = response.data.value.map((g) => g.displayName);
+                            return groups;
+                        } catch (error) {
+                            parent.authLog('setupDomainAuthStrategy', 'Error: ' + error) 
+                            return [];
+                       }
+                   }

Call the function:

-                    parent.authLog('setupDomainAuthStrategy', 'Azure profile: ' + JSON.stringify(userex));
+                    const groups = await fetchUserGroups(accessToken)
+                    parent.authLog('setupDomainAuthStrategy', 'Azure profile: ' + JSON.stringify(userex) + ' ' + JSON.stringify(groups));

Fill groups in user struct with the result:

-                        var user = { sid: '~azure:' + userex.unique_name, name: userex.name, strategy: 'azure' };
+                        var user = { sid: '~azure:' + userex.unique_name, name: userex.name, strategy: 'azure', groups: groups };

It has new axios dependency (something else can be used as well).

@mstrhakr I hope this makes sense.

[mstrhakr]

Yea that's probably easier than how I accomplished it. Check out my getGroups function. We may be able to reuse it for the Azure strategy.

However this version of the OIDC strategy includes an Azure preset that does already pull groups from the Graph API, although I manually made the request instead of using another module.

It would not work for existing azure users though as it uses a different identifier for the users and there is currently no code to migrate the users to the new identifier.

[exander77]

Yea that's probably easier than how I accomplished it. Check out my getGroups function. We may be able to reuse it for the Azure strategy.

However this version of the OIDC strategy includes an Azure preset that does already pull groups from the Graph API, although I manually made the request instead of using another module.

It would not work for existing azure users though as it uses a different identifier for the users and there is currently no code to migrate the users to the new identifier.

I see, but does this work, is it enabled? I am using your PR, but doesn't fetch groups for me. I can debug to see what is wrong.

[exander77]

Maybe I just don't know how to set it up, that code is never called if I use Azure Strategy:

      "authStrategies": {
        "azure": {
          "callbackurl": "...",
          "newAccounts": true,
          "clientid": "...",
          "clientsecret": "...",
          "tenantid": "...",
          "groups": {
            "required": [ "..." ],
            "siteadmin": [ "..." ],
            "sync": {
              "enable": true,
              "filter": [ "..." ]
            }
          }
        }
      }

[exander77]

            } else if ((typeof strategy.issuer.issuer == 'string') && (typeof strategy.custom.preset == 'string')) {
                let error = new Error(`OIDC: PRESET: ${strategy.custom.preset.toUpperCase()}: PRESET OVERRIDDEN: CONFIG ISSUER: ${strategy.issuer.issuer} PRESET ISSUER: ${presetIssuer}`);
                parent.authLog('setupDomainAuthStrategy', error.message);
                console.warn(error)
            }

This code is erroneous, produces presetIssuer not defined.

[mstrhakr]

Check out the documentation I made for the presets. I didn't modify the code for the Azure strategy, I added a preset configuration to the oidc strategy for azure that does what you want already. Its possible we could modify that getGroups function to work for the Azure strategy as well but currently it wouldn't be a drop in.

Basically you'd need to use the oidc strategy and enable the azure preset within it to use groups as is. I added an entire file of documentation for just the oidc strategy in this branch, but I'm happy to help however I can.

[exander77]

Check out the documentation I made for the presets. I didn't modify the code for the Azure strategy, I added a preset configuration to the oidc strategy for azure that does what you want already. Its possible we could modify that getGroups function to work for the Azure strategy as well but currently it wouldn't be a drop in.

Basically you'd need to use the oidc strategy and enable the azure preset within it to use groups as is. I added an entire file of documentation for just the oidc strategy in this branch, but I'm happy to help however I can.

Trying to get it to work.

This code:

            // Discover additional information if available, use endpoints from config if present
            let issuer
            try {
                issuer = await strategy.obj.openidClient.Issuer.discover(strategy.issuer.issuer);
            } catch(err) {
                let error = new Error('OIDC: Discovery failed.', { cause: err });
                parent.authLog('setupDomainAuthStrategy', `ERROR: ${JSON.stringify(error)} ISSUER_URI: ${strategy.issuer.issuer}`);
                throw error
            } finally {
                if (Object.keys(strategy.issuer).length > 1) {
                    parent.authLog('setupDomainAuthStrategy', `OIDC: Adding Issuer Metadata: ${JSON.stringify(strategy.issuer)}`);
                    issuer = new strategy.obj.openidClient.Issuer(Object.assign(issuer.metadata, strategy.issuer));
                }
                strategy.issuer = issuer.metadata
                strategy.obj.issuer = issuer
            }

Can't work, finally accesses issuer.metadata which fails if an exception actually happened in try block.

[exander77]

In sample_config_advanced.json. This is not consistend with docs:

        "oidc": {
          "issuer": {
            "issuer": "https://sso.server.com",
            "end_session_endpoint": "https://sso.server.com/logout"
          },
          "client": {
            "clientid": "00000000-0000-0000-0000-000000000000",
            "clientsecret": "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
          },
          "groups": {
            "required": [ "groupA", "groupB", "groupC" ],
            "siteadmin": [ "groupA" ],
            "sync": {
              "enable": true,
              "filter": [ "groupB", "groupC" ]
            }
          },
          "newAccounts": true
        }

[exander77]

And this from docs:

"oidc": {
    "client": {
        "client_id": "a1gkl04i-40g8-2h74-6v41-2jm2o2x0x27r",
        "client_secret": "AxT6U5K4QtcyS6gF48gndL7Ys22BL15BWJImuq1O"
    },
    "custom": {
        "preset": "azure",
        "tenant_id": "46a6022g-4h33-1451-h1rc-08102ga3b5e4"
    }
}

Will not work as it fails on missing issuer.metadata and fails on the error mentioned above:

meshcentral  | ERR: /opt/meshcentral/meshcentral/webserver.js:7374
meshcentral  |                 strategy.issuer = issuer.metadata
meshcentral  |                                          ^
meshcentral  | 
meshcentral  | TypeError: Cannot read properties of undefined (reading 'metadata')

[exander77]

So, no it will actually work like this:

        "oidc": {
          "client": {
            "client_id": "...",
            "client_secret": "..."
          },
          "groups": {
            "required": [ "...", "..." ],
            "siteadmin": [ "..." ],
            "sync": {
              "enable": true,
              "filter": [ "...", "..." ]
            }
          },
          "custom": {
            "preset": "azure",
            "tenant_id": "..."
          },
          "newAccounts": true,
          "callbackURL": "..."
        },

[exander77]

I added notes in the PR code, see that. Testing it. Looks good.

So the actual idea is to phase auth old auth strategies and use oidc.

[exander77]

Can you rebase it on top of current master? And can We merge it? It is becoming impractical for testing with features that are in master.

[mstrhakr]

@exander77 I can try but I'm actually not a real programmer and have no idea what I'm doing. As far as merging I think we're waiting for a review from @Ylianst

[Ylianst]

Ok. I am looking into this today. Lots of changes, but the PR quality looks very good. This may break existing installations, however, it's a good update. I need to read this line-by-line and will approve soon if no issues.

[exander77]

Ok. I am looking into this today. Lots of changes, but the PR quality looks very good. This may break existing installations, however, it's a good update. I need to read this line-by-line and will approve soon if no issues.

Thank you very much, I will be testing it after it is merged and if anything out of ordinary is found, I will sure report it.

[mstrhakr]

So the actual idea is to phase auth old auth strategies and use oidc.

Yes basically, I'd like to do the same with saml via node-saml eventually. That should cover 99% of use cases between the two of them.

[mstrhakr]

Ok. I am looking into this today. Lots of changes, but the PR quality looks very good. This may break existing installations, however, it's a good update. I need to read this line-by-line and will approve soon if no issues.

This has basically been rewritten since you saw it the first time. Almost all of the code for the other strategies is essentially untouched to hopefully ensure no issues with the other strategies. I had to make web server startup asynchronously to get the discovery to work correctly but I think it is worth it for be able to get groups from the API's and such. I've said it before but I'm not a real programmer, I'm just learning and working on things that I find interesting so hopefully there aren't too many major oversights in this. Anyway, I really appreciate you taking another look at this after all this time.