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 SMTP brokerpak and supporting #5

Merged
merged 66 commits into from
Sep 10, 2024
Merged

Add SMTP brokerpak and supporting #5

merged 66 commits into from
Sep 10, 2024

Conversation

jameshochadel
Copy link
Contributor

Changes proposed in this pull request:

  • I've been developing on a topic branch to avoid a PR workflow while in early stages of development, but I'm going to start PRing changes for each issue I resolve now.
  • Adds the SMTP brokerpak, including provision and bind code, manifest and service definition yaml files, and a Go client for testing service instances.
  • Adds local development tools for working with the CSB and brokerpaks locally.

Things to check

  • For any logging statements, is there any chance that they could be logging sensitive data?
  • Are log statements using a logging library with a logging level set? Setting a logging level means that log statements "below" that level will not be written to the output. For example, if the logging level is set to INFO and debugging statements are written with log.debug or similar, then they won't be written to the otput, which can prevent unintentional leaks of sensitive data.

Security considerations

Follows standard practices to avoid credential disclosure.

They're the same underlying value, but looking at the outputs alone, you wouldn't know that.
Download the Terraform provider from GitHub to follow Hashi registry ToS
- Remove txt_verification_record which is redundant with DKIM records
- Mirror tags from https://github.com/cloud-gov/go-broker-tags/blob/main/tags.go#L10
Goal is to minimize disruption of updates.

Resolves: cloud-gov/product#3123
@jameshochadel jameshochadel requested a review from a team as a code owner September 10, 2024 14:49
value = aws_iam_access_key.access_key.id
}

# TODO should we only expose the SMTP interface?
Copy link
Contributor

Choose a reason for hiding this comment

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

What would this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The binding provides a set of SMTP credentials and an AWS access key to the IAM user, which allows customers to use the AWS SES APIs instead of the SMTP interface. I'll take this TODO out, though; it's a desirable behavior so they can use the AWS SDK if they want, and the security impact is the same since the user has privileges limited to their identity only.

tags = {
"broker" = "Cloud Service Broker"
"client" = "Cloud Foundry"
"environment" = "local" # todo, parameterize at CSB level
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we'll follow up on this later

Copy link
Contributor Author

@jameshochadel jameshochadel Sep 10, 2024

Choose a reason for hiding this comment

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

Yep, that's probably in my next issue, which is tackling Terraform for deploying the broker itself.

@jameshochadel jameshochadel merged commit fde3dd9 into main Sep 10, 2024
@jameshochadel jameshochadel deleted the brokerpak-topic branch September 10, 2024 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants