rajanadar / VaultSharp

A comprehensive cross-platform .NET Library for HashiCorp's Vault, a secret management tool
http://rajanadar.github.io/VaultSharp
Apache License 2.0
493 stars 134 forks source link

Wrong usage of Lazy for token publication #345

Closed ivanprosh closed 2 months ago

ivanprosh commented 8 months ago

Code block from Polymath

internal void SetVaultTokenDelegate()
{
    if (_authMethodLoginProvider != null)
    {
        _lazyVaultToken = new Lazy<Task<string>>(_authMethodLoginProvider.GetVaultTokenAsync, LazyThreadSafetyMode.PublicationOnly);
    }
}

The problem here is using Lazy with Task object. In that case if any errors occur inside GetVaultTokenAsync (e.g network exceptions, VaultApiException) then Task will be created anyway, but with Exception as result. Why it's bad? It's bad because we can't just retry request (Task result with exception is cached), so the only way is to recreate whole client.

Sample Code Snippet It's easy to reproduce with custom HttpMessageHandler (we checked with Mock)

public interface IHttpMessageHandler
{
    Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken);
}

var mockMessageHandler = new Mock<HttpMessageHandler>();
mockMessageHandler.Protected().As<IHttpMessageHandler>()
            .SetupSequence(p => p.SendAsync(It.IsAny<HttpRequestMessage>(),
                It.IsAny<CancellationToken>()))
            .ReturnsAsync(new HttpResponseMessage((HttpStatusCode)500))
            .ReturnsAsync(new HttpResponseMessage((HttpStatusCode)501))
            .ReturnsAsync(new HttpResponseMessage((HttpStatusCode)503));

var settings = new VaultClientSettings(connectionOptions.Url, connectionOptions.Authentication.GetMethod())
{
    MyHttpClientProviderFunc = handler => new HttpClient(customHandler)
};
var client = new VaultSharp.VaultClient(settings);
client.V1.Secrets.KeyValue.V2.ReadSecretAsync<TSecret>(...)
client.V1.Secrets.KeyValue.V2.ReadSecretAsync<TSecret>(...)
client.V1.Secrets.KeyValue.V2.ReadSecretAsync<TSecret>(...)

The first exception (500) is cached.

VaultSharp Version 1.13.0.1

rajanadar commented 2 months ago

@ivanprosh it was not meant for continuous errors, only the first one, and have the clients re-initialize, since cred failure is a big deal.