Use Idp Account Name as key for credentials store

[sledigabel]

At the moment the credentials are stored using the idp server url as
a primary key for the keychain store.

This is very useful if you need to authenticate through different
providers or environment (and thus different endpoints), however this
prevents the use case where users may want to use different idp accounts
to authenticate using different users.

A typical example of this is that some users often have a normal user
and an admin user with different properties, roles and limitations.

This change is refactoring how credentials are stored across the code
base to use the idp Name instead of the server URL.

Tested on Linux and Mac with ADFS.

[sledigabel]

The test path impacts Okta and OneLogin, which I couldn't test past the unit tests. It would be great to have someone who is using either of them to validate it!

[sledigabel]

@wolfeidau WDYT about this PR? We're using it internally, it allows our AWS admins to use two different users simultaneously.

[ps-jay]

This would be a useful change. Logging into Okta with username or admin_username depending on the task is a thing in my workplace.

For the moment, the workaround is to use --disable-keychain when using the admin_username, and just deal with inputting the password each time.

[sledigabel]

@wolfeidau have you had the chance to look at this PR?

[codecov-commenter]

Codecov Report

Merging #762 (06cac29) into master (24b3f8e) will increase coverage by 0.29%.
Report is 2 commits behind head on master.
The diff coverage is 84.52%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #762      +/-   ##
==========================================
+ Coverage   36.20%   36.50%   +0.29%     
==========================================
  Files          53       55       +2     
  Lines        7951     8041      +90     
==========================================
+ Hits         2879     2935      +56     
- Misses       4703     4739      +36     
+ Partials      369      367       -2     

Flag	Coverage Δ
unittests	36.50% <84.52%> (+0.29%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
cmd/saml2aws/commands/configure.go	0.00% <0.00%> (ø)
cmd/saml2aws/commands/list_roles.go	0.00% <0.00%> (ø)
pkg/provider/okta/okta.go	22.19% <0.00%> (ø)
helper/credentials/credentials.go	14.28% <33.33%> (ø)
cmd/saml2aws/commands/login.go	18.24% <90.00%> (+0.69%)	⬆️
helper/osxkeychain/osxkeychain.go	55.68% <92.00%> (-15.94%)	⬇️
helper/credentials/saml.go	96.49% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mapkon]

Is this still required?

[sledigabel]

Is this still required?

I think so; currently for mac os the credentials are stored using the server name, which means you can't have two providers with the same server names. The use case is when you have multiple users for the same provider (common for infra staff).

[mapkon]

LGTM - @wolfeidau / @gliptak - any input on this?

[gliptak]

is this a breaking change for currently stored credentials?

[sledigabel]

is this a breaking change for currently stored credentials?

Yes it would, the name of the credential changes. This is the very problem this PR is fixing, but it would have an impact on users, i.e. credentials have to be entered again.

We could make it so the credentials fall back to the old name if the new one doesn't exist, but it would have a side effect of keeping the same current behaviour for users who do need multiple credentials for a single IdP.

[mapkon]

I don't think we can introduce breaking changes without recourse to the code base. Although. the suggested feature makes sense, it will affect more people, and introduces the element of surprise without warning.

What would make sense it to sanely introduce these changes while preserving backwards compatibility. It might be good to start a discussion to see if the community will be willing to bear the change.

Thoughts?

[sledigabel]

Agreed on the element of surprise, though i think it could be justified to introduce it in a major update for instance.

WDYT about the fallback scenario? Try the new credential names and if they don't exist try the old name?
This way new users or users removing their old credentials would benefit from the feature, and previous users would still use it seamlessly?

[mapkon]

Agreed on the element of surprise, though i think it could be justified to introduce it in a major update for instance.

WDYT about the fallback scenario? Try the new credential names and if they don't exist try the old name? This way new users or users removing their old credentials would benefit from the feature, and previous users would still use it seamlessly?

That seems fair enough - I could take that as a middle compromise. As long as it is not a breaking change, and existing installations will continue to function without users having to fudge around with their setups.

I will also heavily defer to @gliptak and @wolfeidau on this

[sledigabel]

I'll amend the PR with this in mind then. Need to dive back into it, it's been a while :-)

[gliptak]

👍 on the fallback approach proposed

[sledigabel]

I've updated the PR with a few additional things:

• I've restored the old Get function and renamed it LegayGet. This is because while most Get functions on OS's are equivalent, the mac version does its special sauce and uses a fixed name (meaning you can't store more than one cred)
• The cred helpers now store based on a key. It's a bit more generic than before.
• I've added some tests to validate the logic of the cred fetching/fallback and the logic behind the specifics of Okta and OneLogin
• Also tests on SaveCredentials.

[sledigabel]

@mapkon @wolfeidau @gliptak Lemme know what you think 😅

[gliptak]

@sledigabel if there is LegacyGet, would there also be LegacyDelete (facilitating a transition into the new format)?

[wolfeidau]

So the reason this is configured in this way is because that is how Apple Keychain works, if you read the documentation for it https://developer.apple.com/documentation/security/keychain_services/keychain_items/adding_a_password_to_the_keychain

So I didn't want to jam something different in just in case Apple added features in and around this construct, domain restrictions ect.

The https://github.com/99designs/keyring which we use in saml2aws for linux and windows uses a newer interface, I have for some time wanted to remove all the DIRECT keychain stuff from saml2aws and just use this abstraction, which in turn uses "named services" instead of Host.

Clearly there needs to be some communication of this change to users who were using host based model and happy, myself included as it will impact the majority of quite happy consumers!

cc @sledigabel @gliptak
We could do a one off migration for users, but I think it needs to be clear that any service name related thing needs to have a default which will cover the majority who use ONE host, and one username :)

Keen to hear peoples thoughts on this, encourage you to consider the silent majority :)

PS. I need to dig more into how this change works BTW, just want to get some background for people.

[sledigabel]

Hi @wolfeidau,

Thanks for your insightful comment and the initial thoughts behind the osx keychain,

Though it's not the initial goal of the PR, it's trying to address it by considering each keychain entry as a key/value (value being username/password) store.

I appreciate this might be disruptive for existing users and was expecting some pushback on this, I was thinking this could be inserted as a future major version release, or like it did happen after the initial comments, find a way to make this backward compatible.

On the OSX keychain bit, I didn't see the documentation indeed, thanks for the pointer.
Further on this page, in search ability, they do mention using further attributes such as labels for more granular search, which is how the implementation in this PR is done.

The route to choosing the idpName as an ID for the credentials made sense to me as it has to be unique for the saml2aws configuration, it's easier to find in the keychain itself and it's already providing a unique URL/username set too.

I think the use of the keyring library as a unifier makes a lot of sense.
Since it addresses the original issue behind this PR, I can have a go at it when I'm back?

[sledigabel]

@sledigabel if there is LegacyGet, would there also be LegacyDelete (facilitating a transition into the new format)?

I need to confirm this when I'm back, but I don't think Delete is called by anything else than the unit tests. Therefore it didn't make sense to keep it as users don't interact with the current or previous implementation.

[sledigabel]

@mapkon @wolfeidau @gliptak please lemme know how you want to proceed on this.

[mapkon]

@mapkon @wolfeidau @gliptak please lemme know how you want to proceed on this.

I will defer to @wolfeidau decision since he has more historical context (and technical gravitas) than me on this. I am happy to merge if he gives the go-ahead

[tinaboyce]

Hi @sledigabel, apologies for the delay in getting back to you. I had a discussion with @mapkon and we agreed to get this pull request going.

There is currently a branch conflict, are you able to get it resolved?

[sledigabel]

Hi @tinaboyce thanks for getting back on this,
I'm AFK at the moment but will get on resolving the conflicts in a couple of days.

[sledigabel]

@tinaboyce I've not forgotten about this, but when I sat down to rebase I found out that some substantial changes were made and that brought new questions around unit testing.
I'll see if I can find some time to refactor this this coming week-end.
Apologies for the delay.