Add SSO support #1990

antepusic · 2024-04-30T08:05:19Z

Description

This module adds Memgraph-side support for SSO.

[master < Task] PR

Provide the full content or a guide for the final git message

Add SSO support

CI Testing Labels

Please select the appropriate CI test labels (CI -build=build-name -test=test-suite)

Documentation checklist

Add the documentation label tag
Add the bug / feature label tag
Add the milestone for which this feature is intended
- If not known, set for a later milestone
Write a release note, including added/changed clauses
- [Release note text]
Link the documentation PR here
- [Documentation PR link]
Tag someone from docs team in the comments

andrejtonev

Looks good.
Would just like to go over it with you. I think we could combine the two authentication implementations into one.

andrejtonev · 2024-05-16T15:43:40Z

src/auth/auth.cpp

@@ -300,6 +302,102 @@ std::optional<UserOrRole> Auth::Authenticate(const std::string &username, const
  return user;
 }

+std::optional<UserOrRole> Auth::SSOAuthenticate(const std::string &scheme,


This function seems very similar to the authentication via module.
Can we reuse the code somehow?

I’ve extracted the code shared between the two into methods of their own.

andrejtonev · 2024-05-16T15:56:47Z

src/glue/SessionHL.cpp

@@ -145,6 +146,10 @@ bool SessionHL::Authenticate(const std::string &username, const std::string &pas
  interpreter_.ResetUser();
  {
    auto locked_auth = auth_->Lock();
+    if (FLAGS_auth_sso_on && (username.empty() || password.empty())) {
+      return false;


Communicate why the authentication failed to the user.

Superseded (this code is now the same as before)

andrejtonev · 2024-05-16T16:01:00Z

src/glue/SessionHL.cpp

+      // No access control -> give empty user
+      user_or_role_ = AuthChecker::GenQueryUser(auth_, std::nullopt);
+      interpreter_.SetUser(AuthChecker::GenQueryUser(auth_, std::nullopt));


Aren't we disabling userless logins when using SSO?

andrejtonev · 2024-05-16T16:01:59Z

src/glue/SessionHL.cpp

@@ -167,6 +172,33 @@ bool SessionHL::Authenticate(const std::string &username, const std::string &pas
  return res;
 }

+bool SessionHL::SSOAuthenticate(const std::string &scheme, const std::string &identity_provider_response) {


I would like to combine the Authenticate and SSOAuthenticate into a single function.
Don't really see why they should be separate.

The two methods have now become significantly different

andrejtonev

Perhaps we need to sync again.
The last product meeting changes the logic quite a bit. Not sure we are all on the same page.

andrejtonev · 2024-05-20T10:43:49Z

src/auth/auth.cpp

-                            std::cerr << "The auth module path doesn't exist or isn't a file!" << std::endl;
-                            return false;
+                          for (auto &[_, path] : memgraph::ModuleMappingsToMap(value)) {
+                            auto status = std::filesystem::status(path);


Can we rename status to module_name or something similar?

andrejtonev · 2024-05-20T10:56:06Z

src/auth/auth.cpp

+
+// NOLINTNEXTLINE(misc-unused-parameters)
+DEFINE_VALIDATED_string(auth_module_mappings, "",
+                        "Associates auth schemas to external modules. A mapping is structured as follows: <scheme>: "


I am not sure that gflags supports arguments with spaces.
Something like --flag arg1 arg2 should generate the flag's value only to "arg1".

You’re right about gflags (and environment variables are like this in general), but this works when the environment variable is set as a string, e.g. --auth-module-mappings="test-admin: admin; test-reader: reader". I’ve corrected the description to clarify that.

andrejtonev · 2024-05-20T11:03:07Z

src/auth/auth.cpp

+std::unordered_map<std::string, auth::Module> PopulateModules(std::string &module_mappings) {
+  std::unordered_map<std::string, auth::Module> module_per_scheme;
+  if (!FLAGS_auth_module_executable.empty()) {
+    module_per_scheme.emplace("basic", FLAGS_auth_module_executable);


What is a basic module?

Basic authentication is the auth scheme that takes a username and a password.

The scheme basic requires a username principal::String and a password credentials::String

– https://neo4j.com/docs/bolt/1.0/bolt/message/#messages-logon

By analogy, modules marked with basic in the scheme → module mapping handle authentication that takes an username + password and sends them to an external module. This covers LDAP.

andrejtonev · 2024-05-20T11:17:09Z

src/auth/auth.hpp

+   *
+   * @return `true` if auth needs to run
+   */
+  bool UsingAuthModule() const { return !modules_.empty(); }


Is this correct?
We need to support multiple authentication schemes and a native authentication.
The user will write something like auth_enable: auth1, auth2, native.
We should disable the users and local authentication only if the native authentication is missing, not if we are using any module.

Just a bit more context for this.
Currently memgraph supports or a native authentication or one via a module.
That is why some functionalities are disabled when the module is active.
We now need to enable those feature, but only for users that have been authorized via the native method.

For example, we do not want to handle any user data locally when a user is authorized via a module. Meaning that we send over the username/password or certificate and get back a role. This role is then used for the authorization.
Currently when updating a role or user, we check to see if we are using a module and allow only role updates in that case.
We also disable crating/deleting a user when we are using a module.
All of these things should be re-enabled, even if we are using a module.

We support native auth, and allow the user to change it as they need.
The only this the module influences is the order the authentication is tried and if a connection can be authenticated using the native auth.

If something is not clear, we can jump on a call :D

Thanks for the detailed explanation! I’ve applied your suggestions so that users logged in via module can modify Memgraph-internal user data, e.g. run CREATE USER or SHOW USERS.

andrejtonev · 2024-05-20T11:25:29Z

src/auth/auth.cpp

  params["scheme"] = scheme;
  params["response"] = identity_provider_response;

-  auto ret = module_.Call(params, FLAGS_auth_module_timeout_ms);
+  auto ret = modules_.at(scheme).Call(params, FLAGS_auth_module_timeout_ms);


Can we generalize the module call and response check?
Basically have a function that takes in the params and scheme and executed everything else.

andrejtonev · 2024-05-20T11:29:27Z

src/auth/auth.cpp

+  if (module_mappings.empty()) {
+    return module_per_scheme;
+  }


Do you need this check?
Shouldn't the ModuleMappingsToMap return an empty set either way?

That’s true – I just wanted to avoid calling the function if there’s no need

andrejtonev · 2024-05-20T11:30:18Z

src/auth/auth.cpp

+    }
+    const auto scheme_name = std::string{utils::Trim(module_and_scheme[0])};
+    const auto module_path = std::string{utils::Trim(module_and_scheme[1])};
+    module_per_scheme.emplace(scheme_name, module_path);


We are supporting only a single module per scheme?

That’s correct (and appropriate because the schemas are granular, e.g. it’s "saml-entra-id" and not just "saml"/"entra-id").

@tonilastre does this cover all the use-cases you're interested in?
I know you were talking about having multiple LDAP modules. Could all those also be defined in this way?

Just to confirm the above re scheme, the scheme contains both the protocol and identity provider, so it is a single module per scheme.

Re: LDAP, not sure because I haven't worked with that + Memgraph.

andrejtonev · 2024-05-20T11:30:45Z

src/auth/auth.cpp

+}  // namespace memgraph
+
+// NOLINTNEXTLINE(misc-unused-parameters)
+DEFINE_VALIDATED_string(auth_module_mappings, "",


How are we enabling/disabling the native auth?

Memgraph receives user credentials from Bolt’s LOGON/HELLO messages, and uses native auth or SSO depending on the value of the scheme field.

There’s no ambiguity as to whether native auth or SSO is used:

SSO → Memgraph Lab sends messages with SSO-specific values, e.g. "saml-okta".

Native auth → the scheme field is always "basic" (see Bolt Protocol message specification).

andrejtonev · 2024-05-20T11:31:39Z

src/auth/auth.cpp

  MigrateVersions(storage_);
 }

 std::optional<UserOrRole> Auth::Authenticate(const std::string &username, const std::string &password) {
-  if (module_.IsUsed()) {
+  if (!modules_.contains("basic")) {


We should allow both LDAP and/or native auth.
Their order should be the order the user defined in the argument list.

The LDAP module is indexed with "basic" while processing the flags (see also #1990 (comment)), i.e. we support them both.

As for auth method ordering, we’ve avoided that (see meeting note: https://www.notion.so/memgraph/2024-04-30-SSO-Lab-Core-Sync-2-ccb4755e30be4cebb8beb7d7cc16f77d?pvs=4#8eab766c1ecd457d8a6c8cd18849b298)

!modules_.contains("basic") is ambiguous.
How are we suppose to disable or enable both?

antepusic · 2024-05-20T14:20:02Z

@andrejtonev Thank you for the review!

Perhaps we need to sync again. The last product meeting changes the logic quite a bit. Not sure we are all on the same page.

In light of that meeting, I’ve updated the spec (changes in green) and we can sync together.

…into sso-saml-module

andrejtonev

@gitbuda are you okay with us enabling all native auth functionalities even when modules are used?
What I mean specifically is that any modifications to the native auth data is disable when we run modules. This made sense when the modules would modify the data. We disabled that some time ago.
We now need to allow both native and module authentication. I think we can allow the user to change the native user data at any time (even when the native auth is disabled).

andrejtonev · 2024-05-23T15:25:18Z

src/auth/auth.cpp

+    }
+    const auto scheme_name = std::string{utils::Trim(module_and_scheme[0])};
+    const auto module_path = std::string{utils::Trim(module_and_scheme[1])};
+    module_per_scheme.emplace(scheme_name, module_path);


@tonilastre does this cover all the use-cases you're interested in?
I know you were talking about having multiple LDAP modules. Could all those also be defined in this way?

andrejtonev · 2024-05-23T15:36:19Z

src/auth/auth.cpp

  MigrateVersions(storage_);
 }

 std::optional<UserOrRole> Auth::Authenticate(const std::string &username, const std::string &password) {
-  if (module_.IsUsed()) {
+  if (!modules_.contains("basic")) {


!modules_.contains("basic") is ambiguous.
How are we suppose to disable or enable both?

tonilastre · 2024-05-23T19:29:40Z

@tonilastre does this cover all the use cases you're interested in?
I know you were talking about having multiple LDAP modules. Could all those also be defined in this way?

@andrejtonev, regarding this: I am sure about LDAP, but SSO + native is something that is used often. When you have a database running you want to establish an SSO provider connected with your directory of employees who can access the database (e.g. analysts, data engineers, etc), but there will be some internal applications where you can't set up an SSO for it, so applications will need a native auth (API key = username, API secret = password).

sonarcloud · 2024-05-24T16:05:01Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

antepusic added 3 commits April 30, 2024 10:01

Add Bolt message handling

3dafcc4

Add draft code for working with the auth module

c3dda39

Add draft SAML module

eaf10b4

antepusic added feature feature enterprise Docs needed Docs needed labels Apr 30, 2024

antepusic added this to the mg-v2.17.0 milestone Apr 30, 2024

antepusic self-assigned this Apr 30, 2024

Merge branch 'master' into sso-saml-module

f2cfa41

gitbuda mentioned this pull request Apr 30, 2024

Implement SSO authentication flow #1859

Open

antepusic and others added 11 commits May 9, 2024 15:48

Add flags

e558c37

Fix communication stack code

c011ab0

Add full auth code

72b072e

Rename authentication function in the glue code

810a4ee

Add reworked SAML SSO external module

29ccbbb

Remove extraneous comment

dfa7b1c

Merge branch 'master' into sso-saml-module

c04a9ba

Fix auth response validation

16c490c

Switch to environment variables

737455e

Fix CMakeLists

63f3f2e

Fix logic

5b47731

This was referenced May 14, 2024

Add SAML authentication module #1839

Closed

Implement Auth Module for Active directory #1292

Open

antepusic added 5 commits May 14, 2024 23:38

Fix build

31a9ed3

Finalize the auth stack

60d7e54

Add remaining fixes to the SAML auth module

3126cac

Add preliminary tests

e2c5bd5

Disable basic auth (no credentials) when SSO is turned on

167e5a9

antepusic requested a review from andrejtonev May 16, 2024 13:59

andrejtonev reviewed May 16, 2024

View reviewed changes

antepusic changed the title ~~Add SSO via SAML~~ Add SSO support May 19, 2024

antepusic added 2 commits May 19, 2024 13:17

Update neo4j client version

0e008c6

Fix neo4j client usage

100a779

antepusic marked this pull request as ready for review May 19, 2024 14:24

antepusic added CI -build=coverage -test=core Run coverage build and core tests on push Ready for review PR is ready for review labels May 19, 2024

antepusic added 4 commits May 19, 2024 18:10

Change LDAP params

8a2aac4

Uncomment tests

bdf7202

Satisfy clang-tidy

252bc69

Remove unused variable

b7d6cdf

antepusic requested a review from andrejtonev May 20, 2024 01:33

antepusic mentioned this pull request May 20, 2024

Add the external module for SAML SSO #2049

Open

8 tasks

Fix module loading <-> the LDAP integration test

8d826d5

andrejtonev reviewed May 20, 2024

View reviewed changes

Merge branch 'master' into sso-saml-module

41c3235

gitbuda modified the milestones: mg-v2.17.0, mg-v2.18.0 May 20, 2024

antepusic and others added 5 commits May 21, 2024 21:05

Add review suggestions

9acec7e

Merge branch 'master' into sso-saml-module

cb767fb

Merge branch 'sso-saml-module' of https://github.com/memgraph/memgraph …

02ce69c

…into sso-saml-module

Merge branch 'master' into sso-saml-module

775d06c

Add review suggestions

90946ef

andrejtonev reviewed May 23, 2024

View reviewed changes

antepusic and others added 4 commits May 24, 2024 11:47

Apply more review suggestions

8e82dd6

Make scheme checking smarter and tidy up auth flags

4ed511d

Comply with clang-tidy

1301846

Merge branch 'master' into sso-saml-module

845934b

Add SSO support #1990

Are you sure you want to change the base?

Add SSO support #1990

Conversation

antepusic commented Apr 30, 2024 • edited

Description

CI Testing Labels

Documentation checklist

andrejtonev 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

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

andrejtonev 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antepusic May 20, 2024 • edited

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

antepusic May 21, 2024 • edited

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

antepusic commented May 20, 2024 • edited

andrejtonev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonilastre commented May 23, 2024

sonarcloud bot commented May 24, 2024

Quality Gate passed

antepusic commented Apr 30, 2024 •

edited

antepusic May 20, 2024 •

edited

antepusic May 21, 2024 •

edited

antepusic commented May 20, 2024 •

edited