search

SPIE-Worksphere / haystack-csharp

Haystack .net client
Academic Free License v3.0
16 stars 6 forks source link

Scram Authentication #112

Closed kuket15 closed 7 months ago

kuket15 commented 11 months ago

I have not checked the scram protocol in detail, but this line of code, reported also below for simplicity

["data"] = Convert.ToBase64String(Encoding.UTF8.GetBytes(c2_no_proof + ",p= " + clientProof)),

works correctly for one endpoint I'm querying (called A) but not for another (called B), belonging to different companies.

After struggling for a couple of hours I've realized the same line without the space after p= works for B but not for A.

["data"] = Convert.ToBase64String(Encoding.UTF8.GetBytes(c2_no_proof + ",p=" + clientProof)), // no space

Who's compliant to the spec? Is that space supposed to be there?

Breederveld commented 8 months ago

Hi @kuket15, sorry for responding so late, last year was quite hectic and I missed some issue reports. According to RFC7804 and RFC5802 it should be without the space. However, like you mention it will break some systems if we change this now.

I suggest we add a flag that determines this behavior and set it to the current behavior by default to preserve backward compatibility. Do you agree with this approach?

kuket15 commented 8 months ago

Hello @Breederveld, thanks for your reply.

I've actually solved the issue doing the same thing you are suggesting in my fork, since I didn't know what was correct and what was wrong.

In an ideal world I'd force everyone to be compliant with the specs, but I also understand it's not possible at the moment, because who's using the client probably doesn't have access or can't fix the api (like in my scenario). At the same time It's also tricky to give a justification and explain why such flag is there and it does the wrong thing by default.

Now I think it's up to you. There is no good or bad solution.

I'd probably release a major version, with breaking changes in evidence, complaining with the specs, but with the chance to set the "addExtraSpaceFlag" to true.

Breederveld commented 7 months ago

Hi @kuket15, I've discussed this with a number of people and we agree the specs should be adhered to primarily. Therefore I have made a breaking change to the code, adhering to the specs, but allowing legacy behavior to function with a simple modification. This has also been specifically mentioned in the example code. The new package is available with version 3.0.0