Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SAML profiles #65

Merged
merged 4 commits into from
Sep 17, 2019
Merged

SAML profiles #65

merged 4 commits into from
Sep 17, 2019

Conversation

tmclaugh
Copy link
Contributor

Add profile support for --with-saml

configuration
[my-profile]
role_arn = <saml_provider_arn>,<role_arn>

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.

@tmclaugh
Copy link
Contributor Author

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'

@mbarneyjr
Copy link
Member

mbarneyjr commented Sep 14, 2019

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!

```
configuration
[my-profile]
role_arn = <saml_provider_arn>,<role_arn>
```

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.
@tmclaugh
Copy link
Contributor Author

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

@mbarneyjr mbarneyjr merged commit 44f9446 into trek10inc:master Sep 17, 2019
@mbarneyjr
Copy link
Member

Looks good, thanks for the PR! I'm gonna push a few bug fixes before I release the change

@mbarneyjr
Copy link
Member

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!

ryansb pushed a commit to ryansb/awsume that referenced this pull request Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants