⚡ Async update

🚥 Status

In review

🗒️ Notes

Received some feedback, addressed it and 🏓 back.

⚒️ MRs

• Increase virtual registry rate limit default to... (!227323) • David Fernandez • 18.11 - in review

/cc @crystalpoole @jaime

Thanks @mkhalifa3 ! 🙇

I added a migration for existing values.

🍋 back to you!

Great catch 👍

Indeed, I was confused with a column default value but here, we use a jsonb attribute which has an entirely different behavior.

Thus, we need a data migration.

I didn't choose a post deployment migration because application settings (the single row) are aggressively cached and so even after executing the post deployment migration we could still be using the old value.

Thanks @dmeshcharakou ! 🙇

I think we are missing some tables in regards to the Allow/Deny feature feature in virtual repositories. Added in !18456 (629e8228)

Hum, 🤔 don't we have a pattern_type column too? I think we want users to be able to use either wildcards or regex. We need to know which type the pattern is to handle it's validation and more importantly, its execution.

Actually, I think every time that we have to target a set of packages with user provided patterns, we need to same 3 columns:

• pattern
• pattern_type
• target_field

This is for allow/deny rules but also access rules.

What do you think?

🤔 Why is this change being made?

Draft MR for https://gitlab.com/gitlab-org/gitlab/-/work_items/593722

🎩 Author and Reviewer Checklist

Please verify the check list and ensure to tick them off before the MR is merged.

• Provided a concise title for this Merge Request (MR)
• Added a description to this MR explaining the reasons for the proposed change, per say why, not just what
  • Copy/paste the Slack conversation to document it for later, or upload screenshots. Verify that no confidential data is added, and the content is SAFE
• Assign reviewers for this MR to the correct
  • The when to get approval handbook section explains when DRI approval is required
  • The who can approve handbook section explains how to identify the DRI
  • If the MR does not require DRI approval, consider asking someone on your team, such as your manager.
  • The approver may merge the MR. If they approve but don't merge, you can merge.
• For transparency, share this MR with the audience that will be impacted.
  • Team: For changes that affect your direct team, share in your group Slack channel
  • Department: If the update affects your department, share the MR in your department Slack channel
  • Division: If the update affects your division, share the MR in your division Slack channel
  • Company: If the update affects all (or the majority of) GitLab team members, post an update in #whats-happening-at-gitlab linking to this MR
    • For high-priority company-wide announcements work with the internal communications team to post the update in #company-fyi and align on a plan to circulate in additional channels like the "While You Were Iterating" Newsletter

Commits

• docs: Update ADR-020 authentication flow from JWT to OIDC

Switch the Artifact Registry authentication decision from the custom JWT token exchange endpoint to a standard OIDC-based approach with per-org issuers and RFC 8693 Token Exchange.

David Fernandez (e340ac12) at 17 Mar 12:54

David Fernandez (e12a185d) at 17 Mar 12:18

@splattael

Wild question here: CNG should solve the same situation, right? We have a web container and a workhorse containers running in the same pod but both need to have those shared folders.

If the above is true, then, we could check how CNG solves this.

Am I missing something?

David Fernandez (57b86530) at 17 Mar 10:32

David Fernandez (90df0168) at 17 Mar 10:32

... and 1 more commit

What does this MR do and why?

When working on a fix to add OAuth support to the Maven Virtual Registries Endpoints, I bumped into an issue where I was receiving a 400 error400 Bad request - Found more than one set of credentials`.

In the MR we add OAuth to the existing strategies !223674 (diffs).

The tests that are failing relate to basic auth.

When I added debug logs to trace where this is failing, and running bundle exec rspec ee/spec/requests/api/virtual_registries/packages/maven/endpoints_spec.rb -e "with a personal_access_token when sent by basic auth"we hit the token_from_namespace_inheritable method in the Authentication module.

The registered strategies that output:

 http_private_token_header => [:personal_access_token]
  http_deploy_token_header => [:deploy_token]
  http_job_token_header => [:job_token]
  http_bearer_token => [:oauth_token]
  access_token_param => [:oauth_token]
  http_basic_auth => [:personal_access_token_with_username, :deploy_token_with_username, :job_token_with_username]

The Authorization header is:

Authorization header: Basic dXNlcjI6Z2xwYXQtSERkZjEteWpPWXN...

When we iterate through the strategies to get the credentials from the request, we hit the token_locator method extract_from_http_basic_auth and get:

http_basic_auth => #<struct Gitlab::APIAuthentication::TokenLocator::UsernameAndPassword username="user2", password="xxxx">

But, as we also have the http_bearer_token strategy, and the request also has an Authorization header, we get another set of credentials. Because the extract_from_http_bearer_token doesn't check if it's the right scheme, it just extracts it.

So found ends up with 2 sets of credentials (I removed passwords from output):

[1] pry(#<#<Class:0x0000000144c74bc0>>)> found
=> {:http_bearer_token=>#<struct Gitlab::APIAuthentication::TokenLocator::UsernameAndPassword username=nil, password="xxxx">, :http_basic_auth=>#<struct Gitlab::APIAuthentication::TokenLocator::UsernameAndPassword username="user2", password="glpat-xxx">}

And we hit this error https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/api/helpers/authentication.rb#L31

Getting help from duo as to why we haven't come across this before, there are currently no other endpoints that combine http_bearer_token with http_basic_auth.

References

Screenshots or screen recordings

How to set up and validate locally

We can validate this via the rails console.

1. On master, start the rails console gdk rails c

2. Testing to show that the bearer token locator will extract basic auth creds:

basic_creds = Base64.strict_encode64('myuser:mypassword')
request = OpenStruct.new(headers: { 'Authorization' => "Basic #{basic_creds}" })
locator = Gitlab::APIAuthentication::TokenLocator.new(:http_bearer_token)
result = locator.extract(request)

This will return the password:

=> #<struct Gitlab::APIAuthentication::TokenLocator::UsernameAndPassword
 username=nil,
 password="bXl1c2VyOm15cGFzc3dvcmQ=">

3. Switch to this branch 587146-update-token-locator-extract-bearer-token and run step 2 again and nil should be returned:

Example:

[9] pry(main)> basic_creds = Base64.strict_encode64('myuser:mypassword')
=> "bXl1c2VyOm15cGFzc3dvcmQ="
[10] pry(main)> request = OpenStruct.new(headers: { 'Authorization' => "Basic #{basic_creds}" })
=> #<OpenStruct headers={"Authorization"=>"Basic bXl1c2VyOm15cGFzc3dvcmQ="}>
[11] pry(main)> locator = Gitlab::APIAuthentication::TokenLocator.new(:http_bearer_token)
=> #<Gitlab::APIAuthentication::TokenLocator:0x000000014b9051e0
 @errors=#<ActiveModel::Errors []>,
 @location=:http_bearer_token,
 @validation_context=nil>
[12] pry(main)> result = locator.extract(request)
=> nil

4. Additional testing to test bearer tokens:

request = OpenStruct.new(headers: { 'Authorization' => 'Bearer asdfbasdf123123' })
locator = Gitlab::APIAuthentication::TokenLocator.new(:http_bearer_token)
result = locator.extract(request)
result.password

# => "asdfbasdf123123" # should return this

5. Lower case test:

request = OpenStruct.new(headers: { 'Authorization' => 'bearer asdfbasdf123123' })
locator = Gitlab::APIAuthentication::TokenLocator.new(:http_bearer_token)
result = locator.extract(request)
result.password

# => "asdfbasdf123123" # should return this

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 #587146

For context, on some paths we force disabled the direct upload.

A good example is all the work done around json bodies (MR example). We basically force disable the direct upload as a way to receive the request body as a temporary file in the rails side. This is useful for running JSON validations.

That's actually what the CommitsUploader is doing, hence the force disabled direct upload.

@splattael So the problem is that Rails.root is not resolved to a correct path? Is that accurate?

workhorse_local_upload_path needs to be resolved to a folder that is shared between the workhorse and the rails process. This is obvious since workhorse will write the uploaded file to it and rails will read the upload file from there.

I'm not sure if replacing Rails.root with root could be disruptive for existing workflows. As stated above, this could be impactful for the CNG conditions.

David Fernandez (391da443) at 17 Mar 09:44

This MR contains the following updates:

View the Renovate pipeline for this MR

Note: The pre-commit manager in Renovate is not supported by the pre-commit maintainers or community. Please do not report any problems there, instead create a Discussion in the Renovate repository if you have any questions.

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 these updates again.

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

This MR has been generated by Renovate Bot.

David Fernandez (059229a0) at 17 Mar 09:44

... and 1 more commit