davidlievrouw / httpmessagesigning Goto Github PK

The v12 of the Internet-Draft is expired.
It is followed by the new Internet-Draft.

Although it is mentioned that the new spec is compatible, we should make sure.

When there are new scenarios or examples, include them in the automated tests.

Since v5.3.0, some KeyId values are prohibited, when creating a Client. This was needed to avoid having a Client in MongoDB with the reserved id _version.

First of all, this limitation only makes sense in the context of MongoDB. We should move that restriction there, and not have it as a general restriction for all implementors.

Secondly, when getting a Client from Mongo with the id _version, it returns an instance, but no property values are filled in. So it returns an invalid instance. This should be avoided. More specifically, the MongoClientStore should return null when requesting a client with a prohibited id.

Setting EnableNonce = false in the SigningSetting does not disable the nonce.

I think the issue here is that the field EnableNonce gets not copied in the Clone method.

According to the RFC (https://tools.ietf.org/html/draft-cavage-http-signatures-12#section-2.3), the request-target should contain the request parameters, which are excluded by the library in the Extensions.HttpRequestMessage class (https://github.com/DavidLievrouw/HttpMessageSigning/blob/90bf3c77818855c092063af6ac363fa3e98fa9d7/src/HttpMessageSigning/Extensions.HttpRequestMessage.cs).

I believe this should be the fix:
var requestForSigning = new HttpRequestForSigning { Method = httpRequestMessage.Method, RequestUri = httpRequestMessage.RequestUri.IsAbsoluteUri ? httpRequestMessage.RequestUri.PathAndQuery.UrlDecode() : httpRequestMessage.RequestUri.OriginalString.UrlDecode() };

When specifying the following connection string:

mongodb://john.doe:p@ssw0rd@localhost:27017/Authentication?authSource=admin

Every database operation is performed on the admin database, not on the Authentication database.

A new version of the Internet Draft has been published.

Noticeable changes, impacting this library:

• The (created), (expires) and (request-target) pseudo-headers have been replaced by specialty fields @request-target and @signature-params.
  • The @signature-params specialty field should be part of the composed signature string. This includes the values of the legacy (created) and (expires) pseudo-headers.
• The signature used to be added to the Authorization-header of the request message. In the new spec, a combination of Signature and Signature-Input headers are used instead. This will allow for multiple signatures in the future.
• HTTP header fields must use LOWERCASED field names, not case-insensitive ones.
• Simplify combination of encryption and hashing algorithms to those from the draft, and specify them in the alg field:
  • rsa-pss-sha512
  • rsa-v1_5-sha256
  • hmac-sha256
  • ecdsa-p256-sha256

As you can see, major breaking changes here.
We will need to provide an upgrade path for existing consumers, especially concerning the enc/hash algorithm combinations.

Other changes will be handled in separate issues:

• #28 Support for Dictionary Structured Field Members.
• #29 Support for List prefixes.
• #30 Support for Multiple signatures in a single request.

We now target:

• .NET Standard 2.0: Core, Signing, Verification and MongoDb packages.
• .NET Core 2.2: ASP.NET Core package
• .NET Core 3.1: ASP.NET Core package
• .NET Framework 4.7.2: Owin package

Investigate if we can target a lower .NET Standard version.
Add more specific target frameworks, as specified in the NuGet guidelines.

This change might require a major version bump, because inner dependencies might change.

We now perform DI registrations using extension methods for IServiceCollection. A common practice is to provide a "builder" (e.g. IHttpMessageSigningVerificationBuilder and IHttpMessageSigningBuilder).

This makes it cleaner and more intuitive to perform DI registrations:

services
    .AddHttpMessageSigning()
    .UseKeyId(...)
    .UseSigningSettings(...);

services
    .AddHttpMessageSignatureVerification()
    .UseInMemoryClientStore()
    .UseMongoNonceStore(provider => new MongoDbNonceStoreSettings {
        ConnectionString = "mongodb://localhost:27017/HttpMessageSigningDb",
        CollectionName = "client_nonces"
    })
    .UseClient(...)
    .UseClient(...);

Because we will change the return type of AddHttpMessageSigning and AddHttpMessageSignatureVerification, this is considered to be a breaking change.

We now support InMemory and MongoDB implementations for IClientStore. To make the verification library more accessible, and lower the threshold for users, we should provide a SqlServer-backed IClientStore out-of-the-box.

The .DecorateWithCaching-method adds a decorator with caching. That cache timeout is configurable.

If you set the cache timeout to TimeSpan.Zero, Client caching is disabled.

However, when calling .Register, that cache is not invalidated. E.g. when updating a SignatureAlgorithm for a client, it will take the [cacheExpiration] amount of time before that change is reflected upon authentication.

Also, for implementers of a custom IClientStore, it could be useful to have the ability to invalidate the cache.

• Invalidate the cache when registering a new client.
• Allow invalidating the cache by implementers of a client store.

Also, make sure that you Dispose an evicted client.

The code samples in the wiki are often incorrect. Bring them up-to-date and fix errors, if there are any.

When setting MongoDbClientStoreSettings.ClientCacheEntryExpiration to TimeSpan.Zero, in order to disable Client caching, registering a new Client is impossible.

The spec now includes Dictionary Structured Field Members. We need to canonicalize them correctly.

In v5.0.1, the escaping is used following the rules from RFC 3986. To facilitate compatibility with other libraries, this behavior should be configurable.

Suggestion is to add property SigningSettings.RequestTargetEscaping with the following options:

• RFC 3986 (default)
• RFC 2396
• OriginalString
• Unescaped

Use this setting in the extension method ToHttpRequestForSigning.

This should also be an option for a verification Client. We can add the same property there, and use it in ToHttpRequestForVerification.

The hotfix 4.1.1 caused a certain amount performance loss during both signing and verifying, because we are not able to reuse hash algorithms during concurrent requests. We should find a way to gain some/all of it back.

Some ideas:

• Hash/HMAC algorithm pools
  • Lease an algorithm per thread, return to pool when the signing/verifying is finished
• Dispose used hash/HMAC algorithms in the background
• Profile and find performance elsewhere

This is the signature of the OnSigningStringComposed event:

public Func<HttpRequestMessage, string, Task> OnSigningStringComposed { get; set; } = (requestToSign, signingString) => Task.CompletedTask;

When modifying the signingString value in the event handler, this change gets lost. This argument needs to be passed by ref.

https://tools.ietf.org/html/draft-ietf-httpbis-message-signatures-04#section-2.1 seems to indicate that headers included in the signature should be listed as comma separate list.

We are able to create signatures that do not list nonce as a required header in the headers= property.

Is this a bug in the library? Since it requires nonce to be a part of the signature, shouldn't it also be a part of the headers= property? Or am I misunderstanding something?

At the moment the library does not allow signatures with (created) and (expires) headers (see

In the current draft those headers are recommended so I think support for those is very important.

Add target frameworks for net5.0 in the solution.
Run benchmarks and validate that the behavior is the same.

The spec now includes List prefixes. We need to canonicalize them correctly.

To use an X509Certificate2 object for a signature algorithm, for signing, you need to declare it as Exportable.

new X509Certificate2(
  "mycert.pfx",
  "pwd",
  X509KeyStorageFlags.Exportable)

This is actually not needed, if we would call the correct constructor internally. It's just a wiring problem.

You should be able to use an X509Certificate2 object for signing, as follows:

new X509Certificate2(
  "mycert.pfx",
  "pwd")

When using MongoDB.Driver v2.19.0, serialization errors occur.

MongoDB.Driver.Linq.ExpressionNotSupportedException: Expression not supported: Convert(r.Id, KeyId) because conversion to Dalion.HttpMessageSigning.KeyId is not supported.
   at MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggregationExpressionTranslators.ConvertExpressionToAggregationExpressionTranslator.Translate(TranslationContext context, UnaryExpression expression)
   at MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggregationExpressionTranslators.UnaryExpressionToAggregationExpressionTranslator.Translate(TranslationContext context, UnaryExpression expression)
   at MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggregationExpressionTranslators.ExpressionToAggregationExpressionTranslator.Translate(TranslationContext context, Expression expression)
   at MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggregationExpressionTranslators.BinaryExpressionToAggregationExpressionTranslator.Translate(TranslationContext context, BinaryExpression expression)
   at MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggregationExpressionTranslators.ExpressionToAggregationExpressionTranslator.Translate(TranslationContext context, Expression expression)
   at MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToFilterTranslators.ExpressionToFilterTranslator.TranslateUsingAggregationOperators(TranslationContext context, Expression expression)
   at MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToFilterTranslators.ExpressionToFilterTranslator.Translate(TranslationContext context, Expression expression, Boolean exprOk)
   at MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToFilterTranslators.ExpressionToFilterTranslator.TranslateLambda(TranslationContext context, LambdaExpression lambdaExpression, IBsonSerializer parameterSerializer, Boolean asRoot)
   at MongoDB.Driver.Linq.Linq3Implementation.LinqProviderAdapterV3.TranslateExpressionToFilter[TDocument](Expression`1 expression, IBsonSerializer`1 documentSerializer, IBsonSerializerRegistry serializerRegistry)
   at MongoDB.Driver.ExpressionFilterDefinition`1.Render(IBsonSerializer`1 documentSerializer, IBsonSerializerRegistry serializerRegistry, LinqProvider linqProvider)
   at MongoDB.Driver.MongoCollectionImpl`1.CreateFindOperation[TProjection](FilterDefinition`1 filter, FindOptions`2 options)
   at MongoDB.Driver.MongoCollectionImpl`1.FindAsync[TProjection](IClientSessionHandle session, FilterDefinition`1 filter, FindOptions`2 options, CancellationToken cancellationToken)
   at MongoDB.Driver.MongoCollectionImpl`1.<>c__DisplayClass48_0`1.<FindAsync>b__0(IClientSessionHandle session)
   at MongoDB.Driver.MongoCollectionImpl`1.UsingImplicitSessionAsync[TResult](Func`2 funcAsync, CancellationToken cancellationToken)
   at Dalion.HttpMessageSigning.Verification.MongoDb.MongoDbClientStore.Get(KeyId clientId)
   at Dalion.HttpMessageSigning.Verification.CachingClientStore.Get(KeyId clientId)

I suspect that the implicit cast from KeyId to string no longer works.

Pass a simple string to the FilterBuilder instead.

The current version of the draft specification (https://tools.ietf.org/html/draft-cavage-http-signatures-12) enforces setting the algorithm parameter to "hs2019" effectively hiding the real used algorithm. All other options for the algorithm parameter are deprecated in Appendix E.2.

This library should support setting the "hs2019". Note that this also changes the semantics of the draft at all positions where they explicitly state rules based on the algorithm parameter.

Furthermore for verification the library should support to provide the verifier with the real used algorithm based on the keyId value (i.e. some sort of callback) so that it can use this algorithm for verification if algorithm=hs2019.

The spec talks about Multiple signatures in a single request.

This is particularly useful in reverse-proxy scenarios, according to the author.

The official name, as used here is MongoDB, not MongoDb.

• Update package, namespaces and source file names.
• Make sure that we don't break the NuGet package, published on nuget.org.

Is it worth the risk?
Should we publish a new package instead, referring the old one to the new one?

Add targets for netstandard2.1.

When an invalid client is specified in the authorization header, an exception is thrown that is caught by the RequestSignatureVerifier. A verification failure is then returned to the client.

This should not be an Exception. It's a known code path. Handle it with a branch statement instead of an exception. Quite a big performance gain for failed verifications, imo.

We should define the nuspec settings in the csproj files, instead of having separate nuspec files to maintain.

We now support InMemory, SqlServer and MongoDB implementations for IClientStore. To make the verification library more accessible, and lower the threshold for users, we should provide a FileSystem-backed IClientStore out-of-the-box.

Currently, the only way to instantiate an IRequestSignerFactory is by using DI (after reading the wiki). I also noticed the classes are marked as internal which prevents my from instantiating them manually.

I prefer not to introduce DI frameworks in my application for one library.

Is this something you would consider changing in future versions?

The constructor of the Client class has more and more arguments, as features are being added, It's getting convoluted and feels like a code smell.

Create a factory or factory methods to more easily create Client instances, specifying only what deviates from the default.

Client.Create and a bunch of overloads.

There should be an additional (small) fix. Why is the requestUri URL decoded? It's not necessary, if the URL is encoded it should be taken this way otherwise the verification process fails.

So this is the change:
var requestForSigning = new HttpRequestForSigning { Method = httpRequestMessage.Method, RequestUri = httpRequestMessage.RequestUri.IsAbsoluteUri ? httpRequestMessage.RequestUri.PathAndQuery : httpRequestMessage.RequestUri.OriginalString.Split('#')[0] };

Originally posted by @dieterde in #10 (comment)

Currently this library produces authentication headers with the scheme SignedHttpRequest, but the draft spec specifies in section 3 (https://tools.ietf.org/html/draft-cavage-http-signatures-12#section-3) that the scheme should be Signature.

I propose either the option to change the scheme name or/and to default it to Signature.

When using HMAC, the shared key is stored as clear-text in MongoDb. Obviously bad.
The shared keys should be encrypted in the data store.

Hashing is not an option, because we need to be able to restore the full original value server-side, for verification.

Currently, we can register a custom IAuthenticationHeaderExtractor for cases where the signature is not in the Authorization header.

Unfortunately, the SignedRequestAuthenticationHandler is currently hard-coded to expect an Authorization header explicitly - even though the actual verification uses the custom extractor.

Can these pre-conditions (lines 23, 24) be replaced with a call to the IAuthenticationHeaderExtractor instead?

The most simple example of signing and verifying in README and the wiki is actually already rather detailed. We should write down a very simple usage example, avoiding the advanced options, and ASP.NET Core or OWIN middleware.

Added more extension points following #33 from @dacspiir.
We need to document them still. Waiting for feedback...

Remove deprecated SDKs for .NET Core 2.1 and 5.0.
Add support for .NET 6.