From 5c344bfceb7d42921d070a43546d2cff3db298ff Mon Sep 17 00:00:00 2001 From: Evgeniy Kulikov Date: Tue, 19 Nov 2019 19:03:42 +0300 Subject: [PATCH] Fix issue with Sign/VerifyRequestHeader proto.Clone proto.Clone couldn't makes copy for custom fields. We should reset and restore MetaHeader before/after Sign/Verify. Add test coverage to check that all works like expected. --- service/meta.go | 24 +++++++++----- service/verify.go | 68 ++++++++++++++++++++++++++++++-------- service/verify_test.go | 37 +++++++++++++++++++++ service/verify_test.pb.go | Bin 13453 -> 14924 bytes service/verify_test.proto | 1 + 5 files changed, 108 insertions(+), 22 deletions(-) diff --git a/service/meta.go b/service/meta.go index cd3459e..52874c1 100644 --- a/service/meta.go +++ b/service/meta.go @@ -8,10 +8,11 @@ import ( type ( // MetaHeader contains meta information of request. - // It provides methods to get or set meta information - // and reset meta header. + // It provides methods to get or set meta information meta header. + // Also contains methods to reset and restore meta header. MetaHeader interface { - ResetMeta() + ResetMeta() RequestMetaHeader + RestoreMeta(RequestMetaHeader) // TTLRequest to verify and update ttl requests. GetTTL() uint32 @@ -22,7 +23,7 @@ type ( SetEpoch(v uint64) } - // TTLCondition is closure, that allows to validate request with ttl + // TTLCondition is closure, that allows to validate request with ttl. TTLCondition func(ttl uint32) error ) @@ -45,14 +46,21 @@ const ( ErrIncorrectTTL = internal.Error("incorrect ttl") ) -// SetTTL sets TTL to RequestMetaHeader +// SetTTL sets TTL to RequestMetaHeader. func (m *RequestMetaHeader) SetTTL(v uint32) { m.TTL = v } -// SetEpoch sets Epoch to RequestMetaHeader +// SetEpoch sets Epoch to RequestMetaHeader. func (m *RequestMetaHeader) SetEpoch(v uint64) { m.Epoch = v } -// ResetMeta sets RequestMetaHeader to empty value -func (m *RequestMetaHeader) ResetMeta() { m.Reset() } +// ResetMeta returns current value and sets RequestMetaHeader to empty value. +func (m *RequestMetaHeader) ResetMeta() RequestMetaHeader { + cp := *m + m.Reset() + return cp +} + +// RestoreMeta sets current RequestMetaHeader to passed value. +func (m *RequestMetaHeader) RestoreMeta(v RequestMetaHeader) { *m = v } // IRNonForwarding condition that allows NonForwardingTTL only for IR func IRNonForwarding(role NodeRole) TTLCondition { diff --git a/service/verify.go b/service/verify.go index d8ede7e..a6ac3a5 100644 --- a/service/verify.go +++ b/service/verify.go @@ -10,7 +10,7 @@ import ( ) type ( - // VerifiableRequest adds possibility to sign and verify request header + // VerifiableRequest adds possibility to sign and verify request header. VerifiableRequest interface { proto.Message Marshal() ([]byte, error) @@ -30,19 +30,19 @@ type ( ) const ( - // ErrCannotLoadPublicKey is raised when cannot unmarshal public key from RequestVerificationHeader_Sign + // ErrCannotLoadPublicKey is raised when cannot unmarshal public key from RequestVerificationHeader_Sign. ErrCannotLoadPublicKey = internal.Error("cannot load public key") - // ErrCannotFindOwner is raised when signatures empty in GetOwner + // ErrCannotFindOwner is raised when signatures empty in GetOwner. ErrCannotFindOwner = internal.Error("cannot find owner public key") ) -// SetSignatures replaces signatures stored in RequestVerificationHeader +// SetSignatures replaces signatures stored in RequestVerificationHeader. func (m *RequestVerificationHeader) SetSignatures(signatures []*RequestVerificationHeader_Signature) { m.Signatures = signatures } -// AddSignature adds new Signature into RequestVerificationHeader +// AddSignature adds new Signature into RequestVerificationHeader. func (m *RequestVerificationHeader) AddSignature(sig *RequestVerificationHeader_Signature) { if sig == nil { return @@ -123,12 +123,14 @@ func newSignature(key *ecdsa.PrivateKey, data []byte) (*RequestVerificationHeade // SignRequestHeader receives private key and request with RequestVerificationHeader, // tries to marshal and sign request with passed PrivateKey, after that adds // new signature to headers. If something went wrong, returns error. -func SignRequestHeader(key *ecdsa.PrivateKey, req VerifiableRequest) error { - msg := proto.Clone(req).(VerifiableRequest) - +func SignRequestHeader(key *ecdsa.PrivateKey, msg VerifiableRequest) error { // ignore meta header if meta, ok := msg.(MetaHeader); ok { - meta.ResetMeta() + h := meta.ResetMeta() + + defer func() { + meta.RestoreMeta(h) + }() } data, err := msg.Marshal() @@ -141,22 +143,28 @@ func SignRequestHeader(key *ecdsa.PrivateKey, req VerifiableRequest) error { return err } - req.AddSignature(signature) + msg.AddSignature(signature) return nil } // VerifyRequestHeader receives request with RequestVerificationHeader, -// tries to marshal and verify each signature from request +// tries to marshal and verify each signature from request. // If something went wrong, returns error. -func VerifyRequestHeader(req VerifiableRequest) error { - msg := proto.Clone(req).(VerifiableRequest) +func VerifyRequestHeader(msg VerifiableRequest) error { // ignore meta header if meta, ok := msg.(MetaHeader); ok { - meta.ResetMeta() + h := meta.ResetMeta() + + defer func() { + meta.RestoreMeta(h) + }() } signatures := msg.GetSignatures() + defer func() { + msg.SetSignatures(signatures) + }() for i := range signatures { msg.SetSignatures(signatures[:i]) @@ -177,3 +185,35 @@ func VerifyRequestHeader(req VerifiableRequest) error { return nil } + +// testCustomField for test usage only. +type testCustomField [8]uint32 + +var _ internal.Custom = (*testCustomField)(nil) + +// Reset skip, it's for test usage only. +func (t testCustomField) Reset() {} + +// ProtoMessage skip, it's for test usage only. +func (t testCustomField) ProtoMessage() {} + +// Size skip, it's for test usage only. +func (t testCustomField) Size() int { return 32 } + +// String skip, it's for test usage only. +func (t testCustomField) String() string { return "" } + +// Bytes skip, it's for test usage only. +func (t testCustomField) Bytes() []byte { return nil } + +// Unmarshal skip, it's for test usage only. +func (t testCustomField) Unmarshal(data []byte) error { return nil } + +// Empty skip, it's for test usage only. +func (t testCustomField) Empty() bool { return false } + +// UnmarshalTo skip, it's for test usage only. +func (t testCustomField) MarshalTo(data []byte) (int, error) { return 0, nil } + +// Marshal skip, it's for test usage only. +func (t testCustomField) Marshal() ([]byte, error) { return nil, nil } diff --git a/service/verify_test.go b/service/verify_test.go index 4aaefb4..1403c67 100644 --- a/service/verify_test.go +++ b/service/verify_test.go @@ -1,9 +1,12 @@ package service import ( + "bytes" + "log" "math" "testing" + "github.com/gogo/protobuf/proto" crypto "github.com/nspcc-dev/neofs-crypto" "github.com/nspcc-dev/neofs-crypto/test" "github.com/pkg/errors" @@ -104,3 +107,37 @@ func TestMaintainableRequest(t *testing.T) { require.EqualError(t, errors.Cause(err), crypto.ErrInvalidSignature.Error()) } } + +func TestVerifyAndSignRequestHeaderWithoutCloning(t *testing.T) { + key := test.DecodeKey(0) + + custom := testCustomField{1, 2, 3, 4, 5, 6, 7, 8} + + b := &TestRequest{ + IntField: math.MaxInt32, + StringField: "TestRequestStringField", + BytesField: []byte("TestRequestBytesField"), + CustomField: &custom, + RequestMetaHeader: RequestMetaHeader{ + TTL: math.MaxInt32 - 8, + Epoch: math.MaxInt64 - 12, + }, + } + + require.NoError(t, SignRequestHeader(key, b)) + require.NoError(t, VerifyRequestHeader(b)) + + require.Len(t, b.Signatures, 1) + require.Equal(t, custom, *b.CustomField) + require.Equal(t, uint32(math.MaxInt32-8), b.GetTTL()) + require.Equal(t, uint64(math.MaxInt64-12), b.GetEpoch()) + + buf := bytes.NewBuffer(nil) + log.SetOutput(buf) + + cp, ok := proto.Clone(b).(*TestRequest) + require.True(t, ok) + require.NotEqual(t, b, cp) + + require.Contains(t, buf.String(), "proto: don't know how to copy") +} diff --git a/service/verify_test.pb.go b/service/verify_test.pb.go index a029c2e2af2b981508f1a30f47410b6c04925a0d..06e7971842ae9bc4d66f6166bb8abb5b1c2c02ce 100644 GIT binary patch delta 2412 zcmZveO^X~=7=?))g57=)B?=Qn$`QlF^o08U80ke2vQtE}3&B)%-O>)z)06I+p+^b* z3k0S70s0s0g*z9n-MIBf2wAugM7;MoozN3AFqf)(-+SKkp7-2a{mtaJ_J`h!-sZ3S z_l0Ty$&1%L=snze`=j^qr4LUQ?QGh8(z~*Hzcq_i@3$xBu=egB&u8tdI%y7et24Ve zjEA%1c6d}y&1frbNLVx+1J#}#o6$2!yWWGv?C4;3d(&(>Y0dPwJ=?uM=znS+o~(x# zf4$U;!mxWhSgfL8==saIapO30W8@D9eSaDF?j=v$Xeu`*p&R4UjTzetVl?_=R)H87 z?pgD3%5&|;3WO*oLE#`)%NUF}ck3}2Lh{KFG6t+Bj6N8tM(rggCT9jazg7lPr`im( zQ3;e&y3$t5#<&PsEhihXC0;t_=lv4fKK-G7^z;HIsoYaWmp2sv2OJ8N<;>o?NV52`hh@LZAR| zOsr(6vFs$V`4g&A>vQVVbIN>3 zwF#LsfHpQn$wF4zOc7d0Pc}ycNgkD%kU)M8ES$|BD!)jQCxKoruPDPt5_YSJ3C)!= z0*VgUMq8JvIfIK#7XX(cRRm6Bn_=_U8Z7dvM{EMiqn%pfd@B}q>6DzQ-g zunUzx#YxgIzyTq`*dxBO8A|Yc170Ocj0PYXEk>&-qlOX@OF3FaV<;Wpn%XMnRZb3&&YMJvajE3AHgNUVG`*2Z zV@bx8C^g_obeSt5CWecUa_W>BE$9p(x8+Pq^2-1T5UDbVhFV$BM4b!+hOTsdCv-LH&h(+GPntvxDMwo9 zYO?XJ4GAUGJ2X*(HayFwVb%Tq+RimwnaElHxkeFDD~?+2S8C8-Uw-|s-fDVt>$B^< zGxClmhu+tNe&7A~7n84ycQEqKMcw?&&hA}z>l1H(Ur`|I^ eP8_RqU>5w^shhPFo>y!4?Tx$Zzu)`*)&Btb+Ys#2kRa%+SAYm+0|e|~NJ)v<0lPpV zAfxJ>LA=E3ak+f2eeIebA3nbMd^+Er-ky)syYrvZ>&wrZ?+;FO?9X2wzdxq!?nT(! zwb*qkwh4E)kHTKH<;UFrKNvVpRq=Ql2V!EeeO+ula*Ah}%Xh-lHRU^!t?mUV7mmFq zs>KFu)s@(me3?xCp0GhiT0A2$6?TP7&1pA+kT@71JVL3Z+8X~>V2-_A3c>)(#O;x0 zRQMy;>QrK^xuu3~1tE}WfpBASB$}aTxht5n$aWj5TY)+Dl?zdlq)4D;Pmw_cD+wZa zEHUy`S}+?+AaX>omUv}TB!#R?2}jsBYfvVb_1HigBK}1y0$Detnqkpk2X1wswZspz zG7!2j0Uarvi6LLh_#KRODSAsk9liUL=|DMRHR)y%oWq1-O58v`Ktm-LJWhP+*T?&|>Hx#HUx#8@HRBh~E8Wq^$T55b0-TZt8s zW{lj}xV@3pNmrN=|3nGZJ+w$`XJ^%Ev?U7wIrPYL%^>a@x6Rs6?PweDI}8oq$#Lg< zLG;ll!)Z_-wrg*p>Jr<}4SF_j8^Bi+Ti;RVLOng9Y8yr9DaawRiS9&#j2@+$Aa)g< z*2<;l+b#^&?}+94%f2aM1@C~J5ze52S()%S?+eHkviW>C``1CI&u&<9AM d`jFdwNl$;DF0Y