What does this MR do and why?

Re-order the permission checks so that we do the cheaper checks first.

I captured a profile in production and saw that most of the time was spent on has_access?:

This happens because we add users with custom notification level as recipients, and then filter them out in suitable_notification_level?. For gitlab-org/gitlab, this means every notification would have to check has_access? for 1k+ users.

This MR fixes the issue by checking suitable_notification_level? first, so that we only call has_access? on the filtered list.

References

Related to #594011

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Looks good to me 👍

I took a look at the timeouts on gitlab.com recently and also observed that it's a pretty low number.

In most cases the timeouts are caused by spending most of the time on gitaly calls.
Spot checking a few it seems like some combination of:

• high number of requests for ListRef which probably comes from TagCheck
  • This is probably due to many tags in one push
    • We might be able to mitigate this by batchloading the tags (when the ref_existence_check_gitaly ff is enabled)
• high number of FindChangedPaths
  • We seem to make these calls from two different checks:
    • DiffCheck
      • This would be consistent with the reported Validating diffs' file paths
      • I think this is one call per branch/tag pushed
      • Might be able to batchload these too (not sure how this rpc works yet)
    • SecretsCheck
      • This check happens after DiffCheck
      • We added some new logging recently to get some backtraces (https://log.gprd.gitlab.net/app/r/s/UzdAP) and it seems like all the timeouts occur before reaching this check

hi 👋 @ghinfey can you do the initial review for this?

Looks good to me 👍

hi 👋 @ghinfey can you do the maintainer review for this?

What does this MR do and why?

Contributes to #593682

Problem

Gitlab::X509::Signature#signer_certificate matches certificates by serial number only. Per RFC 5280 §4.1.2.2, serial numbers are only unique per issuer. When a signing certificate and an intermediate CA certificate in the same PKCS7 bundle share the same serial, the intermediate is returned instead of the signing certificate, causing verification to silently fail.

Solution

Add an issuer comparison alongside the serial number check in signer_certificate, using OpenSSL::PKCS7::SignerInfo#issuer. The combination of issuer + serial is the RFC 5280-compliant unique certificate identifier.

References

• #593682
• RFC 5280 §4.1.2.2

Screenshots or screen recordings

Not applicable — backend logic change only.

How to set up and validate locally

1. The bug requires a private CA where the intermediate and signing certificate share the same serial number (e.g., both serial 1), which is common with small/private CAs
2. Alternatively, verify via the new test:

   bin/rspec spec/lib/gitlab/x509/signature_spec.rb

3. The test prepends a fake intermediate CA certificate with the same serial but different issuer to the PKCS7 bundle, and verifies the correct signing certificate is selected

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Looks good to me @hustewart just one non-blocking idea

Why

Users can configure deploy keys for protected tags and branches via the UI in the Free tier, but the API restricts this functionality to Premium/Ultimate only. This creates an inconsistency between UI and API behavior.

Relates to #545283

What

This MR enables the deploy_key_id parameter in the allowed_to_create array for the Protected Tags API in the Free tier.

This MR enables the deploy_key_id parameter in the allowed_to_push array for the Protected Branches API in the Free tier.

• two private methods have changed names to be more clear about what they do (ce_style_access_levels and ee_style_access_levels) instead of where they are found
• CE helper now accepts "allowed_to_#{type}" with deploy_key_id only
• AccessLevelParams in CE extracts deploy key entries, so that depoloy_key_id can be set in the API in CE
• EE uses uniq to deduplicate any deploy keys from super

User/group granular access (user_id, group_id) remains restricted to Premium/Ultimate tiers.

Reviewer, please note

The params are structured a little differently for these APIs. To make it more clear, please have a look at the docs on how the params are structured

API docs for how to protect a branch: https://docs.gitlab.com/api/protected_branches/#protect-repository-branches

API docs for how to protect tag: https://docs.gitlab.com/api/protected_tags/#protect-a-repository-tag

I made it all one MR because shared Refs code gets updated.

I considered drying up the specs but I think shared testing here is more complicated than it's worth, and these APIs may diverge one day, so there's very little to be gained in trying to DRY this test code up.

How to set up and validate locally

If you want to verify the current behavior, switch to master and make sure to run GDK in FOSS. You'll see the deploy_key_id is ignored and not set.

Protected Tags:

curl --header "PRIVATE-TOKEN: $GDK_TOKEN" \
     --header "Content-Type: application/json" \
     --data '{"name": "v*", "allowed_to_create": [{"deploy_key_id": <id>}]}' \
     "$GDK_URL/api/v4/projects/:id/protected_tags"

Protected Branches:

curl --header "PRIVATE-TOKEN: $GDK_TOKEN" \
     --header "Content-Type: application/json" \
     --data '{"name": "feature-*", "allowed_to_push": [{"deploy_key_id": <id>}]}' \
     "$GDK_URL/api/v4/projects/:id/protected_branches"

Switch to 545283-protected-tags-deploy-keys-api and try to the same API request to verify the response includes deploy_key_id in create_access_levels

suggestion(non-blocking): Should this always be the same as granular_access_levels. Should we refactor granular_access_levels to accept params as an argument and re-use it here?

edit: if we want to refactor let's do it in another MR lest we fail undercoverage 😨

Jerry Seto (49eac90b) at 17 Mar 15:10

... and 4991 more commits

@andrevr I don't think it's a duplicate. We did do work to implement gpg and ssh signed tags and those features are currently behind feature flags (not enabled yet). I suspect that this issue might be resolved once we have fully rolled out those features.

I think we can rollout ssh signed tag support soon. For transparency we had to revert something related to gpg signed tags to fix #590905 so we would need to fix that forward before we can roll out gpg signed tags.

Jerry Seto (cec12976) at 16 Mar 16:31

hi 👋 @hustewart can do the initial review for this? 🙏

Jerry Seto (9b221b17) at 16 Mar 14:39

It's probably ok to close this one now right? @ghinfey

Jerry Seto (350c2f95) at 13 Mar 20:23

Jerry Seto (ed91a579) at 13 Mar 20:00

Jerry Seto (d1720aba) at 13 Mar 19:55

Jerry Seto (096fcfe2) at 13 Mar 18:44

... and 334 more commits