fix(xp-treatment,plugin): Fix treatment service and plugin's method of initiating management service client#89
Conversation
|
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. |
|
Oops sorry for the late reply; I saw your comment but totally forgot about replying before leaving.
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. |
bthari
left a comment
There was a problem hiding this comment.
Apologize for the late review, but LGTM from my side 🚀 thank you for handling this!
|
Ohh @deadlycoconuts got it, I just realise its a client which is used to send http request! Thanks for the detailed answer! |
|
Thanks a lot everyone for looking at this! 🚀 Merging this now! |
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.AuthorizationEnabledconfig is set totrue. 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 theAuthorizationEnabledfield. 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 theManagementService.AuthorizationEnabledfield 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 #