Follow-up fixes to Incident 19090

Summary

The incident issue that this is addressing is here

Some Ultimate customers who have secret push protection (SPP) enabled are seeing some of those pushes blocked with 500 errors. Originally, it appeared that special characters in the pushed changes was the cause, but more testing did called that into question.

From the incident issue:

The error is Encoding::UndefinedConversionError and message is "\xC3" from ASCII-8BIT to UTF-8. Sample logs: https://log.gprd.gitlab.net/app/r/s/qGuHX

Then another engineer replied:

I just tried to commit existing single file changes on master again to check if the issue is reproducible. And I faced the same issue. I even rewrote all the special characters and still not able to push the changes.

Note: I am not using special ASCII characters like — (em dash) and ™ (trademark) anywhere in the diff.

Initial analysis and investigation lead this note:

Below is our root-cause analysis and corrective actions (also shared on Slack):

@eurie and I had spent a bit more time looking into #incident-19090 and we have discussed the following:

  • While the code was already behind the secret_detection_service feature flag which is only enabled on staging at the moment, the calls to SDS were bypassing the flag because the check we had in place was not sufficient. To put it simply, this incident should have had a limited impact because the associated feature was behind a feature flag, but the code was not respective of that flag, causing the feature to be available on production.
  • All logs point to the same conclusion that had been shared multiple times above (and in this Slack thread), the issue happens due to special characters being present in the diff patch. This is due to how Ruby’s gRPC is transcoding the data sent for scanning to SDS between binary (ASCII-8BIT) and UTF-8. While the exception raised is a Ruby-specific encoding one, it’s a side effect of the gRPC call/serialization. Such exceptions are hard to reproduce, and not so easy to test.

Our proposed corrective actions are:

  • Feature flag:
    • Fix the feature flag check to ensure the requests are only sent to SDS when the flag is enabled.
  • Encoding issue:
    • Enforce UTF-8 encoding for all data sent to SDS.
    • Rescue the exception in case similar encoding errors take place.
    • Log an error message when the exception is rescued indicating the problematic character/encodings.

All related logs can be found here:

Steps to reproduce

master has been reverted and therefore will not exhibit the behavior.

Checkout MR branch: git checkout eurie/473440-switch-to-use-SDS-gem-for-secret-detection

Enable spp_scan_diffs FF

> bin/rails c
> Feature.enable(:spp_scan_diffs)

Clone a project from your GDK, e.g., Flight

In that project:

> git checkout -b test-branch
> echo "™" > foo.txt
> git add foo.txt
> git commit -m "special characters"
> git push origin test-branch

The push should fail and the logs should have the stack trace.

Alternatively, you can run the rails app separately to see the stack trace and add breakpoints:

# GDK/gitlab directory
> gdk stop rails-web && GITLAB_RAILS_RACK_TIMEOUT_ENABLE_LOGGING=false PUMA_SINGLE_MODE=true gdk rails s

Example Project

The original MR branch (https://gitlab.com/gitlab-org/gitlab/-/tree/eurie/473440-switch-to-use-SDS-gem-for-secret-detection) has the changes that were exhibiting the errors.

What is the current bug behavior?

git push fails with a 500 error regardless if there's a secret included in the pushed files.j

The Rails stack trace is:

[ee/lib/gitlab/checks/secrets_check.rb:687:in `initialize'
 ee/lib/gitlab/checks/secrets_check.rb:687:in `new'
 ee/lib/gitlab/checks/secrets_check.rb:687:in `build_payload'
 ee/lib/gitlab/checks/secrets_check.rb:679:in `block in build_payloads'
 ee/lib/gitlab/checks/secrets_check.rb:678:in `each'
 ee/lib/gitlab/checks/secrets_check.rb:678:in `inject'
 ee/lib/gitlab/checks/secrets_check.rb:678:in `build_payloads'
 ee/lib/gitlab/checks/secrets_check.rb:265:in `block in standardize_payloads'
 ee/lib/gitlab/checks/secrets_check.rb:263:in `each'
 ee/lib/gitlab/checks/secrets_check.rb:263:in `flat_map'
 ee/lib/gitlab/checks/secrets_check.rb:263:in `standardize_payloads'
 ee/lib/gitlab/checks/secrets_check.rb:107:in `block in validate!'
 lib/gitlab/checks/timed_logger.rb:27:in `log_timed'
 ee/lib/gitlab/checks/secrets_check.rb:105:in `validate!'
 ee/lib/ee/gitlab/checks/changes_access.rb:15:in `bulk_access_checks!'
 lib/gitlab/checks/changes_access.rb:29:in `block in validate!'
 lib/gitlab/checks/timed_logger.rb:27:in `log_timed'
 lib/gitlab/checks/changes_access.rb:28:in `validate!'
 lib/gitlab/git_access.rb:390:in `check_access!'
 lib/gitlab/git_access.rb:378:in `check_change_access!'
 ee/lib/ee/gitlab/git_access.rb:98:in `check_change_access!'
 lib/gitlab/git_access.rb:359:in `check_push_access!'
 lib/gitlab/git_access.rb:96:in `check'
 ee/lib/ee/gitlab/git_access.rb:20:in `check'
 lib/api/helpers/internal_helpers.rb:54:in `access_check!'
 lib/api/helpers/internal_helpers.rb:35:in `block in access_check_result'
 lib/api/helpers/internal_helpers.rb:128:in `with_admin_mode_bypass!'
 lib/api/helpers/internal_helpers.rb:34:in `access_check_result'
 lib/api/internal/base.rb:94:in `check_allowed'
 ee/lib/ee/api/internal/base.rb:22:in `block in check_allowed'
 lib/gitlab/ip_address_state.rb:11:in `with'
 ee/lib/ee/api/internal/base.rb:21:in `check_allowed'
 lib/api/internal/base.rb:171:in `block (2 levels) in <class:Base>'
 ee/lib/gitlab/middleware/ip_restrictor.rb:11:in `call'
 lib/api/api_guard.rb:219:in `call'
 ee/lib/omni_auth/strategies/group_saml.rb:41:in `other_phase'
 lib/gitlab/metrics/elasticsearch_rack_middleware.rb:16:in `call'
 lib/gitlab/middleware/sidekiq_shard_awareness_validation.rb:20:in `block in call'
 lib/gitlab/sidekiq_sharding/validator.rb:42:in `enabled'
 lib/gitlab/middleware/sidekiq_shard_awareness_validation.rb:20:in `call'
 lib/gitlab/middleware/memory_report.rb:13:in `call'
 lib/gitlab/middleware/speedscope.rb:13:in `call'
 lib/gitlab/database/load_balancing/rack_middleware.rb:23:in `call'
 lib/gitlab/middleware/rails_queue_duration.rb:33:in `call'
 lib/gitlab/etag_caching/middleware.rb:21:in `call'
 lib/gitlab/metrics/rack_middleware.rb:16:in `block in call'
 lib/gitlab/metrics/web_transaction.rb:46:in `run'
 lib/gitlab/metrics/rack_middleware.rb:16:in `call'
 lib/gitlab/middleware/go.rb:21:in `call'
 lib/gitlab/middleware/query_analyzer.rb:11:in `block in call'
 lib/gitlab/database/query_analyzer.rb:83:in `within'
 lib/gitlab/middleware/query_analyzer.rb:11:in `call'
 lib/ci/job_token/middleware.rb:11:in `call'
 lib/gitlab/middleware/multipart.rb:173:in `call'
 lib/gitlab/middleware/read_only/controller.rb:50:in `call'
 lib/gitlab/middleware/read_only.rb:18:in `call'
 lib/gitlab/middleware/unauthenticated_session_expiry.rb:18:in `call'
 lib/gitlab/middleware/same_site_cookies.rb:27:in `call'
 lib/gitlab/middleware/path_traversal_check.rb:40:in `call'
 lib/gitlab/middleware/handle_malformed_strings.rb:21:in `call'
 lib/gitlab/middleware/basic_health_check.rb:25:in `call'
 lib/gitlab/middleware/handle_ip_spoof_attack_error.rb:25:in `call'
 lib/gitlab/middleware/request_context.rb:15:in `call'
 lib/gitlab/middleware/webhook_recursion_detection.rb:15:in `call'
 config/initializers/fix_local_cache_middleware.rb:11:in `call'
 lib/gitlab/middleware/compressed_json.rb:44:in `call'
 lib/gitlab/middleware/rack_multipart_tempfile_factory.rb:19:in `call'
 lib/gitlab/middleware/sidekiq_web_static.rb:20:in `call'
 lib/gitlab/metrics/requests_rack_middleware.rb:79:in `call'
 lib/gitlab/middleware/release_env.rb:12:in `call']

What is the expected correct behavior?

git push succeeds unless SPP is enabled and the push includes a secret (and no git commit message flags or command line flags are used to bypass the secret detection).

Relevant logs and/or screenshots

Output of checks

This bug happens on GitLab.com

Results of GitLab environment info

Results of GitLab application Check

Possible fixes

Root cause: TL;DR the Payload objects are being created from standardize_payloads for both execution paths, via direct method calling into the gem (default) and when sending to the SDS when the FF is enabled (which it is not on production).

More specific details on the root cause is in this note.

The fix will be to:

  1. Add exception handling in SecretsCheck#build_payload and log the error
  2. Use String#force_encoding('UTF-8') on the data of the payload
Edited by Ethan Urie