feat/exchange object meta's signatures #2928

carpawell · 2024-09-04T18:14:57Z

No description provided.

codecov · 2024-09-06T03:09:05Z

Codecov Report

Attention: Patch coverage is 33.58209% with 89 lines in your changes missing coverage. Please review.

Project coverage is 23.91%. Comparing base (1a5809e) to head (d7b63cb).
Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/services/object/put/distributed.go	0.00%	23 Missing ⚠️
pkg/network/transport/object/grpc/replication.go	44.44%	18 Missing and 2 partials ⚠️
cmd/neofs-node/transport.go	0.00%	18 Missing ⚠️
pkg/services/object/put/remote.go	0.00%	12 Missing ⚠️
cmd/neofs-node/object.go	0.00%	4 Missing ⚠️
pkg/services/object/put/local.go	0.00%	3 Missing ⚠️
pkg/services/object/put/service.go	0.00%	3 Missing ⚠️
pkg/core/object/replicate.go	90.90%	1 Missing and 1 partial ⚠️
pkg/morph/client/client.go	0.00%	2 Missing ⚠️
cmd/neofs-node/netmap.go	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2928      +/-   ##
==========================================
+ Coverage   23.88%   23.91%   +0.03%     
==========================================
  Files         775      776       +1     
  Lines       45610    45714     +104     
==========================================
+ Hits        10892    10932      +40     
- Misses      33861    33922      +61     
- Partials      857      860       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

carpawell · 2024-09-06T03:22:29Z

Format, "network": number,, valid interval should be discussed. @roman-khimov, @cthulhu-rider

@AnnaShaleva, could you, please, also take a peek at what we are planning to use as a standard message? (see #2876 also)

roman-khimov

In general it's OK for the first version, we just need to catch the basic relations provided by NeoFS.

pkg/services/object/put/distributed.go

pkg/core/object/replicate.go

cthulhu-rider

quite a large amount of work has been done, I like how it fits into the old concepts

in the commit desc u indicate that the signatures are only partially used for now, do we have any pledged proposals for further use? I would also refer to them in order to confirm the motivation for the preliminary changes

cthulhu-rider · 2024-09-06T10:46:18Z

pkg/core/object/replicate.go

+	validInterval  = 10 // in epoches
+	currentVersion = 6  // it is also a number of fields
+
+	cidKey        = "cid"


not critical but i'd make a separate group for these consts

cthulhu-rider · 2024-09-06T10:48:13Z

pkg/core/object/replicate.go

+)
+
+const (
+	validInterval  = 10 // in epoches


Suggested change

validInterval = 10 // in epoches

validInterval = 10 // in epochs

cthulhu-rider · 2024-09-06T10:49:10Z

pkg/core/object/replicate.go

+//	"validuntil": last valid epoch number for meta information
+//
+// Last valid epoch is object's creation epoch + 10.
+func MetaInfo(cID cid.ID, oID oid.ID, pSize uint64, deleted []oid.ID, locked []oid.ID, createdAt uint64) []byte {


Suggested change

func MetaInfo(cID cid.ID, oID oid.ID, pSize uint64, deleted []oid.ID, locked []oid.ID, createdAt uint64) []byte {

func EncodeReplicationMetaInfo(cID cid.ID, oID oid.ID, pSize uint64, deleted []oid.ID, locked []oid.ID, createdAt uint64) []byte {

would be more clear imo

cthulhu-rider · 2024-09-06T10:49:49Z

pkg/core/object/replicate.go

+// Last valid epoch is object's creation epoch + 10.
+func MetaInfo(cID cid.ID, oID oid.ID, pSize uint64, deleted []oid.ID, locked []oid.ID, createdAt uint64) []byte {
+	kvs := make([]stackitem.MapElement, 0, currentVersion)
+	kvs = append(kvs, kv(cidKey, cID[:]))


appends look redundant here, kvs can be initialized in one assignment

cthulhu-rider · 2024-09-06T10:51:50Z

pkg/core/object/replicate.go

+		// all the errors in the stackitem relate only cases when it is
+		// impossible to use serialized values (too many values, unsupported
+		// types, etc.), unexpected errors at all
+		panic(err)


i'd also add context like

Suggested change

panic(err)

panic(fmt.Errorf("unexpected stackitem map serialization failure: %v", err))

cthulhu-rider · 2024-09-06T10:58:18Z

pkg/network/transport/object/grpc/replication.go

+
+		locked = make([]oid.ID, l.NumberOfMembers())
+		l.ReadMembers(locked)
+	default:


forgot code or redundant?

no, why? if we are replicating a lock/tomb object, we add IDs, but if we are adding a regular object, nothing should be done in this switch
or what you suggest here?

cthulhu-rider · 2024-09-06T11:00:20Z

pkg/network/transport/object/grpc/replication.go

@@ -178,7 +180,20 @@ func (s *Server) Replicate(_ context.Context, req *objectGRPC.ReplicateRequest)
 		}}, nil
 	}

-	return new(objectGRPC.ReplicateResponse), nil
+	resp := new(objectGRPC.ReplicateResponse)
+	if req.GetSignObject() {


do u plan to add tests for these cases? there are few of them now

cthulhu-rider · 2024-09-06T11:01:59Z

pkg/services/object/put/distributed.go

+		fmt.Println("DEBUG: trying to verify signatures")
+
+		if !sig.Verify(t.objSharedMeta) {
+			return apistatus.ErrSignatureVerification


quite a few signatures are checked during communication, so I would add context what exact verification has failed

cthulhu-rider · 2024-09-06T11:04:41Z

pkg/services/object/put/distributed.go

+	default:
+	}
+
+	t.objSharedMeta = object.MetaInfo(t.obj.GetContainerID(), t.obj.GetID(), t.obj.PayloadSize(), deletedObjs, lockedObjs, t.obj.CreationEpoch())


t.objSharedMeta is used conditionally while calculated unconditionally. Lets sync

can you elaborate a little? this is calculated before any concurrent code, but is read concurrently, yes. do you mean some comments about it? or you want to see a lock here?

cthulhu-rider · 2024-09-06T11:09:16Z

pkg/services/object/put/distributed.go

 		return fmt.Errorf("could not close object stream: %w", err)
 	}
+
+	if !node.local {


shouldnt this be

Suggested change

if !node.local {

if t.localNodeInContainer && !node.local {

? Seems like ur proposal would fail when request goes through a non-container node

hm, that's interesting. what a non-contianer node should do if it faces an object whose meta signatures it should check? @roman-khimov

Why would it be involved in replication?

what does a node do when it is asked to sign with session key objects of a container it does not belong to?

It's signing and passing the object to container node.

oh, i have overthought some things in this code but yes, t.localNodeInContainer is enough, added. (also added nil signature check)

cthulhu-rider · 2024-09-06T11:27:12Z

pkg/network/transport/object/grpc/replication.go

+	metaInfo := objectcore.MetaInfo(o.GetContainerID(), o.GetID(), o.PayloadSize(), deleted, locked, o.CreationEpoch())
+
+	var sig neofscrypto.Signature
+	err := sig.Calculate(s.signer, metaInfo)


this differs from the original protocol definition

do I understand correctly that we decided to limit the amount of information being signed, and this approach will eventually be "backported" into the protocol?

yes, things have changed with time (or i did not get them from the beginning). we have discussed it with @roman-khimov, a custom message should be provided (see the 2. in the comment mostly)

and this approach will eventually be "backported" into the protocol?

yes, that is right. not release was done since so it should not be a problem

carpawell · 2024-09-06T17:24:40Z

in the commit desc u indicate that the signatures are only partially used for now, do we have any pledged proposals for further use?

Yes, there should be some event contracts and contract-side validation. This is kinda the first iteration, all the other work will be done soon after this PR is discussed and approved.

I would also refer to them in order to confirm the motivation for the preliminary changes

Added contract issues to the commit desc.

It has `Replicate` with meta signatures. Signed-off-by: Pavel Karpy <[email protected]>

Initial replication requires nodes to sign object's main meta information and respond with it. Meta information is not sent on wire and treated as a fixed ordered NEO's map. Signatures are verified, not stored/send anywhere yet. It follows the recent API extension: nspcc-dev/neofs-api#299. Further, this extension is planned to have a contract adoption: nspcc-dev/neofs-contract#413 and nspcc-dev/neofs-contract#414. Closes #2876. Signed-off-by: Pavel Karpy <[email protected]>

When I wanted to use it, I wrote all the code based on `uint32` (because I knew it should be this way), and at the last moment I found that `client` package returns it as `uint64` for no reason. Signed-off-by: Pavel Karpy <[email protected]>

It saves from replay attacks and makes replication operation (and meta information in particular) more explicit. Signed-off-by: Pavel Karpy <[email protected]>

carpawell · 2024-09-11T07:36:41Z

Full report: https://rest.fs.neo.org/HXSaMJXk2g8C14ht8HSi7BBaiYZ1HeWh2xnWPGQCg4H6/1872-1726003656/index.html

carpawell self-assigned this Sep 4, 2024

carpawell force-pushed the feat/exchange-object-meta-signatures branch from d6037df to a30127e Compare September 6, 2024 03:06

carpawell marked this pull request as ready for review September 6, 2024 03:19

carpawell requested review from roman-khimov and cthulhu-rider as code owners September 6, 2024 03:19

roman-khimov reviewed Sep 6, 2024

View reviewed changes

pkg/services/object/put/distributed.go Outdated Show resolved Hide resolved

pkg/core/object/replicate.go Show resolved Hide resolved

cthulhu-rider reviewed Sep 6, 2024

View reviewed changes

carpawell force-pushed the feat/exchange-object-meta-signatures branch from a30127e to 96f271f Compare September 6, 2024 18:10

carpawell requested review from cthulhu-rider and roman-khimov September 6, 2024 18:11

carpawell force-pushed the feat/exchange-object-meta-signatures branch from 96f271f to 0bcc540 Compare September 10, 2024 17:57

carpawell added 4 commits September 10, 2024 20:58

go.mod: update neofs-api-go dependency

6a8c835

It has `Replicate` with meta signatures. Signed-off-by: Pavel Karpy <[email protected]>

pkg/morph: make read network's magic uint32

5d236f3

When I wanted to use it, I wrote all the code based on `uint32` (because I knew it should be this way), and at the last moment I found that `client` package returns it as `uint64` for no reason. Signed-off-by: Pavel Karpy <[email protected]>

node/object/replicate: add network magic object's meta information

d7b63cb

It saves from replay attacks and makes replication operation (and meta information in particular) more explicit. Signed-off-by: Pavel Karpy <[email protected]>

carpawell force-pushed the feat/exchange-object-meta-signatures branch from 0bcc540 to d7b63cb Compare September 10, 2024 17:59

roman-khimov mentioned this pull request Sep 11, 2024

Add split object meta into signed data #2935

Open

roman-khimov approved these changes Sep 11, 2024

View reviewed changes

roman-khimov merged commit 4a1bc79 into master Sep 12, 2024
19 of 21 checks passed

roman-khimov deleted the feat/exchange-object-meta-signatures branch September 12, 2024 16:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat/exchange object meta's signatures #2928

feat/exchange object meta's signatures #2928

carpawell commented Sep 4, 2024

codecov bot commented Sep 6, 2024 •

edited

Loading

carpawell commented Sep 6, 2024

roman-khimov left a comment

cthulhu-rider left a comment

cthulhu-rider Sep 6, 2024

cthulhu-rider Sep 6, 2024

cthulhu-rider Sep 6, 2024 •

edited

Loading

carpawell Sep 6, 2024

cthulhu-rider Sep 6, 2024

carpawell Sep 6, 2024

cthulhu-rider Sep 6, 2024

cthulhu-rider Sep 6, 2024

carpawell Sep 6, 2024

cthulhu-rider Sep 6, 2024

carpawell Sep 6, 2024

cthulhu-rider Sep 6, 2024

cthulhu-rider Sep 6, 2024

carpawell Sep 6, 2024

cthulhu-rider Sep 6, 2024

carpawell Sep 6, 2024

roman-khimov Sep 6, 2024

carpawell Sep 6, 2024

roman-khimov Sep 6, 2024

carpawell Sep 10, 2024

cthulhu-rider Sep 6, 2024 •

edited

Loading

carpawell Sep 6, 2024

carpawell commented Sep 6, 2024

carpawell commented Sep 11, 2024

	validInterval = 10 // in epoches
	validInterval = 10 // in epochs

	func MetaInfo(cID cid.ID, oID oid.ID, pSize uint64, deleted []oid.ID, locked []oid.ID, createdAt uint64) []byte {
	func EncodeReplicationMetaInfo(cID cid.ID, oID oid.ID, pSize uint64, deleted []oid.ID, locked []oid.ID, createdAt uint64) []byte {

	panic(err)
	panic(fmt.Errorf("unexpected stackitem map serialization failure: %v", err))

feat/exchange object meta's signatures #2928

feat/exchange object meta's signatures #2928

Conversation

carpawell commented Sep 4, 2024

codecov bot commented Sep 6, 2024 • edited Loading

Codecov Report

carpawell commented Sep 6, 2024

roman-khimov left a comment

Choose a reason for hiding this comment

cthulhu-rider left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cthulhu-rider Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carpawell commented Sep 6, 2024

carpawell commented Sep 11, 2024

codecov bot commented Sep 6, 2024 •

edited

Loading

cthulhu-rider Sep 6, 2024 •

edited

Loading

cthulhu-rider Sep 6, 2024 •

edited

Loading