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

adding support for service principal-based authentication, and non-default tenants #109

Closed
wants to merge 4 commits into from

Conversation

erincon01
Copy link
Contributor

I have added support to query costs using service principals.

I have added these command options:

    [CommandOption("--tenantId")]
    [Description("The tenant id for authentication. Necessary to access non-default tenants.")]
    [DefaultValue("")]
    public string? TenantId { get; set;}

    [CommandOption("--servicePrincipalId")]
    [Description("The service principal name for Service Principal-based authentication.")]
    [DefaultValue("")]
    public string? ServicePrincipalId { get; set;}

    [CommandOption("--servicePrincipalSecret")]
    [Description("The service principal name secret for Service Principal-based authentication.")]
    [DefaultValue("")]
    public string? ServicePrincipalSecret { get; set;}

that populates the class SecurityCredentials.

public class SecurityCredentials
{
    public SecurityCredentials(string tenantId, string ServicePrincipalId, string ServicePrincipalSecret)
    {
        this.tenantId = tenantId;
        this.ServicePrincipalId = ServicePrincipalId;
        this.ServicePrincipalSecret = ServicePrincipalSecret;
    }
    public string tenantId { get; set; }
    public string ServicePrincipalId { get; set; }
    public string ServicePrincipalSecret { get; set; }
}

It might look many changes, but it is not.
I had to pass the SecurityCredencials object to the RetrieveToken method by parameter.
I am not great in C#, nor in the app, so it might be possible in a different manner.
I had to change the Retrievers parameters, ExecuteCallToCostApi parameter,

Global SecurityCredencials might be easier, but I didn't know how to define it, and I just wanted to add to your good code.

The logic to retrieve the token is simple:

  • if the three values are provided, service principal based authentication.
  • if only temant id, tenant will be used.
  • if no tenanti id is provided, default tenant will be used.

non default tenant id authentication would not be neccesary into the application if you connect using az login --tenant.
But, as the tenant id is necessary for service principal authenthcation, I have added non default tenants.

I've explained the logic in the readme.md too.
Review the wording, as I am not English native.

@erincon01 erincon01 requested a review from mivano as a code owner February 17, 2024 12:21
@mivano
Copy link
Owner

mivano commented Feb 19, 2024

Thanks for the PR, but I think we might be able to do this differently. The current authentication depends on the TokenCredential. Currently, it uses the AzureCliCredentials as the first one (so it will stop after finding a session you setup using az login) or falls back to the other TokenCredentials by using the DefaultAzureCredentials.

The DefaultAzureCredentials contain a bunch of other options, one of them is the EnvironmentCredentials. Which allows you to set the client id, secret and tenant as environment variables. So I think it will already work out of the box, as long as you specify the environment variables (and it cannot find the az cli credentials first).

Another option is to use the az cli to login first. Use the login with a service principle and you have an authenticated connection that the azure cost cli will use to connect to Azure.

At least, that is the theory, as I always use the az login and never tried the other options. :-)

Would this approach also work for you?

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

I do want to say that this option (of specifying credentials) is not obvious and not listed in the readme. The DefaultAzureCredentials has a lot of hidden logic.

@erincon01
Copy link
Contributor Author

The DefaultAzureCredentials contain a bunch of other options, one of them is the EnvironmentCredentials. Which allows you to set the client id, secret and tenant as environment variables. So I think it will already work out of the box, as long as you specify the environment variables (and it cannot find the az cli credentials first).

The SecurityCredential class I have added includes the first part if EnvironmentCredencials. If this can be used, the PR would not be necessary. I tried with az login but I couldn't make it. That's why I added the code. I'll review az login tonight and see how would work.

@erincon01
Copy link
Contributor Author

I do want to say that this option (of specifying credentials) is not obvious and not listed in the readme. The DefaultAzureCredentials has a lot of hidden logic.

I added this to the Readme.md. but as said in previous message, if can be done with az cli, or az login, I'd close the PR. Reason is simplicity of code. I had doubts if adding the security part would add unnecesary logic to your nice app.

"
Non-default tenant authentication
If the Azure account has access to multiple tenants, tenant id must be provided. It tenant id is not provided default tenant id will be used. This can be handle in az login --tenant, or using the --tenantId configuration option in the app:

azure-cost costByResource -s 574385a9-08e9-49fe-91a2-27660d92b8f5 --tenantId xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx -o json
Service principal-based authentication
For service principal based authentication, tenant id, service principal id, and service principal secret must be provided.

azure-cost costByResource -s 574385a9-08e9-49fe-91a2-27660d92b8f5 --tenantId xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx --servicePrincipalId xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxx --servicePrincipalSecret <secret_value> -o json
Authentication order
These are the configuration options for authetication:

Tenant Id: --tenantId
Service principal name: --servicePrincipalId
Service principal secret: --servicePrincipalSecret
The authentication process will follow this path (in order):

If the three values ara provided, service principal-based authentication will be used.
If tenant id is provided, tenant-based authentication will be used.
If no tenant is provided, default tenant-based authentication will be used.
"

@mivano
Copy link
Owner

mivano commented Feb 19, 2024

The ChainedTokenProvider I added favors the AzureCliProvider (to speed up the process), so in that sense overwriting the default first EnvironmentProvider. So if there is already an Azure CLI token, it wont pick up the environment one.

@erincon01
Copy link
Contributor Author

I remove the PR as it is covered already in az login:
az login --service-principal -u -p --tenant

@erincon01 erincon01 closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants