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

[buildkite] Update S3 credential handling #621

Merged
merged 3 commits into from
Aug 5, 2023

Conversation

staticfloat
Copy link
Contributor

We were alerted that our S3 credentials had leaked in a build that dumps
the environment when it errors out [0]. This PR rotates to a new key,
and also wipes out the key in the environment during the build itself,
so that sensitive values won't get written out in debugging information
like that.

[0] https://github.com/SciML/SciMLBenchmarksOutput/blob/8d80b1840228c63e8c6fdc597049211a8b916dea/markdown/DynamicalODE/single_pendulums.md

Copy link

@ai-maintainer ai-maintainer bot left a comment

Choose a reason for hiding this comment

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

AI-Maintainer Review for PR - [buildkite] Update S3 credential handling

👍 Title and Description

The Title and Description are Clear and Informative

The title and description of the pull request are clear and informative. They effectively communicate the purpose of the changes, which is to update the S3 credential handling due to a previous leak. The description provides a link to the issue that prompted the update, which is helpful for understanding the context of the changes.

👍 Scope of Changes

The Changes are Narrowly Focused

The changes in this pull request are narrowly focused on updating the S3 credential handling. The modifications to the .buildkite/run_benchmark.yml file and the benchmarks/Testing/test.jmd file are specifically related to the S3 access key and secret access key. There are no indications that the author is trying to resolve multiple issues simultaneously.

❓ Testing

Testing Details are Not Provided

The description of the pull request does not provide details on how the author tested the changes. It would be beneficial to include information on the testing performed to ensure the changes effectively address the issue and do not introduce new issues.

👍 Documentation

No New Functions, Classes, or Methods Added

Based on the provided diff, there are no newly added functions, classes, or methods. Therefore, there are no docstrings that need to be added.

💡 Recommendations

Please provide details on how the changes were tested. This could include information on any local testing performed, unit tests added or updated, or other testing methods used. This will help reviewers understand the impact of the changes and ensure they effectively address the issue.

Also, please ensure that the new S3 credentials are securely stored and not exposed in any public logs or debugging information. This is crucial for maintaining the security of the system.

Reviewed with AI Maintainer

We were alerted that our S3 credentials had leaked in a build that dumps
the environment when it errors out [0]. This PR rotates to a new key,
and also wipes out the key in the environment during the build itself,
so that sensitive values won't get written out in debugging information
like that.

[0] https://github.com/SciML/SciMLBenchmarksOutput/blob/8d80b1840228c63e8c6fdc597049211a8b916dea/markdown/DynamicalODE/single_pendulums.md
@staticfloat
Copy link
Contributor Author

Looks good now!

@ChrisRackauckas ChrisRackauckas merged commit 6cdf019 into master Aug 5, 2023
1 check passed
@ChrisRackauckas ChrisRackauckas deleted the sf/update_s3_credentials branch August 5, 2023 13:12
@ChrisRackauckas
Copy link
Member

Thanks!

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.

None yet

2 participants