rgw / kmip kms #33996

mdw-at-linuxbox · 2020-03-16T22:03:29Z

This is a preview of a set of patches to add kmip support to ceph.

Included: new submodule "libkmip", links to build that, logic to use kmip as another kms key provider in ceph, and logic to set up "pykmip" in teuthology.

Need to do,
1/ use ceph repo fo libkmip. (this is a branched version from the upstream for libkmip with significant changes (which they will hopefully adopt...))
2/ more teuthology logic to actually use pykmip...
3/ documentation.
also, apparently, 4/ update to latest master.

I'll update this PR as these things get done.

cbodley · 2020-03-17T17:30:09Z

src/rgw/rgw_kms.cc

+{
+	RGWKMIPTransceiver secret_req(cct, RGWKMIPTransceiver::LOCATE);
+
+	secret_req.name = (char *) work.c_str();	// XXX ugh constness


c++17 added a non-const std::string::data(), so this can just be secret_req.name = work.data();

cbodley · 2020-03-18T16:37:11Z

src/rgw/rgw_kms.cc

+protected:
+	KmipGetTheKey(CephContext *cct) : cct(cct) {}
+	KmipGetTheKey& keyid_to_keyname(boost::string_view key_id);
+	KmipGetTheKey& get_uniqueid_for_keyname();


the builder pattern feels really awkward here. std::string work has a different meaning between the steps, and each step has to check failed instead of returning errors directly. can we refactor KmipGetTheKey into static functions that return int and take their state as arguments?

it's kind of the basic idea of the class; work is an internal state; it would be similar to object to the fact that kmip_client is basically std C, constantly allocating objects and buffers that you wouldn't in modern c++--I'm tempted, but it seems better to ask "does it work?" and then valgrind it

cbodley · 2020-03-18T16:39:50Z

src/rgw/rgw_main.cc

@@ -301,6 +304,7 @@ int radosgw_Main(int argc, const char **argv)
  rgw_init_resolver();
  rgw::curl::setup_curl(fe_map);
  rgw_http_client_init(g_ceph_context);
+  rgw_kmip_client_init(*new RGWKMIPManagerImpl(g_ceph_context));


can init() just take g_ceph_context to hide the dependency on RGWKMIPManagerImpl?

should we be relying on g_ceph_context?

cbodley · 2020-03-18T16:42:41Z

src/rgw/rgw_kmip_client_impl.cc

+	}
+	RGWKmipHandleBuilder& set_username(const std::string &v) {
+		const char *s = v.c_str();
+		if (*s) {


std::string::c_str() will not return null

That's good, because if it did, "*s" would core dump. What it's actually checking is that there is at least one non-nul character, ie, not the empty string. The empty string is the default in the config logic and this code interprets that to mean "don't do password authentication", which is a required feature.

src/rgw/rgw_kmip_client_impl.cc

cbodley · 2020-03-18T19:31:22Z

@mdw-at-linuxbox you mentioned that SKLM has new REST apis; if we're able to use those, that could simplify things quite a bit. it would also make it easier to support coroutines, since optional_yield is already built into the http client

mattbenjamin · 2020-03-18T19:34:18Z

@cbodley I'm editing my comment here. I don't think that using libkmip is an inappropriate integration path--in fact, it's desirable because it is an OASIS standard which, we hope, will allow us to interop with other KMIP-enabled key management implementations

mattbenjamin · 2020-04-05T16:53:17Z

@cbodley @mdw-at-linuxbox updated comment on libmip integration

stale · 2020-06-05T03:55:23Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

mdw-at-linuxbox · 2020-07-08T22:27:19Z

I believe sklm's rest interface is unique to it - not likely to work with anything else.

I pushed an update to this rebased against the latest master.

mattbenjamin · 2020-07-28T12:34:01Z

@mdw-at-linuxbox can we get an update to this PR and allow it to merge upstream?

stale · 2020-09-27T08:13:35Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

mdw-at-linuxbox · 2020-09-30T01:10:17Z

This is based on a fresher copy of master.

mdw-at-linuxbox · 2020-10-23T19:11:19Z

I pushed a new version: rebased on later master, & some fixes for string handling recommended by reviewers above. I don't know if this will please "make check", - if not now I know to save the complaints before the details evaporate.

mdw-at-linuxbox · 2020-10-29T16:19:34Z

"make check" objected to the pykmip teuthology task. I've revamped that and verified it could deploy pykmip in a toy environment.

cbodley · 2021-02-11T18:17:43Z

jenkins render docs

ceph-jenkins · 2021-02-11T18:30:43Z

Doc render available at https://ceph--33996.org.readthedocs.build/en/33996/

cbodley

nice work on docs!

cbodley · 2021-02-11T18:24:16Z

src/rgw/rgw_kmip_client.h

+	ceph::mutex lock = ceph::make_mutex("rgw_kmip_req::lock");
+	ceph::condition_variable cond;
+
+	int wait(optional_yield y);


if wait() and process() don't support yielding on the coroutine, they shouldn't take an optional_yield

cbodley · 2021-02-11T18:25:13Z

src/rgw/rgw_kms.cc

+#if 0
+  std::string secret_engine = cct->_conf->rgw_crypt_vault_secret_engine;
+  ldout(cct, 20) << "Vault authentication method: " << cct->_conf->rgw_crypt_vault_auth << dendl;
+  ldout(cct, 20) << "Vault Secrets Engine: " << secret_engine << dendl;
+#endif


please don't leave dead code

cbodley · 2021-02-11T18:39:04Z

src/rgw/rgw_kmip_client_impl.cc

+	explicit RGWKmipHandle() :
+		uses(0), ctx(0), ssl(0), bio(0),
+		need_to_free_kmip(0),
+		encoding(0) {
+			memset(kmip_ctx, 0, sizeof kmip_ctx);
+			memset(textstrings, 0, sizeof textstrings);


nit: please follow ceph's https://github.com/ceph/ceph/blob/master/CodingStyle#L76-L82 re: two-space indentation

cbodley · 2021-02-11T18:41:36Z

doc/radosgw/encryption.rst

+See `OpenStack Barbican Integration`_, `HashiCorp Vault Integration`_.
+and `KMIP Integration`_.


comma instead of period between HashiCorp Vault Integration. and KMIP Integration.

mattbenjamin · 2021-02-17T14:58:25Z

@mdw-at-linuxbox can you please expedite pushing an update addressing @cbodley comments from 2/11/2020?
tx!

mattbenjamin · 2021-02-18T22:14:57Z

doc/radosgw/kmip.rst

+#. `Upload object`_
+
+Before you can use KMIP with Ceph, you will need to do three things.
+You will need to associate ceph with client information in KMIP,


do we care about consistently capitalizing (or not) Ceph?

I dunno. In the doc directory, there are:
3106 Ceph
30976 ceph
8 CEPH
and there are 51 occurrence of "with Ceph". Granted, I'm not being terribly consistent even with myself. I was more worried about how to spell SKLM, especially since IBM seems to have renamed it.

mattbenjamin · 2021-02-18T22:16:28Z

doc/radosgw/kmip.rst

+using SKLM.  Ceph should then be configured (see below) to
+use KMIP and an attempt made to use it.  This will fail,
+but it will leave an "untrusted client device certificate" in SKLM.
+interface to complete the registration process.


sentence fragment here--was it trying to say "Use the SKLM" (interface) and continuing?

What sentence fragment? "Certificates may be created using SKLM." is no fragment. I think in this case github diff formatting is making mental mush with its helpful line rapping.

but it will leave an "untrusted client device certificate in SKLM. <-- full stop here
interface to complete the registration process. (so, is "interface to complete..." connected to certificate in SKLM?

Ah! Yes, that is a fragment. Will fix.

mattbenjamin · 2021-02-18T22:18:01Z

doc/radosgw/kmip.rst

+You may need to change the paths above to match where
+you actually want to store kmip certificate data.
+
+The kmis key template describes how ceph will modify


typo, will fix.

mattbenjamin · 2021-02-18T22:23:28Z

src/rgw/rgw_kmip_client_impl.cc

+  TextString *up = 0;
+	size_t ns;
+
+//??  OPENSSL_init_ssl(0, NULL);  // XXX


commented code

Right, fossil, I'll remove it.

mattbenjamin · 2021-02-18T22:24:16Z

src/rgw/rgw_kmip_client_impl.cc

+  }
+  if (kmip) {
+  } else if (!hostaddr) {
+    // kmip = 0;


commented code

well, it helped me. Evidently not helpful to others; I'll remove.

mattbenjamin · 2021-02-18T22:24:40Z

src/rgw/rgw_kmip_client_impl.cc

+  if (cleaner_shutdown) {
+    release_kmip_handle_now(kmip);
+  } else {
+//    kmip_easy_reset(**kmip);


commented code

this was meant to be a place to recycle any sticky bits in the handle once I had them, similar to how curl works. Only problem is I never had any sticky bits. I'll remove this.

mattbenjamin · 2021-02-18T22:26:34Z

src/rgw/rgw_kmip_client_impl.h

+    RGWKMIPTransceiver &details;
+    Request(RGWKMIPTransceiver &details) : details(details) {}
+  };
+  boost::intrusive::list<Request, boost::intrusive::member_hook< Request,


still love seeing boost::intrusive::list

glad you liked it.

mattbenjamin · 2021-02-18T22:29:27Z

qa/tasks/pykmip.py

+            key1_json = json.dumps(
+                {
+                    "name": secret['name'],
+                    "expiration": "2020-12-31T19:14:44.180394",


what is this date literal (in the past) doing for us?

(does it represent an expired key?)

This code was inherited from task keystone. You'll notice the rest of this method is doing keystone things involving json & http, not kmip things. And it starts off with a try/yield/finally block which in teuthlogy land is a standard no-op. A later commit "rgw/kms/kmip - pykmip.py needs to make keys too." replaces all of this with code that actually works--and is much shorter.

A terrible way to fix this would be to go back and remove the placeholder code from the old commit. It's terrible because this placeholder code in fact has the tested behavior that it does nothing without breaking (and yet is clearly Just Wrong), and any replacement code would (should) be tested to verify the same behavior. Another way to fix this would be to squash this commit in with the earlier code - that's not bad except it loses the commit message which I think might actually be helpful but I don't think belongs inline with the code.

mattbenjamin · 2021-02-18T22:30:41Z

qa/tasks/pykmip.py

+    """
+    assert config is None or isinstance(config, list) \
+        or isinstance(config, dict), \
+        "task keystone only supports a list or dictionary for configuration"


task keystone?

Yes, "task keystone". I dunno, think "Japanese manga?" It's exactly the same language used in 14 other teuthology tasks for about the same problem, so I think this should stay as is.

mdw-at-linuxbox · 2021-02-18T23:26:49Z

I pushed fixes to the issues cbodley addressed above (+rebase to latest master) - looks like there are yet more issues to resolve.

mdw-at-linuxbox · 2021-03-02T09:02:47Z

I've pushed an update that (1) addresses various nits above, and (2) is rebased on the latest master. The vault PR also contains all these commits; I plan to run that through ceph-ci + teuthology.

Signed-off-by: Marcus Watts <mwatts@redhat.com>

First pass at configuration configuration for kmip. Signed-off-by: Marcus Watts <mwatts@redhat.com>

This implements SSE-KMS for the radosgw using kmip. This uses symmetric raw keys with a name attribute in kmip, so providing the same functionality as the "kv" key store in hashicorp vault. Signed-off-by: Marcus Watts <mwatts@redhat.com>

Configure and run a simple pykmip daemon, similar in concept to barbican | vault. Signed-off-by: Marcus Watts <mwatts@redhat.com>

Use string::data and string_view to clean up some string handling, as suggested by reviewers. Signed-off-by: Marcus Watts <mwatts@redhat.com>

python3 requires different imports and there's a different way to get at the first element in a view. This is to match changes introduced in the rest of ceph in these commits: 24e7acc d7258ea Signed-off-by: Marcus Watts <mwatts@redhat.com>

The logic to deploy pykmip in teuthology was not complete. While it deployed all the code and certs to run pykmip, it didn't actually run it. This commit fixes that. Signed-off-by: Marcus Watts <mwatts@redhat.com>

The logic to deploy pykmip in teuthology was not complete. The necessary logic to add kmip keys was missing. Existing logic for other key services providers could use rest based protocols directly from the teuthology host. For kmip, it is necessary to use a special protocol, and it is more convenient to run this directly on the pykmip server. Signed-off-by: Marcus Watts <mwatts@redhat.com>

The pykmip task should be after ceph, and before rgw. kmip needs ssl certs in order to function correctly. Because the openssl_keys task has an indeterminate order of execution, it is best to create the ca as a separate task. The ca can be shared with rgw, but real life deployments of kmip are likely to have their own CA. In order to create kmip secrets, a client certificate is necessary, so must be supplied to the pykmip task. Signed-off-by: Marcus Watts <mwatts@redhat.com>

Pass endpoint configuration from pykmip to radosgw at runtime. Signed-off-by: Marcus Watts <mwatts@redhat.com>

s3tests needs to know key names in order to run kms tests. It seems desirable to have s3tests default to discovering the names that were created by the pykmip task, and that if there is more than one rgw connected to more than one pykmip, that names belonging to the appropriate pykmip instance should be used. This logic does the following: rgw task: save pykmip role name. s3tests task: set kms_key (and kms_keyid2) to these in order of priority 1 s3tests client task property ['kms_key'] (or ['kms_key2']) 2 first (second) secret created in the matching pykmip instance. 3 testkey-1 (testkey-2) For case 2, names from the secrets have an initial "token-" stripped from them. The assumption here is that rgw is being run with a setting such as rgw crypt kmip kms key template: pykmip-$keyid therefore "pykmip-" will be prefixed back onto the key before use. Signed-off-by: Marcus Watts <mwatts@redhat.com>

Actually add kmip to the kms crypt suite. This also makes some ssl certs which is required for use of kmip. Signed-off-by: Marcus Watts <mwatts@redhat.com>

I've written up a brief description of using kmip with ceph. Major features: * ceph configuration. * making keys with a "paste-in" python script. * pointers to PyKMIP and IBM SKLM. Signed-off-by: Marcus Watts <mwatts@redhat.com>

mdw-at-linuxbox · 2021-03-04T00:29:56Z

The "transit" PR which contains these commits and more needed to be rebased on the latest master. I've done so, and I've pushed the kmip subset patches from that PR to this PR.

mdw-at-linuxbox requested review from mattbenjamin, kalebskeithley, scarvalhojr and cbodley March 16, 2020 22:03

mdw-at-linuxbox assigned cbodley Mar 16, 2020

tchaikov added the rgw label Mar 17, 2020

cbodley added the feature label Mar 17, 2020

cbodley reviewed Mar 18, 2020

View reviewed changes

stale bot added the stale label Jun 5, 2020

mdw-at-linuxbox force-pushed the wip-master-kmip branch from e8bdbf9 to 4112db2 Compare July 8, 2020 22:13

stale bot removed the stale label Jul 8, 2020

stale bot added the stale label Sep 27, 2020

mdw-at-linuxbox force-pushed the wip-master-kmip branch from e641e7c to afe10be Compare September 30, 2020 01:07

stale bot removed the stale label Sep 30, 2020

This was referenced Oct 2, 2020

rgw / kmip / backport to nautilus. #37517

Closed

[DNM PENDING CHERRY-PICK RE-DO] nautilus: rgw: backport of "kmip kms" and Hashicorp Vault features to nautilus #37518

Closed

mdw-at-linuxbox force-pushed the wip-master-kmip branch from afe10be to 3df0e5b Compare October 23, 2020 19:06

mdw-at-linuxbox force-pushed the wip-master-kmip branch from 6cd68a1 to 8de47c3 Compare October 29, 2020 16:15

mdw-at-linuxbox force-pushed the wip-master-kmip branch 2 times, most recently from c951d11 to a990aae Compare October 30, 2020 19:48

mdw-at-linuxbox mentioned this pull request Feb 10, 2021

rgw: new vault transit logic for SSE-C #38605

Merged

cbodley reviewed Feb 11, 2021

View reviewed changes

mdw-at-linuxbox force-pushed the wip-master-kmip branch from b4b172d to 99aa7e0 Compare February 18, 2021 21:40

mattbenjamin reviewed Feb 18, 2021

View reviewed changes

mdw-at-linuxbox force-pushed the wip-master-kmip branch from 99aa7e0 to 2f9c791 Compare March 2, 2021 08:52

mdw-at-linuxbox added 15 commits March 3, 2021 19:14

Added libkmip as a submodule.

bb72f10

Signed-off-by: Marcus Watts <mwatts@redhat.com>

cmake glue to build libkmip.

3ba1384

Signed-off-by: Marcus Watts <mwatts@redhat.com>

Including cmake build logic inside of libkmip.

4dee603

Signed-off-by: Marcus Watts <mwatts@redhat.com>

kmip: configuration options.

4804310

First pass at configuration configuration for kmip. Signed-off-by: Marcus Watts <mwatts@redhat.com>

kmip: first pass at implementation logic.

0430ff7

This implements SSE-KMS for the radosgw using kmip. This uses symmetric raw keys with a name attribute in kmip, so providing the same functionality as the "kv" key store in hashicorp vault. Signed-off-by: Marcus Watts <mwatts@redhat.com>

teuthology/rgw: pykmip task

c7afcbd

Configure and run a simple pykmip daemon, similar in concept to barbican | vault. Signed-off-by: Marcus Watts <mwatts@redhat.com>

rgw/kms/kmip - string handling cleanup.

0d46a62

Use string::data and string_view to clean up some string handling, as suggested by reviewers. Signed-off-by: Marcus Watts <mwatts@redhat.com>

rgw/kms/kmip - pykmip.py should actually run pykmip.

1e457c5

The logic to deploy pykmip in teuthology was not complete. While it deployed all the code and certs to run pykmip, it didn't actually run it. This commit fixes that. Signed-off-by: Marcus Watts <mwatts@redhat.com>

rgw/kms/kmip - rgw / kmip test integration.

7105fa6

Pass endpoint configuration from pykmip to radosgw at runtime. Signed-off-by: Marcus Watts <mwatts@redhat.com>

rgw/kms/kmip - rgw / kmip test integration.

809677d

Actually add kmip to the kms crypt suite. This also makes some ssl certs which is required for use of kmip. Signed-off-by: Marcus Watts <mwatts@redhat.com>

mdw-at-linuxbox force-pushed the wip-master-kmip branch from 2f9c791 to 891bf1a Compare March 4, 2021 00:22

cbodley approved these changes Mar 5, 2021

View reviewed changes

mattbenjamin merged commit d2c30d7 into ceph:master Mar 5, 2021

mdw-at-linuxbox mentioned this pull request Mar 12, 2021

pacific: rgw / kmip kms #40096

Closed

		See `OpenStack Barbican Integration`_, `HashiCorp Vault Integration`_.
		and `KMIP Integration`_.

rgw / kmip kms #33996

rgw / kmip kms #33996

Conversation

mdw-at-linuxbox commented Mar 16, 2020

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

cbodley commented Mar 18, 2020

mattbenjamin commented Mar 18, 2020 • edited

mattbenjamin commented Apr 5, 2020

stale bot commented Jun 5, 2020

mdw-at-linuxbox commented Jul 8, 2020

mattbenjamin commented Jul 28, 2020

stale bot commented Sep 27, 2020

mdw-at-linuxbox commented Sep 30, 2020

mdw-at-linuxbox commented Oct 23, 2020

mdw-at-linuxbox commented Oct 29, 2020

cbodley commented Feb 11, 2021

ceph-jenkins commented Feb 11, 2021

cbodley left a 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

mattbenjamin commented Feb 17, 2021

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

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

mdw-at-linuxbox commented Feb 18, 2021

mdw-at-linuxbox commented Mar 2, 2021

mdw-at-linuxbox commented Mar 4, 2021

mattbenjamin commented Mar 18, 2020 •

edited