fix(challenge): urlsafe base64 encoding

[matoous]

This fixes issue with wrong encoding being used for the challenge. According to the specs¹ the challenge should be base64 url-safe encoded. Until now, the package used std encoding which uses slightly different set of characters. This was reported in the linked issue and we also encountered the same issue when testing our implementation of webauthn e2e using github.com/descope/virtualwebauthn.

The commit removes the custom type for challenge and instead passes around the challenge url-safe base64 encoded right from the point of creation. Because the string is fully valid it can be safely used in json as well as stored in redis or other possible storage implementations that might be used by the clients of the library.

I tried to keep the tests as close to the original as possible. In some cases that required transforming the challenge back to bytes considering the previously used padding to be part of the bytes and then re-encoding into url-safe base64 string.

Fix: duo-labs/webauthn#128

Footnotes

1. https://www.w3.org/TR/webauthn-2/#dom-collectedclientdata-challenge ↩

[james-d-elliott]

Hey Matouš! Thanks for contributing! I am relatively certain this change concept is correct. Have you checked this with existing credentials and new credentials by chance? I will probably do so if not just to ensure there are no weird issues.

[matoous]

@james-d-elliott I only tested the whole thing using an integration test setup we (@ company) have with https://github.com/descope/virtualwebauthn. We are currently working on webauthn capabilities and don't yet have the front end ready so I sadly don't have better way to test this end to end in real browser scenario. On the other hand, the https://github.com/descope/virtualwebauthn is what complained about the non-user-safe encoding and now our tests pass so I am confident the changes fix the issue. Still, if you please have a way to test this in real scenario, please do 🙏

[matoous]

@james-d-elliott please let me know if I should do any further testing or if any changes are required.

[james-d-elliott]

Hey I definitely will in the next few days, works just been a bit busy. I had a few questions mostly.

[james-d-elliott]

So after taking a closer look I'm not entirely sure what this changes, there is zero reduction in the use of base64.StdEncoding, it's just moved the use of base64.RawURLEncoding to other areas as far as I can see. Can you show a test which fails prior to this change that demonstrates the need for these specific changes or maybe explain what I'm missing?

[james-d-elliott]

Oh I think I see.. because of the JSON Marshal.

[james-d-elliott]

I think this mostly looks good. The reason it's not encoding it correctly is because the Challenge type is defined as a URLEncodedBase64 but the go compiler actually treats it as a []byte (you'll be able to see that Challenge lacks the implementation of the json.Marshaler and json.Unmarshaler interfaces). There are multiple ways to work around this, we can use a struct with the following signature:

type Challenge struct {
	URLEncodedBase64
}

Or we can get rid of it entirely like you have.

Or we can replace all instances of it with URLEncodedBase64 which may be an better change overall? My suggestions show (roughly) how to do this.

[matoous]

@james-d-elliott thanks for suggestions, applied and adjusted. It's way closer now to the initial implementation.

[james-d-elliott]

@james-d-elliott thanks for suggestions, applied and adjusted. It's way closer now to the initial implementation.

That's what I was going for. Thanks a lot! I'll do a final pass today and merge this hopefully.

[james-d-elliott]

LGTM

[tobiaszheller]

Hey, just want to leave some information in case someone updates from v0.5.0 to v.0.6.0, this PR will cause breaking changes on server - client connection if your client expects base64 instead of base64url encoding.
I think it was not mention explicitly in release notes (or maybe it's not needed on pre v1).

Anyway, thanks for keeping it inline with spec now :)

[james-d-elliott]

I'm on the fence as to if this was the breaking change or the change which introduced this bug was the breaking change. I'm more than willing to make special note of it in the release notes but I'm inclined to decline the idea this is breaking (specifically from a semver standpoint) as anytime something we're not doing to spec we will have the same expectations in many instances; but the spec is the contract.

I'll do some research regarding #93 however.