Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OpenID authentication support using a KeyCloak server #720

Merged
merged 70 commits into from
May 2, 2024
Merged
Changes from 45 commits
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
ff6685e
added userentity authentication type
fmartingr Mar 21, 2024
3b8b843
more descriptive error messages
fmartingr Mar 21, 2024
6f90dcc
handle login using openid from users file
fmartingr Mar 21, 2024
919149c
keycloak provisioning (wip)
fmartingr Mar 27, 2024
a861efe
use updateAppConfig
fmartingr Apr 1, 2024
7c43c23
added sample keycloak realm
fmartingr Apr 2, 2024
b3e5fda
setup openid from config
fmartingr Apr 2, 2024
8217a3d
setup default realm, disable ssl
fmartingr Apr 2, 2024
8cf9d2a
health argument directly in call
fmartingr Apr 2, 2024
5446c20
development by default
fmartingr Apr 2, 2024
56c7df7
docs wip
fmartingr Apr 2, 2024
343e055
disable ssl in master realm and configured realm
fmartingr Apr 3, 2024
a190c7a
autogenerate keycloak users
fmartingr Apr 3, 2024
066e964
override UsersFilePath instead of log
fmartingr Apr 4, 2024
9b4cee4
more docs
fmartingr Apr 4, 2024
b5ac989
jvm settings for large imports
fmartingr Apr 8, 2024
0b8b574
realm import improvements
fmartingr Apr 9, 2024
babebb7
disable ssl in default realm
fmartingr Apr 9, 2024
c8a37f0
updated assets
fmartingr Apr 9, 2024
5dbf56d
Update docs/external_auth_providers.md
fmartingr Apr 10, 2024
f5ac82f
db set to postgres
fmartingr Apr 10, 2024
6e19432
remove default for instance count in tf
fmartingr Apr 10, 2024
e041458
apply suggestion
fmartingr Apr 10, 2024
28c3beb
moved keycloakEnvFileContents closer to usage
fmartingr Apr 10, 2024
aadc041
return early if token is found, err otherwise
fmartingr Apr 10, 2024
003f979
removed now unused var
fmartingr Apr 10, 2024
4a454d2
keycloak db dump support
fmartingr Apr 10, 2024
e556ce6
make assets
fmartingr Apr 10, 2024
20dbc81
remove keycloak_db_instance_engine
fmartingr Apr 10, 2024
49c02dc
remove variable usage
fmartingr Apr 10, 2024
a611473
fixed family
fmartingr Apr 10, 2024
5bbd2a6
import keycloak db dump on keycloak config
fmartingr Apr 10, 2024
49605ea
fixed ingest commands
fmartingr Apr 10, 2024
dc4191d
ingest keycloak db on create only
fmartingr Apr 10, 2024
8182e44
updated docs
fmartingr Apr 10, 2024
eadca08
using bool instead of count to enable the keycloak deployment
fmartingr Apr 10, 2024
c7b08b3
install realm only when no dump is provided
fmartingr Apr 11, 2024
2b9526f
better logging
fmartingr Apr 15, 2024
c54a0a9
add hosts file to agent nodes as well
fmartingr Apr 15, 2024
b1de1d2
psql database setup
fmartingr Apr 15, 2024
bdd340e
disable specific changes using dump
fmartingr Apr 15, 2024
7a86020
get user when authenticating
fmartingr Apr 16, 2024
b3f7dae
simplified keycloak database setup
fmartingr Apr 17, 2024
67d29d4
added metrics
fmartingr Apr 17, 2024
4024790
Merge remote-tracking branch 'origin/master' into feat/openid-users
fmartingr Apr 18, 2024
49ece26
simplify logic and avoid panics
fmartingr Apr 22, 2024
95ccbe5
typo
fmartingr Apr 22, 2024
791d40c
fixed metrics path
fmartingr Apr 22, 2024
5011569
requested changes
fmartingr Apr 22, 2024
4c08af4
Merge remote-tracking branch 'origin/master' into feat/openid-users
fmartingr Apr 22, 2024
4b35f96
fixed database upload
fmartingr Apr 23, 2024
50842ad
ignore exit 5 when stopping keycloak while creating the database
fmartingr Apr 23, 2024
fec4c6e
main function should start keycloak
fmartingr Apr 23, 2024
9ed759f
Merge remote-tracking branch 'origin/master' into feat/openid-users
fmartingr Apr 24, 2024
25c3ba0
allow provisioning of extra SQL files for the mattermost db
fmartingr Apr 24, 2024
ad1fb66
remove path in prometheus target
fmartingr Apr 25, 2024
e9a5945
typo in default annotation
fmartingr Apr 25, 2024
c9aa2ba
add ON_ERROR_STOP to keycloak dump
fmartingr Apr 25, 2024
2384ca6
use siteurl for agent configuration if set up
fmartingr Apr 26, 2024
9de8440
handle failures in keycloak sql scripts
fmartingr Apr 26, 2024
7ca520b
lint
fmartingr Apr 26, 2024
19c839a
fixed site url logic on agents
fmartingr Apr 29, 2024
cba76a8
refactored server url logic to be testable
fmartingr Apr 29, 2024
e9115db
switch keycloak instance type to c7i
fmartingr Apr 30, 2024
1c83664
add externalauthprovidersetting to deployer sample
fmartingr Apr 30, 2024
12b5487
don't initialise unnecessary clients
fmartingr May 2, 2024
d812cea
close ssh client
fmartingr May 2, 2024
ffc628b
TODOs
fmartingr May 2, 2024
e7a406f
TODO
fmartingr May 2, 2024
9fccc75
add reference to users file in docs
fmartingr May 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 26 additions & 10 deletions api/agent.go
Original file line number Diff line number Diff line change
@@ -363,7 +363,7 @@ func NewControllerWrapper(config *loadtest.Config, controllerConfig interface{},

creds, err := getUserCredentials(config.UsersConfiguration.UsersFilePath, config)
if err != nil {
return nil, err
return nil, fmt.Errorf("error getting user credentials from file: %w", err)
}

modAdmins := 0
@@ -373,7 +373,7 @@ func NewControllerWrapper(config *loadtest.Config, controllerConfig interface{},

err = createCustomEmoji(config)
if err != nil {
return nil, err
return nil, fmt.Errorf("error creating custom emoji from config: %w", err)
}
mlog.Info("Custom emoji created")

@@ -383,6 +383,7 @@ func NewControllerWrapper(config *loadtest.Config, controllerConfig interface{},
username := fmt.Sprintf("%s-%d", namePrefix, id)
email := fmt.Sprintf("%s-%d@example.com", namePrefix, id)
password := "testPass123$"
authenticationType := userentity.AuthenticationTypeMattermost

if modAdmins > 0 && id%modAdmins == 0 {
username = ""
@@ -395,15 +396,30 @@ func NewControllerWrapper(config *loadtest.Config, controllerConfig interface{},
username = creds[id].username
email = creds[id].email
password = creds[id].password

// 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:user1@test.mattermost.com user1password"
Comment on lines +400 to +402
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if strings.Contains(username, ":") {
usernameParts := strings.Split(username, ":")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be combined to if parts := strings.Split(username, ":"); len(parts) > 1

authenticationType = usernameParts[0]
username = usernameParts[1]

// Fix the email as well
emailParts := strings.Split(email, ":")
email = emailParts[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? I thought just the username would become like "openid:user1"? The email should be untouched right? Also, we need to do length validation of the slice here to prevent panics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the users.txt file works with email password lines, what this does is parse if theres a : in the email to check for the auth service, hence parsing: authservice:email password. But in order to properly set the email I need to discard the authservice: part.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the authservice is just a prefix for the username right? Or is it for both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in the users txt file only, but ignored for login.

Unsure how the : would affect login in, and I selected that symbol so we easily know in code if there's a custom authservice setup. This could be changed to use openid-username-00001 and check if the first part split by - matches an authservice but it seemed more complicated in my head. Would love your fresh eyes input on this.

}
}

ueConfig := userentity.Config{
ServerURL: config.ConnectionConfiguration.ServerURL,
WebSocketURL: config.ConnectionConfiguration.WebSocketURL,
Username: username,
Email: email,
Password: password,
ServerURL: config.ConnectionConfiguration.ServerURL,
WebSocketURL: config.ConnectionConfiguration.WebSocketURL,
AuthenticationType: authenticationType,
Username: username,
Email: email,
Password: password,
}

store, err := memstore.New(&memstore.Config{
MaxStoredPosts: 250,
MaxStoredUsers: 500,
@@ -413,11 +429,11 @@ func NewControllerWrapper(config *loadtest.Config, controllerConfig interface{},
MaxStoredReactions: 10,
})
if err != nil {
return nil, err
return nil, fmt.Errorf("error creating memory store: %w", err)
}

if err := store.SetServerVersion(serverVersion); err != nil {
return nil, err
return nil, fmt.Errorf("error setting server version: %w", err)
}

ueSetup := userentity.Setup{
@@ -527,7 +543,7 @@ func createCustomEmoji(config *loadtest.Config) error {
}
sysadmin := createSysAdmin(adminStore, config)
if err := sysadmin.Login(); err != nil {
return err
return fmt.Errorf("error login as sysadmin: %w", err)
}

emoji := &model.Emoji{
42 changes: 42 additions & 0 deletions deployment/config.go
Original file line number Diff line number Diff line change
@@ -51,6 +51,8 @@ type Config struct {
ExternalDBSettings ExternalDBSettings
// External bucket connection settings.
ExternalBucketSettings ExternalBucketSettings
// ExternalAuthProviderSettings contains the settings for configuring an external auth provider.
ExternalAuthProviderSettings ExternalAuthProviderSettings
// URL from where to download Mattermost release.
// This can also point to a local binary path if the user wants to run loadtest
// on a custom build. The path should be prefixed with "file://". In that case,
@@ -114,6 +116,8 @@ type StorageSizes struct {
Job int `default:"50"`
// Size, in GiB, for the storage of the elasticsearch instances
ElasticSearch int `default:"20"`
// Size, in GiB, for the storage of the keycloak instances
KeyCloak int `default:"10"`
}

// PyroscopeSettings contains flags to enable/disable the profiling
@@ -179,6 +183,44 @@ type ExternalBucketSettings struct {
AmazonS3SSE bool `default:"false"`
}

// ExternalAuthProviderSettings contains the necessary data
// to configure an external auth provider.
type ExternalAuthProviderSettings struct {
// Enabled is set to true if the external auth provider should be enabled.
Enabled bool `default:"false"`
// DevelopmentMode is set to true if the keycloak instance should be started in development mode.
DevelopmentMode bool `default:"true"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it not be "false" by default? The default config is always aimed towards production usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left it as false because the difference development vs production means. If I use the production option I have to disable some of the features it enables manually (like enforced hostnames, SSL, local cache). I could go that route but in the end we would have the same result but with more work. That is, unless we want to really run it in production mode with all security features enforced as they should be.

// KeycloakVersion is the version of keycloak to deploy.
KeycloakVersion string `default:"24.0.2"`
// KeycloakInstanceType is the type of the EC2 instance for keycloak.
InstanceType string `default:"c5.xlarge"`
// KeycloakAdminUser is the username of the keycloak admin interface (admin on the master realm)
KeycloakAdminUser string `default:"mmuser" validate:"notempty"`
// KeycloakAdminPassword is the password of the keycloak admin interface (admin on the master realm)
KeycloakAdminPassword string `default:"mmpass" validate:"notempty"`
// KeycloakRealmFilePath is the path to the realm file to be uploaded to the keycloak instance.
// If empty, a default realm file will be used.
KeycloakRealmFilePath string `default:""`
// KeycloakDBDumpURI
// An optional URI to a keycloak database dump file to be uploaded on environment
// creation.
// The file is expected to be the h2 folder gzip compressed.
// This can also point to a local file if prefixed with "file://".
// In such case, the dump file will be uploaded to the app servers.
KeycloakDBDumpURI string `default:""`
// GenerateUsersCount is the number of users to generate in the keycloak instance.
GenerateUsersCount int `default:"0" validate:"range:[0,)"`
// KeycloakRealmName is the name of the realm to be used in Mattermost. Must exist in the keycloak instance.
// Is used when creating users and to properly set the OpenID configuration in Mattermost.
KeycloakRealmName string `default:"mattermost"`
// KeycloakClientID is the client id to be used in Mattermost from the above realm.
// Must exist in the keycloak instance
KeycloakClientID string `default:"mattermost-openid"`
// KeycloakClientSecret is the client secret from the above realm to be used in Mattermost.
// Must exist in the keycloak instance
KeycloakClientSecret string `default:"qbdUj4dacwfa5sIARIiXZxbsBFoopTyf"`
}

// ElasticSearchSettings contains the necessary data
// to configure an ElasticSearch instance to be deployed
// and provisioned.
21 changes: 21 additions & 0 deletions deployment/terraform/agent.go
Original file line number Diff line number Diff line change
@@ -129,6 +129,27 @@ func (t *Terraform) configureAndRunAgents(extAgent *ssh.ExtAgent) error {
batch = append(batch, uploadInfo{srcData: strings.Join(splitFiles[i], "\n"), dstPath: dstUsersFilePath, msg: "Uploading list of users credentials"})
}

// If SiteURL is set, update /etc/hosts to point to the correct IP
if t.config.SiteURL != "" {
output, err := t.Output()
if err != nil {
return err
}

// The new entry in /etc/hosts will make SiteURL point to:
// - The first instance's IP if there's a single node
// - The proxy's IP if there's more than one node
ip := output.Instances[0].PrivateIP
if output.HasProxy() {
ip = output.Proxy.PrivateIP
}

proxyHost := fmt.Sprintf("%s %s\n", ip, t.config.SiteURL)
appHostsFile := fmt.Sprintf(appHosts, proxyHost)

batch = append(batch, uploadInfo{srcData: appHostsFile, dstPath: "/etc/hosts", msg: "Updating /etc/hosts to point to the correct IP"})
}
Comment on lines +130 to +149
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remind me why we need the agents to update their hosts file with the SiteURL?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've had a discussion here: https://community.mattermost.com/core/pl/1944im15z3f1pn9thb647kdi5y. I am 0/5 as long as it doesn't break permalinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an straightforward way for me to check that permalinks are working?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the agents host will not break permalinks: the problem was with the app nodes trying to retrieve the data from the permalink. As long as we don't touch that, permalinks will keep working.


if err := uploadBatch(sshc, batch); err != nil {
return fmt.Errorf("batch upload failed: %w", err)
}
41 changes: 32 additions & 9 deletions deployment/terraform/assets/bindata.go

Large diffs are not rendered by default.

77 changes: 77 additions & 0 deletions deployment/terraform/assets/cluster.tf
Original file line number Diff line number Diff line change
@@ -701,3 +701,80 @@ resource "null_resource" "s3_dump" {
command = "aws --profile ${var.aws_profile} s3 cp ${var.s3_bucket_dump_uri} s3://${aws_s3_bucket.s3bucket[0].id} --recursive"
}
}

// Keycloak
resource "aws_instance" "keycloak" {
tags = {
Name = "${var.cluster_name}-keycloak"
}

connection {
# The default username for our AMI
type = "ssh"
user = "ubuntu"
host = self.public_ip
}

ami = var.aws_ami
instance_type = var.keycloak_instance_type
count = var.keycloak_enabled ? 1 : 0
key_name = aws_key_pair.key.id

vpc_security_group_ids = [
aws_security_group.keycloak[0].id,
]

root_block_device {
volume_size = var.block_device_sizes_keycloak
volume_type = var.block_device_type
}

provisioner "remote-exec" {
inline = [
"set -o errexit",
"while [ ! -f /var/lib/cloud/instance/boot-finished ]; do echo 'Waiting for cloud-init...'; sleep 1; done",
"sudo apt-get -y update",
"sudo apt-get install unzip openjdk-17-jre postgresql postgresql-contrib -y",
"sudo mkdir -p /opt/keycloak",
"sudo curl -O -L --output-dir /opt/keycloak https://github.com/keycloak/keycloak/releases/download/${var.keycloak_version}/keycloak-${var.keycloak_version}.zip",
"sudo unzip /opt/keycloak/keycloak-${var.keycloak_version}.zip -d /opt/keycloak",
"sudo mkdir -p /opt/keycloak/keycloak-${var.keycloak_version}/data/import",
"sudo chown -R ubuntu:ubuntu /opt/keycloak",
]
}
}

resource "aws_security_group" "keycloak" {
count = var.keycloak_enabled ? 1 : 0
name = "${var.cluster_name}-keycloak-security-group"
description = "KeyCloak security group for loadtest cluster ${var.cluster_name}"

egress {
from_port = 0
to_port = 0
protocol = "-1"
cidr_blocks = ["0.0.0.0/0"]
}

ingress {
from_port = 22
to_port = 22
protocol = "tcp"
cidr_blocks = local.private_ip != "" ? ["${local.public_ip}/32", "${local.private_ip}/32"] : ["${local.public_ip}/32"]
}

// To access keycloak
ingress {
from_port = 8443
to_port = 8443
protocol = "tcp"
cidr_blocks = ["0.0.0.0/0"]
}

ingress {
from_port = 8080
to_port = 8080
protocol = "tcp"
cidr_blocks = ["0.0.0.0/0"]
}
}
Loading