Skip to content

Support the JWT params set by Workhorse in upload requests

David Fernandez requested to merge 214785-support-jwt-param-from-workhorse-for-file-uploads into master

What does this MR do?

This MR is related to #214785 (closed) and more broadly to #213288 (closed).

🌲 Context

When a supported file upload request goes through workhorse, here is what happens (simplified):

(Disable Dark Mode if you want to read this diagram properly 🙂 )

sequenceDiagram
    autonumber
    participant C as Client
    participant W as Workhorse
    participant R as Rails
    C->>W: File Upload
    W->>R: /authorize
    R->>W: Response
    W->>R: File Upload
    R->>W: Response
    W->>C: Response

When Workhorse receives (3), it will create a set of parameters (such as name, path, size, sha256 and friends) and pass them in (4) to Rails.

🔥 Problem

Currently, Workhorse will pass these parameters as a request parameters to rails. When Rails reads them, it can't say if these parameters have been set by workhorse or by a smart (😈) user. This situation caused a few issues by the past.

🚒 Solution

To fix the situation, gitlab-workhorse!490 (merged) has been created: in addition to current parameters, Workhorse will now set a specific parameter (one per file in the file upload request) for request (4). This specific parameter is set to a hash containing all the upload parameters and more importantly it is signed by JWT using the secret that Workhorse and Rails share. This way, rails can verify the signature and if someone has been tampering them, it will reject the request. Note that parameters are merely signed. In particular, they are not encrypted.

This MR is the rails part of the above modification:

  • It updates multipart.rb so that upload parameters are extracted from the corresponding JWT parameter. The signature is verified while doing so.

🛠 Design choices

  • This change is global for all uploads as we're updating the middleware piece in charge of handling the uploads.
  • To have an additional safety net, this change is gated behind a feature flag: upload_middleware_jwt_params_handler
    • This allows us to quickly switch between reading the JWT parameter or reading the params in the request.
    • The corresponding tracking issue is here: #233895 (closed)
  • To increase the confidence in this change, we choose to implement features test for 8 different uploads scenarios (read more about them here: #214785 (closed)).
    • Those tests were added in a prior MR: !39941 (merged)
    • Having feature tests gives us the chance to properly test the whole chain on file uploads as an embedded workhorse server runs during those tests. We will use these to properly test the whole request 🏓 between Workhorse and Rails (requests from (2) to (5) in the schema above).
    • In addition, running workhorse within the version set in GITLAB_WORKHORSE_VERSION can prevent some 🤦 moments such as this one: #225259 (comment 384929110).
  • Note that this MR is quite big (~700 lines changed) but the bulk of the changes are the changes/additions on the test side.

⚙ Implementation

  • You can see that multipart.rb has an internal class (called Handler) to handle the request.
  • To ease this refactoring, we introduced HandlerForJWTParams which inherits directly from Handler. This way HandlerForJWTParams only have to override the interesting methods.
    • This is also to ease the complexity of the feature flag removal MR: we will just need to replace Handler functions by HandlerForJWTParams overrides. 🚀
  • At runtime, given the state of the feature flag, the proper class is selected

🔬 Tests

  • multipart_spec.rb has been renamed to multipart_with_handler_spec.rb
  • multipart_with_handler_for_jwt_params_spec.rb has been created. It's almost an exact copy of multipart_with_handler_spec with additional tests for the JWT signed parameter.
    • Why duplicate the test code instead of using shared examples. Both handlers have a slightly different behavior and those differences are not easy to parameterize in shared examples.
    • In the examples, we use #expect_next_instance_of with the Handler class. So we would have to pass the handler class to the shared examples.
    • In short, having shared examples would force us to support different parameters to configure them properly. This could be confusing.
    • To not add more confusion on top of the existing complex spec, we choose to duplicate the specs.
      • Notice, that this will help us for the feature flag removal MR: just delete the multipart_with_handler_spec.rb file and rename multipart_with_handler_for_jwt_params_spec.rb to multipart_spec.rb. 🚀
  • The above approach is also applied to spec/lib/gitlab/middleware/multipart/handler_for_jwt_params_spec.rb

🧪 Manual Tests

In addition to the automatic feature tests, we conducted some manual tests. Note that the following object storage conditions have been tested:

With the ⛳ feature flag (:upload_middleware_jwt_params_handler) disabled:

Test Object Storage off Object Storage on / Direct Upload off Object Storage on / Direct Upload on
nuget package ✅ ❌ (unrelated see #209074) ✅
maven package ✅ ❌ (unrelated see #209074) ✅
graphql ✅ ✅ ✅
CI artifact ✅ ✅ ✅
user avatar ✅ ✅ ✅
group import ✅ ✅ ✅
project import ✅ ✅ ✅
git LFS ✅ ✅ ✅

With the ⛳ feature flag (:upload_middleware_jwt_params_handler) enabled:

Test Object Storage off Object Storage on / Direct Upload off Object Storage on / Direct Upload on
nuget package ✅ ❌ (unrelated see #209074) ✅
maven package ✅ ❌ (unrelated see #209074) ✅
graphql ✅ ✅ ✅
CI artifact ✅ ✅ ✅
user avatar ✅ ✅ ✅
group import ✅ ✅ ✅
project import ✅ ✅ ✅
git LFS ✅ ✅ ✅

Please note #209074 make some tests fail under specific conditions. The failure is unrelated with the changes of this MR.

🔮 Possible follow ups

  • Running feature tests with an inline workhorse is a good step but for file uploads there is a missing piece. This tests run under the condition object storage disabled and this is not the configuration of gitlab.com. We could improve the situation by having a new rspec type :object_storage that would start a minio server. Being a go project, I don't see any complex obstacle here as the setup would be similar to what we have for the embedded workhorse server.
    • The above would give use the flexibility to run the upload feature tests under two contexts: with object storage disabled and with object storage enabled. This would increase further our confidence in the upload middleware code.

Screenshots

Not available.

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by David Fernandez

Merge request reports