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

Secret output properties are not marked secret in the state #1380

Closed
t0yv0 opened this issue Sep 19, 2023 · 6 comments · Fixed by #1503
Closed

Secret output properties are not marked secret in the state #1380

t0yv0 opened this issue Sep 19, 2023 · 6 comments · Fixed by #1503
Assignees
Labels
impact/security kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed size/S Estimated effort to complete (1-2 days).
Milestone

Comments

@t0yv0
Copy link
Member

t0yv0 commented Sep 19, 2023

What happened?

When working with pulumi/pulumi-aws#2791 I noticed that simply making a property Secret in SchemaInfo is insufficient to guarantee that the values of this property never appear in the plain the Pulumi state files.

It appears that we have some machinery to do this called MarkSchemaSecrets, but it is only applied in Check that is it applies to the inputs of a resource. If something is an output property that only becomes available during Create or Update, MarkSchemaSecrets does not touch it and it is then plain texted in the state file.

Example

In AWS provider, mark tags_all as Secret, provision a bucket with tags set and observe that the tags_all is not secret in the state.

Output of pulumi about

CLI          
Version      3.74.0
Go Version   go1.20.5
Go Compiler  gc

Host     
OS       darwin
Version  13.4.1
Arch     x86_64

Backend        
Name           pulumi.com
URL            https://app.pulumi.com/t0yv0
User           t0yv0
Organizations  t0yv0, pulumi

Pulumi locates its logs in /var/folders/gk/cchgxh512m72f_dmkcc3d09h0000gp/T/ by default

Additional context

N/A

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@t0yv0 t0yv0 added needs-triage Needs attention from the triage team kind/bug Some behavior is incorrect or out of spec labels Sep 19, 2023
@mikhailshilkov
Copy link
Member

This sounds bad on its face value... Do you think we should prioritise this?

@t0yv0
Copy link
Member Author

t0yv0 commented Sep 20, 2023

Yes!

It's a real quick one to workaround and very much worth doing. If I don't get to it in this iteration next one is good. There's some more subtlety here, if I understand the flow correctly, secret-ness at the provider level is not propagated to Callbacks and it's not propagated to default value handling, so if default values are inherited from provider it unmasks any secrets.

On the other hand I don't know of any practical scenarios where users are affected. And it's probably should not be the long-term strategy to patch things in this way; in the longer term we should consider opting into receiving secrets in the bridge and tracking their flow properly instead of relying on engine heuristics. In quick discussions with the team so far it feels like a larger lift due to concerns about observable behavior changes that users may depend on currently.

@t0yv0 t0yv0 added the size/S Estimated effort to complete (1-2 days). label Sep 20, 2023
@mikhailshilkov mikhailshilkov added impact/security and removed needs-triage Needs attention from the triage team labels Sep 20, 2023
@mikhailshilkov mikhailshilkov modified the milestone: 0.95 Sep 20, 2023
@VenelinMartinov
Copy link
Contributor

I believe this issue might have been fixed since. I ran the following test:

  1. Removed this line https://github.com/pulumi/pulumi-aws/blob/7c79fe28e58bfce60d3d9764c1fd6e067d73eeba/provider/resources.go#L7186-L7187
  2. Built the provider and sdk.
  3. Created an s3 bucket with a tag.
  4. Exported the state with pulumi stack export
  5. Observed the tagsAll property is marked as secret and encrypted in the state.

I also did the same but with this line removed https://github.com/pulumi/pulumi-aws/blob/7c79fe28e58bfce60d3d9764c1fd6e067d73eeba/provider/resources.go#L7183
and observed that the property is no longer secret. Hence the Secret property in the SchemaInfo must be working correctly.

I'll raise a PR to remove the unnecessary ensureTagsAllSecret and TODO.

Let me know if I missed something.

@mikhailshilkov
Copy link
Member

@VenelinMartinov @t0yv0 If so, any ideas what could have fixed? The issue isn't old, so it must be some recent change?

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 2, 2023

I do not think it's fixed. Pick a different property from tags_all for the repro as tags_all is increasingly special. Find a schema-secret output property and observe the issue.

@t0yv0 t0yv0 self-assigned this Nov 2, 2023
@t0yv0
Copy link
Member Author

t0yv0 commented Nov 2, 2023

@VenelinMartinov it came onto the critical path of pulumi/pulumi-aws#2895 so I'm fixing this as I go, PR shortly.

t0yv0 added a commit that referenced this issue Nov 3, 2023
Fixes #1380 

Similarly to Plugin Framework resources, SDKv2 based resources will now
proactively mark properties as secrets in the results of Check method if
the upstream schema says that these properties are sensitive or else if
SchemaInfo in the bridged provider specifies the Secret true option.
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Nov 3, 2023
@t0yv0 t0yv0 added this to the 0.96 milestone Nov 3, 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 resolution/fixed This issue was fixed size/S Estimated effort to complete (1-2 days).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants