fix: Do not allow broken session token by SN #2727
Conversation
carpawell
requested review from
roman-khimov and
cthulhu-rider
as code owners
carpawell
force-pushed
the
fix/container-session
branch
2 times, most recently
from
84c62b9
to
e571e4d
Compare
| @@ -19,6 +19,7 @@ Changelog for NeoFS Node | |||
| - Created files are not group writable (#2589) | |||
| - IR does not create new notary requests for the SN's bootstraps but signs the received ones instead (#2717) | |||
| - IR can handle third-party notary requests without reliance on receiving the original one (#2715) | |||
| - SN validates container session token's issuer to be container's owner (#2466) | |||
There was a problem hiding this comment.
not only ownership is validated
There was a problem hiding this comment.
do you mean providing a list there? the issue has the exact problem described
There was a problem hiding this comment.
imo issue is just a ref, changelog contains meaningful changes. Full list is an overhead, i'd say
| - SN validates container session token's issuer to be container's owner (#2466) | |
| - SN now validates container session tokens (#2466) |
| return fmt.Errorf("session session was issued not by the owner, issuer: %x", issuer) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
why are not all fields checked?
There was a problem hiding this comment.
what else fields to check? that is not IR validation before we try to change smth, just a logical validation that you are not trying to make totally incorrect things (container owner do not know about your operation but you still trying it). did not want to copy IR behavior
epoch can not be ensured: the final check will be done on the IR side, no one knows at what state and when
There was a problem hiding this comment.
there are other stateless conditions like lifetime field presence, signature correctness, etc. And some stateful conditions are consistent, e.g. expiraiton claim
There was a problem hiding this comment.
there are other stateless conditions like lifetime field presence
i would say it is more like a protocol validation (if we are talking about the presence, and epoch values i wouldn't check by the intermediate SN ever, as I've already said)
signature correctness
well... ok, done but not valuable to me
There was a problem hiding this comment.
well... ok, done but not valuable to me
words of some undefined behavior...
carpawell
force-pushed
the
fix/container-session
branch
from
e571e4d
to
71fc7ee
Compare
Seems so. But that is how the issues like "do not want to wait too long before IR reacts, pls, make it on the SN side" are fixed. Do not see any problems here.
Can be checked with |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2727 +/- ##
==========================================
+ Coverage 28.81% 28.84% +0.03%
==========================================
Files 415 415
Lines 32397 32427 +30
==========================================
+ Hits 9335 9355 +20
- Misses 22228 22233 +5
- Partials 834 839 +5 ☔ View full report in Codecov by Sentry. |
carpawell
force-pushed
the
fix/container-session
branch
from
71fc7ee
to
53dc640
Compare
If container session token is incorrect, there is no need in Alphabet's check, the error can be returned immediately to a client; otherwise he has to wait for the TX to persist infinitely with not feedback. Closes #2466. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
carpawell
force-pushed
the
fix/container-session
branch
from
53dc640
to
f8620ff
Compare
not everybody will be ready for this. I'd add a remark to |
Not sure what an updater should see in this line. Not sure how @532910 will react. But I tried a little. |
anything i can look at? |
Oh, has not pushed, my bad. |
If container session token is incorrect, there is no need in Alphabet's check, the error can be returned immediately to a client; otherwise he has to wait for the TX to persist infinitely with not feedback. Closes #2466.