Skip to content

Conversation

@deadlycoconuts
Copy link
Collaborator

@deadlycoconuts deadlycoconuts commented Jan 21, 2025

What this PR does / why we need it:
In the XP plugin manager, the Management Service client is always initialised with a Google client, meaning that it always expects a Google service account's or user's credentials to always be present for authenticating all outgoing requests from the plugin. Failing to configure either of these will cause the plugin manager to fail at start up since the Google client cannot be initialised properly. This PR removes the need of any Google credentials and instead allows the Management Service client to be started up with a default HTTP client.

In a somewhat similar fashion, the XP Treatment Service also attempts to initialise a Google client with the aforementioned credentials (and fails at start up if they aren't configured) if the ManagementService.AuthorizationEnabled config is set to true. This is problematic in two ways, the first is similar to what is described above - if there aren't any Google credentials configured, the application would simply fail at start up, and the second is with the inconsistent naming/usage of the AuthorizationEnabled field. Across all the CaraML products (including the XP Management Service), authorization is handled by a separate enforcer layer that determines the permissions associated with a certain user/request but in the XP Treatment Service, this is taken to mean authentication instead, since the initialisation of the Google client only serves to append identity-specific headers to identify the sender of all outgoing requests. This PR thus also refactors away the ManagementService.AuthorizationEnabled field and instead just attempts to initialise a Google client but if that fails, uses a regular default HTTP client instead.

Which issue(s) this PR fixes:
Fixes #

@deadlycoconuts deadlycoconuts added the bug Something isn't working label Jan 21, 2025
@deadlycoconuts deadlycoconuts self-assigned this Jan 21, 2025
@deadlycoconuts deadlycoconuts marked this pull request as ready for review January 21, 2025 09:48
@vinoth-gojek
Copy link
Collaborator

Hi @deadlycoconuts the changes looks good to me, but I have below question from a system security standpoint.

The googleClient initiates a client with oAuthToken. If so, with this change anyone could access the system without valid authentication?

This MR sounds to me like a MR with security concerns just by reading the description. As you know this better, could you shed some light on what will be the implication of not using googleClient from security perspective.

@mbruner mbruner requested a review from bthari March 3, 2025 03:56
@deadlycoconuts
Copy link
Collaborator Author

Oops sorry for the late reply; I saw your comment but totally forgot about replying before leaving.

The googleClient initiates a client with oAuthToken. If so, with this change anyone could access the system without valid authentication? This MR sounds to me like a MR with security concerns just by reading the description. As you know this better, could you shed some light on what will be the implication of not using googleClient from security perspective.

No I think you might've been confused by where authentication (the process not the credential) actually happens. Setting up a default HTTP client instead of a Google client only means that all outgoing requests from the client do not contain the Google OAuth token in their headers. Whether these requests are allowed to reach their destination or not is dependent on the recipient of the requests (and whatever authentication mechanisms the recipient has up in place). For example, we may have some form of authentication set up on a service mesh (Istio) that performs this authentication (using the OAuth tokens in the headers of each request) before routing them again to their final destination, which in this case, is the XP Management Service. At no point is the process of authentication dependent on the client itself.

In other words, the responsibility of ensuring only authenticated requests get to the destination (and rejecting others), or rather, protecting the security of the destination service, does not lie with the client that sends outgoing requests. Another way to look at it is, yes, the client needs to provide the correct credentials to its requests if they are required but it's not the client's job to reject requests - that responsibility lies with something else downstream.

Copy link

@bthari bthari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologize for the late review, but LGTM from my side 🚀 thank you for handling this!

@vinoth-gojek
Copy link
Collaborator

Ohh @deadlycoconuts got it, I just realise its a client which is used to send http request! Thanks for the detailed answer!

@deadlycoconuts
Copy link
Collaborator Author

Thanks a lot everyone for looking at this! 🚀 Merging this now!

@deadlycoconuts deadlycoconuts merged commit 80442c9 into caraml-dev:main Mar 12, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants