Impossible to reuse signed session token

carpawell commented 7 months ago

func TestCoolButWhy(t *testing.T) {
    priv1, err := keys.NewPrivateKey()
    require.NoError(t, err)

    signer1 := user.NewAutoIDSigner(priv1.PrivateKey)

    priv2, err := keys.NewPrivateKey()
    require.NoError(t, err)

    signer2 := user.NewAutoIDSigner(priv2.PrivateKey)

    s1 := sessiontest.Container()
    require.NoError(t, s1.Sign(signer1))

    var s2 sessionsdk.Container
    s1.CopyTo(&s2)

    require.NoError(t, s2.Sign(signer2))

    require.False(t, s1.Issuer().Equals(s2.Issuer()))
}

Current Behavior

Do not pass.

Expected Behavior

Pass.

Possible Solution

https://github.com/nspcc-dev/neofs-sdk-go/blob/1db2fbf3c1ad047d1ab3dd74d7c3a930856cf0f0/session/common.go#L199-L202

Allow signer overriding.

Steps to Reproduce

See go unit test above.

Context

Wanted to write a test, needed to reuse a session struct to change token's issuer.

Regression

No idea.

Your Environment

v1.0.0-rc.11.0.20231121125354-d72d76ccb0ba

roman-khimov commented 7 months ago

https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.11/session#Container.Sign doesn't specify its behavior wrt issuer handling. However https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.11/session#Container.SetIssuer suggests that signer can be set automatically unless SetIssuer is used. You're not using it, but you're making a copy via https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.11/session#Container.CopyTo and your token has issuer already set. I'd say that we can:

either always replace signer in Sign as you suggest (but then SetIssuer is useless and can be deleted and you can no longer create some "special" token)
or just document the current behavior as is and use SetIssuer for your case

And I'd choose the second one.

carpawell commented 7 months ago

SetIssuer's docs:

Using this method is not required when Sign is used (issuer will be derived from the signer automatically).

But it does not in the test I've done: It signs but does not derive the issuer.

You're not using it, but you're making a copy via https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.11/session#Container.CopyTo and your token has issuer already set.

And that is what I wanted to do: I have one "good" token and sometimes I wanted to make it bad for some test (but keep the original one, so CopyTo is here). CopyTo does not relate to the issue, you can just reuse some token e.g. per RPC request and you will face the same issue, cause Sign does not change the issuer (but says it will do).

And I'd choose the second one.

But why? How that even possible that the signer is not the issuer?

roman-khimov commented 7 months ago

How that even possible that the signer is not the issuer?

You want to make a bad token, for example. You're signing externally and you have to use SetSigner anyway to have a properly serialized token.

carpawell commented 7 months ago

You want to make a bad token, for example.

Sign + explicit SetIssuer? I do not want to deprecate/remove SetIssuer. I only was surprised that signing can make tokens unusable and I even had to debug a little. Unexpected to me.

cthulhu-rider commented 6 months ago

i agree with described expected behavior: after Sign succeeded, Issuer is the one from the Sign arg

to cover all possible cases, we may provide following API:

func (*T) Sign(user.Signer) error
func (*T) SetSignature(neofscrypto.Signer) error
func (*T) SetIssuer(user.ID)

and:

issuer is always overridden by both Sign and SetIssuer
normally, Sign is used
SetSignature+SetIssuer allows to implement less safe but also more custom logic. We'll mark in docs that Sign is more preferable

@roman-khimov @carpawell

nspcc-dev / neofs-sdk-go