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

Wrong usage of Lazy for token publication #345

Closed
ivanprosh opened this issue Mar 4, 2024 · 1 comment
Closed

Wrong usage of Lazy for token publication #345

ivanprosh opened this issue Mar 4, 2024 · 1 comment
Assignees
Milestone

Comments

@ivanprosh
Copy link

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 rajanadar self-assigned this Sep 8, 2024
@rajanadar rajanadar added this to the 1.17.5 milestone Sep 8, 2024
@rajanadar
Copy link
Owner

@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.

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

No branches or pull requests

2 participants