-
Notifications
You must be signed in to change notification settings - Fork 2
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 identity provider modules #650
Changes from 27 commits
9016261
9bbc5de
321e873
33550ee
1441550
dbd45a9
78cb519
b2137e5
1d72080
9c6b3a2
61fc8b6
546ca8e
c36bbf1
54b8fba
c8bf7d2
96c771b
dcf394b
c24f582
7560036
bc414e5
d1ab6af
fb4e88e
f551243
34728aa
0957f2c
08008e4
906b3c3
9928371
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,42 @@ | ||||||||
# Identity provider configuration. | ||||||||
# If the notification service is configured, the identity provider will use the | ||||||||
# SES-verified email to send notifications. | ||||||||
locals { | ||||||||
# If your application should redirect users, after successful authentication, to a | ||||||||
# page other than the homepage, specify the path fragment here. | ||||||||
# Example: "profile" | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
callback_url_path = "" | ||||||||
|
||||||||
# If your application should redirect users, after signing out, to a page other than | ||||||||
# the homepage, specify the path fragment here. | ||||||||
# Example: "logout" | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
logout_url_path = "" | ||||||||
|
||||||||
identity_provider_config = var.enable_identity_provider ? { | ||||||||
identity_provider_name = "${local.prefix}${var.app_name}-${var.environment}" | ||||||||
|
||||||||
password_policy = { | ||||||||
password_minimum_length = 12 | ||||||||
temporary_password_validity_days = 7 | ||||||||
} | ||||||||
|
||||||||
# Optionally configure email template for resetting a password. | ||||||||
# Set any attribute to a non-null value to override AWS Cognito defaults. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
verification_email = { | ||||||||
verification_email_message = null | ||||||||
verification_email_subject = null | ||||||||
} | ||||||||
|
||||||||
# Do not modify this block directly. | ||||||||
client = { | ||||||||
callback_urls = concat( | ||||||||
var.domain_name != null ? ["https://${var.domain_name}/${local.callback_url_path}"] : [], | ||||||||
var.extra_identity_provider_callback_urls | ||||||||
) | ||||||||
logout_urls = concat( | ||||||||
var.domain_name != null ? ["https://${var.domain_name}/${local.logout_url_path}"] : [], | ||||||||
var.extra_identity_provider_logout_urls | ||||||||
) | ||||||||
} | ||||||||
} : null | ||||||||
} |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,15 @@ | ||||||||
# Notifications configuration | ||||||||
locals { | ||||||||
notifications_config = var.enable_notifications ? { | ||||||||
# Set to an SES-verified email address to be used when sending emails. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
sender_email = null | ||||||||
|
||||||||
# Configure the name that users see in the "From" section of their inbox, so that it's | ||||||||
# clearer who the email is from. | ||||||||
sender_display_name = null | ||||||||
|
||||||||
# Configure the REPLY-TO email address if it should be different from the sender. | ||||||||
# Note: Only used by the identity-provider service. | ||||||||
reply_to_email = null | ||||||||
} : null | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,8 @@ locals { | |
database_config = local.environment_config.database_config | ||
storage_config = local.environment_config.storage_config | ||
incident_management_service_integration_config = local.environment_config.incident_management_service_integration | ||
identity_provider_config = local.environment_config.identity_provider_config | ||
notifications_config = local.environment_config.notifications_config | ||
|
||
network_config = module.project_config.network_configs[local.environment_config.network_name] | ||
} | ||
|
@@ -150,22 +152,38 @@ module "service" { | |
} | ||
} : null | ||
|
||
extra_environment_variables = merge({ | ||
FEATURE_FLAGS_PROJECT = module.feature_flags.evidently_project_name | ||
BUCKET_NAME = local.storage_config.bucket_name | ||
}, local.service_config.extra_environment_variables) | ||
|
||
secrets = [ | ||
for secret_name in keys(local.service_config.secrets) : { | ||
extra_environment_variables = merge( | ||
{ | ||
FEATURE_FLAGS_PROJECT = module.feature_flags.evidently_project_name | ||
BUCKET_NAME = local.storage_config.bucket_name | ||
}, | ||
module.app_config.enable_identity_provider ? { | ||
COGNITO_USER_POOL_ID = module.identity_provider[0].user_pool_id | ||
COGNITO_CLIENT_ID = module.identity_provider_client[0].client_id | ||
lorenyu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} : {}, | ||
local.service_config.extra_environment_variables | ||
) | ||
|
||
secrets = concat( | ||
[for secret_name in keys(local.service_config.secrets) : { | ||
name = secret_name | ||
valueFrom = module.secrets[secret_name].secret_arn | ||
} | ||
] | ||
|
||
extra_policies = { | ||
feature_flags_access = module.feature_flags.access_policy_arn, | ||
storage_access = module.storage.access_policy_arn | ||
} | ||
}], | ||
module.app_config.enable_identity_provider ? [{ | ||
name = "COGNITO_CLIENT_SECRET" | ||
valueFrom = module.identity_provider_client[0].client_secret_arn | ||
}] : [] | ||
) | ||
|
||
extra_policies = merge( | ||
{ | ||
feature_flags_access = module.feature_flags.access_policy_arn, | ||
storage_access = module.storage.access_policy_arn | ||
}, | ||
module.app_config.enable_identity_provider ? { | ||
identity_provider_access = module.identity_provider_client[0].access_policy_arn, | ||
} : {} | ||
) | ||
|
||
is_temporary = local.is_temporary | ||
} | ||
|
@@ -192,3 +210,29 @@ module "storage" { | |
name = local.storage_config.bucket_name | ||
is_temporary = local.is_temporary | ||
} | ||
|
||
module "identity_provider" { | ||
count = module.app_config.enable_identity_provider ? 1 : 0 | ||
source = "../../modules/identity-provider" | ||
is_temporary = local.is_temporary | ||
|
||
name = local.identity_provider_config.identity_provider_name | ||
password_minimum_length = local.identity_provider_config.password_policy.password_minimum_length | ||
temporary_password_validity_days = local.identity_provider_config.password_policy.temporary_password_validity_days | ||
verification_email_message = local.identity_provider_config.verification_email.verification_email_message | ||
verification_email_subject = local.identity_provider_config.verification_email.verification_email_subject | ||
|
||
sender_email = local.notifications_config == null ? null : local.notifications_config.sender_email | ||
sender_display_name = local.notifications_config == null ? null : local.notifications_config.sender_display_name | ||
reply_to_email = local.notifications_config == null ? null : local.notifications_config.reply_to_email | ||
} | ||
|
||
module "identity_provider_client" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (non-blocking question) what's the reasoning behind splitting this up into two terraform modules? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! This work is adapted from a another project where they were already two modules. Not a great reason on its own, but it was the starting point and in combination with the following reasons, I felt it was the right choice here:
Thoughts? |
||
count = module.app_config.enable_identity_provider ? 1 : 0 | ||
source = "../../modules/identity-provider-client" | ||
|
||
name = local.identity_provider_config.identity_provider_name | ||
cognito_user_pool_id = module.identity_provider[0].user_pool_id | ||
callback_urls = local.identity_provider_config.client.callback_urls | ||
logout_urls = local.identity_provider_config.client.logout_urls | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
data "aws_caller_identity" "current" {} | ||
data "aws_region" "current" {} | ||
|
||
resource "aws_iam_policy" "cognito_access" { | ||
name = "${var.name}-cognito-access" | ||
policy = data.aws_iam_policy_document.cognito_access.json | ||
} | ||
|
||
data "aws_iam_policy_document" "cognito_access" { | ||
statement { | ||
actions = ["cognito-idp:*"] | ||
effect = "Allow" | ||
resources = ["arn:aws:cognito-idp:${data.aws_region.current.name}:${data.aws_caller_identity.current.id}:userpool/${var.cognito_user_pool_id}"] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,37 @@ | ||||||||
resource "aws_cognito_user_pool_client" "client" { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is something I like to do, especially for complex terraform resources. I still remember years ago being a terraform newbie and not knowing how to connect the configuration to the documentation |
||||||||
name = var.name | ||||||||
user_pool_id = var.cognito_user_pool_id | ||||||||
|
||||||||
callback_urls = var.callback_urls | ||||||||
logout_urls = var.logout_urls | ||||||||
supported_identity_providers = ["COGNITO"] | ||||||||
refresh_token_validity = 1 | ||||||||
access_token_validity = 60 | ||||||||
id_token_validity = 60 | ||||||||
token_validity_units { | ||||||||
refresh_token = "days" | ||||||||
access_token = "minutes" | ||||||||
id_token = "minutes" | ||||||||
} | ||||||||
Comment on lines
+8
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (non-blocking question) I'm assuming you just guessed at these numbers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I brought these values over from a previous project that established these as reasonable defaults. Do you think they should be something else? |
||||||||
|
||||||||
generate_secret = true | ||||||||
allowed_oauth_flows_user_pool_client = true | ||||||||
allowed_oauth_flows = ["code"] | ||||||||
allowed_oauth_scopes = ["phone", "email", "openid", "profile"] | ||||||||
explicit_auth_flows = ["ALLOW_ADMIN_USER_PASSWORD_AUTH", "ALLOW_REFRESH_TOKEN_AUTH"] | ||||||||
|
||||||||
# Avoid security issue where error messages indicate when a user doesn't exist | ||||||||
prevent_user_existence_errors = "ENABLED" | ||||||||
|
||||||||
enable_token_revocation = true | ||||||||
enable_propagate_additional_user_context_data = false | ||||||||
|
||||||||
read_attributes = ["email", "email_verified", "phone_number", "phone_number_verified", "updated_at"] | ||||||||
write_attributes = ["email", "updated_at", "phone_number"] | ||||||||
Comment on lines
+17
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole section could use some inline documentation, but I don't wanna block on that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably establish some guidelines on how much we want to inline document modules, which we are currently anticipating most projects should not modify, vs the |
||||||||
} | ||||||||
|
||||||||
resource "aws_ssm_parameter" "client_secret" { | ||||||||
name = "/${var.name}/identity-provider/client-secret" | ||||||||
type = "SecureString" | ||||||||
value = aws_cognito_user_pool_client.client.client_secret | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
output "access_policy_arn" { | ||
value = aws_iam_policy.cognito_access.arn | ||
} | ||
|
||
output "client_id" { | ||
description = "The ID of the user pool client" | ||
value = aws_cognito_user_pool_client.client.id | ||
} | ||
|
||
output "client_secret_arn" { | ||
description = "The arn for the SSM parameter storing the user pool client secret" | ||
value = aws_ssm_parameter.client_secret.arn | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
variable "callback_urls" { | ||
type = list(string) | ||
description = "The URL(s) that the identity provider will redirect to after a successful login" | ||
default = [] | ||
} | ||
|
||
variable "cognito_user_pool_id" { | ||
type = string | ||
description = "The ID of the user pool that the client will be associated with" | ||
} | ||
|
||
variable "logout_urls" { | ||
type = list(string) | ||
description = "The URL that the identity provider will redirect to after a successful logout" | ||
default = [] | ||
} | ||
|
||
variable "name" { | ||
type = string | ||
description = "Name of the application or service that will act as a client to the identity provider" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tangential thought: i've been thinking of renaming these at some point to: