Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encrypted blob store repository #50846

Open
wants to merge 94 commits into
base: repository-encrypted-client-side
from

Conversation

@albertzaharovits
Copy link
Contributor

albertzaharovits commented Jan 10, 2020

This builds upon the data encryption streams from #49896 to create an encrypted snapshot repository. The repository encryption works with the following existing repository types: FS, Azure, S3, GCS. The encrypted repository is password protected. The platinum license is required to snapshot to the encrypted repository, but no license is required to list or restore already encrypted snapshots.

Example how to use the encrypted FS repository:

  • similarly to the un-encrypted FS repository, specify the mount point of the shared FS in the elasticsearch.yml conf file (on all the cluster nodes), eg: path.repo: ["/tmp/repo"]
  • add the password used to derive the encryption keys to the elasticsearch keystore on all the cluster nodes, eg for the test_enc repository name:
./bin/elasticsearch-keystore add repository.encrypted.test_enc.password
  • start-up the cluster, and create the new encrypted repository, eg:
curl -X PUT "localhost:9200/_snapshot/test_enc?pretty" -H 'Content-Type: application/json' -d'
{
  "type": "encrypted",
  "settings": {
    "location": "/tmp/repo/enc",
    "delegate_type": "fs"
  }
}
'

Overview how it works

Every data blob is encrypted (AES/GCM) independently with a randomly generated AES256 secret key. The key is stored in another metadata blob, which is itself encrypted (AES/GCM) with a key derived from the repository password. The metadata blob tree structure mimics the data blob tree structure, but it is rooted by the fixed blob container encryption-metadata.

I will detail more how each piece works by commenting in the code source.

Relates #49896
Obsoletes #48221

WIP
WIP
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

elasticmachine commented Jan 10, 2020

Pinging @elastic/es-security (:Security/Security)

listener.onFailure(new RepositoryException(metadata.name(),
"The local node's value for the keystore secure setting [" + passwordSettingForThisRepo.getKey() + "] does not " +
"match the master's"));
}

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 15, 2020

Author Contributor

Hi @tvernum @jkakavas @original-brownbear ! I feel I need your input regarding the password consistency check when snapshotting.

This check here assures that a snapshot cannot contain shards which are encrypted with different passwords. More precisely, given how the ConsistentSettingsService works, this check assures that a successful snapshot contains shards that are all encrypted with the current master's password for the repository. This works as intended and with no caveats.

What I wish to discuss is the case where passwords on some data nodes are different to the master's. In this case, only the shards allocated to data nodes which have the repository password identical to the repository password on the master node will be included in the snapshot. Here is an excerpt of the response to the caller:

{
  "snapshot" : {
    "snapshot" : "snapshot_4",
    "uuid" : "5pOG9Y9BQHGjxBXUYkgRCg",
    "version_id" : 8000099,
    "version" : "8.0.0",
    "indices" : [
      "twitter",
      "twitter2",
      "twitter3"
    ],
    "include_global_state" : true,
    "state" : "PARTIAL",
    "start_time" : "2020-01-15T12:56:42.078Z",
    "start_time_in_millis" : 1579093002078,
    "end_time" : "2020-01-15T12:56:42.687Z",
    "end_time_in_millis" : 1579093002687,
    "duration_in_millis" : 609,
    "failures" : [
      {
        "index" : "twitter3",
        "index_uuid" : "twitter3",
        "shard_id" : 0,
        "reason" : "org.elasticsearch.repositories.RepositoryException: [test_enc] Password mismatch for repository [test_enc]. The local node's value of the keystore secure setting [repository.encrypted.test_enc.password] is different from the master's\n\tat org.elasticsearch.repositories.encrypted.EncryptedRepository.snapshotShard(EncryptedRepository.java:101)\n\tat org.elasticsearch.snapshots.SnapshotShardsService.snapshot(SnapshotShardsService.java:341)\n\tat org.elasticsearch.snapshots.SnapshotShardsService.lambda$startNewShards$1(SnapshotShardsService.java:287)\n\tat org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:629)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\n\tat java.base/java.lang.Thread.run(Thread.java:830)\n",
        "node_id" : "pN1b441YTV6fKug6AkF2Vg",
        "status" : "INTERNAL_SERVER_ERROR"
      }
    ],
    "shards" : {
      "total" : 5,
      "failed" : 1,
      "successful" : 4
    }
  }
}

And the relevant excerpt on the data node:

[2020-01-15T14:56:42,453][WARN ][o.e.c.s.ConsistentSettingsService] [ElasticMBP.local] the published hash [2MuYL08uwIKlzrCKoSEbqKsNK2m5V9vdMcd48nRj2D73L8JlPAWUsMpDNlpbgX7PHX+C256ifRVih9aB6AEIjA==] of the consistent secure setting [repository.encrypted.test_enc.password] differs from the locally computed one [0U7M0OWwH5gj3vJBGbpIFzAME6CBht1kqH6IxDRuTB9T/SThMQ9VSGcffALpl4hs3lGIo6aCBJwm1Y3q7qXPhQ==]
[2020-01-15T14:56:42,468][WARN ][o.e.s.SnapshotShardsService] [ElasticMBP.local] [[twitter3][0]][test_enc:snapshot_4/5pOG9Y9BQHGjxBXUYkgRCg] failed to snapshot shard
org.elasticsearch.repositories.RepositoryException: [test_enc] Password mismatch for repository [test_enc]. The local node's value of the keystore secure setting [repository.encrypted.test_enc.password] is different from the master's
        at org.elasticsearch.repositories.encrypted.EncryptedRepository.snapshotShard(EncryptedRepository.java:101) [repository-encrypted-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.snapshots.SnapshotShardsService.snapshot(SnapshotShardsService.java:341) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.snapshots.SnapshotShardsService.lambda$startNewShards$1(SnapshotShardsService.java:287) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:629) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
        at java.lang.Thread.run(Thread.java:830) [?:?]

Given the way the "Repository" fits into the snapshot/restore framework, I don't see a better way without significant code refactoring. For example, a better way might be to completely fail the snapshot. But this is difficult to achieve because the snapshot process fans-out to all nodes, with no pre-flight check calls, and because nodes cannot push to master that their keystore settings are different.
Another possibly better alternative is to fail the creating the repository in the case that passwords differ. But in this case, the consistency check will race with the actual publication of the keystore hashes; failing features at startup on the basis of keystore consistency is a core design limitation.

What do you think? Is this good enough given the substantial changes required by alternatives that fail faster in the snapshotting process? Do you have other ideas we should explore? Note that in this case repository verification fails, so the admin has a common way to learn that the encrypted repository is healthy.

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jan 15, 2020

Member

@albertzaharovits I personally think fail fast is not absolutely necessary here. With the incremental nature of snapshotting, it's not like the partial snapshot resulting from some broken nodes will be a complete waste of effort.
With the upcoming changes to the snapshot process that will allow parallel repository operations, I think I'll be able to provide you with better hooks for failing faster in the future if we want that, but for now it's just fine IMO.

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 16, 2020

Author Contributor

Thank you for the feedback Armin!

I will anyway try to make the password-consistency-check work on the master node and then call it on the Repository#getRepositoryData method, which is always invoked on the master node.

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jan 16, 2020

Member

:) posted 20s after #50846 (comment) => I like the idea!

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 22, 2020

Author Contributor

Hi @original-brownbear . I've been working to come up with a solution for how to check that all the nodes (master and data) have the same repository password, but ultimately failed.

Against the appearances, this is a fundamentally different problem than what the ConsistentSettingsService currently does. That's because the publishing of the cluster state, i.e. broadcasting from the master to the other nodes, which is what ConsistentSettingsService does, is exactly the opposite of the communication pattern in which nodes must inform the master of password hashes. For this chief reason, unifying them under one service class is unsuitable. Moreover, this communication pattern in which the master node agrees that at least all of the nodes that could take part in the snapshot have the same value, has to account for changing cluster topology, i.e. adding a new node, replacing the master, and possibly reloading the keystore; this all is complex work. Also, it is not enough only for the master node to verify it before the snapshot starts, ultimately the node doing the snapshot must also verify the password against the others in the cluster, not to rely on the master's previous check because otherwise this could be manifesting as a use-after-check type of error (although I couldn't give concrete example situations).

I was trying to prototype a way to hook-up repository verification (client.admin().cluster().prepareVerifyRepository) in the Repository#getRepositoryData call of the encrypted repository, when @tvernum raised a very good point that the case of a wrong password on some of the nodes is analogous to the case of wrong client credentials for the cloud-based repositories. I think that's exactly the case. In this situation the feature to validate that credentials are consistent among the participating nodes should be useful across the board, for all cloud repositories.

Given this, I propose we backtrack to the original proposal to only validate that the password on the data node snapshotting the shard is the same as the master's and accept the usability shortfall that the end result might be an incomplete snapshot (including an empty snapshot). In addition, we should raise a feature request issue for the support of credential consensus.

@original-brownbear would you agree to this course of action?

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 22, 2020

Author Contributor

If this sounds correct to you, that indeed this is a separate issue, do you consider that issue a blocker for encrypted snapshots feature? I would say no (although I'm obviously biased in this matter), because the analogous problem of cloud repository credentials has been extant for so long.

(l, e) -> l.onFailure(new RepositoryException(metadata.name(), "Password mismatch for repository [" + metadata.name() +
"]. The local node's value of the keystore secure setting [" +
passwordSettingForThisRepo.getKey() + "] is different from the master's"))));
}

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 15, 2020

Author Contributor

@original-brownbear What do you think about this, in the case where we know the restoring will fail because the data node has the wrong password? I think attempting the restore is a more "robust" way.

Here are the logs, on the node:

[2020-01-15T15:19:17,865][WARN ][o.e.i.c.IndicesClusterStateService] [ElasticMBP.local] [twitter2][0] marking and sending shard failed due to [failed recovery]
org.elasticsearch.indices.recovery.RecoveryFailedException: [twitter2][0]: Recovery failed on {ElasticMBP.local}{pN1b441YTV6fKug6AkF2Vg}{uHr4xFDrQzWtykhxV_VO9Q}{127.0.0.1}{127.0.0.1:9301}{dilm}{ml.machine_memory=17179869184, xpack.installed=true, ml.max_open_jobs=20}
        at org.elasticsearch.index.shard.IndexShard.lambda$executeRecovery$21(IndexShard.java:2606) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.action.ActionListener$1.onFailure(ActionListener.java:71) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.lambda$recoveryListener$6(StoreRecovery.java:350) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.action.ActionListener$1.onFailure(ActionListener.java:71) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.lambda$restore$8(StoreRecovery.java:469) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.action.ActionListener$1.onFailure(ActionListener.java:71) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.restore(StoreRecovery.java:491) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.recoverFromRepository(StoreRecovery.java:278) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.IndexShard.restoreFromRepository(IndexShard.java:1850) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.IndexShard.lambda$startRecovery$17(IndexShard.java:2553) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.action.ActionRunnable$2.doRun(ActionRunnable.java:73) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:688) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
        at java.lang.Thread.run(Thread.java:830) [?:?]
Caused by: org.elasticsearch.index.shard.IndexShardRecoveryException: failed recovery
        ... 14 more
Caused by: org.elasticsearch.index.snapshots.IndexShardRestoreFailedException: restore failed
        ... 12 more
Caused by: org.elasticsearch.repositories.RepositoryException: [test_enc] could not read repository data from index blob
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1167) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1073) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.restore(StoreRecovery.java:482) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        ... 9 more
Caused by: java.io.IOException: Failure to decrypt metadata for blob [index-6]
        at org.elasticsearch.repositories.encrypted.EncryptedRepository$EncryptedBlobContainer.readBlob(EncryptedRepository.java:243) ~[?:?]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1153) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1073) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.restore(StoreRecovery.java:482) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        ... 9 more
Caused by: javax.crypto.AEADBadTagException: Tag mismatch!
        at com.sun.crypto.provider.GaloisCounterMode.decryptFinal(GaloisCounterMode.java:623) ~[?:?]
        at com.sun.crypto.provider.CipherCore.finalNoPadding(CipherCore.java:1116) ~[?:?]
        at com.sun.crypto.provider.CipherCore.fillOutputBuffer(CipherCore.java:1053) ~[?:?]
        at com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:853) ~[?:?]
        at com.sun.crypto.provider.AESCipher.engineDoFinal(AESCipher.java:446) ~[?:?]
        at javax.crypto.Cipher.doFinal(Cipher.java:2266) ~[?:?]
        at org.elasticsearch.repositories.encrypted.PasswordBasedEncryptor.decrypt(PasswordBasedEncryptor.java:137) ~[?:?]
        at org.elasticsearch.repositories.encrypted.EncryptedRepository$EncryptedBlobContainer.readBlob(EncryptedRepository.java:237) ~[?:?]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1153) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1073) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.restore(StoreRecovery.java:482) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        ... 9 more
[2020-01-15T15:19:17,870][WARN ][o.e.i.c.IndicesClusterStateService] [ElasticMBP.local] [twitter][0] marking and sending shard failed due to [failed recovery]
org.elasticsearch.indices.recovery.RecoveryFailedException: [twitter][0]: Recovery failed on {ElasticMBP.local}{pN1b441YTV6fKug6AkF2Vg}{uHr4xFDrQzWtykhxV_VO9Q}{127.0.0.1}{127.0.0.1:9301}{dilm}{ml.machine_memory=17179869184, xpack.installed=true, ml.max_open_jobs=20}
        at org.elasticsearch.index.shard.IndexShard.lambda$executeRecovery$21(IndexShard.java:2606) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.action.ActionListener$1.onFailure(ActionListener.java:71) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.lambda$recoveryListener$6(StoreRecovery.java:350) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.action.ActionListener$1.onFailure(ActionListener.java:71) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.lambda$restore$8(StoreRecovery.java:469) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.action.ActionListener$1.onFailure(ActionListener.java:71) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.restore(StoreRecovery.java:491) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.recoverFromRepository(StoreRecovery.java:278) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.IndexShard.restoreFromRepository(IndexShard.java:1850) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.IndexShard.lambda$startRecovery$17(IndexShard.java:2553) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.action.ActionRunnable$2.doRun(ActionRunnable.java:73) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:688) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
        at java.lang.Thread.run(Thread.java:830) [?:?]
Caused by: org.elasticsearch.index.shard.IndexShardRecoveryException: failed recovery
        ... 14 more
Caused by: org.elasticsearch.index.snapshots.IndexShardRestoreFailedException: restore failed
        ... 12 more
Caused by: org.elasticsearch.repositories.RepositoryException: [test_enc] could not read repository data from index blob
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1167) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1073) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.restore(StoreRecovery.java:482) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        ... 9 more
Caused by: java.io.IOException: Failure to decrypt metadata for blob [index-6]
        at org.elasticsearch.repositories.encrypted.EncryptedRepository$EncryptedBlobContainer.readBlob(EncryptedRepository.java:243) ~[?:?]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1153) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1073) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.restore(StoreRecovery.java:482) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        ... 9 more
Caused by: javax.crypto.AEADBadTagException: Tag mismatch!
        at com.sun.crypto.provider.GaloisCounterMode.decryptFinal(GaloisCounterMode.java:623) ~[?:?]
        at com.sun.crypto.provider.CipherCore.finalNoPadding(CipherCore.java:1116) ~[?:?]
        at com.sun.crypto.provider.CipherCore.fillOutputBuffer(CipherCore.java:1053) ~[?:?]
        at com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:853) ~[?:?]
        at com.sun.crypto.provider.AESCipher.engineDoFinal(AESCipher.java:446) ~[?:?]
        at javax.crypto.Cipher.doFinal(Cipher.java:2266) ~[?:?]
        at org.elasticsearch.repositories.encrypted.PasswordBasedEncryptor.decrypt(PasswordBasedEncryptor.java:137) ~[?:?]
        at org.elasticsearch.repositories.encrypted.EncryptedRepository$EncryptedBlobContainer.readBlob(EncryptedRepository.java:237) ~[?:?]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1153) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.repositories.blobstore.BlobStoreRepository.getRepositoryData(BlobStoreRepository.java:1073) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        at org.elasticsearch.index.shard.StoreRecovery.restore(StoreRecovery.java:482) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
        ... 9 more

But the restore response is confusing:

curl -u elastic:password -X POST "localhost:9200/_snapshot/test_enc/snapshot_3/_restore?pretty&wait_for_completion=true"
{
  "snapshot" : {
    "snapshot" : "snapshot_3",
    "indices" : [
      "twitter",
      "twitter2"
    ],
    "shards" : {
      "total" : 3,
      "failed" : 0,
      "successful" : 3
    }
  }
}

I think the confusion is true regardless of the reason restoring the shard? I feel there should be way for the caller to learn that restoring the shard failed, but I don't know why it doesn't work in this case. Can you help me please?

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jan 15, 2020

Member

The fact that we get no failed counts in the response here looks like a bug. Almost looks like something is broken with the way the exceptions are are wrapped and unwrapped by org.elasticsearch.index.shard.StoreRecovery#recoveryListener. I will look into this as soon as I can, but org.elasticsearch.index.shard.StoreRecovery#recoveryListener is probably a good start to see how that exception is dealt with.

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 15, 2020

Author Contributor

Thank you Armin! I will also look delve deeper given your confirmation.

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 16, 2020

Author Contributor

@original-brownbear Debugging further I've learned that restoring to a node with a different password (and which cannot decrypt the blobs) is an allocation failure, but it's not "fatal" as if the snapshot were two versions behind. Instead the allocation is retried (

if (unassignedInfo.getFailure() != null && Lucene.isCorruptionException(unassignedInfo.getFailure().getCause())) {
). And then it's retried on a node that has the correct password, were it succeeds. Afterwards I assume the shard can be rebalanced around. Does this scenario sound plausible to you?

At first glance this might look "nice"-ish, because apparently we can tolerate some nodes with wrong passwords during restore, but there is no allocation decider to say where shards from a certain repository can be restored to; It's a game of try and miss. And the decider sounds too complicated to implement.

Instead, I have a better idea! I see Repository#getRepositoryData is always called on the master node before the snapshot and restore operations (and others) fan-out. I should be adding the license and password consistency check there, and hence fail the operation ASAP before it even gets to the restore and snapshot repository callbacks on the data nodes. (I need to make the password consistency check work on the master node) Does this sound correct to you?

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jan 16, 2020

Member

Instead, I have a better idea! I see Repository#getRepositoryData is always called on the master node before the snapshot and restore operations (and others) fan-out. I should be adding the license and password consistency check there, and hence fail the operation ASAP before it even gets to the restore and snapshot repository callbacks on the data nodes. (I need to make the password consistency check work on the master node) Does this sound correct to you?

Yea I like that idea, that will also generally make the repository unusable until the setting consistency is fixed which is a good thing IMO. It would also make snapshot creation fail fast btw so that problem would be dealt with automatically also :)

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 16, 2020

Author Contributor

Thank you Armin!

Thinking further, if we go with this idea, then an encrypted repository would be unusable (wrt all operations) if the password is not the same across the nodes.

An argument can be made that we're overdoing it, and that technically we could allow other operations, such as listing, deleting or cleaning-up snapshots, if only the password on the master node coincides with the true password of the blob store repository (disregarding the password on the data nodes). But I think users relying on this feature is a bad thing, because the master can change at any time so the availability of those operations is unreliable. Moreover those are not crucially important operations, i.e. you do not install a repository on a big cluster (onto which you have trouble to consistently set the password) just so you can list, delete or clean-up a repository; you can do that on a one node cluster on the laptop if you wish.

Another argument could be made that, if the repository is completely unusable, then we should maybe fail earlier, for example not creating the repository in the first place or failing nodes to join the cluster. But I would say that these two behaviors lead to poorer user experience. Not creating the repository after a cluster restart leads to confusion when trying to use a missing repository; the caller doesn't get a response back about why the repository is not there, only that it's missing and it has to look in the logs. Also, failing a node to join a cluster because of the repository password mismatch is to big of a heavy-handed approach; the node can work just fine as part of the cluster, maybe it's hastily provided by some process that doesn't have access to the secrets in the keystore, it's only a non-crucial feature which is unusable.

Therefore, I would continue to implement this plan, of disabling all operations on the repository until passwords are consistent, unless @tvernum or @jkakavas have anything against it. This requires making the password consistency check work on the master node. I would also hope that in this case we could make the repository plugin reloadable so that we can update the password without restarting the node, but I'm not yet sure about it yet depends how things fall into place.

This comment has been minimized.

Copy link
@tvernum

tvernum Jan 17, 2020

Contributor

Therefore, I would continue to implement this plan, of disabling all operations on the repository until passwords are consistent, unless @tvernum or @jkakavas have anything against it

I am not opposed. We can always come back and relax that requirement in the future if we find a compelling reason. But if we don't do it from the beginning, we will find that some people don't have consistent passwords for some reason (e.g. allocation rules mean that the shards they want to encrypt aren't on those nodes) and then it will be harder to enforce it.

This comment has been minimized.

Copy link
@jkakavas

jkakavas Jan 20, 2020

Contributor

I concur. I like this as a strict default and Tim's argumentation line on the possibility of relaxing ( as opposed to the possibility of restricting ) is spot on

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 22, 2020

Author Contributor

In the light of #50846 (comment) we would lose the ability to check that the password is the same across the data nodes doing the shards restore, before the restore process is started.
In this case, some nodes are not able to restore shards from the given repository. From my debugging and reading of the code, I believe a failed restore on a node will be allocated to another node, without any "smartiness" involved (i.e. it could be allocated to another node which also has the wrong password). This again is the same problem as when cloud-based repositories have wrong client credentials.

Therefore, the code to better handle this case should not be specific to the encrypted repository. I would suggest we consider this a different issue and handle separately.

throw new RepositoryVerificationException(metadata.name(), "Repository password mismatch. The local node's [" + localNode +
"] value of the keystore secure setting [" + passwordSettingForThisRepo.getKey() + "] is different from the master's");
}
}

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 15, 2020

Author Contributor

Verification fails nevertheless, overriding is not necessary, but this gets a friendlier error message:

{
  "error" : {
    "root_cause" : [
      {
        "type" : "repository_verification_exception",
        "reason" : "[test_enc] [[pN1b441YTV6fKug6AkF2Vg, 'org.elasticsearch.transport.RemoteTransportException: [ElasticMBP.local][127.0.0.1:9301][internal:admin/repository/verify]']]",
        "suppressed" : [
          {
            "type" : "repository_verification_exception",
            "reason" : "[test_enc] Repository password mismatch. The local node's [{ElasticMBP.local}{pN1b441YTV6fKug6AkF2Vg}{uHr4xFDrQzWtykhxV_VO9Q}{127.0.0.1}{127.0.0.1:9301}{dilm}{ml.machine_memory=17179869184, xpack.installed=true, ml.max_open_jobs=20}] value of the keystore secure setting [repository.encrypted.test_enc.password] is different from the master's"
          }
        ]
      }
    ],
    "type" : "repository_verification_exception",
    "reason" : "[test_enc] [[pN1b441YTV6fKug6AkF2Vg, 'org.elasticsearch.transport.RemoteTransportException: [ElasticMBP.local][127.0.0.1:9301][internal:admin/repository/verify]']]",
    "suppressed" : [
      {
        "type" : "repository_verification_exception",
        "reason" : "[test_enc] Repository password mismatch. The local node's [{ElasticMBP.local}{pN1b441YTV6fKug6AkF2Vg}{uHr4xFDrQzWtykhxV_VO9Q}{127.0.0.1}{127.0.0.1:9301}{dilm}{ml.machine_memory=17179869184, xpack.installed=true, ml.max_open_jobs=20}] value of the keystore secure setting [repository.encrypted.test_enc.password] is different from the master's"
      }
    ]
  },
  "status" : 500
}
}
} else {
listener.onFailure(LicenseUtils.newComplianceException(
EncryptedRepositoryPlugin.REPOSITORY_TYPE_NAME + " snapshot repository"));

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 15, 2020

Author Contributor

@tvernum @jkakavas We discussed and agreed that we require the platinum license for encrypted repositories (I lost the link to the discussion). But I think it makes sense to require the license only for the snapshotting operation. To me it feels like a rip-off to require a license to retrieve the data which has been backed up when a license was in place.

Do we agree that only the snapshot operation falls under the license requirement? If so, this is the best I could come up with. This is again less than ideal because the snapshot operation does not fail as soon as possible. Here is how the response for an invalid license looks like:

{
  "snapshot" : {
    "snapshot" : "snapshot_6",
    "uuid" : "1NTz-Tm7SWeIQBGegbvROQ",
    "version_id" : 8000099,
    "version" : "8.0.0",
    "indices" : [
      "twitter2",
      "twitter"
    ],
    "include_global_state" : true,
    "state" : "PARTIAL",
    "start_time" : "2020-01-15T13:51:02.234Z",
    "start_time_in_millis" : 1579096262234,
    "end_time" : "2020-01-15T13:51:02.234Z",
    "end_time_in_millis" : 1579096262234,
    "duration_in_millis" : 0,
    "failures" : [
      {
        "index" : "twitter",
        "index_uuid" : "twitter",
        "shard_id" : 0,
        "reason" : "org.elasticsearch.ElasticsearchSecurityException: current license is non-compliant for [encrypted snapshot repository]\n\tat org.elasticsearch.license.LicenseUtils.newComplianceException(LicenseUtils.java:26)\n\tat org.elasticsearch.repositories.encrypted.EncryptedRepository.snapshotShard(EncryptedRepository.java:104)\n\tat org.elasticsearch.snapshots.SnapshotShardsService.snapshot(SnapshotShardsService.java:341)\n\tat org.elasticsearch.snapshots.SnapshotShardsService.lambda$startNewShards$1(SnapshotShardsService.java:287)\n\tat org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:629)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\n\tat java.base/java.lang.Thread.run(Thread.java:830)\n",
        "node_id" : "tA8IRz40TAq5gJpo3i4paA",
        "status" : "INTERNAL_SERVER_ERROR"
      },
      {
        "index" : "twitter2",
        "index_uuid" : "twitter2",
        "shard_id" : 1,
        "reason" : "org.elasticsearch.ElasticsearchSecurityException: current license is non-compliant for [encrypted snapshot repository]\n\tat org.elasticsearch.license.LicenseUtils.newComplianceException(LicenseUtils.java:26)\n\tat org.elasticsearch.repositories.encrypted.EncryptedRepository.snapshotShard(EncryptedRepository.java:104)\n\tat org.elasticsearch.snapshots.SnapshotShardsService.snapshot(SnapshotShardsService.java:341)\n\tat org.elasticsearch.snapshots.SnapshotShardsService.lambda$startNewShards$1(SnapshotShardsService.java:287)\n\tat org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:629)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\n\tat java.base/java.lang.Thread.run(Thread.java:830)\n",
        "node_id" : "tA8IRz40TAq5gJpo3i4paA",
        "status" : "INTERNAL_SERVER_ERROR"
      },
      {
        "index" : "twitter2",
        "index_uuid" : "twitter2",
        "shard_id" : 0,
        "reason" : "org.elasticsearch.ElasticsearchSecurityException: current license is non-compliant for [encrypted snapshot repository]\n\tat org.elasticsearch.license.LicenseUtils.newComplianceException(LicenseUtils.java:26)\n\tat org.elasticsearch.repositories.encrypted.EncryptedRepository.snapshotShard(EncryptedRepository.java:104)\n\tat org.elasticsearch.snapshots.SnapshotShardsService.snapshot(SnapshotShardsService.java:341)\n\tat org.elasticsearch.snapshots.SnapshotShardsService.lambda$startNewShards$1(SnapshotShardsService.java:287)\n\tat org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:629)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\n\tat java.base/java.lang.Thread.run(Thread.java:830)\n",
        "node_id" : "tA8IRz40TAq5gJpo3i4paA",
        "status" : "INTERNAL_SERVER_ERROR"
      }
    ],
    "shards" : {
      "total" : 3,
      "failed" : 3,
      "successful" : 0
    }
  }
}

I think this is reasonable, given the hooks we currently have in place. The error and the reason for the user are clear, although it does create a "snapshot stub" , i.e. a snapshot only for the global metadata. What do you think?

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 16, 2020

Author Contributor

I've learned that the Repository#getRepositoryData method is always called on the master node before the request fans out. The license check is better suited there. I will move the check and return back with the new user experience.

EDIT: but I'm not sure yet how to confine it to snapshot only.

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 16, 2020

Author Contributor

I have an idea of making the repository read-only if the license is non-platinum (and non-trial). I think this would require a little bit of code shuffling, because the read-only flag is assumed to not change, but I believe it's feasible. What do you think @original-brownbear ? Making the repo read-only precludes snapshot deletes and clean-ups, which is unfortunate but it's not terrible. Also, it might take a while to learn why the repository flipped to read-only, although you'd probably hit other surprises first on an expired platinum license.

Otherwise, I would propose we add a new method to the Repository class isSnapshotEnabled(); I envisage there might be other future cases where only partial operations on the repo are enabled but the conditions for their availability can externally change.

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jan 16, 2020

Member

read-only isn't enough to make restores fail-fast though. I guess we could enable all other read operations somehow so you would still have the snapshot status APIs but it would require some API changes and I'm wondering if the limited usefulness of that is worth it?

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 16, 2020

Author Contributor

My assumption, but I have to get @tvernum and @jkakavas input, is that restore IS allowed for a non-platinum license, but snapshot IS NOT (and delete and clean-up are collateral). We could simply disable the repository completely in a case of non-conformant license (failing the Repositry#getRepositoryData call) but to me this sounds like racketeering: if you at some point have created a snapshot, then you must pay us whenever in the future you need to restore it.

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 16, 2020

Author Contributor

You still can get a trial or hack the license check code, so there won't be any tears, but still it sounds dishonest.

This comment has been minimized.

Copy link
@jkakavas

jkakavas Jan 20, 2020

Contributor

I see nothing dishonest with tying a feature ( " if you at some point have created a snapshot, " --> " if you at some point have created an encrypted snapshot, " ) to a license level. Maybe worth taking in the view of @bytebilly from a PM perspective. (and I just saw you had already done that below so nevermind me :) )

* the key to decrypt the ciphertext (which also contains the IV). Decryption also does not store the generated key
* on disk, but caches it in memory because generating the key from the password is computationally expensive on purpose.
*/
public final class PasswordBasedEncryptor {

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 15, 2020

Author Contributor

@tvernum @jkakavas Can you also please mull over this as well? This describes how metadata encryption works. It is different compared to how blobs are encrypted because for the metadata, compared to the actual blob, we should reuse the key (which is derived from the password); but if we reuse the key we then have to carefully plan the construction of IVs. In https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf Section 8.2 it is recommended to recycle the key after a certain number of randomly generated IVs . To generate multiple keys from the same password I use here a random salt. The salt is then prepended to the ciphertext for the decryption to be able to regenerate the key. The key is never stored on disk (so it's not stored near the salt).

I believe this is secure. Would you agree?

This replaces CMS, because of its complicated API and algorithm limitations.

This comment has been minimized.

Copy link
@jkakavas

jkakavas Jan 21, 2020

Contributor

I agree that the proposed implementation adheres to the recommendations and doesn't violate any of the limitations that would render its use insecure.

Have you consider the possibility/performance of the naive approach of generating a new salt for each encryption operation ? We can be certain that this is less performant, but I can't necessarily gauge how much and whether or not it would make sense as a trade-off in order to make the implementation less complex.

This comment has been minimized.

Copy link
@albertzaharovits

albertzaharovits Jan 21, 2020

Author Contributor

Thanks a lot for verifying it!

I haven't benchmarked it.
I think it's hard to say what performance penalty would be acceptable, because it must be compared with the latency of the "put/get-blob" operation, which has a lot of variability and also the PBKDF2 function itself, I suspect, has a lot of variability on different architectures. Basically, I worry that the results will be inconclusive.

I wonder what @tvernum thinks about the complexity here?

In the mean time, I will create tests for this class to see if we can cover for the complexity.

@jkakavas Do you think I should maybe fork this class, and its tests, into its own PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.