Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(x/accounts/base): Add passkey support to base account type. #21755

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 214 additions & 4 deletions x/accounts/defaults/base/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,26 @@ package base

import (
"context"
"crypto"
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rand"
"crypto/sha256"
"encoding/base64"
"encoding/hex"
"encoding/json"
"errors"
"testing"

cometcrypto "github.com/cometbft/cometbft/crypto"
gogoproto "github.com/cosmos/gogoproto/proto"
types "github.com/cosmos/gogoproto/types/any"
dcrd_secp256k1 "github.com/decred/dcrd/dcrec/secp256k1/v4"
"github.com/stretchr/testify/require"

"cosmossdk.io/core/store"
"cosmossdk.io/x/accounts/accountstd"
authn "cosmossdk.io/x/accounts/defaults/base/keys/authn"
v1 "cosmossdk.io/x/accounts/defaults/base/v1"
aa_interface_v1 "cosmossdk.io/x/accounts/interfaces/account_abstraction/v1"
"cosmossdk.io/x/tx/signing"
Expand All @@ -22,15 +32,74 @@ import (
"github.com/cosmos/cosmos-sdk/types/tx"
)

type CeremonyType string

type TokenBindingStatus string

type TokenBinding struct {
Status TokenBindingStatus `json:"status"`
Id string `json:"id"`
}

type CollectedClientData struct {
// Type the string "webauthn.create" when creating new credentials,
// and "webauthn.get" when getting an assertion from an existing credential. The
// purpose of this member is to prevent certain types of signature confusion attacks
// (where an attacker substitutes one legitimate signature for another).
Type CeremonyType `json:"type"`
Challenge string `json:"challenge"`
Origin string `json:"origin"`
TokenBinding *TokenBinding `json:"tokenBinding,omitempty"`
// Chromium (Chrome) returns a hint sometimes about how to handle clientDataJSON in a safe manner
Hint string `json:"new_keys_may_be_added_here,omitempty"`
}

func GenerateAuthnKey(t *testing.T) (*ecdsa.PrivateKey, authn.AuthnPubKey) {
t.Helper()
curve := elliptic.P256()
privateKey, err := ecdsa.GenerateKey(curve, rand.Reader)
require.NoError(t, err)
pkBytes := elliptic.MarshalCompressed(curve, privateKey.PublicKey.X, privateKey.PublicKey.Y)
pk := authn.AuthnPubKey{
KeyId: "f482bd153df0ca2ea17d7b1e0178c14ff959628f",
Key: pkBytes,
}

return privateKey, pk
}

func GenerateClientData(t *testing.T, msg []byte) []byte {
t.Helper()
clientData := CollectedClientData{
// Purpose of this member is to prevent certain types of signature confusion attacks
// (Where an attacker substitutes one legitimate signature for another).
Type: "webauthn.create",
Challenge: base64.RawURLEncoding.EncodeToString(msg),
Origin: "https://cosmos.network",
TokenBinding: nil,
Hint: "",
}
clientDataJSON, err := json.Marshal(clientData)
require.NoError(t, err)
return clientDataJSON
}

func setupBaseAccount(t *testing.T, ss store.KVStoreService) Account {
t.Helper()
deps := makeMockDependencies(ss)
handler := directHandler{}

createAccFn := NewAccount("base", signing.NewHandlerMap(handler), WithPubKeyWithValidationFunc(func(pt *secp256k1.PubKey) error {
_, err := dcrd_secp256k1.ParsePubKey(pt.Key)
return err
}))
createAccFn := NewAccount(
"base",
signing.NewHandlerMap(handler),
WithPubKeyWithValidationFunc(func(pt *secp256k1.PubKey) error {
_, err := dcrd_secp256k1.ParsePubKey(pt.Key)
return err
}),
WithPubKeyWithValidationFunc(func(pt *authn.AuthnPubKey) error {
return nil
}),
Comment on lines +99 to +101
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implement validation logic in WithPubKeyWithValidationFunc

The validation function for authn.AuthnPubKey at lines 99-101 currently returns nil without performing any checks. Implementing validation logic ensures that public keys meet expected criteria and helps detect invalid keys during testing.

Apply this diff to add basic validation:

 WithPubKeyWithValidationFunc(func(pt *authn.AuthnPubKey) error {
-    return nil
+    if pt == nil || len(pt.Key) == 0 {
+        return errors.New("invalid AuthnPubKey: key is nil or empty")
+    }
+    // Additional validation logic can be added here
+    return nil
 }),
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
WithPubKeyWithValidationFunc(func(pt *authn.AuthnPubKey) error {
return nil
}),
WithPubKeyWithValidationFunc(func(pt *authn.AuthnPubKey) error {
if pt == nil || len(pt.Key) == 0 {
return errors.New("invalid AuthnPubKey: key is nil or empty")
}
// Additional validation logic can be added here
return nil
}),

)
_, acc, err := createAccFn(deps)
baseAcc := acc.(Account)
require.NoError(t, err)
Expand Down Expand Up @@ -58,6 +127,7 @@ func TestInit(t *testing.T) {
},
false,
},

{
"invalid pubkey",
&v1.MsgInit{
Expand Down Expand Up @@ -267,3 +337,143 @@ func toAnyPb(t *testing.T, pm gogoproto.Message) *codectypes.Any {
require.NoError(t, err)
return pb
}

func TestAuthenticateAuthn(t *testing.T) {
ctx, ss := newMockContext(t)
baseAcc := setupBaseAccount(t, ss)
privKey, pk := GenerateAuthnKey(t)
pkAny, err := codectypes.NewAnyWithValue(&pk)
require.NoError(t, err)
_, err = baseAcc.Init(ctx, &v1.MsgInit{
PubKey: toAnyPb(t, &pk),
})
require.NoError(t, err)

ctx = accountstd.SetSender(ctx, address.Module("accounts"))
require.NoError(t, err)

transaction := tx.Tx{
Body: &tx.TxBody{},
AuthInfo: &tx.AuthInfo{
SignerInfos: []*tx.SignerInfo{
{
PublicKey: pkAny,
ModeInfo: &tx.ModeInfo{
Sum: &tx.ModeInfo_Single_{
Single: &tx.ModeInfo_Single{
Mode: 1,
},
},
},
Sequence: 0,
},
},
},
Signatures: [][]byte{},
}

bodyByte, err := transaction.Body.Marshal()
require.NoError(t, err)
authByte, err := transaction.AuthInfo.Marshal()
require.NoError(t, err)

txDoc := tx.SignDoc{
BodyBytes: bodyByte,
AuthInfoBytes: authByte,
ChainId: "test",
AccountNumber: 1,
}
signBytes, err := txDoc.Marshal()
require.NoError(t, err)

clientDataJson := GenerateClientData(t, signBytes)
clientDataHash := sha256.Sum256(clientDataJson)
authenticatorData := cometcrypto.CRandBytes(37)
payload := append(authenticatorData, clientDataHash[:]...)

h := crypto.SHA256.New()
h.Write(payload)
digest := h.Sum(nil)

sig, err := ecdsa.SignASN1(rand.Reader, privKey, digest)

require.NoError(t, err)

cborSig := authn.Signature{
AuthenticatorData: hex.EncodeToString(authenticatorData),
ClientDataJSON: hex.EncodeToString(clientDataJson),
Signature: hex.EncodeToString(sig),
}

sigBytes, err := json.Marshal(cborSig)

require.NoError(t, err)

transaction.Signatures = append(transaction.Signatures, sigBytes)

rawTx := tx.TxRaw{
BodyBytes: bodyByte,
AuthInfoBytes: authByte,
Signatures: transaction.Signatures,
}

_, err = baseAcc.Authenticate(ctx, &aa_interface_v1.MsgAuthenticate{
RawTx: &rawTx,
Tx: &transaction,
SignerIndex: 0,
})
require.NoError(t, err)

// testing with invalid signature

// update sequence number
transaction = tx.Tx{
Body: &tx.TxBody{},
AuthInfo: &tx.AuthInfo{
SignerInfos: []*tx.SignerInfo{
{
PublicKey: pkAny,
ModeInfo: &tx.ModeInfo{
Sum: &tx.ModeInfo_Single_{
Single: &tx.ModeInfo_Single{
Mode: 1,
},
},
},
Sequence: 1,
},
},
},
Signatures: [][]byte{},
}
authByte, err = transaction.AuthInfo.Marshal()
require.NoError(t, err)

txDoc.BodyBytes = []byte("invalid_msg")
txDoc.AuthInfoBytes = authByte
signBytes, err = txDoc.Marshal()
require.NoError(t, err)

invalidClientDataJson := GenerateClientData(t, signBytes)
invalidClientDataHash := sha256.Sum256(invalidClientDataJson)
invalidAuthenticatorData := cometcrypto.CRandBytes(37)
invalidPayload := append(invalidAuthenticatorData, invalidClientDataHash[:]...)

ivh := crypto.SHA256.New()
ivh.Write(invalidPayload)
ivdigest := ivh.Sum(nil)

invalidSig, err := ecdsa.SignASN1(rand.Reader, privKey, ivdigest)
require.NoError(t, err)

transaction.Signatures = append([][]byte{}, invalidSig)

rawTx.Signatures = transaction.Signatures

_, err = baseAcc.Authenticate(ctx, &aa_interface_v1.MsgAuthenticate{
RawTx: &rawTx,
Tx: &transaction,
SignerIndex: 0,
})
require.Equal(t, errors.New("signature verification failed"), err)
}
123 changes: 123 additions & 0 deletions x/accounts/defaults/base/keys/authn/authn_pubkey.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
package authn

import (
"bytes"
"crypto"
ecdsa "crypto/ecdsa"
"crypto/elliptic"
"crypto/sha256"
"encoding/base64"
"encoding/hex"
"encoding/json"
"fmt"

cometcrypto "github.com/cometbft/cometbft/crypto"
"github.com/cosmos/gogoproto/proto"

cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
"github.com/cosmos/cosmos-sdk/types/address"
)

type Signature struct {
AuthenticatorData string `json:"authenticatorData"`
ClientDataJSON string `json:"clientDataJSON"`
Signature string `json:"signature"`
}

const (
keyType = "authn"
PubKeyName = "tendermint/PubKeyAuthn"
)

var (
_ cryptotypes.PubKey = (*AuthnPubKey)(nil)
)

const AuthnPubKeySize = 33

func (pubKey *AuthnPubKey) Address() cometcrypto.Address {
if len(pubKey.Key) != AuthnPubKeySize {
panic("length of pubkey is incorrect")

Check warning

Code scanning / CodeQL

Panic in BeginBock or EndBlock consensus methods Warning

Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
}

return address.Hash(proto.MessageName(pubKey), pubKey.Key)
}
Comment on lines +38 to +44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid using panic in library code.

According to the Uber Go Style Guide, panics should be avoided in library code. Instead of panicking when the key length is incorrect, consider validating the key length upon creation of the AuthnPubKey instance or redesigning the method to return an error.

Tools
GitHub Check: CodeQL

[warning] 40-40: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt


func (pubKey *AuthnPubKey) Bytes() []byte {
return pubKey.Key
}

func (pubKey *AuthnPubKey) String() string {
return fmt.Sprintf("PubKeyAuthn{%X}", pubKey.Key)
}

func (pubKey *AuthnPubKey) Type() string {
return keyType
}

func (pubKey *AuthnPubKey) Equals(other cryptotypes.PubKey) bool {
return pubKey.Type() == other.Type() && bytes.Equal(pubKey.Bytes(), other.Bytes())
}

func (pubKey *AuthnPubKey) VerifySignature(msg, sigStr []byte) bool {
sig := Signature{}
err := json.Unmarshal(sigStr, &sig)
if err != nil {
return false
}

clientDataJSON, err := hex.DecodeString(sig.ClientDataJSON)
if err != nil {
return false
}
Comment on lines +69 to +72
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Verify that the correct decoding functions are used for ClientDataJSON and AuthenticatorData.

Currently, hex.DecodeString is used to decode sig.ClientDataJSON and sig.AuthenticatorData. Ensure that these fields are actually hex-encoded. Often, these fields are base64 or base64url encoded in authentication protocols like WebAuthn. Using the incorrect decoding method may cause signature verification to fail.

Also applies to: 106-109


clientData := make(map[string]interface{})
err = json.Unmarshal(clientDataJSON, &clientData)
if err != nil {
return false
}
Comment on lines +74 to +78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use typed structs instead of map[string]interface{} for JSON unmarshalling.

Employing a typed struct for unmarshalling clientDataJSON can enhance type safety and reduce reliance on type assertions, leading to clearer and more maintainable code.


challengeBase64, ok := clientData["challenge"].(string)
if !ok {
return false
}
challenge, err := base64.RawURLEncoding.DecodeString(challengeBase64)
if err != nil {
return false
}

// Check challenge == msg
if !bytes.Equal(challenge, msg) {
return false
}

publicKey := &ecdsa.PublicKey{Curve: elliptic.P256()}

publicKey.X, publicKey.Y = elliptic.UnmarshalCompressed(elliptic.P256(), pubKey.Key)
if publicKey.X == nil || publicKey.Y == nil {
return false
}

signatureBytes, err := hex.DecodeString(sig.Signature)
if err != nil {
return false
}

authenticatorData, err := hex.DecodeString(sig.AuthenticatorData)
if err != nil {
return false
}

// check authenticatorData length
if len(authenticatorData) < 37 {
return false
}

clientDataHash := sha256.Sum256(clientDataJSON)
payload := append(authenticatorData, clientDataHash[:]...)

h := crypto.SHA256.New()
h.Write(payload)

return ecdsa.VerifyASN1(publicKey, h.Sum(nil), signatureBytes)
}
Comment on lines +62 to +123
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider refactoring VerifySignature for better readability.

The VerifySignature method is lengthy and handles multiple steps such as unmarshalling, decoding, and verification processes. Breaking it into smaller helper functions would improve readability and maintainability.

Loading
Loading