-
Notifications
You must be signed in to change notification settings - Fork 39.8k
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
Add an AEAD encrypting transformer for storing secrets encrypted at rest #41939
Add an AEAD encrypting transformer for storing secrets encrypted at rest #41939
Conversation
[APPROVALNOTIFIER] This PR is APPROVED The following people have approved this PR: smarterclayton Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
64d9adf
to
8e40525
Compare
80bc935
to
a3ccbba
Compare
@smarterclayton did you want this for 1.6? |
a3ccbba
to
a5c8bb9
Compare
a5c8bb9
to
2ba5ace
Compare
2ba5ace
to
e723a4b
Compare
e723a4b
to
c7afb98
Compare
c7afb98
to
cc89981
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, would be good to get this in, we have someone starting on key management and config.
Tweak the ValueTransformer interface slightly to support additional context information (to allow authenticated data to be generated by the store and passed to the transformer). Add a prefix transformer that looks for known matching prefixes and uses them. Add an AES GCM transformer that performs AEAD on the values coming in and out of the store.
Adds context argument which must be set for AES GCM authenticated data to be passed.
28290f7
to
7827899
Compare
@smarterclayton: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@k8s-bot kops aws e2e test this |
Note that we can always add new transformer implementations at any time - this is intentionally flexible. |
Applying label as per comment - this doesn't activate this code, so subsequent PRs can easily accommodate requests for change. |
Automatic merge from submit-queue (batch tested with PRs 45709, 41939) |
if err != nil { | ||
b.Fatal(err) | ||
} | ||
// reverse the key order if stale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton I don't see in the code how order is changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, will add in a follow up. Just a benchmark.
|
||
// TransformToStorage uses the first transformer and adds its prefix to the data. | ||
func (t *prefixTransformers) TransformToStorage(data []byte, context Context) ([]byte, error) { | ||
transformer := t.transformers[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that the transformers
is nil here? Should we add a check for that case in NewPrefixTransformers()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally don't guard against passing nil to constructors since it's coding error to get to that point.
|
||
func TestPrefixFrom(t *testing.T) { | ||
testErr := fmt.Errorf("test error") | ||
transformErr := fmt.Errorf("test error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having two different errors with the same message is confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The equality test is different.
"k8s.io/apiserver/pkg/storage" | ||
"k8s.io/apiserver/pkg/storage/etcd3" | ||
"k8s.io/apiserver/pkg/storage/storagebackend" | ||
"k8s.io/apiserver/pkg/storage/value" | ||
) | ||
|
||
func newETCD3Storage(c storagebackend.Config) (storage.Interface, DestroyFunc, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the similar change for the newETCD2Storage()
that still use hard-coded etcd.IdentityTransformer
. Was it on purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here, any plan to support etcd2?
…former Automatic merge from submit-queue (batch tested with PRs 46550, 46663, 46816, 46820, 46460) Add configuration for encryption providers ## Additions Allows providing a configuration file (using flag `--experimental-encryption-provider-config`) to use the existing AEAD transformer (with multiple keys) by composing mutable transformer, prefix transformer (for parsing providerId), another prefix transformer (for parsing keyId), and AES-GCM transformers (one for each key). Multiple providers can be configured using the configuration file. Example configuration: ``` kind: EncryptionConfig apiVersion: v1 resources: - resources: - namespaces providers: - aes: keys: - name: key1 secret: c2vjcmv0iglzihnly3vyzq== - name: key2 secret: dghpcybpcybwyxnzd29yza== - identity: {} ``` Need for configuration discussed in: kubernetes#41939 [Encryption](https://github.com/destijl/community/blob/3418b4e4c6358f5dc747a37b90a97bc792f159ee/contributors/design-proposals/encryption.md) **Pathway of a read/write request**: 1. MutableTransformer 2. PrefixTransformer reads the provider-id, and passes the request further if that matches. 3. PrefixTransformer reads the key-id, and passes the request further if that matches. 4. GCMTransformer tries decrypting and authenticating the cipher text in case of reads. Similarly for writes. ## Caveats 1. To keep the command line parameter parsing independent of the individual transformer's configuration, we need to convert the configuration to an `interface{}` and manually parse it in the transformer. Suggestions on better ways to do this are welcome. 2. Flags `--encryption-provider` and `--encrypt-resource` (both mentioned in [this document](https://github.com/destijl/community/blob/3418b4e4c6358f5dc747a37b90a97bc792f159ee/contributors/design-proposals/encryption.md) ) are not supported in this because they do not allow more than one provider, and the current format for the configuration file possibly supersedes their functionality. 3. Currently, it can be tested by adding `--experimental-encryption-provider-config=config.yml` to `hack/local-up-cluster.sh` on line 511, and placing the above configuration in `config.yml` in the root project directory. Previous discussion on these changes: sakshamsharma#1 @jcbsmpsn @destijl @smarterclayton ## TODO 1. Investigate if we need to store keys on disk (per [encryption.md](https://github.com/destijl/community/blob/3418b4e4c6358f5dc747a37b90a97bc792f159ee/contributors/design-proposals/encryption.md#option-1-simple-list-of-keys-on-disk)) 2. Look at [alpha flag conventions](https://github.com/kubernetes/kubernetes/blob/master/pkg/features/kube_features.go) 3. Need to reserve `k8s:enc` prefix formally for encrypted data. Else find a better way to detect transformed data.
Automatic merge from submit-queue (batch tested with PRs 49850, 47782, 50595, 50730, 51341) Fix benchmarks to really test reverse order of the keys **What this PR does / why we need it**: This PR modifies the code to do what comments says -- reverse the order of keys. It also fixes the logic that was wrong and didn't allow stale data. **Special notes for your reviewer**: This change resolves the following review comments: - #41939 (comment) - #46916 (comment) - #46916 (comment) **Release note**: ```release-note NONE ``` PTAL @smarterclayton
Tweak the ValueTransformer interface slightly to support additional
context information (to allow authenticated data to be generated by the
store and passed to the transformer). Add a prefix transformer that
looks for known matching prefixes and uses them. Add an AES GCM
transformer that performs AEAD on the values coming in and out of the
store.
Implementation of https://docs.google.com/document/d/1lFhPLlvkCo3XFC2xFDPSn0jAGpqKcCCZaNsBAv8zFdE/edit# and #12742