nspcc-dev / neofs-sdk-go

Go implementation of NeoFS SDK
Apache License 2.0
6 stars 14 forks source link

Simpilfy user ID derivation from key #428

Closed roman-khimov closed 1 year ago

roman-khimov commented 1 year ago

Problem: user.IDFromSigner returns an error which is very inconvenient. It can't really happen in normal code, but we still have to handle it.

We can add ID() user.ID to Signer itself and let this be a Signer responsibility. It can internally do IDFromKey or whatever else (GetScriptHash(), since it's likely to have a key as a proper key).

Refs. #422.

smallhive commented 1 year ago

Making method ID() user.ID inside Signer interface looks not very good. crypto package should know about users. We may update IDFromSigner to something next:

func IDFromSigner(id *ID, signer neofscrypto.Signer) {
    public := signer.Public()

    key := make([]byte, public.MaxEncodedSize())
    key = key[:public.Encode(key)]

    var pk keys.PublicKey
    if err := pk.DecodeBytes(key); err != nil {
        panic(err)
    }

    id.SetScriptHash(pk.GetScriptHash())
}

I think we may do this, because IDFromSigner takes neofscrypto.Signer which already strong restriction. If error in this situation can't really happen in normal code, we shouldn't bother about panic. Eventually we remove the error from function interface

roman-khimov commented 1 year ago

The problem is that in our system the concept of user is tightly related to the VM (yes, NeoGo VM) and actual keys people use in this VM. I'd like to leverage that in various ways including using non-standard scripts and this breaks IDFromSigner right away, because it only works with one particular kind of VM code. Signer on the other hand might have the script inside even if it's non-standard (which reminds me of https://pkg.go.dev/github.com/nspcc-dev/neo-go/pkg/wallet#Account abilities).

IDFromSigner is also very ineffective, if you're to look into Signer or SignerRFC6979 --- they both have appropriate keys inside and they can avoid encoding/decoding the key if they're to provide ID().

cthulhu-rider commented 1 year ago

the common practice is to use user ID and credentials for authentication/authorization separately. I don't think ID method in Signer is a good solution since these entities are from different planes, and I would not put one into the other. Signer is just a signing utility, user.ID identifies user in the system.

there is NeoFS ID contract deployed in the NeoFS Sidechain. It provides method to bind any bunch of keys to the particular user. This completely breaks Signer->user.ID conversion uniqueness.

scenario I expect from any service:

  1. I register in the system providing required information
  2. if registration successful, I receive my identifity - user.ID
  3. then there is IAM layer that contains ways to authenticate/authorize me using particular credentials (private keys bound to my account)

when I perform any operation requiring authentication, I specify my ID and attach credentials. Notice that I can use any credentials that auth me as the user.

according to all this, I propose to NOT add ID() user.ID method to Signer, but explicitly accept user.ID everywhere it's needed regardless of Signer (it just signs).

thougths?

smallhive commented 1 year ago

For me, passing user.ID directly to all required places is a good idea. In this case we even remove IDFromKey and IDFromSigner

smallhive commented 1 year ago

After a discussion with @roman-khimov @cthulhu-rider, we decided to add a new Signer interface into user package:

type Signer interface {
    neofscrypto.Signer
    UserID() user.ID
}

In case of any function needs user.ID it accepts user.Signer otherwise neofscrypto.Signer. By default SDK works with user.Signer