blowdart / idunno.Authentication

A filled with self-loathing implementation of Basic Authentication, and Certificate Authentication to make me feel like a real security person, all for for ASP.NET Core
Apache License 2.0
409 stars 70 forks source link

Documentation typos in idunno.Authentication.SharedKey #81

Closed fpicot-sokube closed 1 year ago

fpicot-sokube commented 2 years ago

I have tried to implement a python client library to access an API secured with idunno.Authentication.SharedKey, and I believe there are 2 typos in the documentation :

In specifying-the-authorization-header, the example given is Authorization="[SharedKey] <Key Identifier>:<Signature>" but should be Authorization="SharedKey <Key Identifier>:<Signature>"

In building-a-signature, it is stated regarding the query parameter that "If a parameter has multiple values the values should be sorted lexicographically and append as a comma separated list". However successful authentication only happens when keeping the values in the same order than in the request.

blowdart commented 2 years ago

First one, definitely.

The second, that's a little odd, in framework they were returned in order, so I'll look at that, and see if I can put them in the right order, I had thought 6.0 did, and that would certainly be a better approach.

blowdart commented 2 years ago

@fpicot-sokube OK, that was messed up.

I can't read documentation, and assumed when it mentioned sorting it include values as well.

So now there's a whole new function, and some actual tests for the sort of values too.

Try grabbing idunno.Authentication.SharedKey.2.3.0-g6b05d831cc.nupkg from the artifacts feed once it publishes, for some weird reason it's not there yet, or grab the artifacts from the build.

Please let me know if it's now behaving as you expected, and then I'll finally get around to doing a full build, sign and publish

fpicot-sokube commented 2 years ago

Thanks for looking into this! Unfortunately I don't have hands on a machine with the server-side of the lib, I was only implementing the client-side to connect to an API using it. However, I'm afraid than by changing the code to align it with the documentation, you will break backwards compatibility and client using an old version won't be able to connect to server using a newer version, without much else than a 401 to diagnose the issue. Wouldn't it be safer to update the documentation to reflect what the code actually does ?

blowdart commented 2 years ago

The problem is the ordering can change as a request passes through a proxy, and you can't guarantee that enumerating it server side remains the same from .net version to .net version, hence the need to sort into a known order.

As for back compat, the shared key authentication piece has never been released, it's never made it outside of development builds, so breaking now is the much better thing to do imo rather than do it once it's publicly released.

blowdart commented 1 year ago

Given you don't control the server, and that I'm ok with breaking the implementation for correctness, rather than updated the docs I'll consider this closed. @fpicot-sokube if the folks on the server side want to reach out rather than update I'll wait for a week before finally moving everything to an actual release.

blowdart commented 1 year ago

Finally published to nuget as v2.3.0