Add HMAC authentication options. #250

JimBobSquarePants · 2022-04-05T14:11:56Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This PR builds upon an idea of @ronaldbarendse to allow adding optional Hash-based Message Authentication Code (HMAC) authentication to image requests. This replaces the previous, flaky sanitation found in ImageSharpMiddlewareOptions.OnParseCommandsAsync and allows authentication of requests against public facing web applications.

Two new properties have been added to ImageSharpMiddlewareOptions

HMACSecretKey a byte[] containing a key used for HMAC hashing
OnComputeHMACAsync a Func<ImageCommandContext, byte[], Task<string>> that contains the default HMAC hashing implementation.

In addition a public HMACUtilities has been implemented allowing hashing at 256, 384, and 512 bits and a CaseHandlingUriBuilder exposed to allow URI generation.

By default this authentication is turned off but can be enabled by setting the HMACSecretKey property to an array of length greater than 0.

Once enabled, the default implementation uses a hash size of 256 bits and creates the hash from an invariant lowercase version of the relative image request uri.

/myimage.jpeg?width=100&v=12345678

Hashes should be appended to the uri in the following format.

&hmac=HASH

where HASH is calculated in the following manner when using the default implementation.

string token= HMACUtilities.ComputeHMACSHA256(uri.ToLowerInvariant(), options.HMACSecretKey);
uri = uri + "&" + HMACUtilities.TokenCommand + "=" + token;

The hmac command key is a constant found at HMACUtilities.TokenCommand.

Token resolution is one of the very first operations performed in the middleware and, as such, authentication failure will result in a ~~401~~ 400 response with no further processing.

JimBobSquarePants · 2022-04-05T14:19:24Z

I need to fix the tests here. I've got tests running multiple separate servers running in parallel hitting the same diskspace

lahma · 2022-04-05T14:27:40Z

src/ImageSharp.Web/Caching/SHA256CacheHash.cs

-                return HashValue(value, length, buffer.AsSpan(0, byteCount));
+                // Allocating a buffer from the pool is ~27% slower than stackalloc so use that for short strings
+                Span<byte> bytes = byteCount <= 128
+                    ? stackalloc byte[byteCount]


There was a Twitter thread recently about using static value for stackalloc being better, like stackalloc[128].AsSpan(requested).

https://twitter.com/EgorBo/status/1508069816275513344?t=zxpY4AQ_9JddgbLKat0Yrw&s=19

Ah yeah, then you slice it. I remember that Tweet. Thanks!

EDIT. I'm not using SkipLocalsInit here (target framework support) so my initial pattern might actually be faster.

Cool, I can't remember if SkipLocalsInit could be just shim for older frameworks via custom attribute and compiler would pick it up. Great work here as always!

No idea tbh! It's really hard to keep track of what is shimmable. Thanks!

codecov · 2022-04-05T14:34:17Z

Codecov Report

Merging #250 (3fde177) into main (3626616) will increase coverage by 0%.
The diff coverage is 84%.

@@         Coverage Diff         @@
##           main   #250   +/-   ##
===================================
  Coverage    85%    85%           
===================================
  Files        74     75    +1     
  Lines      2019   2044   +25     
  Branches    293    297    +4     
===================================
+ Hits       1720   1751   +31     
+ Misses      216    210    -6     
  Partials     83     83

Flag	Coverage Δ
unittests	`85% <84%> (+<1%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...b/Commands/Converters/SimpleCommandConverter{T}.cs	`75% <ø> (ø)`
...harp.Web/Middleware/ImageSharpMiddlewareOptions.cs	`92% <76%> (+3%)`	⬆️
.../ImageSharp.Web/Middleware/ImageSharpMiddleware.cs	`81% <80%> (+4%)`	⬆️
src/ImageSharp.Web/HMACUtilities.cs	`86% <86%> (ø)`
src/ImageSharp.Web/Caching/SHA256CacheHash.cs	`100% <100%> (ø)`
src/ImageSharp.Web/Caching/UriAbsoluteCacheKey.cs	`100% <100%> (ø)`
...p.Web/Caching/UriAbsoluteLowerInvariantCacheKey.cs	`100% <100%> (ø)`
src/ImageSharp.Web/Caching/UriRelativeCacheKey.cs	`100% <100%> (ø)`
...p.Web/Caching/UriRelativeLowerInvariantCacheKey.cs	`100% <100%> (ø)`
src/ImageSharp.Web/CaseHandlingUriBuilder.cs	`76% <100%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3626616...3fde177. Read the comment docs.

ronaldbarendse · 2022-04-05T17:20:45Z

You might want to look into ASP.NET Core Data Protection, so the key configuration/storage/rotation can be handled for you. You can also use different purpose names to support multi-tenancy (so one tenant can't create valid HMACs for other tenants).

I also think returning a 403 Forbidden or otherwise 400 Bad Request is more appropriate then the 401 Unauthorized, as that should include a WWW-Authenticate header to allow reauthentication.

deanmarcussen · 2022-04-05T18:05:39Z

src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

+            {
+                // Compare the passed token to our generated mac.
+                string mac = await this.options.OnComputeHMACAsync(imageCommandContext, secret);
+                if (mac != token)


Just looking quickly, but isn't this going to apply to every request
Even requests not meant for ImageSharp?

In theory looks good, I did something similar for OC some time ago, but processed it in OnParseCommandsAsync (and then @lahma made it faster). Think I quite heavily memory cached the query strings, so we weren't calculating hashes all the time.

I went with HMAC as well, rather than DP.
When using DP, the keys rotate / an encrypted value, may not always be the same hash, which causes issues with browser caching, as the same request, may not look like the same query string anymore, due to a different hash.

So I felt it better to use a fixed key.

We aren't returning an unauthorized, I probably don't see it as that kind of feature - sounds like something for an actual auth handler.

Our approach was just to strip the commands, if the hash was invalid. Which then lets the request be parsed to the next middleware, i.e. static files, so the image is served, but at full size.

Will look more tomorrow when back at keyboard.

Just looking quickly, but isn't this going to apply to every request
Even requests not meant for ImageSharp?

That is a VERY good point, will shift it till after I have a match.

I chose to actually reject the call because I want to avoid all further processing overheads.

JimBobSquarePants · 2022-04-05T22:33:02Z

@ronaldbarendse Still not sure about the response code - good point about the header.

Re the libraries you pointed out. Thanks but I don’t really want to use rotation as a fixed key is easier an suitable for our needs and HMAC isn’t hard to implement. The key itself can be stored wherever, it just needs to be assigned at startup.

I chose app relative generated tokens as default because that makes it easier for users to generate them since most request uri’s are app relative. I can avoid shifting everything to lowercase though as that will avoid an allocation on the front end. If someone wants to use absolute tokens they can simply replace the method in the options.

JimBobSquarePants · 2022-04-06T00:43:45Z

@deanmarcussen I've moved the handling until after we've verified it's a valid image request and now temporarily cache the generated tokens.

@ronaldbarendse I've added the header to the response now. 401 is explicitly designed for auth so will use it. I've also made the token generation case sensitive. This is to avoid users doing imageUrl.ToLowerInvariant() in the calling code when generating tokens. I can't decide whether this is wise though (it shouldn't be an attack vector) so would value your input.

ronaldbarendse · 2022-04-06T09:42:18Z

src/ImageSharp.Web/CaseHandlingUriBuilder.cs

        public enum CaseHandling
        {
+            /// <summary>
+            /// No adjustments to casing are made.


The query string is always converted to lowercase though!

This behaviour describes any alterations to the input value. However, you raise a good point. There's no guarantee that client querystrings will be passed in lowercase. Perhaps I should just default to lower invariant generation after all to save confusion?

I've switched back to lower invariant for the default generation.

ronaldbarendse · 2022-04-06T10:02:51Z

src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

            CommandCollection commands = this.requestParser.ParseRequestCommands(httpContext);

+            // First check for a HMAC token and capture before the command is stripped out.
+            byte[] secret = this.options.HMACSecretKey;


How about fetching the secret using a callback on the options (passing in the HttpContext), so you can support multi-tenancy based on e.g. the domain name?

There's no issue sharing a shared secret across a multitenant install. This is designed to prevent external bad actors not internal resolution of requests.

You would use a custom IImageProvider or authorization middleware that sites in front of this middleware to segregate tenants.

ronaldbarendse · 2022-04-06T10:06:14Z

src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

-                new ImageCommandContext(httpContext, commands, this.commandParser, this.parserCulture));
+            ImageCommandContext imageCommandContext = new(httpContext, commands, this.commandParser, this.parserCulture);
+
+            await this.options.OnParseCommandsAsync.Invoke(imageCommandContext);


Wouldn't altering the commands here cause the HMAC token to become invalid, even though this is completely in your control?

Yup. Will fix.

ronaldbarendse · 2022-04-06T10:11:45Z

src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

+        private static void SetUnauthorized(HttpContext httpContext)
+        {
+            httpContext.Response.Clear();
+            httpContext.Response.Headers.Add("WWW-Authenticate", "HMAC realm=\"" + httpContext.Request.Host + "\"");


Adding this header could prompt the user to to specify authentication details and re-request with an Authentication header (that isn't handled anyway): https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization.

Even though you're validating the request, returning 400 Bad Request does seem to be more correct (and explicitly notes that the client should not repeat the request without modification).

I actually got the same advice from the creator of OpenID-addict. I bow to your superior knowledge.

ronaldbarendse · 2022-04-25T15:26:02Z

src/ImageSharp.Web/Middleware/ImageSharpMiddleware.cs

+                // the token will not match our validating HMAC, however, this would be indicative of an attack and should be treated as such.
+                //
+                // As a rule all image requests should contain valid commands only.
+                hmac = await HMACTokenLru.GetOrAddAsync(token, _ => this.options.OnComputeHMACAsync(imageCommandContext, secret));


@JimBobSquarePants I'm sorry to inform you that caching the computed HMAC based on solely the token completely breaks this implementation, as this allows you to re-use any valid token with any other commands while it's still returned from this cache...

Requesting /myimage.jpeg?width=100&v=12345678&hmac=HASH with a valid hash will put Key = "HASH", Value = "HASH" in the HMACTokenLru cache (having an identical key and value should already indicate the issue);

Requesting /myimage.jpeg?width=200&v=12345678&hmac=HASH will fetch HASH from the cache, which already exists (with a valid hash), so it doesn't need to be re-computed using the current ImageCommandContext (which would return an invalid hash).

It's also possible to poison the cache by collecting valid tokens (from the sites HTML) and requesting images with invalid commands: this will cache the invalid hash, which will also be used for the valid request and thus return 400 Bad Requests to all clients 😢

The computed HMAC should probably be cached using the cache key generated by the ICacheKey implementation (similar to the other LRU caches, but without generating the ICacheHash). This basically removes the need for the OnComputeHMACAsync callback, as you already have a way to get the relevant value to hash and are tightly coupling the computed value on that cache key anyway.

JimBobSquarePants added 3 commits April 5, 2022 19:47

Create HMACUtilities.cs

be62f48

Merge branch 'main' into js/hmac

98b24da

Add HMAC functionality

30e873b

JimBobSquarePants added enhancement api security labels Apr 5, 2022

JimBobSquarePants added this to the v2.0.0 milestone Apr 5, 2022

Try running tests sequentially via base attribute

d41b783

JimBobSquarePants requested review from deanmarcussen and a team April 5, 2022 14:25

lahma reviewed Apr 5, 2022

View reviewed changes

This was referenced Apr 5, 2022

Improve default OnParseCommandsAsync sanitization #248

Closed

Expose ExifOrientationUtilities and ICommandConverter implementations #241

Merged

Update SimpleCommandConverter{T}.cs

50db0b2

deanmarcussen reviewed Apr 5, 2022

View reviewed changes

Only block image requests and cache generated tokens.

2394539

ronaldbarendse suggested changes Apr 6, 2022

View reviewed changes

JimBobSquarePants added 2 commits April 6, 2022 21:12

Fix validation timing

026b776

Use lower invariant again

3fde177

JimBobSquarePants mentioned this pull request Apr 7, 2022

MissingMethodException when using the latest ImageSharp release #251

Closed

4 tasks

JimBobSquarePants merged commit 72b9368 into main Apr 22, 2022

JimBobSquarePants deleted the js/hmac branch April 22, 2022 11:17

ronaldbarendse reviewed Apr 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HMAC authentication options. #250

Add HMAC authentication options. #250

JimBobSquarePants commented Apr 5, 2022 •

edited

Loading

JimBobSquarePants commented Apr 5, 2022 •

edited

Loading

lahma Apr 5, 2022

JimBobSquarePants Apr 5, 2022 •

edited

Loading

lahma Apr 5, 2022

JimBobSquarePants Apr 5, 2022

codecov bot commented Apr 5, 2022 •

edited

Loading

ronaldbarendse commented Apr 5, 2022

deanmarcussen Apr 5, 2022

JimBobSquarePants Apr 5, 2022

JimBobSquarePants commented Apr 5, 2022 •

edited

Loading

JimBobSquarePants commented Apr 6, 2022 •

edited

Loading

ronaldbarendse Apr 6, 2022

JimBobSquarePants Apr 6, 2022 •

edited

Loading

JimBobSquarePants Apr 6, 2022

ronaldbarendse Apr 6, 2022

JimBobSquarePants Apr 6, 2022

ronaldbarendse Apr 6, 2022

JimBobSquarePants Apr 6, 2022

ronaldbarendse Apr 6, 2022

JimBobSquarePants Apr 6, 2022

ronaldbarendse Apr 25, 2022

Add HMAC authentication options. #250

Add HMAC authentication options. #250

Conversation

JimBobSquarePants commented Apr 5, 2022 • edited Loading

Prerequisites

Description

JimBobSquarePants commented Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

JimBobSquarePants Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 5, 2022 • edited Loading

Codecov Report

ronaldbarendse commented Apr 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants commented Apr 5, 2022 • edited Loading

JimBobSquarePants commented Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

JimBobSquarePants Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants commented Apr 5, 2022 •

edited

Loading

JimBobSquarePants commented Apr 5, 2022 •

edited

Loading

JimBobSquarePants Apr 5, 2022 •

edited

Loading

codecov bot commented Apr 5, 2022 •

edited

Loading

JimBobSquarePants commented Apr 5, 2022 •

edited

Loading

JimBobSquarePants commented Apr 6, 2022 •

edited

Loading

JimBobSquarePants Apr 6, 2022 •

edited

Loading