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

last-applied-configuration contains plain text secret values #1118

Closed
nesl247 opened this issue May 12, 2020 · 20 comments
Closed

last-applied-configuration contains plain text secret values #1118

nesl247 opened this issue May 12, 2020 · 20 comments
Assignees
Labels
impact/security kind/bug Some behavior is incorrect or out of spec last-applied-configuration Issues related to the last-applied-configuration annotation resolution/fixed This issue was fixed

Comments

@nesl247
Copy link

nesl247 commented May 12, 2020

Problem description

Creating a Secret with pulumi is creating the last-applied-configuration annotation. This contains the raw values of the secret.

Maintainer note: These values are automatically encrypted in Pulumi state; the plain text values are on the actual Kubernetes Secret resource.

Suggestions for a fix

According to my research

kubectl apply should not be used with secrets. I am not sure if this is what pulumi is using or not, but it seems like a possibility.

@nesl247
Copy link
Author

nesl247 commented May 12, 2020

I guess this is a repeat of #965. However, I really do think this should be changed, regardless of if people who have access to see the annotation would have access to the values or not. The difference is when doing an get -o yaml, you will see the values on screen. If for some reason (potentially screen sharing), you do not want this, it will happen regardless.

@confiq
Copy link

confiq commented May 12, 2020

Creating a Secret with pulumi is creating the last-applied-configuration annotation.

I'm not sure that pulumi that creates kubectl.kubernetes.io/last-applied-configuration, it's how k8s manages the declarative config as explained in #965

@nesl247
Copy link
Author

nesl247 commented May 12, 2020

@confiq I'm aware. I wasn't stating that pulumi is creating it, just that creating a secret with pulumi is creating the annotation.

@jaxxstorm
Copy link
Contributor

jaxxstorm commented May 12, 2020

This is a tricky problem as mentioned in #965 and I'm open to ideas about solutions.

For those not following, this is a downstream consideration with kubectl apply and has been set as wontfix/by design.

We have a few non ideal solutions:

  • Don't use kubectl apply style logic for applying secrets, which isn't ideal because we lose the declarative management technique we get with it
  • Use a magic transformation to remove the annotation.

It's worth noting that describe and get -o yaml will dump "plaintext" data to stdout, but of course it's base64 encoded, so much harder to be worried about in a screen share environment.

Considering this is set as "by design" upstream I'm inclined to close it, but happy to take feedback/considerations.

@nesl247
Copy link
Author

nesl247 commented May 12, 2020

I don't think I saw an explanation as to what we lose by switching away from the declarative method?

@jaxxstorm
Copy link
Contributor

I don't actually know how we would be able to switch away from the declarative object configuration. The Kubernetes imperative management has no knowledge of the previous state so it would be a very dramatic change in how we operate on the cluster.

As an example:

If we use kubectl create secret style logic to create a secret, the initial secret created would not have the annotation in plaintext. However, if/when that secret is updated, pulumi would need to then import the state and manage the update itself. Using kubectl apply style logic means we don't have to manage and maintain this within the app, we can defer to the upstream logic.

@nesl247
Copy link
Author

nesl247 commented May 13, 2020

Why would pulumi need to import the state?

Create secret -> secret stored in statefile -> update secret -> check diff against statefile -> diff detected -> replace secret

@jaxxstorm
Copy link
Contributor

check diff against statefile

Sorry if it wasn't clear, that's largely what I meant by saying "import the state".
This is a large amount of logic to implement for a single resource that isn't being fixed upstream.

@nesl247
Copy link
Author

nesl247 commented May 13, 2020

The thing is it wouldn't just benefit Secrets. Look at #1048 which has an issue due to last-applied-configuration.

@jaxxstorm
Copy link
Contributor

That's totally understandable. It's not clear how we work around this upstream limitation at this time, due to how we actually perform the apply logic.

@farvour
Copy link

farvour commented Dec 1, 2020

The entire concept of the last-applied-configuration is such a k8s screw-up IMO. I don't even know where to begin. There's really no reason why this value cannot simply be a hash or checksum of sorts, and the actual content logged to the API servers somewhere if desired. This type of problem extends way beyond Pulumi. It's almost like they designed this really cool declarative config system... and forgot about Secrets, then decided to tell users "Well you should really not use apply with secrets..."

Excuses.. excuses... there's plenty of ways this situation could have been avoided/protected from snooping. If the real values needed to exist then the values could simply be encrypted using the API servers certificates some how.

Ah well.... pipe dreams I guess... since k8s doesn't yet have an access level that separates "get metadata" vs "get entire object" it's kind of a non-issue, but the day that granularity comes [if it does] this issue needs to be revisited by the k8s core team again. It's like 6 year old issue... not sure why hashing the value 'breaks' any of this but I digress.

@GauravJain98
Copy link

Issue is there but you can remove the annonate using

kubectl annotate secret <secret-name> kubectl.kubernetes.io/last-applied-configuration- -n <namespace>

@farvour
Copy link

farvour commented Jan 12, 2021

Issue is there but you can remove the annonate using

kubectl annotate secret <secret-name> kubectl.kubernetes.io/last-applied-configuration- -n <namespace>

This is a sufficient workaround for CI/CD processes, but would be best to not have it at all. The other problem with last applied is it limits configmaps to half of the actual available capacity. Although, one can argue having very large configmaps is probably not a great idea, it would be ideal if Pulumi could support some sort of patch capability instead of relying on apply-only.

@lblackstone
Copy link
Member

This should be fixed now that we automatically mark Secret data and stringData fields as Pulumi secrets.

@lblackstone lblackstone added the resolution/fixed This issue was fixed label Jul 28, 2021
@lblackstone lblackstone added the kind/bug Some behavior is incorrect or out of spec label Jul 28, 2021
@nesl247
Copy link
Author

nesl247 commented Jul 28, 2021

To clarify, it's fixed in how pulumi displays it, or it's fixed as in pulumi no longer uses apply and therefore the annotation is not created?

From what it sounds like, it's only fixed in how pulumi displays it, which should mean this issue should remain open then.

@lblackstone
Copy link
Member

Oh, I missed your earlier comment about the concern being the raw values being viewable from Kubernetes directly. From the Pulumi perspective, those values will always be encrypted now, but are still visible in the annotation with e.g., kubectl.

I'll reopen this for now, and it would also be addressed by #1659 in the future.

@lblackstone lblackstone reopened this Jul 28, 2021
@lblackstone lblackstone removed the resolution/fixed This issue was fixed label Jul 28, 2021
@lblackstone lblackstone changed the title Secret created with pulumi has last-applied-configuration leaking secret last-applied-configuration contains plain text secret values Jul 28, 2021
@farvour
Copy link

farvour commented Feb 2, 2022

I have a stack with about 200+ resources and somehow every single one of them has a ciphertext on the last applied configuration. When I switched from using hashivault -> default (service)... it made my stack previews take 5+ minutes. Is nobody else having this problem? This seems like an oversight to blanket encrypting the secrets for this, as now every single resource's last-applied-configuration will require encryption and the pulumi service with the default key seems too slow to be up for the job.

@lukehoban
Copy link
Member

When I switched from using hashivault -> default (service)... it made my stack previews take 5+ minutes

This is most likely due to pulumi/pulumi#6445, which was fixed in the most recent CLI release https://github.com/pulumi/pulumi/blob/master/CHANGELOG.md#3230-2022-01-26. Are you using the latest and still seeing this?

@farvour
Copy link

farvour commented Feb 2, 2022

This did not seem to make a difference. In fact, converting from hashivault -> default took 5+ minutes, again, the exact same time. I even did some updates/previews against the state with 3.23.2+ so it should have cleaned out anything in the stack file if that was a requirement for the fix to take hold...

This issue is specifically to EVERY resource that has a last-applied-configuration "saved" in the existing stack. It seems to still want to decrypt all of them serially according to the fix that you linked to... is it possible this issue still affects the kubernetes provider and has yet to be fixed even as of pulumi-kubernetes 3.15+?

I posted a gist to the issue I'm describing, which I think is the root of the problem here:
https://gist.github.com/farvour/6b9b0fe1d2e68d8f14d084f6e54e2b03

Pulumi must be "detecting" a secret in every one of these Deployment resources (200+ in my stack) and so it's just blindly marking the whole annotation data IN THE STACK as a secret... normally that sounds great, but,.. in reality, it's probably just pulumi detecting a poor secret combination that happens to match and just start masking everything it sees... I suspect this is a bigger issue at play all over the place and I'm hitting one of its edge cases.

adding in here, if I attempt to export the stack with --show-secrets true, it takes just as long as it does to get to the first stage of the preview/update/whatever operation against the service secrets provider backend.
p stack export --show-secrets=true > tmp1.json

@lblackstone
Copy link
Member

Fixed in #2445

@lblackstone lblackstone added the last-applied-configuration Issues related to the last-applied-configuration annotation label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/security kind/bug Some behavior is incorrect or out of spec last-applied-configuration Issues related to the last-applied-configuration annotation resolution/fixed This issue was fixed
Projects
None yet
Development

No branches or pull requests

7 participants