Closed tmclaugh closed 4 years ago
Looks like the setup for my change breaks --list-profiles
Traceback (most recent call last):
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/bin/awsumepy", line 12, in <module>
sys.exit(main())
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/awsume/awsumepy/main.py", line 29, in main
run_awsume(sys.argv[1:])
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/awsume/awsumepy/main.py", line 17, in run_awsume
awsume.run(argument_list)
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/awsume/awsumepy/app.py", line 272, in run
profiles = self.get_profiles(args)
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/awsume/awsumepy/app.py", line 107, in get_profiles
profiles=profiles,
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in __call__
return self._hookexec(self, self.get_hookimpls(), kwargs)
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/pluggy/manager.py", line 87, in _hookexec
return self._inner_hookexec(hook, methods, kwargs)
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/pluggy/manager.py", line 81, in <lambda>
firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/pluggy/callers.py", line 208, in _multicall
return outcome.get_result()
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/pluggy/callers.py", line 80, in get_result
raise ex[1].with_traceback(ex[2])
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall
res = hook_impl.function(*args)
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/awsume/awsumepy/default_plugins.py", line 242, in post_collect_aws_profiles
profile_lib.list_profile_data(profiles, arguments.list_profiles == 'more')
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/awsume/awsumepy/lib/profile.py", line 192, in list_profile_data
formatted_profiles = format_aws_profiles(profiles, get_extra_data)
File "/Users/n1508035/.local/share/virtualenvs/awsume-lm-saml-plugin-Qm4G-w3w/lib/python3.7/site-packages/awsume/awsumepy/lib/profile.py", line 173, in format_aws_profiles
source_profile = profile['source_profile'] if is_role_profile else 'None'
KeyError: 'source_profile'
I pushed a change to fix the --list-profiles
crash to master
You're right, our options are overloading the role_arn
property (which works right now I assume), or add a new non-standard property like principal_arn
. If we use role_arn
we could run into issues later since other components/plugins might expect the role_arn
property to contain only the role_arn. I think we should use a non-standard principal_arn
property which has only the principal arn, and we combine the principal_arn
and role_arn
to check against the available roles from the saml assertion. We'd need to update profile_lib.validate_profile
to make sure we don't throw if we find a profile that looks like that. I'm open to a discussion on this though
There's a configuration option fuzzy-match
that can be used to turn on/off the saml role fuzzy matching. In app.py
, you can check if self.config.get('fuzzy-match')
is true or not, so this would resolve your FIXME
note, although I don't think we should use fuzzy-matching when using a profile to select the saml role anyway, but happy to hear feedback if you have other thoughts
I don't think we should somehow support both --role-arn
and the profile_name
argument, since there's no way to know what the user really intends. Whether we prioritize one by using elif
at app.py:L143
(which would prioritize role_arn
since it's checked first), or we reject entirely if both are supplied, I'm not sure and am open to discussion
Thanks for the PR! This will be a great addition to awsume!
Okay, added support for principal_arn
. I agree on not using fuzzy matching. I found it giving me credentials different than the ones I had intended while I was testing. I also gave precedence to --role-arn
over a profile name
Looks good, thanks for the PR! I'm gonna push a few bug fixes before I release the change
Released in 4.1.3
As an aside, I added a --principal-arn
argument to go with the --role-arn
when you're doing saml assumptions (for the same reason we broke out the overloading of the role_arn
property)
And fuzzy-matching will only apply to using those arguments to select a saml role, but is toggle-able with the fuzzy-match
config property
Again, thanks for the PR!
Add profile support for
--with-saml
I'm possibly misusing role_arn in the credentials file by having it include both the SAML provider ARN and role_arn but the other option was to add a non-standard config value to the credentials file.