Added support for Trusted Signing

[dlemstra]

This PR adds support for Trusted Signing that was requested in #683 with the help of the Azure.CodeSigning.Sdk library.

[clairernovotny]

Do we want to consider a common base of common parameters across providers? In this case, Key Vault and Trusted Signing use these same values. There's clearly a balance between keeping providers separate vs duplicating code/functionality/parameters.

Another thought is whether Auth should be a separate set of parameters/provider? Entra would be one in-box option (using Azure.Identity) that both KV and TS providers use. Certificate store doesn't need auth so that's easy. Potentially another provider could use Entra auth or provide another auth mechanism that's specific to it.

Should we factor out auth into its own thing?

[dlemstra]

We could move this out and share it between these two. But that would mean we would need to pass in the Option objects to that method to get proper error reporting?

[clairernovotny]

Why would we need to pass option objects around? I was thinking of a new auth entity/abstraction that handles these? Ultimately both TS and KV use Azure.Identity with either a DefaultAzureCredential or TokenCredential, so I imagine there's a good way to do this>

[dlemstra]

Maybe something like this:

internal sealed class AzureAuthOptions
{
    internal Option<bool> ManagedIdentityOption { get; } = new(["-azm", "--azure-managed-identity"], getDefaultValue: () => false, Resources.ManagedIdentityOptionDescription);
    internal Option<string?> TenantIdOption { get; } = new(["-azt", "--azure-tenant-id"], Resources.TenantIdOptionDescription);
    internal Option<string?> ClientIdOption { get; } = new(["-azi", "--azure-client-id"], Resources.ClientIdOptionDescription);
    internal Option<string?> ClientSecretOption { get; } = new(["-azs", "--azure-client-secret"], Resources.ClientSecretOptionDescription);

    internal TokenCredential CreateTokenCredential(InvocationContext context)
    {
       // The logic that is now duplicated in both commands.
    }
}

[dlemstra]

I have update the PR to include these changes. Please let me know what you think about this approach.

I am wondering if we should change the --azure-managed-identity option to something like --azure-auth-type and for now have the values default or client-secret so we can extend this in the future? I also wonder if we should make default the default so people would consider using federated credentials / managed identity in their pipeline instead of a client-secret.

In a followup PR we could add more options and settings for other auth types. I also think we should set ExcludeInteractiveBrowserCredential in the DefaultAzureCredentialOptions to prevent a timeout inside an Github Action or AzureDevOps pipeline and make that an opt-in option instead? Maybe we could add an option for this --interactive false and default this to true and set some options differently depending on whether it is set to true or false. But I think we can add extra functionality and options after the merge of this?

[dlemstra]

I even wonder if we still need the --azure-credential option. What if leaving it out would always create a DefaultAzureCredential but we keep --azure-tenant-id, --azure-client-id, --azure-client-secret. When those options are specified we return a ClientSecretCredential instead so people who use a client-id/client-secret will only need to rename their arguments. We could even keep the old --azure-key-vault-* names and then report a warning they have been renamed and should no longer be used. And we could also add options to specify --exclude-environment-credential and the rest of those exclude options. We could then make --exclude-interactive-browser-credential default to false to make sure users don't get a "browser interaction" in their CI. I think this approach would make things a lot easier?

[joelverhagen]

Most of the options discussed here appear to be a matter of taste (IMO). I think the only "controversial" decisions are:

• Do we support client secret at all? My opinion: no. They can use Az CLI client secret auth (az login --password) + DefaultAzureCredential to workaround.
• Do we default interactive or not? My opinion (and appearing consensus here): no
• Do we have breaking changes with the existing --azure-key-vault-* names? My opinion (pretty worthless, I don't have the context): make the breaking change, we are prerelease on the Sign CLI. Warning seems like a nice thing too but is extra work.

Other options like whether to have explicit Managed Identity support or whether to opt-in or opt-out modes of DefaultAzureCredential seem like we can make a good guess (choose something that is consistent and looks like enough) and listen to customers.

The behavior of DefaultAzureCredential can be customized with ExcludeXXXCredential options to opt out of specific unwanted strategies.

I am not a huge can of this personally since, if you know exactly what credential you want, you need to keep playing whack-a-mole with the Exclude* flags as Azure.Identity adds more modes to DefaultAzureCredential. I think the idea of @dlemstra to have something like --azure-auth-type allows us to add environment-credential or visual-studio types.

Said another way, I think there are two main categories of users (if I were to guess): "embrace the magic" (use DefaultAzureCredential with default options) or "I know what I'm doing" (use a specific TokenCredential implementation not DefaultAzureCredential). The --exclude or opt-out pattern is somewhere in between those two camps of users which feels awkward to me.

Whether or not to have --azure-managed-identity (or the equivalent --azure-auth-type managed-identity idea) is guesswork for me. I think it's not worth it since it wouldn't be useful in GitHub Actions where I hear the most users asking for it (I am not plugged in like Damon and Claire though to feedback). Do we really expect people to be signing from Azure compute directly? This seems more like a CI or devbox tool where DefaultAzureCredential (or an explicit --azure-auth-type interactive-browser) would be more useful.

[dlemstra]

I can agree with Joel their answers to the questions that we need to answer. We would then only need a new --azure-auth-type option and allow an explicit opt-in to one of the types (e.g. EnvironmentCredential, ManagedIdentityCredential) or use the default value DefaultAzureCredential. We should probably also allow specifying the rest of the DefaultAzureCredentialOptions. I am not sure if we would want to do that now or at this at a later moment? Maybe adding just --azure-tenant-id to set DefaultAzureCredentialOptions.TenantId would be sufficient for now?

This would be a breaking change for all the users but as Joel said we should be allowed to do that in a prerelease? I do think there should be some really good release notes that explain these changes and also explain why these changes were made?

While writing this reply I am also wondering if --azure-credential would be a better name than --azure-auth-type?

[dtivel]

I think we should take up further changes to Azure authentications in a separate issue/PR with a succinct proposal of the changes.

I love the idea of removing all existing options and Azure Key Vault and Trusted Signing signature providers using DefaultAzureCredential implicitly by default. Also, having an option that enables a user to opt in to a specific authentication strategy instead of the trial-and-error sequence of authentication strategies that DefaultAzureCredential uses.

[dlemstra]

Sounds like a good idea to discuss this in a separate PR. I have a proof of concept ready so I can create a PR for that once this is merged. I do think we should make DefaultAzureCredential the default instead and that would mean we can make the -kvm option obsolete. We will use ClientSecretCredential when TenantIdOption, ClientIdOption and ClientSecretOption are specified otherwise we default to DefaultAzureCredential. I have updated the PR with this proposal.

[dlemstra]

The build is failing because if this error:

src\Sign.SignatureProviders.TrustedSigning\Sign.SignatureProviders.TrustedSigning.csproj(0,0): error NU1101: (NETCORE_ENGINEERING_TELEMETRY=Restore) Unable to find package Azure.CodeSigning.Sdk. No packages exist with this id in source(s): dotnet-8, dotnet-eng, dotnet-libraries, dotnet-public, dotnet-tools

I have this package in my local cache and that is why "it works on my machine". Which source should I add to make this work?

[Jaxelr]

@dlemstra Isn't the package publicly available in nuget? https://www.nuget.org/packages/Azure.CodeSigning.Sdk

[dlemstra]

@dlemstra Isn't the package publicly available in nuget? https://www.nuget.org/packages/Azure.CodeSigning.Sdk

The nuget.org feed is not included in the NuGet.Config file of this project. Wondering if I can add it?

[dtivel]

@dlemstra Isn't the package publicly available in nuget? https://www.nuget.org/packages/Azure.CodeSigning.Sdk

The nuget.org feed is not included in the NuGet.Config file of this project. Wondering if I can add it?

No, see https://github.com/dotnet/arcade/blob/main/Documentation/MirroringPackages.md. I followed those directions and mirrored the package.

I'm reviewing the rest of your PR.

[joelverhagen]

I think some of these descriptions mention Key Vault but these don't make sense when the options are used in the TrustedSigningCommand

[dlemstra]

Thanks for finding that. I will update the documentation once it is clear that this new AzureAuthOptions class is a move in the right direction.

[dlemstra]

I have updated the documentation.