Implement secret sub-variables #282

yoshi-1224 · 2019-04-29T20:20:55Z

Fixes issue #131

This PR is just for the feedback on the implementation. No tests are added yet either (temporarily commented out the assertion for compile hash equivalence).

Proposed Changes

notation: ?{<ref_type>:<file_path>@<yaml_path>} (only implemented for gpg Ref so far).
Users are expected to run kapitan secrets --write ... where the secret file is a yaml file.

Implementation idea

set yaml_path attribute to Ref (sub-class) instances. Use this to check if the secret is a sub-variable, and if so, after decryption in reveal function, index into the yaml_path and return the value
decryption is cached using lru_cache. But it is inefficient because the parameter is the data itself, rather than ref file path. We should ideally use ref file path as the parameter since it is a just short string.
hash of the secrets after kapitan compile is the hash of the entire file content. e.g. if we have ?{gpg:file1@var1} and ?{gpg:file1@var2} then the hash for the two secrets are identical. This should be secure still based on collision resistance.
during kapitan compile, it doesn't check if the sub-vars are actually present in the encrypted yaml file. Should we implement this as well?

uberspot · 2019-05-02T16:04:48Z

during kapitan compile, it doesn't check if the sub-vars are actually present in the encrypted yaml file. Should we implement this as well?

I think yes, we need to make sure that's covered as well otherwise it will fail silently when deployed instead of during compiling and will make debugging more painful for users.

decryption is cached using lru_cache. But it is inefficient because the parameter is the data itself, rather than ref file path. We should ideally use ref file path as the parameter since it is a just short string.

👍 Makes sense as an approach.

examples/kubernetes/secrets/subvars.yml

uberspot · 2019-05-02T16:15:26Z

kapitan/refs/secrets/gpg.py

    def _decrypt(self, data):
        """decrypt data"""
-        dec = gpg_obj().decrypt(data, **GPG_KWARGS)
+        dec = self.decrypt_with_cache(data)


Maybe it's better to do the caching in the place that calls _decrypt already?

You’re right. I was afraid caching takes place per instance method but it does so as a class. Will correct this

ramaro · 2019-05-04T09:35:52Z

during kapitan compile, it doesn't check if the sub-vars are actually present in the encrypted yaml file. Should we implement this as well?

@yoshi-1224 how is this going to work? We can't decrypt on compile - I'm not sure implementing this will be possible.

uberspot · 2019-05-04T09:47:45Z

What about storing the structure of the subvars in the secret itself(like we store the encoding)? Just the keys of the yaml, decrypted names same as they would be in individual files. You could store that as a base64'ed json. It is a bit divergent again from the simpler secret concept we had but it's an alternative if decrypting during compile proves too challenging.

ramaro · 2019-05-04T09:48:22Z

@yoshi-1224 @uberspot I'm not totally sure this should be implemented for each ref type. Ideally this should be abstracted away from gpg/akms/gkms and offered as a given feature, regardless of the type. It doesn't make sense to me for each of these to have to implement it again and again.

How about exploring your ideas instead in the Revealer (methods reveal_obj() and _reveal_replace_match()) and RevealedObj classes?

ramaro · 2019-05-04T10:00:35Z

What about storing the structure of the subvars in the secret itself(like we store the encoding)? Just the keys of the yaml, decrypted names same as they would be in individual files. You could store that as a base64'ed json. It is a bit divergent again from the simpler secret concept we had but it's an alternative if decrypting during compile proves too challenging.

I don't know if it will be necessary to add extra complexity for this feature. I like to think that subvars are simply a reveal filter feature for yaml files encrypted into secrets. The workflow to "validate" these on compile would have to be by passing the --reveal flag on compile - it should fail if revealing subvars errors out. Compile per se should not validate subvars (unless we think of a less contrived way).

googlebot · 2019-05-04T18:40:39Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

yoshi-1224 · 2019-05-04T18:50:47Z

@ramaro @uberspot
Thanks for the feedback!

How about exploring your ideas instead in the Revealer (methods reveal_obj() and _reveal_replace_match()) and RevealedObj classes?

Thanks for pointing this out. I believe this is a much better idea. I just implemented it in the most recent commit (sorry the rebasing really messed things up. ~~We should continue this discussion on a new PR~~). Please have a look at refs/base.py.

Regarding the validation, it necessitates decryption which may not be desirable. Also, as @ramaro pointed out, reveal will automatically fail if the sub-variables do not exist so I agree it is not strictly necessary to validate.

googlebot · 2019-05-05T17:33:06Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

yoshi-1224 · 2019-05-05T17:42:41Z

hi @ramaro @uberspot
The CLA issue resolved as I force-pushed, so let me continue the discussion here. I believe this new approach allows us to make caching & decryption generic, so please provide some reviews!

TODO:

write test for subvars
update examples

kapitan/refs/base.py

tests/test_compile.py

ramaro · 2019-07-03T15:03:46Z

@yoshi-1224 I think this is good to go! Let's just add the comment in subvar.yml and we can merge.

googlebot · 2019-07-03T15:32:30Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-07-03T15:36:49Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

uberspot · 2019-07-03T15:58:59Z

Merged, nice work! :) I'll do a small rc release to keep this as a tested feature

uberspot requested review from uberspot, adrianchifor and ramaro and removed request for uberspot and adrianchifor April 30, 2019 09:34

uberspot reviewed May 2, 2019

View reviewed changes

examples/kubernetes/secrets/subvars.yml Show resolved Hide resolved

uberspot reviewed May 2, 2019

View reviewed changes

yoshi-1224 force-pushed the secret_subvars branch from 82153c8 to 223fcae Compare May 5, 2019 17:33

yoshi-1224 marked this pull request as ready for review May 5, 2019 17:34

uberspot reviewed May 6, 2019

View reviewed changes

kapitan/refs/base.py Show resolved Hide resolved

uberspot reviewed May 6, 2019

View reviewed changes

kapitan/refs/base.py Outdated Show resolved Hide resolved

yoshi-1224 force-pushed the secret_subvars branch 2 times, most recently from 6dc96db to e5a0359 Compare May 11, 2019 02:47

uberspot reviewed May 11, 2019

View reviewed changes

tests/test_compile.py Outdated Show resolved Hide resolved

yoshi-1224 added 3 commits May 22, 2019 23:07

Implement GPG subvariable

c63d954

Add test and logs

c26a0fd

Update hash equivalence test

72904fe

yoshi-1224 force-pushed the secret_subvars branch from e5a0359 to 72904fe Compare May 22, 2019 14:50

ramaro approved these changes Jul 3, 2019

View reviewed changes

Add a subvars.yml comment that explains what it is.

17ea5e1

uberspot approved these changes Jul 3, 2019

View reviewed changes

uberspot added cla cla: yes and removed cla labels Jul 3, 2019

uberspot merged commit 7197874 into kapicorp:master Jul 3, 2019

Dauliac mentioned this pull request Jul 10, 2019

[bug] subvars reveal is not revealing #319

Closed

yoshi-1224 deleted the secret_subvars branch August 18, 2019 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement secret sub-variables #282

Implement secret sub-variables #282

yoshi-1224 commented Apr 29, 2019

uberspot commented May 2, 2019 •

edited

Loading

uberspot May 2, 2019

yoshi-1224 May 3, 2019

ramaro commented May 4, 2019

uberspot commented May 4, 2019

ramaro commented May 4, 2019 •

edited

Loading

ramaro commented May 4, 2019

googlebot commented May 4, 2019

yoshi-1224 commented May 4, 2019 •

edited

Loading

googlebot commented May 5, 2019

yoshi-1224 commented May 5, 2019

ramaro commented Jul 3, 2019

googlebot commented Jul 3, 2019

googlebot commented Jul 3, 2019

uberspot commented Jul 3, 2019

Implement secret sub-variables #282

Implement secret sub-variables #282

Conversation

yoshi-1224 commented Apr 29, 2019

Proposed Changes

Implementation idea

uberspot commented May 2, 2019 • edited Loading

uberspot May 2, 2019

Choose a reason for hiding this comment

yoshi-1224 May 3, 2019

Choose a reason for hiding this comment

ramaro commented May 4, 2019

uberspot commented May 4, 2019

ramaro commented May 4, 2019 • edited Loading

ramaro commented May 4, 2019

googlebot commented May 4, 2019

yoshi-1224 commented May 4, 2019 • edited Loading

googlebot commented May 5, 2019

yoshi-1224 commented May 5, 2019

ramaro commented Jul 3, 2019

googlebot commented Jul 3, 2019

googlebot commented Jul 3, 2019

uberspot commented Jul 3, 2019

uberspot commented May 2, 2019 •

edited

Loading

ramaro commented May 4, 2019 •

edited

Loading

yoshi-1224 commented May 4, 2019 •

edited

Loading