nspcc-dev/neofs-node

Fork 0

mirror of https://github.com/nspcc-dev/neofs-node.git synced 2026-03-01 04:29:10 +00:00

Static session tokens are silently mutated by CLI #1084

New issue

Open

opened 2025-12-28 17:21:44 +00:00 by sami · 4 comments

sami commented

2025-12-28 17:21:44 +00:00

Owner

Originally created by @cthulhu-rider on GitHub (Aug 9, 2023).

currently, NeoFS CLI completes and signs session tokens in commands

$ neofs-cli object put --session token.json

and

$ neofs-cli object delete --session token.json

the token is finalized unconditionally: even if it is signed, it is mutated. The problem occured in testsuite:

in previous revision, CLI re-signed specified token which led to issuer's rewrite (then this session led to Access Denied)
in current revision, CLI also re-signs the token, but now it doesn't rewrite issuer (this seems correct, but due to this so we see invalid session token owner)

Proposal

from my pov, current behavior with silent invasion is incorrect, and i propose to require tokens specified via --session to be correctly formed and signed (*) because static sessions are going from the outworld and MUST NOT be modified by CLI

(*) we provide util sign session-token for this

overall, I'd make NeoFS CLI to not touch user input at all except some UI translations like public-read-write -> uint32

Originally created by @cthulhu-rider on GitHub (Aug 9, 2023). currently, NeoFS CLI completes and signs session tokens in commands ``` $ neofs-cli object put --session token.json ``` and ``` $ neofs-cli object delete --session token.json ``` the token is finalized unconditionally: even if it is signed, it is mutated. The problem occured in [testsuite](https://http.fs.neo.org/HXSaMJXk2g8C14ht8HSi7BBaiYZ1HeWh2xnWPGQCg4H6/424-1691517028/index.html#suites/caeb93ac7a2ae98f9e4a3c59e66fe114/30268395c646f1eb/): * in previous revision, CLI re-signed specified token which led to issuer's rewrite (then this session led to `Access Denied`) * in current revision, CLI also re-signs the token, but now it doesn't rewrite [issuer](https://github.com/nspcc-dev/neofs-sdk-go/blob/38848dc49ac019e74410b9e2bf26a8e61e39200c/session/common.go#L156-L160) (this seems correct, but due to this so we see `invalid session token owner`) ## Proposal from my pov, current behavior with silent invasion is incorrect, and i propose to require tokens specified via `--session` to be correctly formed and signed (*) because static sessions are going from the outworld and MUST NOT be modified by CLI (*) we provide `util sign session-token` for this overall, I'd make NeoFS CLI to not touch user input at all except some UI translations like `public-read-write` -> `uint32`

sami added the

bug

neofs-cli

labels

2025-12-28 17:21:44 +00:00

sami commented

2025-12-28 17:21:45 +00:00

Author

Owner

@cthulhu-rider commented on GitHub (Aug 10, 2023):

i restored how we came to this

static sessions currently make no sense for object put|delete|lock commands according to docs (code follows)

once #1216 happened and we optimized dynamic sessions:

user opens dynamic session via neofs-cli session create --json --out token.json
for write op, e.g. put, user may pass this token same way as static sessions neofs-cli object put --session token,json
command fills session context (which depends on exact call), sign the token and sends request with it

in total, from this moment unsigned dynamic sessions started to collide with static ones (that's why we may re-sign already signed as mentioned in issue body)

lets resolve issues one-by-one:

do we think re-usable dynamic sessions makes sense so we should keep them?
if so, may be we'll separate them from static ones with --dynamic-session flag?
since static sessions currently not working, maybe we should get rid of them in object write ops? (*)

(*) static sessions may become needed with #2482 in the future. Right now using static session in, for example, object put always leads to

rpc error: finish object stream: status: code = 4096 message = session token not found

my own answers are:

technically re-usable dynamic sessions are useful, but i dont think users will mess with them. So i'd drop this feature with 80/20 confidence
yes if 1
drop --session flag until #2482

@roman-khimov @carpawell pls share ur thoughts

@cthulhu-rider commented on GitHub (Aug 10, 2023): i restored how we came to this static sessions currently make no sense for `object put|delete|lock` commands according to [docs](https://github.com/nspcc-dev/neofs-node/blob/master/docs/cli-sessions.md#object) (code follows) once #1216 happened and we optimized dynamic sessions: * user opens dynamic session via `neofs-cli session create --json --out token.json` * for write op, e.g. `put`, user may pass this token same way as static sessions `neofs-cli object put --session token,json` * command fills session context (which depends on exact call), sign the token and sends request with it in total, from this moment unsigned dynamic sessions started to collide with static ones (that's why we may re-sign already signed as mentioned in issue body) lets resolve issues one-by-one: 1. do we think re-usable dynamic sessions makes sense so we should keep them? 2. if so, may be we'll separate them from static ones with `--dynamic-session` flag? 3. since static sessions currently not working, maybe we should get rid of them in object write ops? (*) (*) static sessions may become needed with #2482 in the future. Right now using static session in, for example, `object put` always leads to ``` rpc error: finish object stream: status: code = 4096 message = session token not found ``` my own answers are: 1. technically re-usable dynamic sessions are useful, but i dont think users will mess with them. So i'd drop this feature with 80/20 confidence 2. yes if 1 3. drop `--session` flag until #2482 @roman-khimov @carpawell pls share ur thoughts

sami commented

2025-12-28 17:21:45 +00:00

Author

Owner

@roman-khimov commented on GitHub (Aug 10, 2023):

I'd say that we should separate two flows then, but dropping something that we already have doesn't seem right to me (it'd be a regression), so I'd say:

move to 0.39.0
use a different flag for "session template"
fix #2482 in the same release

@roman-khimov commented on GitHub (Aug 10, 2023): I'd say that we should separate two flows then, but dropping something that we already have doesn't seem right to me (it'd be a regression), so I'd say: * move to 0.39.0 * use a different flag for "session template" * fix #2482 in the same release

sami commented

2025-12-28 17:21:45 +00:00

Author

Owner

@carpawell commented on GitHub (Aug 17, 2023):

What static sessions are for? Why we called them "sessions"? Is there any component of NeoFS that relies on static sessions?
I would research if we can opt out of this feature. If yes, it would become so much easier to explain what it is for, to understand what we are developing them for, and so on.

@carpawell commented on GitHub (Aug 17, 2023): What static sessions are for? Why we called them "sessions"? Is there _any_ component of NeoFS that relies on static sessions? I would research if we can opt out of this feature. If yes, it would become so much easier to explain what it is for, to understand what we are developing them for, and so on.

sami commented

2025-12-28 17:21:45 +00:00

Author

Owner

@carpawell commented on GitHub (Aug 17, 2023):

I static sessions are needed that much I would provide a separate grpc message for them and name them differently.

@carpawell commented on GitHub (Aug 17, 2023): I static sessions are needed _that much_ I would provide a separate grpc `message` for them and name them differently.