{"action":"created","issue":{"url":"https://api.github.com/repos/docker/swarmkit/issues/1598","repository_url":"https://api.github.com/repos/docker/swarmkit","labels_url":"https://api.github.com/repos/docker/swarmkit/issues/1598/labels{/name}","comments_url":"https://api.github.com/repos/docker/swarmkit/issues/1598/comments","events_url":"https://api.github.com/repos/docker/swarmkit/issues/1598/events","html_url":"https://github.com/docker/swarmkit/pull/1598","id":180826885,"number":1598,"title":"Wrapper around the etcd wal and snap packages for encryption","user":{"login":"cyli","id":223200,"avatar_url":"https://avatars.githubusercontent.com/u/223200?v=3","gravatar_id":"","url":"https://api.github.com/users/cyli","html_url":"https://github.com/cyli","followers_url":"https://api.github.com/users/cyli/followers","following_url":"https://api.github.com/users/cyli/following{/other_user}","gists_url":"https://api.github.com/users/cyli/gists{/gist_id}","starred_url":"https://api.github.com/users/cyli/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cyli/subscriptions","organizations_url":"https://api.github.com/users/cyli/orgs","repos_url":"https://api.github.com/users/cyli/repos","events_url":"https://api.github.com/users/cyli/events{/privacy}","received_events_url":"https://api.github.com/users/cyli/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/docker/swarmkit/labels/status/0-triage","name":"status/0-triage","color":"ededed"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2016-10-04T07:53:06Z","updated_at":"2016-10-05T05:42:50Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/docker/swarmkit/pulls/1598","html_url":"https://github.com/docker/swarmkit/pull/1598","diff_url":"https://github.com/docker/swarmkit/pull/1598.diff","patch_url":"https://github.com/docker/swarmkit/pull/1598.patch"},"body":"A simple implementation of a wrapper that would implement the on-disk snapshot encryption and on-disk WAL encryption as described here:  https://docs.google.com/document/d/1YxMH2oIv-mtRcs1djRkm0ndLfiteo0UzBUFKYhLKYkQ/edit#.\r\n\r\nThis is going to rely on `raft.go` or `storage.go` to tell it whether something is encrypted, and with which algorithm.  For WALs, such information can be included in the metadata of the file, but no such metadata exists for snapshots.  Another option is to base this on the snapshot and WAL directory locations/names, or on a metadata file that lives alongside the raft store.\r\n\r\nI also have an attempt at a more [complex implementation](https://github.com/docker/swarmkit/compare/master...cyli:wal-snapshot-wrapper-mux?expand=1) that attempts to wrap some of the encrypted data in a structure that includes such metadata (encryption algorithm) and would be able to read both encrypted and unencrypted data (old and new format data).  However, we might have to put the digest of the data or some other magic data into the wrapped message in order to correctly determine whether some protobuf packed data is actually a wrapped message or unwrapped regular data.\r\n\r\nWould be cool to upstream this at some point in the future into etcd, though I'm not sure if it can go into the existing layer that writes to file, since none of the serialization formats, etc. provide support for specifying your own decoder/encoder.  May be a breaking API/format change, if so."},"comment":{"url":"https://api.github.com/repos/docker/swarmkit/issues/comments/251587020","html_url":"https://github.com/docker/swarmkit/pull/1598#issuecomment-251587020","issue_url":"https://api.github.com/repos/docker/swarmkit/issues/1598","id":251587020,"user":{"login":"cyli","id":223200,"avatar_url":"https://avatars.githubusercontent.com/u/223200?v=3","gravatar_id":"","url":"https://api.github.com/users/cyli","html_url":"https://github.com/cyli","followers_url":"https://api.github.com/users/cyli/followers","following_url":"https://api.github.com/users/cyli/following{/other_user}","gists_url":"https://api.github.com/users/cyli/gists{/gist_id}","starred_url":"https://api.github.com/users/cyli/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cyli/subscriptions","organizations_url":"https://api.github.com/users/cyli/orgs","repos_url":"https://api.github.com/users/cyli/repos","events_url":"https://api.github.com/users/cyli/events{/privacy}","received_events_url":"https://api.github.com/users/cyli/received_events","type":"User","site_admin":false},"created_at":"2016-10-05T05:42:50Z","updated_at":"2016-10-05T05:42:50Z","body":"> My preference would be to wait for the whole encryption feature to be ready before merging this, if that works for you. It will let people review it in context and make sure it fits the model.\r\n\r\n@aaronlehmann Sure, no worries - was just trying to keep the PRs smaller.  :)\r\n\r\n> So maybe some other mechanism, like file naming, is better. I guess we should start by asking what exactly information we want to store.\r\n\r\nI was originally thinking the encryption algorithm, the hash if we're not going to do GCM and hence need to provide our own digest.  We could  possibly the ID of the key, if we store the encryption key as a secret.\r\n\r\n> Is there a way to simply encrypt the stream, rather than having the encryption work at the message-level? This approach intertwines the encryption format with the message structure, which is a level of coupling that isn't really necessary. We can see this in the fact that term and index aren't actually encrypted.\r\n\r\n@stevvooe I agree it's not great to tie things to the `Record` structure, although I was thinking the index and the term would give us a non-repeating IV (which would have to be stored somewhere unencrypted anyway, maybe prefixed to the ciphertext, or generated with a shared nonce).\r\n\r\nThe WAL records and snapshots are serialized inside the `wal` and `snap` packages before writing to disk (none of the attributes of the `WAL` and `Snapshotter` types are exported - I couldn't find a good place to inject anything, but I may have just missed it), so I think we'd basically have to maintain a fork of at least those packages (or get this upstream) in order to do so as you suggest.  They'd have to accept the writer or some `cipher.*`, and also write in the encryption format of the file somewhere (filename or in the metadata object).  \r\n\r\nAnother approach @NathanMcCauley suggested is that we can encrypt above the raft layer.\r\n\r\nSo we can do that at the `Store` before it gets into the snapshot and logs, perhaps here:  https://github.com/docker/swarmkit/blob/master/manager/state/store/memory.go#L663  and here  https://github.com/docker/swarmkit/blob/master/manager/state/store/memory.go#L441?  Or during per-object serialization.\r\n\r\nWe can pass a [`cipher.Stream`](https://golang.org/pkg/crypto/cipher/#Stream) or something to `MemoryStore`.  This would mean we'd have to change the [`api.StoreAction`](https://github.com/docker/swarmkit/blob/master/api/raft.proto#L119) and [`api.Snapshot`](https://github.com/docker/swarmkit/blob/master/api/snapshot.proto) protocols... But maybe this way https://github.com/docker/swarmkit/issues/363 can be addressed at the same time.\r\n\r\nThis also means that the snapshots and raft logs will also be sent encrypted (beyond just TLS) between peers, which is a feature we were looking for but not necessarily a bad thing.  Each node would have to be bootstrapped with the encryption key before being able read the `Store` part of snapshots, or apply raft log entries involving the `Store`, but we would have needed to unlock the nodes on restart at any rate, with the previous design.  \r\n\r\nThe problem is that encryption key distribution to all the nodes using the `Store` becomes more complex, since we don't necessarily know that all the nodes (just a majority) have gotten the key (please correct me if my assumption is incorrect - I think we only apply an update to the `Store` when the proposed change has been committed, which means the log has successfully been replicated to a majority of the nodes).  So if we need to catch up a node that's really far behind and needs a snapshot, that snapshot may be encrypted with a new key.  This isn't great because it would require unlocking after a particularly egregious network partition during a key rotation, as opposed to just on restart.\r\n\r\nPossibly this can be dealt with using the 3 key network rotation thing, and just let users rotate key encryption keys.  Or maybe we can use the manager TLS keys to encrypt the `Store` encryption key (and encrypt the TLS certificates themselves).\r\n\r\nI think that wrapping the `wal` and `snapshot` packages is the simplest, but I agree not the cleanest.  I'm not sure which of the above two options would be better - each have their tradeoffs too."}}
{"actor":{"display_login":"cyli"}}