Resolve auth TODOs for services called from internal kubernetes api

**UPDATE: Closing this without action, based on thorough discussion in the thread below: #409038 (comment 1375291424) **

Description

See the following TODO regarding:

        # TODO: Add proper authorization check here. Not sure what it should check, we can look at other services
        #       which are called from ee/lib/ee/api/internal/kubernetes.rb for inspiration, e.g.
        #       StarboardVulnerabilityCreateService and StarboardVulnerabilityResolveService which
        #       use :admin_vulnerability

• ee/app/services/remote_development/agent_config/update_service.rb (https://gitlab.com/gitlab-org/gitlab/-/blob/remote_dev/ee/app/services/remote_development/agent_config/update_service.rb#L10-10)
• ee/app/services/remote_development/workspaces/reconcile_service.rb (https://gitlab.com/gitlab-org/gitlab/-/blob/remote_dev/ee/app/services/remote_development/workspaces/reconcile_service.rb#L23)

Note that there is already agent-token-based auth in place at these REST API endpoints at the Grape DSL level, this is just to follow our practices of defense in depth to authorize at the service levels too.

added to epic &10377 (closed)

changed milestone to %16.0

added Category:Remote Development Deliverable backend devopscreate featureaddition groupide quad-planningcomplete-action sectiondev typefeature workflowin review labels

changed the description

assigned to @j.seto

removed quad-planningcomplete-action label

mentioned in issue #403623 (closed)

@j.seto I created this follow up issue to Complete remaining auth TODOs prior to appsec r... (#403623 - closed) with two more TODOs, and assigned to you.

Can you please assign a weight and pick this up as part of your work on Remote Development Beta - Auth (&10377 - closed)?

Thanks!

set weight to 2

added workflowin dev label and removed workflowin review label

changed iteration to Workspaces Apr 24, 2023 - Apr 30, 2023

changed the description

removed iteration Workspaces Apr 24, 2023 - Apr 30, 2023

changed iteration to Workspaces May 1, 2023 - May 7, 2023

mentioned in issue gitlab-org/quality/triage-reports#12229 (closed)

Some questions I'm thinking about:

• Is the user we use to check authorization for an agent token the user that created the agent token, or the user that created the agent? (these are different sometimes? e.g when a different user generates a new token)
• Under what circumstances do we want to prevent ReconcileService from executing?
  • e.g if a user has the authorization to reconcile a workspace revoked, then the workspaces go out of sync with the actual state. Is that ever desirable?
• Similarly does it ever make sense to let agent configuration go out of sync?

Is the user we use to check authorization for an agent token the user that created the agent token, or the user that created the agent? (these are different sometimes? e.g when a different user generates a new token)

What do you mean? Authz check for an agent shouldn't depend on who created the agent or the token if that's what you are asking about.

Right, that's what I was about to say. There's no concept of a user involved - it's the agent which has been registered which provides the authentication scope.

But for our case, I'm not sure what authorization based on the agent we could or should check here?

What actions does the agent perform using those APIs?

What actions does the agent perform using those APIs?

This is the part of the architecture that handles the reconciliation polling loop, to reconcile the workspaces' actual state with the user's specified desired state.

There's much more detail of this process here: https://gitlab.com/gitlab-org/remote-development/gitlab-remote-development-docs/-/blob/main/doc/architecture.md#gitlab-agent-for-kubernetes-remote-development-module-architecture

What I'm struggling to wrap my head around is what "unauthorized" would mean in this scenario.

If an agent is registered and running in a cluster, and has authenticated and connected successfully through the kas proxy, when would we ever want to prevent it from doing its job of successfully completing a reconciliation polling loop?

when would we ever want to prevent it from doing its job of successfully completing a reconciliation polling loop?

The only thing I can think of is if the feature flag or license is disabled, but we are handling that already with the merge of Put backend workspaces logic behind feature flag (#403155 - closed).

Authorization checks are unrelated to flag/license checks, and that's what this issue is about.

It is notable that StarboardVulnerabilityCreateService and StarboardVulnerabilityResolveServicedo checkagent.created_by_user` for authorization, but that might make sense for that feature.

What I'm struggling to wrap my head around is what "unauthorized" would mean in this scenario.

If an agent is registered and running in a cluster, and has authenticated and connected successfully through the kas proxy, when would we ever want to prevent it from doing its job of successfully completing a reconciliation polling loop?

This is where I got stuck too hence my last 2 questions

Does it seem like there's perhaps nothing to add here wrt to authorization? at least at the moment? i.e the task is to just remove the TODOs

It is notable that StarboardVulnerabilityCreateService and StarboardVulnerabilityResolveService do check agent.created_by_user for authorization, but that might make sense for that feature.

Yes, that's specific to that feature. The author is used later to check that the user who created the agent has permission to administer the vulnerability.

https://gitlab.com/gitlab-org/gitlab/-/blob/2f049239d48c23b8b44e020697384c4249505805/ee/app/services/vulnerabilities/create_service_base.rb#L18-18:

    def authorized?
      can?(@author, :admin_vulnerability, @project)
    end

But in our case, this request from the agent is handling potentially any workspace owned by any user.

In other words, we don't have any relationship between the user/project associated with the agent, and the user/project associated with a workspace. They are completely independent, other than the fact that the workspace's agent defines where the workspace is created and runs.

Does it seem like there's perhaps nothing to add here wrt to authorization? at least at the moment? i.e the task is to just remove the TODOs

That's what I'm leaning towards, but I wanted to discuss it with others, and also get @ash2k to weigh in on it.

I don't have anything to add here. I don't see what extra authz would make sense.

But instead of just removing the TODOs, I would change them into a NOTE which links to this discussion, and briefly summarizes why we are not doing authorization for defense in depth in this service method.

@ameyadarshan @fvpotvin Does it seem reasonable to not add any extra authentication here?

@j.seto

From my understanding this seems fine too. The agent only has access to resources it created, and communication with the agent is done through the Gitlab Workspaces Proxy which has authz checks, right?

The agent only has access to resources it created

Correct, an agent only handles reconciliation for the workspaces it is associated with.

and communication with the agent is done through the Gitlab Workspaces Proxy which has authz checks, right?

Yes, there is token based authentication, although I don't know if this would strictly be considered authorization. I think the token validity (i.e. authentication) is all that's really checked.

Here's what I believe is the relevant code:

https://gitlab.com/gitlab-org/gitlab/-/blob/fe2583267014fdd8ab838e0139334cbf19723b84/lib/api/internal/kubernetes.rb#L16-16:

        def authenticate_gitlab_kas_request!
          render_api_error!('KAS JWT authentication invalid', 401) unless Gitlab::Kas.verify_api_request(headers)
        end

ee/lib/ee/api/internal/kubernetes.rb (no link because it's still on the integration branch and will change SHAs with rebases)

                route_setting :authentication, cluster_agent_token_allowed: true
                post '/reconcile', feature_category: :remote_development do
                 ...

OK, thanks everyone for the due diligence on this one! I am going to go ahead and close this out without action.

@ash2k / @bwill , y'all can carry on the separate thread in #409038 (comment 1374303546) below, or pull it to a separate issue.

Continuing (separate thread) from #409038 (comment 1374294498)

It is notable that StarboardVulnerabilityCreateService and StarboardVulnerabilityResolveService do check agent.created_by_user for authorization, but that might make sense for that feature.

@bwill @adamcohen @smtan Could you clarify why the code needs to check the user? If the user becomes suspended/etc, agent might still be used by a customer. Features we (groupenvironments) build don't depend on the agent creator user for authz, I think we should be consistent.

Agent should be viewed as an independent identity, although we lack a way to actually make it an identity. One day we might get Service accounts and use them, but looks like the plan is to put them into GitLab Premium and we might not be able to use them for the agent in that case.

@ash2k I agree that isn't not ideal, but the agent doesn't have an identity and vulnerabilities are required to have an author. The only idea I have to get around that is to use an internal user instead.

added quad-planningcomplete-no-action label

mentioned in commit 358a0286

mentioned in merge request !119350 (merged)

changed the description

closed

mentioned in commit 17982785

mentioned in commit 37ce23db

mentioned in commit 13735b31

mentioned in commit 286d3de7

mentioned in commit 22c86430

mentioned in commit 3282d21c

mentioned in commit bd02ba9b

added rd-workflowdone label