Skip to content
This repository has been archived by the owner. It is now read-only.

Cpd 101 #102

Merged
merged 3 commits into from Apr 17, 2017

Conversation

Projects
None yet
2 participants
@ashcrow
Copy link
Member

ashcrow commented Mar 2, 2017

For discussion per #101

@ashcrow ashcrow requested a review from mbarnes Mar 2, 2017

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Mar 2, 2017

/cc @gbraad

---------
The likelihood of having a ``Storage`` system that is used only for Commissaire seems
low. More than likely the same instance will be used for other applications as well.
By adding encryption to the keys we mitigate could mitigate access from those with

This comment has been minimized.

@mbarnes

mbarnes Mar 3, 2017

Contributor

I think "keys we mitigate" is supposed to be "keys we store"?

This comment has been minimized.

@ashcrow

ashcrow Mar 3, 2017

Author Member

I should have been 'to the keys, we could mitigate'. Will update.

------
The server(s) would house a GnuPG configuration and keys allowing them to encrypt
and decrypt data. The servers would either need to have the same key across all
instances or each server would need a copy of other services public keys.

This comment has been minimized.

@mbarnes

mbarnes Mar 3, 2017

Contributor

Can you give more details about what the configuration might look like?

This comment has been minimized.

@ashcrow

ashcrow Mar 3, 2017

Author Member

I don't think there would be any configuration updates. However, the public/private keys for use would need to be imported into the keyring at /etc/commissaire/$PLACEWEDECIDEON

the key encrypted as long as possible.

The HTTP Server would need to handle decrypting the key back into a base64 encoded
string before handing it back to the authenticated requestor.

This comment has been minimized.

@mbarnes

mbarnes Mar 3, 2017

Contributor

ClusterExecService, InvestigatorService and WatcherService must also be able to decrypt the keys, right?

This comment has been minimized.

@ashcrow

ashcrow Mar 3, 2017

Author Member

Hrm, that's a good point. A few things come immediately to mind. We could:

  • Have the key decrypted at the StorageService level. (Easiest but removes some of the protection since the key may flow over the same backend service)
  • Have the services request the decrypted key from the REST server
  • Have services generate and register a gpg key with the server (this would require a new endpoint but would be doable and probably cleaner)

This comment has been minimized.

@ashcrow

ashcrow Mar 6, 2017

Author Member

The more I think about it the more I feel it makes sense for StorageService to do the decryption. We'd need to explicitly note in the documentation that the bus level would need to be trusted.

This comment has been minimized.

@mbarnes

mbarnes Mar 6, 2017

Contributor

Kombu supports SSL/TLS at the Connection layer for some transports:

SSL compatibility

SSL currently only works with the py-amqp, amqplib, and qpid transports.
For other transports you can use stunnel.

Perhaps we could pass SSL/TLS options for kombu.Connection from a config file?

source

This comment has been minimized.

@ashcrow

ashcrow Mar 6, 2017

Author Member

@mbarnes Yeah, I think that would make sense. We will need to add a quick example on how to use stunnel as well but that is pretty trivial.

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Mar 6, 2017

Updated.

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Mar 16, 2017

Updated. I need to re-add the stunnel stuff back to it though.

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Mar 16, 2017

⬆️

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Mar 21, 2017

@mbarnes ⬆️

string before handing it back to the requesting service.
The StorageService would need to to know when to use Custodia versus the configured
``StorageHandler``(s). It would look at the ``_secrets`` attribute on the instance and,
if set to ``True`` would use the secrets handler.

This comment has been minimized.

@mbarnes

mbarnes Mar 21, 2017

Contributor

This sentence was confusing at first since the _secrets attribute isn't defined until a few paragraphs below.

This comment has been minimized.

@ashcrow

ashcrow Mar 21, 2017

Author Member

Fair enough. On next update I'll try to clarify.

The models would match based on their primary keys: ``address``.

The ``Model`` class would also gain a new attribute to denote the model is secret. A secret model would
denote ``_secrets = True``.

This comment has been minimized.

@mbarnes

mbarnes Mar 21, 2017

Contributor

Instead of an attribute, I suggest a class hierarchy.

       Model
         |
  +------+------+
  |             |
  |        SecretModel
  |             |
 Host       HostCreds

If nothing else, it would make for clearer documentation in APIs where we expect a secret model as opposed to any type of model.

:param secret_model: A model containing sensitive data
:type secret_model: commissaire.models.SecretModel

This comment has been minimized.

@ashcrow

ashcrow Mar 21, 2017

Author Member

I like that. I'll update.

New HTTP Handler
~~~~~~~~~~~~~~~~
A new handler called ``secrets`` could be added. This would proxy requests back to the
Custodia instance.

This comment has been minimized.

@mbarnes

mbarnes Mar 21, 2017

Contributor

Okay, so if the "secrets" handler is using a Unix socket to proxy to Custodia, does that imply Custodia has to be running on the same machine as commissaire-server? In the example below, it looks like commissaire-storage-service has the Custodia config, and that service may be running on a different machine than commissaire-server.

Would it make sense for commissaire-storage-service to call back to our own /api/v0/secrets endpoint, instead of accessing Custodia directly?

I feel like I'm not seeing a piece of this clearly.

This comment has been minimized.

@ashcrow

ashcrow Mar 21, 2017

Author Member

commissaire-storage-service would call back to /api/v0/secrets. We'd be proxying the requests back to custodia on the server side.

This comment has been minimized.

@mbarnes

mbarnes Mar 21, 2017

Contributor

Got'cha. So in that case do we still need the "custodia_api_id" and "custodia_api_key" things in the storage config? It's not clear to me what those are for. Wait, I think I get it now. That's linkage with the Custodia config, right? Didn't look at that part closely.

This comment has been minimized.

@ashcrow

ashcrow Mar 21, 2017

Author Member

Correct. Those would be used as the auth into custodia through our endpoint. Since we'd just be proxying access and allowing custodia to handle it's own access we'd need to add the auth changes listed.

This comment has been minimized.

@ashcrow

ashcrow Mar 21, 2017

Author Member

Admittedly there are a lot of small changes to get this to work.

This comment has been minimized.

@mbarnes

mbarnes Mar 21, 2017

Contributor

True. I think I understand now how this all fits together, conceptually at least.

This comment has been minimized.

@ashcrow

ashcrow Mar 21, 2017

Author Member

That's linkage with the Custodia config, right? Didn't look at that part closely.

Yup!

This comment has been minimized.

@mbarnes

mbarnes Mar 21, 2017

Contributor

One other piece to this is how commissaire-storage-service learns where the /api/v0/secrets endpoint is. Might be nice if commissaire-http could slip this info to it somehow so we can avoid another line item in storage.conf.

(... or bring back the configuration-in-etcd concept from MVP ...)

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Mar 21, 2017

⬆️

@@ -180,7 +178,7 @@ These are examples and likely will not work without modification.
handler is used. Raises KeyError if no handler is registered
for that type of model.
"""
if model._secret:
if issubclass(model.__class__, models.SecretModel):

This comment has been minimized.

@mbarnes

mbarnes Mar 21, 2017

Contributor

simpler: if isinstance(model, models.SecretModel):

@mbarnes

This comment has been minimized.

Copy link
Contributor

mbarnes commented Mar 21, 2017

Looking good! 👍

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Mar 22, 2017

@mbarnes is there anything else you can think of that needs to be addressed in the CPD?

Of course I'd like to get another eye on it as well before it's ratified /cc @gbraad @portdirect

@mbarnes mbarnes referenced this pull request Mar 24, 2017

Merged

Add ListModel subclass #119

@mbarnes

This comment has been minimized.

@mbarnes

This comment has been minimized.

Copy link
Contributor

mbarnes commented Mar 29, 2017

@mbarnes

This comment has been minimized.

@mbarnes

This comment has been minimized.

Copy link
Contributor

mbarnes commented Mar 29, 2017

  • Verify a card for installing Custodia is created

@ashcrow: About that ⬆️ ... now that I'm taking a closer look at Custodia, it's not clear to me whether the responsibility of installing Custodia and configuring it correctly falls on Commissaire or our users. Who will own Custodia's config file and how to you envision this all getting set up?

Should we create a container image for commissaire-http + Custodia, configured the way we want (Unix socket, encrypted-etcd backend, etc)? That might significantly simplify the setup instructions.

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Mar 29, 2017

@mbarnes I believe the version we use internally should be installed by us ... either via container, pip, or whatever. Exposing other instances would be up ops folks who want to provide them as extra services.

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Apr 4, 2017

Per CPD-1 let's go ahead and vote on this. I proposed it so I obviously 👍

@mbarnes PTAL

@mbarnes

This comment has been minimized.

Copy link
Contributor

mbarnes commented Apr 4, 2017

Some details may need clarified or changed as we get into the implementation, but I think the CPD looks good as an initial design document. 👍

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Apr 5, 2017

By rule this passes. @mbarnes please merge and update to accepted!

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Apr 12, 2017

@mbarnes ⬆️

@ashcrow ashcrow force-pushed the ashcrow:cpd-101 branch from 652950e to a75f012 Apr 17, 2017

@ashcrow

This comment has been minimized.

Copy link
Member Author

ashcrow commented Apr 17, 2017

I'll go ahead and mark it accepted and merge since @mbarnes 👍'd it

@ashcrow ashcrow force-pushed the ashcrow:cpd-101 branch from a75f012 to 53d9b14 Apr 17, 2017

@ashcrow ashcrow merged commit 972affd into projectatomic:master Apr 17, 2017

2 checks passed

Red Hat CI All tests passed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mbarnes

This comment has been minimized.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.