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

update false positive secret detection allowlist #3239

Merged
merged 1 commit into from Sep 28, 2023

Conversation

peakematt
Copy link
Contributor

@peakematt peakematt commented Sep 27, 2023

Implements

This updates the Apollo-specific secret allowlisting file in this repo to allowlist additional false positive / benign positive detections.

This PR allowlists a series of detections across git history in various *.xcscmblueprint files. From my research, all of these values are identifiers and not secret, so these detections by Gitleaks, our selected secret scanning tool were spurious. To create the allowlisting config to properly ensure that net-new detections won't be generated when these files are modified in the future, I additionally needed to update the allowlisting config for line

guard fragment.0.key == fragment.1.key else {
. This is why I removed the rule that allowlisted a commit hash from the generic-api-key signature and swapped to file-based allowlisting.

Additionally, I added a bunch of commentary at the top of the file about the purpose of the .gitleaks.toml file to provide more information to contributors to this repo.

I tested this by replicating our secret scanning process on this repo locally. I confirmed that no detections were generated.

@netlify
Copy link

netlify bot commented Sep 27, 2023

Deploy Preview for apollo-ios-docs canceled.

Name Link
🔨 Latest commit 67881d2
🔍 Latest deploy log https://app.netlify.com/sites/apollo-ios-docs/deploys/651474679749ba0008b31af9

@peakematt peakematt marked this pull request as ready for review September 27, 2023 18:37
# https://github.com/apollographql/apollo-ios/blob/474554504e7e33cef2a71774f825d5b3947ff797/Tests/ApolloCodegenTests/TestHelpers/ASTMatchers.swift#L72
# This was previously allowlisted via commit hash, but updating that rule
# To support allowlisting false positive detections in the files below as well.
'''Tests/ApolloCodegenTests/TestHelpers/ASTMatchers.swift''',
Copy link
Member

Choose a reason for hiding this comment

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

@BobaFetters - this file location will need to change with the move to apollo-ios-dev right?

@peakematt - you may not know about the upcoming change to our repo structure which is going to affect these changes. You can read about the change here. It's probably still worth merging in these changes and then adjusting the secret detection once we do the merge of that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! That's really good to know!

Yeah I'll get these merged in and then can make adjustments to the new repos if/when needed.

Copy link
Member

Choose a reason for hiding this comment

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

Yea the Tests folder in its entirety is going to exist in the new apollo-ios-dev repo so will probably just need to move this there.

'''Tests/ApolloCodegenTests/TestHelpers/ASTMatchers.swift''',

# Allowlist the various high-entropy strings in xcscmblueprint files
'''Apollo.xcodeproj/project.xcworkspace/xcshareddata/Apollo.xcscmblueprint$''',
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, these files shouldn't be a problem once we merge the repo change because they no longer exist.

Copy link
Member

Choose a reason for hiding this comment

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

Although we will instead have apollo-ios-xcframework which will still contain an xcode project file.

Copy link
Member

Choose a reason for hiding this comment

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

Yea the xcodeproj are all gone from everywhere except the apollo-ios-xcframework repo @calvincestari mentioned so it may be the only place this is needed going forward after the repo changes are fully released.

@peakematt peakematt merged commit d1b3da6 into main Sep 28, 2023
16 checks passed
@peakematt peakematt deleted the peake/allowlist-fp-secret-detections branch September 28, 2023 10:58
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

3 participants