Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow clientId and tenantId hint when using workload identity #100

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

nilfr
Copy link
Contributor

@nilfr nilfr commented Feb 27, 2024

Workload identity supports the usage of multiple identities in the same pod. This PR attempts to add support for it.

We need to evaluate if we would like to also backport this to the legacyCredentials.

@nilfr nilfr requested a review from a team as a code owner February 27, 2024 10:05
@CLAassistant
Copy link

CLAassistant commented Feb 27, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@nilfr
Copy link
Contributor Author

nilfr commented Mar 5, 2024

@asimpson @bossinc @aangelisc @alyssabull Can we get someone to review this?

Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

Hi @nilfr,

Thank you for the contribution! This looks good to me. Are you also planning on creating a PR for the data sources to ensure this is usable? Tagging @kostrse to review as well.

@aangelisc aangelisc requested a review from kostrse March 5, 2024 15:18
@aangelisc aangelisc added external enhancement New feature or request labels Mar 5, 2024
@nilfr
Copy link
Contributor Author

nilfr commented Mar 6, 2024

@aangelisc As far as I know, this is already usable when using the new format with azureCredentials, unless the datasource does some validation on the jsonData. I am happy to update the documentation if you could point me to the relevant repository.

The question is if this should be supported with the legacyCredentials via the getFromLegacy function?

@kostrse
Copy link
Collaborator

kostrse commented Mar 6, 2024

Did you consider security implications?

My concern that this may give ability to probe other identities in the Kubernetes cluster even if they aren't supposed to be used by Grafana and belong to other services hosted in the same cluster.

For this reason TenatID and ClientID were added to Grafana config (here), so they are not accessible by user.

@nilfr
Copy link
Contributor Author

nilfr commented Mar 7, 2024

@kostrse I did. Security is actually the main driver for this. We have different teams that use the Azure Monitor datasource. We want to manage the datasources for them to restrict the scope and subscriptions that the datasource has access to, so each of them should use a separate ManagedIdentity. Today, this is possible with multiple ServicePrincipals, but not with ManagedIdentities. The issue with using ServicePrincipals is then lifecycle management of the ClientSecret.

Probing for other managed identities would not work since the FederatedIdentityCredential you create is tied to the subject claim of the Grafana ServiceAccount.

@aangelisc
Copy link
Contributor

@nilfr ideally the data sources would need to be updated to provide the client ID and tenant ID of the identity via the UI to make this functionality more accessible (otherwise it will only be usable via provisioned data sources).

@kostrse this functionality looks good to me and I'm happy to add this to the SDK if you see no issues with it.

Signed-off-by: Nicklas Frahm <nilfr@vestas.com>
@nilfr
Copy link
Contributor Author

nilfr commented Mar 18, 2024

@aangelisc I have updated the branch to be up-to-date with main.

I can look into submitting a PR for the UI once this is merged.

Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @nilfr!

@aangelisc
Copy link
Contributor

Apologies for this falling through the cracks! I've merged main to allow me to merge this

@aangelisc aangelisc merged commit c955531 into grafana:main Mar 28, 2024
3 checks passed
@nilfr
Copy link
Contributor Author

nilfr commented Apr 2, 2024

@aangelisc Anything I can do to help get a tag and a release for this, so that we can add this to grafana/grafana?

@aangelisc
Copy link
Contributor

Hi @nilfr! There are a couple things I'm trying to get merged and then I'll cut a new release (hopefully this week).

@aangelisc
Copy link
Contributor

Hi @nilfr, apologies for the delay, I've published a new version of the SDK now 😊 Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request external
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants