HumioAction: How to improve secret handling?

[fanicia]

Both issue 636 as well as 479 point out that the current implementation of Humio Action CRs causes secrets to be exposed as plaintext values in the resources.

I had a few hours to look into how this could be improved and I made this PR mostly to spark some discussion. This PR is not meant to be merged as-is. I just wanted to get the maintainer's thoughts on it before going too far in either direction.

My thoughts on the topic

I just spent a little time looking into it, and it seems like the issue comes from the pattern of resolving secrets into the plaintext fields of the ha objects as is done here.

In my opinion, this can be fixed in a couple of ways:

1. Switch pattern so the secret is passed along with the ha object down to the point where the HumioClient can reconcile with Humio, such that the ha object only ever has a reference to the secret and the secret is never set on the actual ha resource. This would mean removing the plaintext fields such as ha.Spec.SlackPostMessageProperties.ApiToken from the resources altogether.
2. Make a "hack" where the secrets are removed just before updating the resource's definitions on the kubernetes side. This way, fields like ha.Spec.SlackPostMessageProperties.ApiToken never make it into kubernetes, but they can still be used for DTO.

In my opinion 1) would be the cleaner solution. However, it would require some changes in how secrets are handled in the Humio Actions and it would be a breaking change 🙁 It should be noted that this is the recommended way of handling this from a kubernetes point of view.

Option 2) is much less clean, but it would likely be quicker to implement, and it is not a breaking change as it allows people to continue using the plain text versions of the secrets if they so choose.

What I actually implemented

I wanted to get a feel for what option 2) would look like, even though I don't feel like it is the best solution. As such, I added a hack to ha.Spec.SlackPostMessageProperties.ApiToken a few places just before r.Update is called. It is definitely the ugly way to solve the issue.

What do the maintainers think? Should something like 2) be made for backward compatibility, should 1) be considered, or am I just completely off base here? 😄

[SaaldjorMike]

I agree with you that having fields with the plaintext secret isn't the way to go, and we should probably stick to only allowing the user to specify secretKeyRef's for the secrets. Not all of the action types currently supports that though, so that needs to be added, and yes, we should probably deprecate/remove the old plaintext string type fields holding the plaintext versions when we have a better way for all the action types.

Oh, and yes, I agree that option 1 is the much better way to go. We've not done a whole lot of breaking changes to the resource types thus far, and thus we've been able to stick to the same API version the entire time, but perhaps this is a good candidate for diving into the world of API version bump's so that we can actually get rid of the old fields entirely and not have to keep them around just to not break anything.

[fanicia]

Alright. So it sounds like you would be up for the idea of deprecating the plaintext fields and going for option 1)?
If that's the case, I'll add it to my never-ending list of TODOs and see if I get around to it 🙂 😅

[SaaldjorMike]

That would be my preferred route here, yes.

[fanicia]

I'll close this PR and reopen a new one if I get around to properly implementing a version of solution 1)