node/container: check session tokens on SN side #2903
Conversation
carpawell
force-pushed
the
fix/check-session-token-expiration
branch
3 times, most recently
from
9f67a9f
to
ef47bea
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2903 +/- ##
==========================================
+ Coverage 23.64% 23.66% +0.01%
==========================================
Files 774 774
Lines 44872 44887 +15
==========================================
+ Hits 10610 10621 +11
- Misses 33417 33419 +2
- Partials 845 847 +2 ☔ View full report in Codecov by Sentry. |
| @@ -337,6 +341,22 @@ func (s *morphExecutor) validateToken(t *sessionV2.Token, cIDV2 *refs.ContainerI | |||
| return fmt.Errorf("incorrect token signature: %w", err) | |||
| } | |||
|
|
|||
| if lt := t.GetBody().GetLifetime(); lt != nil { | |||
There was a problem hiding this comment.
why u decided to make lifetime optional? IR denies such tokens
There was a problem hiding this comment.
tbh, it was not easy to check:
- API does not claim it as a required field: https://github.com/nspcc-dev/neofs-api/blob/4bd8b4da96c810e69a9a1336adb5e4e84bac62eb/session/types.proto#L98-L110
- IR unmarshals it (and it can be done without any errors for a token without lifetime) and only when i looked at
InvalidAti found that it should be set (so it is not required now in fact, it is: unset lifetime == expired lifetime)
fixed, but not sure why it was so difficult to prove (and not sure what error should be)
There was a problem hiding this comment.
API does not claim it as a required field
u goddamn right! This brings us to the next topic
lets consider corner case not really expected in practice:
- current epoch is 0
- 1st request has no embedded lifetime field
- 2nd one has it with all zeros
according to lifetime claim docs and protobuf, both tokens are valid. Then why we require the field? I think we should start treating the field as optional (fix SDK code) and simply work with values as they are (defaulting to 0). What do u think?
There was a problem hiding this comment.
What do u think?
i think in any case we should add some words to the API (optional/required). about optionality: i think it is ok to have a token without a lifetime, though it can be considered a security issue
There was a problem hiding this comment.
We better have lifetime mandatory.
Closes #2898. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell
force-pushed
the
fix/check-session-token-expiration
branch
from
ef47bea
to
456b7fa
Compare
Closes #2898.