Password issue

[alexhung]

This is continuation of discussion from #283. I'll attempt to document all the known use cases surrounding the password field and possible solutions. Please post your use cases if they aren't already documented.

Context

Before v2.3.1, the password attribute was optional and the provider would automatically generate a password up to and including v2.2.15.

From v2.3.1 (August 2021), password is made required. Auto password generation was removed. Validation was added to ensure it is valid for RT.

In v2.3.3 (September, 2021), password requirement is removed.

In v2.9.1 (January 2022), password is made required again. Validation is removed.

Use cases

1. Create new user with temp password, then users reset their passwords in UI.
2. Create new user with no password. Does not use TF to manage password. Rely on provider generates a temp password for creation (<=v2.2.15).
3. Use provider to manage password lifecycle. Password is managed externally and supplies to the provider during creation/update.

Any other use cases?

In all cases, users may or may not reset their passwords in UI. Provider should not see changes and attempts to update the user resource.

GitHub Issues

#246
#252
#260
#283
#340
#347

Affected Resources

artifactory_user
artifactory_single_replication_config

Any other resources?

Possible Solutions

• Retain artifactory_user as user resource
• For artifactory_single_replication_config, remove Computed attribute and ignore read state. Then in update(), check for change for password and update Artifactory(?)

User resource

Keep the current artifactory_user as is. password attribute continues to be required and no validation applies. Users are expected to supply valid password to the resource (and thus Artifactory), and manage the password through external means (other providers, etc.) For example Terraform “random_string” resource (https://registry.terraform.io/providers/hashicorp/random/latest/docs/resources/string)

How to prevent state drift with password

In both resources, we can store the hash (SHA256?) of the supplied/generated password in the state file because we don’t get password back from Artifactory, and thus state is ‘null’. This will allow Terraform to detect drift correctly from the HCL side.

Provider will not update the state when reading from Artifactory. When password is updated (HasChange()), update Artifactory with new password.

[alexhung]

@jcogilvie @blyles @ShreyasNBS @tjstansell @vasanthig21 @korjek @michael-sabrnak-swi Please help us with your use cases (if not already in here) as well as any suggestions. We are hoping we can come to a solution that satisfy most of you.

[tjstansell]

At this point, for us, we just provide an initial password and tell terraform to ignore password changes. It works for our use-case. We were just surprised it was required during a point release update.

[jcogilvie]

There's also a use case for import. And there are also backwards compatibility concerns to think through.

I am pinned to a version before 2.9.1 because of the change to the requiredness of a password. And adding a password to these existing users has behavior that I'm not entirely clear on (but it shouldn't force everyone to reset again).

So, when the ultimate solution to this is released, in order to opt in, I should not have to go add a password field to an object that doesn't have one (the case of an imported user pre-2.9.1).

In order to coexist with import functionality, either: password field cannot be required in the provider, or import would need to make available some sort of representation of the password (and then probably ignore it in the future)

As to creating a new user, I think creating and then discarding a one-time password within the provider if one is not provided is the right play here. Or otherwise give me a flag to generate a user without a password and initiate a reset flow to their email (that'd be nice!). And then maybe if that flag is set we ignore the entire password field (which works well with my suggestion for import).

[alexhung]

@jcogilvie PR #396 has support for importing anonymous user

[chb0github]

Here's my thinking:

1. Password in TF becomes optional - all rules surrounding passwords we punt to RT. If it fails late, it fails.
2. Because passwords are trivial to generate in TF, I would rather not reimplement that functionality - I simply don't see a need. And, this goes back to number 1. If we generate a pass internally, when none is supplied, then we are stepping back into the password busines
3. use lifecycle ignore on password.

I think being as loose as possible in TF and delegating almost all decisions to RT and our users is the way to go. In a nutshell, make it be like 2.9.1, except we don't generate the password if none is supplied; we will sense a blank password along.

[jcogilvie]

Password in TF becomes optional - all rules surrounding passwords we punt to RT. If it fails late, it fails.

This kind of solves the import case, but for the new-user case, does the capability for a passwordless user exist in artifactory? If yes, great! If not, then you're just deferring my pain to where it's out of the provider's control. That's not super useful since it's still artifactory pain, and as a customer I'd kind of expect the provider to mitigate as much of that as it can.

A user is functionally going to have to have a password at some stage right?

Because passwords are trivial to generate in TF, I would rather not reimplement that functionality - I simply don't see a need. And, this goes back to number 1. If we generate a pass internally, when none is supplied, then we are stepping back into the password business

If it's trivial, but the user's going to throw it away, do it internally. Otherwise, it looks like:

1. create an arti user in tf with a first time password
2. nuke the password tf object and remove it from the artifactory_user resource

This means every new user requires two tf commits. (This is my current day, and it's not good UX. There have been many questions, concerns, and complaints.) Worse is that the provider used to handle this, which I would argue is the most user-friendly way. And as I understand it, the rationale to move away from doing it was corner cases in password policy enforcement... which isn't even something any of us have mentioned in the whole discussion as a concern worthy of a use case.

use lifecycle ignore on password.

Is there an actual use case for changing (maintaining?) someone's password in terraform? Some kind of system user? This isn't my use case, but it may be worth talking about.

[chb0github]

I didn't realize generating the password internally was at all helpful. It's kind of a hidden state - but then again, the fact that we can't get the password back anyways means it doesn't matter. I guess we could internally generate the password and emit a warning (literally, that's how it was before).

You make a compelling case to simply roll it all back to 2.9.1 functionality

As a side note: if you can extract the master password from the RT machine, I believe you could decrypt the password - but it's unconfirmed.

[jcogilvie]

I didn't realize generating the password internally was at all helpful.

It's super helpful for my use case because it means I only need one tf resource per artifactory user, and I only need to deal with it once at create-time.

It's kind of a hidden state

It is, but for my use case it's transient state, because the user then resets their password. The transient nature of it is pretty key to the argument against making (then deleting) tf password resources.

if you can extract the master password from the RT machine, I believe you could decrypt the password - but it's unconfirmed.

An interesting note, although I'm not sure why I would do this. Is the expectation that I would then transcribe it into tf code and commit it to tf state?

[chb0github]

Well, if we could decrypt the password it could be stored ina hash form. It's musing though, because I've never been able to get a clear answer from the RT team

[tjstansell]

According to the JFrog REST API for Users, the password field is mandatory in create/replace calls. So, it does seem reasonable for the terraform provider to require a password. It seems odd to leave it to the provider to silently choose a password for you if you don't provide one. Why not just provide a random string or empty password or whatever you feel is appropriate?

I also agree that this does affect the ability to import existing users into terraform. Ideally the password can be provided so that importing existing users doesn't force the password to get changed. Of course, if you know/manage the password yourself you could just provide that to terraform and it would set the password to the same thing. If you don't want to manage the password with terraform, use the lifecycle ignore.

[jcogilvie]

It seems odd to leave it to the provider to silently choose a password for you if you don't provide one.

The lens through which this makes sense is that you have to consider that you are provisioning real humans. Real humans don't use passwords that are generated or are stored in terraform state. Admins want a way to create a user with an onboarding flow that goes through email or password reset or something, and users in turn expect to use a password they've chosen.

Why not just provide a random string or empty password or whatever you feel is appropriate?

An empty string is invalid; I tried. A random string requires creating extra terraform objects for the sole purpose of passing this onboarding hurdle, which we then have to either manage in state (despite diverging from reality) or remove later. It's built-in overhead; it only has a one-time use.

As I mentioned above, this represents my current day. Adding a user with a password, then removing the password. It requires a second PR/approval/apply cycle, when the entire point of tf is that it should be declarative. I want to declare that terraform doesn't care about this user's password, but I also want to declare that the user should nevertheless be created.

[chb0github]

@tjstansell - I mean, I am onboard with your logic, but I do want to try to make life simpler for our users.

Have we considered 2 different resources?

artifactory_managed_user and artifactory_unmanaged_user ? If they are 2 seperate resources we can uterally and easily divert logic and have different schema. We could :

unmanaged is as @jcogilvie suggests: 1-and-done. managed is for cases where users are entirely managed in TF, like say service accounts. Maybe managed could even be called service_user

We don't have to use the same resource.

[tjstansell]

Yeah. I see your point about having these extra terraform resources to manage the password laying around just to satisfy the requirement to create a user. We don't have that many, ultimately, and just leave them around in TF (it's part of our simple module for defining a user and permissions). They're really just service users for us. Our human users all use SAML SSO logins and have nothing to do with TF.

[tjstansell]

I don't think you need an entirely different resource type. And the more I think about it, you don't even need much special handling. Since the REST API states that password is (write-only, never returned) when querying a user resource, you'll never see changes to the user from that perspective, even if someone manually changes the password. If password is not provided to the artifactory_user resource, it should just default to null (and stored as such in the state). Then, you special case a null password when doing a create or replace action to dynamically use a random string for the password. You don't track that password or store it anywhere. This lets folks who do set password to continue to manage things as they would expect but if you do NOT provide a password, everything "just works".

[alexhung]

Out of the box, default behavior is to refresh the TF state by reading from the source (e.g. Artifactory) after a create/update. Since Artifactory does not return the password field, currently null is stored in TF state for password. Thus the next time terraform apply is run, TF will detect (by mistake) that a drift has occurred and tries to update the password again.

Whether we manage the user password or not, we still need to prevent password state being refresh from reading Artifactory. As you said, @tjstansell, we probably should just store the password from HCL and use HasChanged() in TF SDK to decide when to apply update.

[alexhung]

FYI I'm taking some time off starting tomorrow. So this issue will not be fixed for at least 3-4 weeks. In the meantime, the JFrog crew (@chb0github, @dasmanas, @maheshjfrog) will try to enumerate the solution and plan for the implementation.

[jcogilvie]

Any movement on your end, @alexhung? Been a while since there's been much activity here.

[chb0github]

@alexhung is out on leave for a bit more. I know this, and other password issues are high on the list to be resolved. If he's feeling up to it, I'll let him give more detail. Sorry this is taking some time

[ShreyasNBS]

@alexhung @chb0github
Morning, did not realise there was a discussion happening here. As per the OP, use case 2 (Create new user with no password. Does not use TF to manage password. Rely on provider generates a temp password for creation (<=v2.2.15) is the best course in my eyes. I do not want to supply password when creating new users, nor do I suddenly want to supply passwords from terraform for existing users and then have them update it further on in their pipelines by generating new api keys and access tokens. In an enterprise scenario, where we have hundreds of local users (we are in process of migrating to AD integration for Artifactory and will be getting rid of local users in the future), it's disruptive to ask them to halt their work, update their pipelines with new creds, and then resume.
Not sure if tf import will help, because as suggested above, Artifactory does not return password in api calls.

However, I am not sure if the above will work for single replication config and ldap manager password? I believe there was a bug raised for the latter where the manager password for not being set?

[alexhung]

@ShreyasNBS See #390

[tjstansell]

So I now realize this is also affecting artifactory_remote_repository resource passwords as well (I'm assuming all passwords inside of artifactory). We used to be able to specify a username/password to use to access a remote repo, but now when a password is set, it's being encrypted with the master key (presumably) even though we do not have "password encryption" enabled in the artifactory security settings.

So, as others have stated in other issues related to this that I'm now browsing through, it seems that passwords should be handled as unverifiable inside of the terraform provider. If you specify a password in the config, it should store it internally (probably hashed) and will only notice if the parameter value changes. No verification on the artifactory side would be able to be done because even if you have an API that can return a password, it's encrypted with a rotating salt that you can't verify anyway.

[tjstansell]

That said, some of these resources have access to the encrypted password. The provider could query and store that result immediately after any changes are made as a separate computed value to do drift detection on that. So internally you might have the password provided by the config and encrypted_password that's computed only and pulled from artifactory. At least that way you could detect if someone changes things on the remote side and trigger an update from terraform. Someone could have set it to the same password, but the rotating salt would have changed the result. This, of course, would only apply to resources where you can actually query the password like the *_repository resources, not artifactory_user that won't even return a password.

[alexhung]

@tjstansell In case you missed it, I fixed (one of?) the password issue with remote repositories #385

[alexhung]

I'm back from my leave and will start work on this ASAP.

The fixes to this problem will involve multiple iteration/major releases. The top priority is to unblock some of you from unable to use the artifactory_user resource, or upgrade the provider to later version.

Here's our game plan:

• 1st major release - Revert the changes from GH-260, PTRENG-3557. Password is required, validation is offloaded to the Artifactory side #274. Password will be made optional again. Validation will be updated to match current/latest Artifactory policy. In addition, if password attribute is omitted in the HCL, a password will be generated automatically based on Artifactory policy. The provider will ignore the password state from the read() (implementation TBD) so avoid unnecessary state drift.
• 2nd major release - Add three new user resources: artifactory_unmanaged_user, artifactory_managed_user, and artifactory_anonymous_user. artifactory_unmanaged_user is alias of the current artifactory_user. artifactory_managed_user will require password value, no auto generation. Finally artifactory_anonymous_user is the special edge case where one wishes to create/import an anonymous user, and this has to deal with the requirement for email value by the API (implementation TBD).
• Minor releases - Update password state management for other resources that has passwords but do not require the same behavior. For example artifactory_remote_maven_repository in artifactory_remote_maven_repository always updating password in plan/apply #347.

[alexhung]

Updates:

• Still working on the changes for rolling back user password behavior. Should have PR ready in next day or so.
• I fixed couple of password related issues (User password is changed after changing an attribute with terraform #340 is merged and artifactory_remote_maven_repository always updating password in plan/apply #347 still pending PR review) last week. They may be of interest to you and unblock some of you.

[alexhung]

PR for first part: #390

[alexhung]

New user resources added in PR #396

[chb0github]

All, let me ask this question: Would you be willing to do some beta testing on various iterations of this while we experiment with the correct approach? We can push to a branch - all you would have to do is pull the code and build it - it will install a bumped version into your local cache to use.

We currently test on mac and linux - we know there is no windows build (volunteers, anyone?).

There are a lot of competing ideas, a lot of divergent cases, and it still isn't 100% clear how to please everyone. We would love some testing volunteers before making a release. If you're willing to test and give feedback, we can incorporate it into the next release

[ShreyasNBS]

@chb0github Normally, I would be very willing, but I have recently moved away from the Artifactory Team at my org. I will try out releases as they are sent out though :)

[jcogilvie]

I may be able to help with this, depending on the complexity of the build. Our builds are in github actions so adding a new clone/build command won't be hard, so long as there's not extensive configuration required for the build to succeed.

[akashgupta-nbs]

many thanks for providing the fix, I can confirm its working fine with no error for user resources after bumping the provider version to 5.0.0.. @alexhung
also thanks for #380, much needed change to keep repo name per package type!

thanks

[ShreyasNBS]

@chb0github @alexhung Sorry, but can you make sure that the password rollback mentioned in step 1 (1st major release) is released before this PR? https://github.com/jfrog/terraform-provider-artifactory/pull/380/files

There are a lot of breaking changes in this one, and if the above is merged before the password change, then we will be unable to upgrade to latest provider version with the password fixes, unless we do a host of other refactoring around repository resource types.

[alexhung]

@ShreyasNBS Once #390 is approved and merged, then we will merge #380

[chb0github]

I will create a discussion for 380

[chb0github]

Done: #384

For those with interest you can register your preference. We can't hold off forever. If you decide a timeline doesn't work for you, you're welcome to stay with a version of TF of your choosing until you can move forward. Understanding that we will continue feature development and bug fixes

[chb0github]

All, the first password update has been released in a major version bump. Please see the CHANGELOG.md

Please let us know if this helps. We will be moving on to the next set of changes