msg/async, v2: authenticated onwire encryption and new preamble format #26466

rzarzynski · 2019-02-17T04:16:21Z

No description provided.

liewegas · 2019-02-17T15:10:21Z

src/msg/async/crypto_onwire.cc

+  if(1 != EVP_EncryptUpdate(ectx.get(),
+	reinterpret_cast<unsigned char*>(filler.c_str()),
+	&update_len,
+	reinterpret_cast<const unsigned char*>(plaintext.c_str()),


we may eventually want to avoid c_str() on plaintext to allow a gatherlist? as a todo item?

Definitely TODO.

rjfd

The changes to ProtocolV2.{cc,hh} look good. Just left some minor comments.

rjfd · 2019-02-18T10:28:39Z

src/msg/async/ProtocolV2.cc

-    memcpy(payload, out.c_str(), out.length());
-    length = out.length();
-  }
+void ProtocolV2::authencrypt_payload(bufferlist &payload) {


We can remove this function, right?

rjfd · 2019-02-18T10:29:02Z

src/msg/async/ProtocolV2.cc

-  ldout(cct, 21) << __func__ << " length=" << length << " total_len=" << total_l
-                 << " sig_pad_len=" << sig_pad_l << " enc_pad_len=" << enc_pad_l
-                 << dendl;
+void ProtocolV2::authdecrypt_payload(char *payload, uint32_t &length) {


Same here (remove it?)

Removed as well.

rjfd · 2019-02-18T10:31:46Z

src/msg/async/ProtocolV2.cc

+
+using segment_t = ProtocolV2::segment_t;
+
+// V2 preamble is consisted with one or many preable blocks depending


nit: fixing some typos:

// V2 preamble consists of one or more preamble blocks depending // on the number of segments a particular frame needs. Each block holds // up to 2 segments and has its own CRC.

rjfd · 2019-02-18T10:34:26Z

src/msg/async/ProtocolV2.cc

+
+  std::array<ProtocolV2::segment_t, MAX_NUM_SEGMENTS> segments;
+
+  uint8_t num_extra_preamble_blocks() const {


This method is not used anywhere. Should we remove it, or will it be used when implementing the support for reading multiple preamble blocks?

Yeah, it was for preamble chaining. We still have possibility to start doing that in the future but, at the moment, it's unnecessary as preamble became 32 bytes long. To be dropped.

src/msg/async/crypto_onwire.cc

rzarzynski · 2019-02-18T11:10:47Z

src/msg/async/crypto_onwire.h

+  //
+  // It's undefined what will happen if client doesn't follow the order.
+  //
+  // TODO: switch to always_aligned_t


TODO: block size alignment. GCM can live (and lives) without it as it's a stream cipher but ultimately we don't want to be restricted to stream ciphers only.

liewegas · 2019-02-18T13:19:21Z

   -76> 2019-02-17 17:52:05.181 7f442e9bc700 10 -- [v2:172.21.15.104:3300/0,v1:172.21.15.104:6789/0] dispatch_throttle_release 55 to dispatch throttler 32/104857600
   -75> 2019-02-17 17:52:05.183 7f442e9bc700 -1 /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos7/DIST/centos7/MACHINE_SIZE/huge/release/14.0.1-3869-g1405567/rpm/el7/BUILD/ceph-14.0.1-3869-g1405567/src/common/Throttle.cc: In function 'virtual int64_t Thr
ottle::put(int64_t)' thread 7f442e9bc700 time 2019-02-17 17:52:05.182104
/home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos7/DIST/centos7/MACHINE_SIZE/huge/release/14.0.1-3869-g1405567/rpm/el7/BUILD/ceph-14.0.1-3869-g1405567/src/common/Throttle.cc: 225: FAILED ceph_assert(count >= c)

 ceph version 14.0.1-3869-g1405567 (1405567b10ef50ad2e6d97fcd8b0e801204841b2) nautilus (dev)
 1: (ceph::__ceph_assert_fail(char const*, char const*, int, char const*)+0x14a) [0x7f4438958336]
 2: (ceph::__ceph_assertf_fail(char const*, char const*, int, char const*, char const*, ...)+0) [0x7f4438958504]
 3: (Throttle::put(long)+0x3e9) [0x7f44389df339]
 4: (DispatchQueue::dispatch_throttle_release(unsigned long)+0x41) [0x7f4438b44b81]

//a/sage-2019-02-17_17:29:20-rados-wip-sage-testing-2019-02-17-0921-distro-basic-smithi/3604419

rzarzynski · 2019-02-18T17:37:21Z

@liewegas: it looks dispatch_queue->dispatch_throttle_release() was operating on not-yet-parsed current_header instead of rx_segments_todo_rev. Pushed a fixup.

src/include/inline_memory.h