Repository: go-webauthn/webauthn
Pull request #82 (2022-12-06 17:13:51)

fix(challenge): urlsafe base64 encoding
This fixes issue with wrong encoding being used for the challenge. According to the specs[^1] 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.

[^1]: https://www.w3.org/TR/webauthn-2/#dom-collectedclientdata-challenge

Fix: https://github.com/duo-labs/webauthn/issues/128

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: Oh I think I see.. because of the JSON Marshal. 
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.

Patch:
diff --git a/protocol/base64.go b/protocol/base64.go
index 261d40e..99362b2 100644
--- a/protocol/base64.go
+++ b/protocol/base64.go
@@ -11,6 +11,10 @@ import (
 // decoded into a byte slice.
 type URLEncodedBase64 []byte
 
+func (e URLEncodedBase64) String() string {
+	return base64.RawURLEncoding.EncodeToString(e)
+}
+
 // UnmarshalJSON base64 decodes a URL-encoded value, storing the result in the
 // provided byte slice.
 func (e *URLEncodedBase64) UnmarshalJSON(data []byte) error {
diff --git a/protocol/challenge.go b/protocol/challenge.go
index be8e76a..6da9f25 100644
--- a/protocol/challenge.go
+++ b/protocol/challenge.go
@@ -2,26 +2,18 @@ package protocol
 
 import (
 	"crypto/rand"
-	"encoding/base64"
 )
 
 // ChallengeLength - Length of bytes to generate for a challenge
 const ChallengeLength = 32
 
-// Challenge that should be signed and returned by the authenticator
-type Challenge URLEncodedBase64
-
-// Create a new challenge to be sent to the authenticator. The spec recommends using
-// at least 16 bytes with 100 bits of entropy. We use 32 bytes.
-func CreateChallenge() (Challenge, error) {
+// Create a new challenge that should be signed and returned by the authenticator.
+// The spec recommends using at least 16 bytes with 100 bits of entropy. We use 32 bytes.
+func CreateChallenge() (URLEncodedBase64, error) {
 	challenge := make([]byte, ChallengeLength)
 	_, err := rand.Read(challenge)
 	if err != nil {
 		return nil, err
 	}
-	return challenge, nil
-}
-
-func (c Challenge) String() string {
-	return base64.RawURLEncoding.EncodeToString(c)
+	return URLEncodedBase64(challenge), nil
 }
diff --git a/protocol/options.go b/protocol/options.go
index ca70779..c682d22 100644
--- a/protocol/options.go
+++ b/protocol/options.go
@@ -15,7 +15,7 @@ type CredentialAssertion struct {
 // In order to create a Credential via create(), the caller specifies a few parameters in a CredentialCreationOptions object.
 // See §5.4. Options for Credential Creation https://www.w3.org/TR/webauthn/#dictionary-makecredentialoptions
 type PublicKeyCredentialCreationOptions struct {
-	Challenge              Challenge                `json:"challenge"`
+	Challenge              URLEncodedBase64         `json:"challenge"`
 	RelyingParty           RelyingPartyEntity       `json:"rp"`
 	User                   UserEntity               `json:"user"`
 	Parameters             []CredentialParameter    `json:"pubKeyCredParams,omitempty"`
@@ -30,7 +30,7 @@ type PublicKeyCredentialCreationOptions struct {
 // Its challenge member MUST be present, while its other members are OPTIONAL.
 // See §5.5. Options for Assertion Generation https://www.w3.org/TR/webauthn/#assertion-options
 type PublicKeyCredentialRequestOptions struct {
-	Challenge          Challenge                   `json:"challenge"`
+	Challenge          URLEncodedBase64            `json:"challenge"`
 	Timeout            int                         `json:"timeout,omitempty"`
 	RelyingPartyID     string                      `json:"rpId,omitempty"`
 	AllowedCredentials []CredentialDescriptor      `json:"allowCredentials,omitempty"`
diff --git a/webauthn/login.go b/webauthn/login.go
index 5d2dccd..babfb68 100644
--- a/webauthn/login.go
+++ b/webauthn/login.go
@@ -2,7 +2,6 @@ package webauthn
 
 import (
 	"bytes"
-	"encoding/base64"
 	"net/http"
 
 	"github.com/go-webauthn/webauthn/protocol"
@@ -64,7 +63,7 @@ func (webauthn *WebAuthn) beginLogin(userID []byte, allowedCredentials []protoco
 	}
 
 	sessionData := SessionData{
-		Challenge:            base64.RawURLEncoding.EncodeToString(challenge),
+		Challenge:            challenge.String(),
 		UserID:               userID,
 		AllowedCredentialIDs: requestOptions.GetAllowedCredentialIDs(),
 		UserVerification:     requestOptions.UserVerification,
diff --git a/webauthn/registration.go b/webauthn/registration.go
index 5b0c993..4d6aab1 100644
--- a/webauthn/registration.go
+++ b/webauthn/registration.go
@@ -2,7 +2,6 @@ package webauthn
 
 import (
 	"bytes"
-	"encoding/base64"
 	"net/http"
 
 	"github.com/go-webauthn/webauthn/protocol"
@@ -57,7 +56,7 @@ func (webauthn *WebAuthn) BeginRegistration(user User, opts ...RegistrationOptio
 
 	response := protocol.CredentialCreation{Response: creationOptions}
 	newSessionData := SessionData{
-		Challenge:        base64.RawURLEncoding.EncodeToString(challenge),
+		Challenge:        challenge.String(),
 		UserID:           user.WebAuthnID(),
 		UserVerification: creationOptions.AuthenticatorSelection.UserVerification,
 	}


Test patch:
diff --git a/protocol/assertion_test.go b/protocol/assertion_test.go
index 5ff7510..91c952f 100644
--- a/protocol/assertion_test.go
+++ b/protocol/assertion_test.go
@@ -151,7 +151,7 @@ func TestParsedCredentialAssertionData_Verify(t *testing.T) {
 		Raw                       CredentialAssertionResponse
 	}
 	type args struct {
-		storedChallenge    Challenge
+		storedChallenge    URLEncodedBase64
 		relyingPartyID     string
 		relyingPartyOrigin string
 		verifyUser         bool
diff --git a/protocol/attestation_test.go b/protocol/attestation_test.go
index 85dd7eb..8b8159d 100644
--- a/protocol/attestation_test.go
+++ b/protocol/attestation_test.go
@@ -83,7 +83,7 @@ func TestPackedAttestationVerification(t *testing.T) {
 var testAttestationOptions = []string{
 	// Direct Self Attestation with EC256 - MacOS
 	`{"publicKey": {
-		"challenge": "rWiex8xDOPfiCgyFu4BLW6vVOmXKgPwHrlMCgEs9SBA=",
+		"challenge": "rWiex8xDOPfiCgyFu4BLW6vVOmXKgPwHrlMCgEs9SBA",
 		"rp": {
 		"name": "http://localhost:9005",
 		"id": "localhost"
@@ -108,7 +108,7 @@ var testAttestationOptions = []string{
 	}}`,
 	// Direct Attestation with EC256
 	`{"publicKey": {
-		"challenge": "+Ri5NZTzJ8b6mvW3TVScLotEoALfgBa2Bn4YSaIObHc=",
+		"challenge": "-Ri5NZTzJ8b6mvW3TVScLotEoALfgBa2Bn4YSaIObHc",
 		"rp": {
 		"name": "https://webauthn.io",
 		"id": "webauthn.io"
@@ -134,7 +134,7 @@ var testAttestationOptions = []string{
 	// None Attestation with EC256
 	`{
 		"publicKey": {
-		  "challenge": "sVt4ScceMzqFSnfAq8hgLzblvo3fa4/aFVEcIESHIJ0=",
+		  "challenge": "sVt4ScceMzqFSnfAq8hgLzblvo3fa4_aFVEcIESHIJ0",
 		  "rp": {
 			"name": "https://webauthn.io",
 			"id": "webauthn.io"
diff --git a/protocol/challenge_test.go b/protocol/challenge_test.go
index f3ad87d..9044241 100644
--- a/protocol/challenge_test.go
+++ b/protocol/challenge_test.go
@@ -9,12 +9,12 @@ import (
 func TestCreateChallenge(t *testing.T) {
 	tests := []struct {
 		name    string
-		want    Challenge
+		want    URLEncodedBase64
 		wantErr bool
 	}{
 		{
 			"Successfull Challenge Create",
-			Challenge{},
+			URLEncodedBase64{},
 			false,
 		},
 	}
@@ -42,7 +42,7 @@ func TestChallenge_String(t *testing.T) {
 	wantChallenge := base64.RawURLEncoding.EncodeToString(newChallenge)
 	tests := []struct {
 		name string
-		c    Challenge
+		c    URLEncodedBase64
 		want string
 	}{
 		{
diff --git a/protocol/client_test.go b/protocol/client_test.go
index 6e77015..e5039c3 100644
--- a/protocol/client_test.go
+++ b/protocol/client_test.go
@@ -1,19 +1,16 @@
 package protocol
 
 import (
-	"encoding/base64"
 	"net/url"
 	"testing"
 )
 
-func setupCollectedClientData(challenge []byte) *CollectedClientData {
-	ccd := &CollectedClientData{
-		Type:   CreateCeremony,
-		Origin: "example.com",
+func setupCollectedClientData(challenge URLEncodedBase64) *CollectedClientData {
+	return &CollectedClientData{
+		Type:      CreateCeremony,
+		Origin:    "example.com",
+		Challenge: challenge.String(),
 	}
-
-	ccd.Challenge = base64.RawURLEncoding.EncodeToString(challenge)
-	return ccd
 }
 
 func TestVerifyCollectedClientData(t *testing.T) {
@@ -28,7 +25,7 @@ func TestVerifyCollectedClientData(t *testing.T) {
 	originURL, _ := url.Parse(ccd.Origin)
 	err = ccd.Verify(storedChallenge.String(), ccd.Type, FullyQualifiedOrigin(originURL))
 	if err != nil {
-		t.Fatalf("error verifying challenge: expected %#v got %#v", Challenge(ccd.Challenge), storedChallenge)
+		t.Fatalf("error verifying challenge: expected %#v got %#v", ccd.Challenge, storedChallenge)
 	}
 }
 
@@ -42,9 +39,8 @@ func TestVerifyCollectedClientDataIncorrectChallenge(t *testing.T) {
 	if err != nil {
 		t.Fatalf("error creating challenge: %s", err)
 	}
-	storedChallenge := Challenge(bogusChallenge)
-	err = ccd.Verify(storedChallenge.String(), ccd.Type, ccd.Origin)
+	err = ccd.Verify(bogusChallenge.String(), ccd.Type, ccd.Origin)
 	if err == nil {
-		t.Fatalf("error expected but not received. expected %#v got %#v", Challenge(ccd.Challenge), storedChallenge)
+		t.Fatalf("error expected but not received. expected %#v got %#v", ccd.Challenge, bogusChallenge)
 	}
 }
diff --git a/protocol/credential_test.go b/protocol/credential_test.go
index 157286f..5beee5a 100644
--- a/protocol/credential_test.go
+++ b/protocol/credential_test.go
@@ -162,7 +162,7 @@ func TestParsedCredentialCreationData_Verify(t *testing.T) {
 		Raw                       CredentialCreationResponse
 	}
 	type args struct {
-		storedChallenge    Challenge
+		storedChallenge    URLEncodedBase64
 		verifyUser         bool
 		relyingPartyID     string
 		relyingPartyOrigin string
@@ -221,7 +221,7 @@ func TestParsedCredentialCreationData_Verify(t *testing.T) {
 				},
 			},
 			args: args{
-				storedChallenge:    byteChallenge,
+				storedChallenge:    URLEncodedBase64(byteChallenge),
 				verifyUser:         false,
 				relyingPartyID:     `webauthn.io`,
 				relyingPartyOrigin: `https://webauthn.io`,
diff --git a/protocol/options_test.go b/protocol/options_test.go
index e5d0e81..1bbf26c 100644
--- a/protocol/options_test.go
+++ b/protocol/options_test.go
@@ -7,7 +7,7 @@ import (
 
 func TestPublicKeyCredentialRequestOptions_GetAllowedCredentialIDs(t *testing.T) {
 	type fields struct {
-		Challenge          Challenge
+		Challenge          URLEncodedBase64
 		Timeout            int
 		RelyingPartyID     string
 		AllowedCredentials []CredentialDescriptor
@@ -22,7 +22,7 @@ func TestPublicKeyCredentialRequestOptions_GetAllowedCredentialIDs(t *testing.T)
 		{
 			"Correct Credential IDs",
 			fields{
-				Challenge: Challenge([]byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}),
+				Challenge: URLEncodedBase64([]byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}),
 				Timeout:   60,
 				AllowedCredentials: []CredentialDescriptor{
 					{