Daniele Bracciani (3e7c74f4) at 18 Mar 16:42

... and 1 more commit

This MR contains the following updates:

MR created with the help of gitlab-org/frontend/renovate-gitlab-bot

Release Notes

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻️ Rebasing: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this MR and you won't be reminded about this update again.

• If you want to rebase/retry this MR, check this box

This MR has been generated by Renovate Bot.

Thanks! Since we don't have implementation details yet, I have just opened a placeholder issue with the two concerns to handle: GitLab Rails: implement token revocation for IA... (#593992)

Daniele Bracciani (dc750790) at 18 Mar 11:28

... and 1 more commit

Daniele Bracciani (86e16865) at 18 Mar 11:28

What does this MR do and why?

This MR is another preparatory refactor in support of #546175

It touches on two specs shared examples:

• The Geo::ProjectRepositoryRegistry shared example required callers to define several variables and helper methods (model_record_key, model_record_id, create_model_record, registry_project_id, etc.) as an interface contract, which made the shared example fragile and hard to extend for a V2 implementation. This MR simplifies that contract by having the shared example derive what it needs directly from the registry class itself (via model_class, model_foreign_key, and a factory name helper), eliminating the need for callers to provide those overrides.

• the GeoNodeStatus for project repository replication shared example also now includes the project fixtures (project_1–project_4) that were defined at the top level of geo_node_status_spec.rb, where they actually belong, and the redundant #projects_count and #repositories_count tests are removed from the spec file since they are covered by the shared example.

References

• Extracted from !223027
• Preparatory work for #546175

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.

  audience: {{ dig "audience" "gitlab-rails" . | quote }}
  jwks_cache_ttl: {{ dig "jwksCacheTtl" 3600 . | int }}

Can we add also the jwks_cache_ttl value? Should be configurable, with default value 3600. Adding the suggestion just as a reference

@mkaeppler thanks for the heads-up. Yes, this will slightly change my plans, as I planned to create a base client for both JwksClient (JWKS fetching for IAM JWT validation) and other services (Login acceptance, consent acceptance, etc.), then migrate to gRPC once available. This isn't an issue, as is meant to be a follow-up and I haven't started work on that yet.In this case, we'll likely need two separate base clients on rails: one public and one internal.

This MR handles configuration and wiring in the GitLab chart; are you planning to deploy services with endpoint differentiation already? This could affect my current implementation, which relies on HTTP endpoints.

@jknabl-gitlab thanks for the changes! Approved for backend and groupauthentication ✔️

What does this MR do and why?

Remove archive_authentication_events feature flag.

This flag guarded a recurring background worker that deletes rows from the authentication_events table. The worker is now guarded by an application setting instead.

The FF was rolled out in #573714 and has been at 100% for 4 months.

We couldn't remove it until the application setting was implemented and confirmed safe. That's now done, so we're good to remove this flag.

References

• FF issue: #573714
• Cleaning up FF docs: https://docs.gitlab.com/development/feature_flags/controls/#cleaning-up

Screenshots or screen recordings

How to set up and validate locally

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.

Related to #573714

@pgascouvaillancourt sorry, I am currently at capacity. @rliu-gl could you please assign to another reviewer from the roulette 🙏

Hey @rshambhuni, no worries and thanks for reviewing it!

I see that jti is a required claim however I do not see any validation of that claim to prevent replay attacks.

We haven't open a dedicated issue for Token Revocation yet, but I already noted it in a comment under main OAuth for Protocells - Production when we discussed last time. I will open a placeholder issue for the token revocation and add this point there, so we don't lose it. Let me check with the team.

Same with scope claim, no validation of its value. Is this going to be tackled in a follow up MR?

This was actually discussed and tracked under Follow-up from "Add IAM JWT validation service ... (#585894 - closed) , concluding that the size limitation will be good enough for the validation (see this comment) (resolved by this MR).

Also, take into account that the changes are part of GitLab rails: Accept External OAuth Token (#580758 - closed), the IAM JWT will later serve as an access_token and validated by the AccessTokenValidationService. Are we covered, or should we implement additional validation?

@skundapur @ifarkas I have a few questions before formalizing the implementation details:

1. The consent flow will need a new endpoint, and the most appropriate would be /oauth/iam/consent, but this runs into the same Envoy Gateway routing concern raised in the login integration. What is your recommendation for the path?
2. Can you clarify Store user's consent record/Store user's consent rejection record: are them audit events? Or should GitLab also persist a local record?
3. Regarding Redirect user to appropriate denial confirmation page: the Hydra implementation actually shows that, in case of rejection, the browser is redirected by Hydra with res.redirect(String(body.redirect_to)) (See consent endpoint code example). Our design document https://gitlab.com/gitlab-org/auth/iam/-/merge_requests/110+ also doesn't include a redirect_to in the reject response, which could be consistent with this requirement but diverges from Hydra's behavior. Can you clarify the expected behavior: should we follow the Hydra pattern (redirect via IAM back to the client), or does the IAM service intentionally not return a redirect_to on reject, meaning we need a local denial page?
4. Regarding the Slack conversation on trusted clients: is the follow-up implementation for the IAM service side? According to the Hydra flow, looks like on rails I could rely just on skip value to auto-accept with all requested_scope: what are the considerations around it?

Additionally, should we take multi-cell into consideration? I believe it follows a similar pattern to the login flow: since consent requires an authenticated user and the user has already authenticated during the login flow, the consent endpoint will be hit on the cell where the user's session lives. If so, this should work without additional cross-cell logic. What is your opinion?

@c_fons approving and enabling auto-merge ✔️

What does this MR do and why?

This MR is another preparatory refactor in support of #546175

It touches on two specs shared examples:

• The Geo::ProjectRepositoryRegistry shared example required callers to define several variables and helper methods (model_record_key, model_record_id, create_model_record, registry_project_id, etc.) as an interface contract, which made the shared example fragile and hard to extend for a V2 implementation. This MR simplifies that contract by having the shared example derive what it needs directly from the registry class itself (via model_class, model_foreign_key, and a factory name helper), eliminating the need for callers to provide those overrides.

• the GeoNodeStatus for project repository replication shared example also now includes the project fixtures (project_1–project_4) that were defined at the top level of geo_node_status_spec.rb, where they actually belong, and the redundant #projects_count and #repositories_count tests are removed from the spec file since they are covered by the shared example.

References

• Extracted from !223027
• Preparatory work for #546175

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.

@c_fons apologies for the noise on this one, the danger Bot is right: looking more carefully at the implementation, pipeline_project is used by let(:registry) across 3 different examples. With let, the 10-ref project is recreated for each example instead of being shared, resulting in 3 expensive project creations vs 2 with let_it_be. Since the project must be persisted for the test, build/build_stubbed aren't options either. The original let_it_be on lines 83 and 91 is the most efficient choice here. Could you please revert it? 🙏

self-review: this is more readable than SecureRandom.hex(32), but I can use a more realistic scenario if preferred.

self-review: I wanted to be defensive here.

question: should we have on challenge or login_challenge? The designed implementation uses challenge while the current IAM implementation and hydra login flow uses login_challenge, so would make sense to stick with the latter.