typesafehub / conductr-cli

CLI for Lightbend ConductR
Other
16 stars 21 forks source link

Handle multiple credentials #477

Closed huntc closed 6 years ago

huntc commented 7 years ago

From slack:

Follow up.. @agemooij and me found a workaround. The issue was that @agemooij had two credentials in the ~/.lightbend/commercial.credentials file and the second one haven't had access to lightbend/commercial-releases. Currently, we are sourcing only the user and password of the second entry. @agemooij will create an issue against conductr-cli so that we can improve this logic.

agemooij commented 7 years ago

@huntc @markusjura sorry for forgetting to report this guys. It was a very chaotic day with lots of stuff breaking on DCOS at the last possible moment 😢

I also ran into a reverse version of this problem when trying to do a dcos conduct load (the dcos is probably not important) using a private bundle repo on Bintray. This time again, the command failed until I moved the username/password of the private bundle repo to the bottom of the file.

The sbt-bintray plugin is a good example of the expected behavior. It nicely tries all username/password combinations it find in a credentials file and only fails if none work. It also fails with an error message that explains that this is a credentials problem instead of a network problem or some other incorrect behavior. When conductr-cli fails, it's sometimes hard to figure out why.

huntc commented 7 years ago

Could we not select by realm? I don't like the idea of trying "all of the keys in the lock". Sending credentials to unknown targets is generally poor practice.

agemooij commented 7 years ago

In at least my case, for my Bintray credentials, that would not work since I have a private account for my open source stuff and a Lightbend account. Both share the same realm.

Perhaps making it easier to use several different files at once?

markusjura commented 7 years ago

The following issue is related to this topic: https://github.com/typesafehub/conductr-cli/issues/346

I'd favor a solution in which the user can also add one or more Bintray credentials files, similar to sbt:

credentials += Credentials(Path.userHome / ".bintray" / ".credentials")
credentials += Credentials(Path.userHome / ".bintray" / ".lightbend-credentials")
credentials += Credentials(Path.userHome / ".lightbend" / "commercial.credentials")

We could store this information in the ~/.conductr/settings.conf file. Additionally, to the specified credentials file we should always source the credentials from ~/.lightbend/commercial.credentials file (similar to an sbt default resolver).

The sbt-bintray plugin is a good example of the expected behavior. It nicely tries all username/password combinations it finds in a credentials file and only fails if none work.

I think we should try all username and password combinations in each credential file, similar to sbt-bintray. What we could do is consider only specific realms, if this makes sense.

It also fails with an error message that explains that this is a credentials problem instead of a network problem or some other incorrect behavior. When conductr-cli fails, it's sometimes hard to figure out why.

I agree and I think we are able to display a more concrete error message. When resolving a bundle during conduct load we already distinguish between a bundle not found due to invalid credentials or general network errors: https://github.com/typesafehub/conductr-cli/blob/master/conductr_cli/resolvers/bintray_resolver.py#L106-L116

fsat commented 7 years ago

@markusjura - I have raised an issue to improve the error reporting within CLI resolvers #478

fsat commented 7 years ago

@agemooij - the CLI now allows for multiple credentials to be present in the file, and it will look for the credential under the "Bintray" realm to match the Getting Started page.

This is implemented #514 and released in CLI 1.2.16: https://github.com/typesafehub/conductr-cli/releases/tag/1.2.16

If this fix works for you, can I close this issue?

agemooij commented 7 years ago

@fsat Thanks, that sounds great!

Do I read that new getting started page right in that you now have a hardcoded set of credentials that everyone will use? Or does that page generate a personalized set of credentials? If the latter, it might be good to mention that since I can't recognize them as such by eyeballing so I have an automatic urge to replace those hardcoded credentials with my own.

It's also not clear to me whether this is compatible with existing credentials file since you seem to have changed the real value from dl.bintray.com to Bintray. Will I have to migrate my existing file?

Why the special treatment of this specific realm name and why use such a generic realm name instead of something more specific to production suite, like Lightbend Enterprise or similar?

fsat commented 7 years ago

@agemooij - thanks for the feedback.

The getting started page has your own set of credentials, so ideally users shouldn't have to replace anything.

In relation to your credentials file, since the CLI now supports multiple credentials, I'd suggest adding another credentials with Bintray realm in your file. The Bintray realm credentials can be your own (i.e. based on Bintray login using Google sign-in) as long as it has access to lightbend/commercial-releases, or the credentials from the getting started page.

As far as I know, the Bintray realm is picked to match with the credentials required RP. I think the aim is the same ~/.lightbend/commercial.credentials file can be used for both RP and ConductR CLI. I'll confirm with my team if this is the case.

agemooij commented 7 years ago

@fsat I would add a note to the download page that ensures the user that the credentials set in the code block shown are their own personal ones. It reads like an example and can be confusing.

The new Bintray realm is not the same as it used to be; e.g. the old Conductr download documentation used either dl.bintray.com or even lightbend.bintray.com.

If the new code only looks at the Bintray realm, the fix is not backwards compatible. Moving towards more consistency is a good idea but if you break backwards compatibility it might be good to tell users explicitly that they will have to change things.

I'll try to find some time to experiment with the new version of conductr-cli to see what it does with my existing credentials file but we're currently working on other things and I don't have a fully active local setup to quickly run a test against.

longshorej commented 7 years ago

Maybe we should have the CLI recognize any realms that we've previously documented - so Bintray, dl.bintray.com and lightbend.bintray.com. WDYT @agemooij @fsat?

agemooij commented 7 years ago

That would seem to be the most user-friendly approach, yes. At least dl.bintray.com was previously documented as the one true realm.

The lightbend one I think was older and a few weeks ago I think I saw Ray also had a credentials file containing that realm but I'm not sure where we got that from; probably an earlier version of the getting started page.

longshorej commented 6 years ago

This is handled via realm declaration in the credentials file. Tagging as there's enhancement that technically could be done regarding documentation, but closing since EOL.