Skip to content

Integrate parameter validation for Azure Key Vault client auth#375

Open
AsmCord wants to merge 8 commits intovcsjones:mainfrom
AsmCord:main_adapted
Open

Integrate parameter validation for Azure Key Vault client auth#375
AsmCord wants to merge 8 commits intovcsjones:mainfrom
AsmCord:main_adapted

Conversation

@AsmCord
Copy link

@AsmCord AsmCord commented Mar 2, 2026

New options allows more control which computer can sign and use the codesigning certificate, because it does not depend on the knowledge of the secret in the build pipeline

Integrate the ability to use a dedicated certificate to secure and validate the communication between the caller of the AzureSignTool (typically a build machine) and the Key Vault which might be located elsewhere.

Add new parameter options to control the behavior.

return new AzureKeyVaultMaterializedConfiguration(credential, certificate, keyId);
}

private X509Certificate2 LoadCertificateByThumbprint(string thumbprint, System.Security.Cryptography.X509Certificates.StoreLocation storeLocation)
Copy link
Owner

Choose a reason for hiding this comment

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

Does this compile without warning? I would expect the nullability analyzer to be complaining about returning null from a non-null returning method.

Suggested change
private X509Certificate2 LoadCertificateByThumbprint(string thumbprint, System.Security.Cryptography.X509Certificates.StoreLocation storeLocation)
private X509Certificate2? LoadCertificateByThumbprint(string thumbprint, System.Security.Cryptography.X509Certificates.StoreLocation storeLocation)

Copy link
Author

Choose a reason for hiding this comment

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

I missed the #nullable enable pragma in the file. I adapted the null handling

else if (!string.IsNullOrWhiteSpace(configuration.AzureCertificateThumbprint))
{
string certificateThumbPrint = configuration.AzureCertificateThumbprint;
X509Certificate2 clientCertificate = LoadCertificateByThumbprint(certificateThumbPrint, StoreLocation.CurrentUser);
Copy link
Owner

Choose a reason for hiding this comment

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

Would it make sense to try the LocalMachine store if it couldn't find one in CurrentUser?

Suggested change
X509Certificate2 clientCertificate = LoadCertificateByThumbprint(certificateThumbPrint, StoreLocation.CurrentUser);
X509Certificate2? clientCertificate = LoadCertificateByThumbprint(certificateThumbPrint, StoreLocation.CurrentUser);
clientCertificate ??= LoadCertificateByThumbprint(certificateThumbPrint, StoreLocation.LocalMachine);

Copy link
Author

Choose a reason for hiding this comment

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

I updated the code accordingly

Copy link
Author

Choose a reason for hiding this comment

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

I updated the pull request

}

if (KeyVaultClientId is not null && KeyVaultClientSecret is null)
if (KeyVaultClientId is not null && KeyVaultClientSecret is null && KeyVaultClientAuthCertificate is null)
Copy link
Owner

Choose a reason for hiding this comment

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

It would be nice if we did some validation on the thumbprint here to catch typos. Something like:

static bool IsValidHex(string input)
{
    if (input is not { Length: 40 })
    {
        return false;
    }
    
    foreach (char c in input)
    {
        switch (c)
        {
            case >= 'a' and <= 'f':
            case >= 'A' and <= 'F':
            case >= '0' and <= '9':
                continue;
            default:
                return false;
        }
    }
    
    return true;
}

If it returns false then we should write an error to the context.

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.

3 participants