-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Do not allow broken session token by SN #2727
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,16 @@ | ||
package container | ||
|
||
import ( | ||
"bytes" | ||
"context" | ||
"errors" | ||
"fmt" | ||
|
||
"github.com/mr-tron/base58" | ||
"github.com/nspcc-dev/neofs-api-go/v2/container" | ||
"github.com/nspcc-dev/neofs-api-go/v2/refs" | ||
sessionV2 "github.com/nspcc-dev/neofs-api-go/v2/session" | ||
"github.com/nspcc-dev/neofs-api-go/v2/util/signature" | ||
containercore "github.com/nspcc-dev/neofs-node/pkg/core/container" | ||
containerSvc "github.com/nspcc-dev/neofs-node/pkg/services/container" | ||
cid "github.com/nspcc-dev/neofs-sdk-go/container/id" | ||
|
@@ -80,6 +83,11 @@ func (s *morphExecutor) Put(_ context.Context, tokV2 *sessionV2.Token, body *con | |
if err != nil { | ||
return nil, fmt.Errorf("invalid session token: %w", err) | ||
} | ||
|
||
err = s.validateToken(tokV2, nil, sessionV2.ContainerVerbPut) | ||
if err != nil { | ||
return nil, fmt.Errorf("session token validation: %w", err) | ||
} | ||
} | ||
|
||
idCnr, err := s.wrt.Put(cnr) | ||
|
@@ -120,6 +128,11 @@ func (s *morphExecutor) Delete(_ context.Context, tokV2 *sessionV2.Token, body * | |
if err != nil { | ||
return nil, fmt.Errorf("invalid session token: %w", err) | ||
} | ||
|
||
err = s.validateToken(tokV2, body.GetContainerID(), sessionV2.ContainerVerbDelete) | ||
if err != nil { | ||
return nil, fmt.Errorf("session token validation: %w", err) | ||
} | ||
} | ||
|
||
var rmWitness containercore.RemovalWitness | ||
|
@@ -228,6 +241,11 @@ func (s *morphExecutor) SetExtendedACL(_ context.Context, tokV2 *sessionV2.Token | |
if err != nil { | ||
return nil, fmt.Errorf("invalid session token: %w", err) | ||
} | ||
|
||
err = s.validateToken(tokV2, body.GetEACL().GetContainerID(), sessionV2.ContainerVerbSetEACL) | ||
if err != nil { | ||
return nil, fmt.Errorf("session token validation: %w", err) | ||
} | ||
} | ||
|
||
err = s.wrt.PutEACL(eaclInfo) | ||
|
@@ -274,3 +292,72 @@ func (s *morphExecutor) GetExtendedACL(_ context.Context, body *container.GetExt | |
|
||
return res, nil | ||
} | ||
|
||
type sessionDataSource struct { | ||
t *sessionV2.Token | ||
size int | ||
} | ||
|
||
func (d sessionDataSource) ReadSignedData(buff []byte) ([]byte, error) { | ||
if len(buff) < d.size { | ||
buff = make([]byte, d.size) | ||
} | ||
|
||
res := d.t.GetBody().StableMarshal(buff) | ||
|
||
return res[:d.size], nil | ||
} | ||
|
||
func (d sessionDataSource) SignedDataSize() int { | ||
return d.size | ||
} | ||
|
||
func newDataSource(t *sessionV2.Token) sessionDataSource { | ||
return sessionDataSource{ | ||
t: t, | ||
size: t.GetBody().StableSize(), | ||
} | ||
} | ||
|
||
func (s *morphExecutor) validateToken(t *sessionV2.Token, cIDV2 *refs.ContainerID, op sessionV2.ContainerSessionVerb) error { | ||
c := t.GetBody().GetContext() | ||
cc, ok := c.(*sessionV2.ContainerSessionContext) | ||
if !ok { | ||
return errors.New("session is not container-related") | ||
} | ||
|
||
if verb := cc.Verb(); verb != op { | ||
return fmt.Errorf("wrong container session operation: %s", verb) | ||
cthulhu-rider marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
err := signature.VerifyDataWithSource(newDataSource(t), t.GetSignature) | ||
if err != nil { | ||
return fmt.Errorf("incorrect token signature: %w", err) | ||
} | ||
|
||
if cIDV2 == nil { // can be nil for PUT or wildcard may be true | ||
return nil | ||
} | ||
|
||
if sessionCID := cc.ContainerID().GetValue(); !bytes.Equal(sessionCID, cIDV2.GetValue()) { | ||
return fmt.Errorf("wrong container: %s", base58.Encode(sessionCID)) | ||
} | ||
|
||
var cID cid.ID | ||
|
||
cthulhu-rider marked this conversation as resolved.
Show resolved
Hide resolved
|
||
err = cID.ReadFromV2(*cIDV2) | ||
if err != nil { | ||
return fmt.Errorf("invalid container ID: %w", err) | ||
} | ||
|
||
cnr, err := s.rdr.Get(cID) | ||
if err != nil { | ||
return fmt.Errorf("reading container from the network: %w", err) | ||
} | ||
|
||
if issuer := t.GetBody().GetOwnerID().GetValue(); !bytes.Equal(cnr.Value.Owner().WalletBytes(), issuer) { | ||
return fmt.Errorf("session was not issued by the container owner, issuer: %q", issuer) | ||
} | ||
|
||
return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are not all fields checked? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there are other stateless conditions like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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)
well... ok, done but not valuable to me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
words of some undefined behavior... |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not only ownership is validated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean providing a list there? the issue has the exact problem described
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo issue is just a ref, changelog contains meaningful changes. Full list is an overhead, i'd say