tnc1997 / azure-app-configuration-emulator

Please note that Emulator for Azure App Configuration is unofficial and not endorsed by Microsoft.
MIT License
3 stars 3 forks source link

refactor: support signatures that exclude host header port #38

Closed paulirwin closed 8 months ago

paulirwin commented 8 months ago

Describe the change If you run the emulator via your IDE or dotnet run, the signature validation fails because the "host" includes a port (i.e. localhost:5000) on the emulator side but it does not include a port on the client side (since via the client SDK, Uri.Host does not return the port component). This causes the signature validation to fail.

This strips the port component from the Host header if it is present, as well as it breaks apart the several operations involved in computing a signature into separate variables to aid in debugging.

Current behavior The signature validation fails if running via dotnet run or via the IDE.

New behavior The signature validation passes if running via dotnet run or via the IDE.

Additional context If you're open to it, I'd love to contribute more work to this project as it would be very valuable to my team!

tnc1997 commented 8 months ago

Hi @paulirwin, thank you very much for your contribution!

Unfortunately, it appears that the proposed changes break the SDK for other languages (e.g. https://learn.microsoft.com/en-us/javascript/api/overview/azure/app-configuration-readme), because they handle the host header as per the specification (including the port number if it is non-default).

My preference would be to patch the .NET SDK in order to align its behaviour with the other languages but I appreciate that this is unlikely although it might be worth opening an issue.

If there isn't a desire to align the behaviour of the SDK across different languages then we could definitely look at putting in a workaround in the emulator to allow for non-default ports.

If you're open to it, I'd love to contribute more work to this project as it would be very valuable to my team!

If you have anything in mind then I would absolutely welcome you to open feature requests / bug reports as issues :smile:

paulirwin commented 8 months ago

Thanks for the response. It does indeed seem to be a bug in the .NET SDK, but I figured it would be best to try to work with it as-is rather than hope for one day getting a patch merged there. Would you be open to a fallback signature approach? First attempt to sign the host header as-is, then if that fails, fall back to stripping the port? If so I can update this PR.

Edit: I've gone ahead and done this given that the PR was unlikely to be merged as-is, let me know what you think.

tnc1997 commented 8 months ago

I figured it would be best to try to work with it as-is rather than hope for one day getting a patch merged there

It would be good to open an issue in the .NET SDK if you're comfortable as that would avoid the need for a workaround here.

Worst case scenario is that the maintainers of the .NET SDK choose to leave it as is and we have to workaround the issue here.

paulirwin commented 8 months ago

I could do that, but I have very low confidence that they will merge it to effectively just support this emulator, as it is not needed for normal usage. I think they would have to consider that a supported scenario.

I also just confirmed that unless you map this to port 80/443, the docker approach from the README does not work either with the .NET SDK, so this is more than just for IDE/dotnetrunning.

Given that the latest changes in this PR retain the default signature algorithm and only fall back to the port-less approach if that fails, it seems like this is a reasonable change to support an existing SDK that is in wide use. I'd appreciate consideration of merging this (or let me know if any additional changes are needed) as it allows it to work without needing default ports or having to make Microsoft change their SDK. Thanks!

paulirwin commented 8 months ago

It looks like the Microsoft team has deprioritized getting the fix out for the SDK, which is fair. My team that wants to use this is currently having to pull my changes in manually. Any way we can consider getting this merged as a temporary fix? I'd be happy to revert it later once the SDK fix is published and stable.