Updateable secrets & WriteOnly attributes #144
Conversation
If we've deleted a secret, the read call to verify afterwards succeeds, but we're not actually storing a secret in the state file, so we shouldn't warn.
We don't care about ordering here, and the API might return things in a different order to that which the user specifies them in the hcl.
This way we don't break the previous behaviour (expected to omit the leading / in hcl)
If the TF user doesn't have read, then all other operations fail. Can't even delete it, because TF tries to do a read before any other operation.
Add a mutex for policy updates; updates to resources can happen concurrently, and we have issues if we try to do policy updates on the same branch at the same time. Whilst we're here, also check if the permissions have changed and only send a policy update if they have - hopefully reduce the number of locks we need to have.
So that there's a mechanism to trigger a value update
aidy
force-pushed
the
write_only_secrets
branch
from
1ebe132 to
7c0f7d9
Compare
gl-johnson
left a comment
gl-johnson
left a comment
There was a problem hiding this comment.
Haven't done a full review, but wanted to jot down a couple thoughts for further research
| } | ||
| if !reflect.DeepEqual(state.Annotations, data.Annotations) { | ||
| policyMutex.Lock() | ||
| _, err = r.client.LoadPolicy(conjurapi.PolicyModePatch, strings.TrimLeft(data.Branch.ValueString(), "/"), bytes.NewReader(annYml)) |
There was a problem hiding this comment.
AFAIK existing annotations cannot be removed without replacing the whole resource in Conjur policy. It seems like using patch here will lead to a mismatch in TF state and the Conjur resource if you were to attempt removing an existing annotation.
Maybe this is still a better option since you'd be able to manually recreate the resource if the annotation truly needs deletion, but this probably warrants some additional validation/logging to make it clear why state drift is occurring.
There was a problem hiding this comment.
I think I handled this by setting removed annotations to empty string, and then ignoring them for state - it means that the state isn't a true representation of the resource, but it seemed like the least worst option.
| Privileges: pvs, | ||
| }) | ||
| } | ||
| data.Permissions = perms |
There was a problem hiding this comment.
My concern with this is that even within the TF provider, there are a handful of ways permissions can be granted outside the scope of this permissions attribute. conjur_membership, conjur_permission, or being added as owner for example. It seems to me this would cause the provider to always interpret a drift between the Conjur API response (state) and the plan data when there is some external permissioning happening, and the HCL is not manually updated to match what the API returns.
Need to think about this a bit more, but maybe permissions should be Computed so TF will not try to enforce a match between the HCL and state, and HCL updates can just be treated as additive to any external permissioning
There was a problem hiding this comment.
This is good thinking - I hadn't dug that deeply into it, and I wasn't aware of those things, thanks for bringing them to my attention.
I think the way to think about it is, "How would a Platform Engineer want/expect to manage secrets, and what's the minimum set of features that they need to be able to do so without leaving beartraps all over the place?"
I feel like conjur's a bit of a swiss army knife, there are loads of levers to cater to a multitude of different use cases, and it's probably worth some consideration about which ones and how we expose them for this use-case. To me, permissions seem pretty tightly coupled with the management of the secret - but you'll know the domain better than me, what do you think?
There was a problem hiding this comment.
I think there's not a perfect solution, so we should probably aim for what is most predictable and intuitive to a user. I imagine users don't know/care about the relational aspects of Conjur's policy tree, so individual resources should, when possible, be able to be managed directly on the corresponding TF resource via the relevant attributes.
I suppose the question is whether the permissions attribute on a conjur_secret resource should take priority over other sources. To me this seems a bit arbitrary and likely to be a source of confusion if we are revoking privileges that were granted elsewhere just because they are not duplicated in the HCL. I think perhaps the safest approach is to only track privileges defined directly on the resource in state, so we have a picture of which privileges can be revoked if changes are made. We should not attempt to reconcile existing permissions that may have been granted via other mechanisms IMO
|
We're going to prioritize adding an |
2 participants
Desired Outcome
I've had a chat with @gl-johnson about this.
This adds update-ability for various attributes of secret resources. Currently instead of attempting to update, we just replace the secret - this is a problem if any attributes have been set outside of terraform (e.g. by conjur policy load). As the permissions that are available to the V2 api are limited (only
readandexecute), it is likely that people will want to apply permissions via other means, and replacing the entire secret for trivial updates effectively removes those permissions (and any other changes that might have happened) without warning.Also this adds a check that the terraform user is set to have at least
readprivileges on any secret it manages. If we don't do this, and the tf user doesn't own the branch, we end up with a very broken terraform state (because we try to do a read before any operation, including a destroy).This also adds a
value_womechanism for writing the secret - this allows the writing of the secret value without it being stored in the state.The commits in this should be relatively well structured. Recommend going commit by commit to make sense of the changes.
Implemented Changes
Describe how the desired outcome above has been achieved with this PR. In
particular, consider:
Connected Issue/Story
Resolves #[relevant GitHub issue(s), e.g. 76]
CyberArk internal issue ID: [insert issue ID]
Definition of Done
At least 1 todo must be completed in the sections below for the PR to be
merged.
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
READMEs) were updated in this PRBehavior
Security