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

Topic merge: Reputation alarms, rename resources, use FIPS endpoints #9

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

jameshochadel
Copy link
Contributor

Changes proposed in this pull request:

  • This catches the main branch up with my topic branch.
  • Reviewing individual commits will be easier than reviewing it all at once. See the individual messages for the list of changes.

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

None.

Also automatically add recipient emails that bounce or complain to the suppression list for the configuration set
Further preparation for SES and updater packages.
Changes:

- Create the platform notifications topic subscription in the helper deployment
- Create new package and handler for brokerpak-specific functionality
- Create WIP handler for SNS notifications

There are several outstanding TODOs, but I wanted to make a commit:

- The endpoint is untested with SNS, just unit tests
- The endpoint does not verify the SNS request, so anyone can send requests to it
- The endpoint does not yet confirm the SNS subscription, which it must do: https://docs.aws.amazon.com/sns/latest/dg/SendMessageToHttp.confirm.html
- Implement unmarshaller interface, which is more idiomatic
- Create SNS client for confirming topic subscription
@jameshochadel jameshochadel requested a review from a team as a code owner February 3, 2025 20:43
JasonTheMain
JasonTheMain previously approved these changes Feb 3, 2025
Copy link

@JasonTheMain JasonTheMain left a comment

Choose a reason for hiding this comment

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

Looks good to me

This commit implements and tests the full SNS endpoint, which can confirm the subscription and act on notifications. It includes a tested implementation of SNS signature verification. It also includes one experimental fuzz test for the SNS verification. There's currently no CI infrastructure for running tests at all, let alone fuzzing, but I've run them locally and they pass.
Copy link

@rcgottlieb rcgottlieb left a comment

Choose a reason for hiding this comment

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

One small issue about the repo name otherwise it's good

ci/pipeline.yml Show resolved Hide resolved
rcgottlieb
rcgottlieb previously approved these changes Feb 12, 2025
- Inline the CSB and Helper routes to their app resources
- Each app resource only supports one route, so drop the /docs route for now
- Related to issue: cloudfoundry/terraform-provider-cloudfoundry#126
Calling sns.Client.Options().BaseEndpoint was not the correct way to get the base endpoint if that endpoint was automatically populated -- only if it was set manually. The correct method is now used in main.go: Use NewDefaultEndpointResolver in the service package (e.g `service/sns`). As of this commit the code passes the test locally, using a local server.
- CI terraform jobs should use FIPS endpoints
- Correct envvar is AWS_USE_FIPS_ENDPOINT, not _ENDPOINTS plural; fix CSB
- FIPS usage in Helper TBD
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.

3 participants