Compliance violation scheduler erronenously assumes event_name is present for audit events
In gitlab-com/gl-infra/production#20177 (closed), we saw the following exceptions in Sentry (https://new-sentry.gitlab.net/organizations/gitlab/issues/1726075):
NoMethodError: undefined method `to_sym' for nil:NilClass (NoMethodError)
definitions[key.to_sym]
^^^^^^^
from lib/gitlab/audit/type/definition.rb:77:in `get'
from audit_events/compliance_violation_scheduler.rb:22:in `schedule_compliance_check'
from audit_events/compliance_violation_scheduler.rb:13:in `block in execute'
from audit_events/compliance_violation_scheduler.rb:12:in `each'
from audit_events/compliance_violation_scheduler.rb:12:in `execute'
from ee/gitlab/audit/logging.rb:12:in `block in log_events'
from <internal:kernel>:90:in `tap'
from ee/gitlab/audit/logging.rb:11:in `log_events'
from lib/gitlab/audit/logging.rb:17:in `block in log_to_new_tables'
from lib/gitlab/audit/logging.rb:16:in `each'
from lib/gitlab/audit/logging.rb:16:in `flat_map'
from lib/gitlab/audit/logging.rb:16:in `log_to_new_tables'
from app/services/audit_event_service.rb:133:in `log_security_event_to_database'
from app/services/audit_event_service.rb:60:in `security_event'
from ee/audit_event_service.rb:95:in `security_event'
from gitlab/auth/group_saml/membership_updater.rb:97:in `log_audit_event'
from gitlab/auth/group_saml/membership_updater.rb:34:in `add_default_membership'
from gitlab/auth/group_saml/membership_updater.rb:20:in `execute'
from gitlab/auth/group_saml/identity_linker.rb:53:in `update_group_membership'
from ee/gitlab/auth/saml/identity_linker.rb:14:in `link'
from gitlab/auth/group_saml/identity_linker.rb:20:in `link'
from app/controllers/omniauth_callbacks_controller.rb:200:in `link_identity'
from groups/omniauth_callbacks_controller.rb:30:in `link_identity'
from app/controllers/omniauth_callbacks_controller.rb:176:in `omniauth_flow'
from groups/omniauth_callbacks_controller.rb:21:in `group_saml'
from action_controller/metal/basic_implicit_render.rb:6:in `send_action'
from abstract_controller/base.rb:224:in `process_action'
from action_controller/metal/rendering.rb:165:in `process_action'It looks like this occurred due to the enable_project_compliance_violations feature flag enabled in !195469 (merged).
The core issue appears that in https://gitlab.com/gitlab-org/gitlab/-/blob/d98ddbe09b5d812161dcd46226f3e4267ce4d502/ee/app/services/audit_events/compliance_violation_scheduler.rb#L22, audit_event.event_name may return nil:
event_definition = Gitlab::Audit::Type::Definition.get(audit_event.event_name)In the case of a SAML membership audit event:
-
Gitlab::Auth:GroupSaml::MembershipUpdater#log_audit_eventis called here: https://gitlab.com/gitlab-org/gitlab/-/blob/48921fc8f38a8d3da5aeff0442bb950beeac4c2d/ee/lib/gitlab/auth/group_saml/membership_updater.rb#L92-98 -
AuditEventService#for_memberdoes not addevent_name: https://gitlab.com/gitlab-org/gitlab/-/blob/4216b7b7fc844acff542e2840bd4277baa548ef6/ee/app/services/ee/audit_event_service.rb#L13-89 - Neither does
#security_event: https://gitlab.com/gitlab-org/gitlab/-/blob/4216b7b7fc844acff542e2840bd4277baa548ef6/ee/app/services/ee/audit_event_service.rb#L92-101
It seems to me that the ComplianceViolationScheduler should gracefully handle an empty event_name because event_name is not guaranteed to be present in the database. Notice there is no not null constraint:
gitlabhq_development=# \d+ project_audit_events;
Partitioned table "public.project_audit_events"
Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description
----------------+--------------------------+-----------+----------+------------------------------------------------+----------+-------------+--------------+-------------
id | bigint | | not null | nextval('shared_audit_event_id_seq'::regclass) | plain | | |
created_at | timestamp with time zone | | not null | | plain | | |
project_id | bigint | | not null | | plain | | |
author_id | bigint | | not null | | plain | | |
target_id | bigint | | | | plain | | |
event_name | text | | | | extended | | |
details | text | | | | extended | | |
ip_address | inet | | | | main | | |
author_name | text | | | | extended | | |
entity_path | text | | | | extended | | |
target_details | text | | | | extended | | |
target_type | text | | | | extended | | |A potential fix:
diff --git a/ee/app/services/audit_events/compliance_violation_scheduler.rb b/ee/app/services/audit_events/compliance_violation_scheduler.rb
index e86b133352ee..39f2a27c4c26 100644
--- a/ee/app/services/audit_events/compliance_violation_scheduler.rb
+++ b/ee/app/services/audit_events/compliance_violation_scheduler.rb
@@ -19,7 +19,11 @@ def execute
def schedule_compliance_check(audit_event)
return unless should_schedule_compliance_check?(audit_event)
- event_definition = Gitlab::Audit::Type::Definition.get(audit_event.event_name)
+ event_name = audit_event.event_name
+
+ return unless event_name
+
+ event_definition = Gitlab::Audit::Type::Definition.get(event_name)
return unless event_definition&.compliance_controls.present?
::ComplianceManagement::ComplianceViolationDetectionWorker.perform_async(If event_name is required, we may want to log this payload so that we can add event_name to all payloads going forward.
Edited by 🤖 GitLab Bot 🤖