Add OpenID authentication support using a KeyCloak server #720

fmartingr · 2024-03-27T18:26:42Z

Summary

Allow users to authenticate with Mattermost by using an OpenID provider
Adds provisioning for Keycloak server in in development mode with a postgresql database bundled in the same server
A default realm bundled into the load test with some default credentials.
- An optional import file can be provided, if none is set, the default one is used.
- Also a database dump can be provided in the form of an URI and the data will be imported into the SQL database.
An optional parameter to generate users in the Keycloak instance
Automatic configuration of the Mattermost instance by using the generated Keycloak server with the provided KeycloakClientID, KeycloakClientSecret and KeycloakRealmName (only needed when a custom import realm file is provided if the default values do not match).
Documentation

deployment/terraform/create.go

agarciamontoro

This is brilliant, @fmartingr, thank you so much! I left some comments below, mostly nits but some other that do need discussing. I also have some higher-level questions:

Do we need to expose the settings for Keycloak's DB? I'd lean towards being more opinionated whenever possible so that we expose less settings. Given that we don't want to monitor/load-test this server, can we use a one-size-fits-all instance type? Maybe not only for the DB but also for the Keycloak server? Or do the specs differ a lot when using more users? In any case, I think we can at least hardcode the usage of mysql or postgres here.
Why do the docs say that production mode is not implemented? Is there anything missing? Also: do we really need the distinction? I guess development mode is just cheaper to deploy because of the lack of DB, right?
Could you add some docs on the logic around the prefix for the usernames? The overwriting of the users file path was a bit confusing for me at first.

docs/external_auth_providers.md

deployment/terraform/assets/cluster.tf

deployment/terraform/assets/variables.tf

deployment/config.go

deployment/terraform/keycloak.go

agarciamontoro · 2024-04-05T15:10:24Z

deployment/terraform/keycloak.go

+	// Check if users file exists. Prevents from creating users multiple times.
+	if _, err := os.Stat(usersTxtPath); err == nil || os.IsExist(err) {
+		return nil
+	}


Not sure if this is a dumb question, but: could it happen that the users file exists but that it doesn't contain the users we want? For example, we create a deployment with 10 users and then recreate it with 50.

Yeah, I see that hapenning if that's an usual way of using the load test. Alternatively you could remove the file and the keycloak database to start from scratch.

I wouldn't discard that edge case, to be honest, so we should either:

Check the contents of the keycloak-users.txt file, not only that it exists

Clarify it in the docs
Your call, I'm good with both :)

+1 to this. We should do a quick check to ensure that # of users in keycloak file is equal to # users in users.txt file. And if possible, have a way to add new users, or fail clearly in the logs. But importantly, there should never be a case of some users partially logging in via keycloak and some via email/pass.

Changed it to create the number of users setup in the configuration. If the number is higher it will create more users accordingly, using the number of lines in the users.txt file as the source. It will skip user creation if the number of lines is the same or higher than the expected number of users.

deployment/terraform/keycloak.go

agarciamontoro · 2024-04-05T15:12:36Z

deployment/terraform/keycloak.go

+		username := fmt.Sprintf("keycloak-auto-%06d", i) // this is the password too
+		email := username + "@test.mattermost.com"
+
+		_, err := sshc.RunCommand(fmt.Sprintf("%s/kcadm.sh create users -r %s -s username=%s -s enabled=true -s email=%s", keycloakBinPath, t.config.ExternalAuthProviderSettings.KeycloakRealmName, username, email))


Oh wow there is no way to create more than one user at a time? 😭

We could call the API I guess, but there's no guarantee it would be faster.

deployment/config.go

agnivade · 2024-04-10T03:11:22Z

Is this ready to review now?

fmartingr · 2024-04-10T05:46:25Z

Is this ready to review now?

I have to address Alejandro's comments and make some changes from the work in the LT environment. I will assign you again when that's done.

Co-authored-by: Alejandro García Montoro <[email protected]>

deployment/config.go

deployment/terraform/agent.go

agnivade

New commits look good to me. The only small thing is that let's change the default keycloak instance spec to c7i series. Thanks again for all the hard work!

streamer45

Very quickly skimmed through this. Looks great overall. Left some comments.

streamer45 · 2024-04-30T17:02:33Z

api/agent.go

+			// Check if the user has a custom authentication type. Custom authentication types are
+			// specified by prepending the username with the authentication type followed by a colon.
+			// Example: "openid:[email protected] user1password"


Should we add this information to docs/external_auth_providers.md?

streamer45 · 2024-04-30T17:03:02Z

api/agent.go

+			if usernameParts := strings.Split(username, ":"); len(usernameParts) > 1 {
+				authenticationType = usernameParts[0]
+				username = usernameParts[1]
+
+				// Fix the email as well
+				if emailParts := strings.Split(email, ":"); len(emailParts) > 1 {
+					email = emailParts[1]
+				}
+			}


I think this could become a util function.

I'm going to make a note of this but refrain from modifying too much logic until this is merged now that we have this working. I can address the rest of the comments in individual PRs later.

streamer45 · 2024-04-30T17:07:26Z

loadtest/user/userentity/user.go

@@ -33,12 +33,19 @@ type UserEntity struct {
 	wsServerSeq int64
 }

+const (
+	AuthenticationTypeMattermost = "mattermost"


streamer45 · 2024-04-30T17:11:15Z

loadtest/user/userentity/actions.go

+			me, _, err := ue.client.GetMe(context.Background(), "")
+			if err != nil {
+				return fmt.Errorf("error while getting user: %w", err)
+			}
+
+			if err := ue.store.SetUser(me); err != nil {
+				return fmt.Errorf("error while setting user: %w", err)
+			}


Do we need to do this here? I'd expect this method to purely authenticate (e.g. set the token) and then deal with everything else in SignUp/Login.

There's a mix of logic here, is either I get the user and set it here, or I get the user by username later and set it in the other method. I've set a TODO to change the logic in a separate PR since I don't want to compromise or delay the test we're going to run today.

streamer45 · 2024-04-30T17:16:41Z

deployment/terraform/dump.go

+	appClients := make([]*ssh.Client, len(output.Instances))
+	for i, instance := range output.Instances {
+		client, err := extAgent.NewClient(instance.PublicIP)
+		if err != nil {
+			return fmt.Errorf("error in getting ssh connection %w", err)
+		}
+		defer client.Close()
+		appClients[i] = client
+	}


Why are we initializing N clients if we just use the first?

Good catch.

streamer45 · 2024-04-30T17:19:14Z

deployment/terraform/keycloak.go

+	sshc, err := extAgent.NewClient(t.output.KeycloakServer.PublicIP)
+	if err != nil {
+		return fmt.Errorf("error in getting ssh connection %w", err)
+	}


Should we close this client on returning?

streamer45 · 2024-04-30T17:24:23Z

deployment/terraform/keycloak.go

+	// Values for the keycloak.env file
+	keycloakEnvFileContents := []string{
+		// Enable health endpoints
+		"KC_HEALTH_ENABLED=true",


Maybe we could use a template file as we do with other plaintext assets.

Yeah, that's probably better with the current logic. At the beginning this was changuing depending on the deployer configuration that's why I followed this approach as it was simpler, but now that variables are fixed it makes more sense to have this on a static asset. Making a note for that.

streamer45 · 2024-04-30T17:29:34Z

deployment/terraform/keycloak.go

+
+	// Check if users file exists and has the expected number of users. If the file has less users than expected,
+	// we will start creating users from the next number, otherwise we will skip user creation.
+	if _, err := os.Stat(usersTxtPath); err == nil || os.IsExist(err) {


Why would Stat return an error if the file exists?

I think this was supposed to be os.IsNotExist in a previous apporach or something else with a different logic and I just inherited it here, this doesn't make any sense at the moment.

agarciamontoro

Again, amazing work, @fmartingr. Nothing else to add. Thank you!

agnivade · 2024-05-02T09:25:22Z

Merge it at your convenience. 😄

fmartingr · 2024-05-02T10:48:07Z

Merge it at your convenience. 😄

@streamer45 I've put comments on heavy logic changes that you mentioned so I can address those on separate PRs so I can merge this for now since it's getting too big and not easy to review. Thank you for reviewing!

The only logic change was: 12b5487

fmartingr added 4 commits March 21, 2024 08:59

added userentity authentication type

ff6685e

more descriptive error messages

3b8b843

handle login using openid from users file

6f90dcc

keycloak provisioning (wip)

919149c

agnivade reviewed Mar 28, 2024

View reviewed changes

deployment/terraform/create.go Outdated Show resolved Hide resolved

fmartingr added 11 commits April 1, 2024 16:01

use updateAppConfig

a861efe

added sample keycloak realm

7c43c23

setup openid from config

b3e5fda

setup default realm, disable ssl

8217a3d

health argument directly in call

8cf9d2a

development by default

5446c20

docs wip

56c7df7

disable ssl in master realm and configured realm

343e055

autogenerate keycloak users

a190c7a

override UsersFilePath instead of log

066e964

more docs

9b4cee4

fmartingr requested review from agnivade and agarciamontoro April 4, 2024 11:01

fmartingr self-assigned this Apr 4, 2024

fmartingr changed the title ~~(WIP) Add OpenID authentication support using a KeyCloak server~~ Add OpenID authentication support using a KeyCloak server Apr 4, 2024

fmartingr marked this pull request as ready for review April 4, 2024 12:23

agarciamontoro requested changes Apr 5, 2024

View reviewed changes

fmartingr added 4 commits April 8, 2024 18:03

jvm settings for large imports

b5ac989

realm import improvements

0b8b574

disable ssl in default realm

babebb7

updated assets

c8a37f0

fmartingr removed the request for review from agnivade April 10, 2024 05:45

Update docs/external_auth_providers.md

5dbf56d

Co-authored-by: Alejandro García Montoro <[email protected]>

fmartingr force-pushed the feat/openid-users branch from 5012e4e to 25c3ba0 Compare April 24, 2024 18:04

fmartingr added 6 commits April 25, 2024 07:24

remove path in prometheus target

ad1fb66

typo in default annotation

e9a5945

add ON_ERROR_STOP to keycloak dump

c9aa2ba

use siteurl for agent configuration if set up

2384ca6

handle failures in keycloak sql scripts

9de8440

lint

7ca520b

agnivade reviewed Apr 26, 2024

View reviewed changes

deployment/config.go Outdated Show resolved Hide resolved

agnivade reviewed Apr 29, 2024

View reviewed changes

deployment/terraform/agent.go Outdated Show resolved Hide resolved

fmartingr added 2 commits April 29, 2024 11:50

fixed site url logic on agents

19c839a

refactored server url logic to be testable

cba76a8

agnivade added 2: Dev Review Requires review by a core committer and removed Do Not Merge/ Awaiting Load-Test labels Apr 30, 2024

agnivade approved these changes Apr 30, 2024

View reviewed changes

fmartingr added 2 commits April 30, 2024 07:34

switch keycloak instance type to c7i

e9115db

add externalauthprovidersetting to deployer sample

1c83664

fmartingr requested a review from agarciamontoro April 30, 2024 05:35

streamer45 reviewed Apr 30, 2024

View reviewed changes

agarciamontoro approved these changes May 1, 2024

View reviewed changes

fmartingr added 3 commits May 2, 2024 08:43

don't initialise unnecessary clients

12b5487

close ssh client

d812cea

TODOs

ffc628b

agnivade added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels May 2, 2024

fmartingr added 2 commits May 2, 2024 12:40

TODO

e7a406f

add reference to users file in docs

9fccc75

fmartingr merged commit 0c4ddae into master May 2, 2024
1 check passed

fmartingr deleted the feat/openid-users branch May 2, 2024 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenID authentication support using a KeyCloak server #720

Add OpenID authentication support using a KeyCloak server #720

fmartingr commented Mar 27, 2024 •

edited

Loading

agarciamontoro left a comment

agarciamontoro Apr 5, 2024

fmartingr Apr 10, 2024

agarciamontoro Apr 19, 2024

agnivade Apr 22, 2024

fmartingr Apr 23, 2024

agarciamontoro Apr 5, 2024

fmartingr Apr 10, 2024

agnivade commented Apr 10, 2024

fmartingr commented Apr 10, 2024

agnivade left a comment

streamer45 left a comment

streamer45 Apr 30, 2024

streamer45 Apr 30, 2024

fmartingr May 2, 2024

streamer45 Apr 30, 2024

streamer45 Apr 30, 2024

fmartingr May 2, 2024

streamer45 Apr 30, 2024

fmartingr May 2, 2024

streamer45 Apr 30, 2024

streamer45 Apr 30, 2024

fmartingr May 2, 2024

streamer45 Apr 30, 2024

fmartingr May 2, 2024

agarciamontoro left a comment •

edited

Loading

agnivade commented May 2, 2024

fmartingr commented May 2, 2024 •

edited

Loading

Add OpenID authentication support using a KeyCloak server #720

Add OpenID authentication support using a KeyCloak server #720

Conversation

fmartingr commented Mar 27, 2024 • edited Loading

Summary

agarciamontoro 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

agnivade commented Apr 10, 2024

fmartingr commented Apr 10, 2024

agnivade left a comment

Choose a reason for hiding this comment

streamer45 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

agarciamontoro left a comment • edited Loading

Choose a reason for hiding this comment

agnivade commented May 2, 2024

fmartingr commented May 2, 2024 • edited Loading

fmartingr commented Mar 27, 2024 •

edited

Loading

agarciamontoro left a comment •

edited

Loading

fmartingr commented May 2, 2024 •

edited

Loading