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

s3.get module not working due to invalid role_arn handling #30899

Closed
kluoto opened this issue Feb 4, 2016 · 16 comments
Closed

s3.get module not working due to invalid role_arn handling #30899

kluoto opened this issue Feb 4, 2016 · 16 comments
Labels
Bug broken, incorrect, or confusing behavior Execution-Module P2 Priority 2 RIoT Relates to integration with cloud providers, hypervisors, API-based services, etc. severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Milestone

Comments

@kluoto
Copy link

kluoto commented Feb 4, 2016

Salt version 2015.8.4

2016-02-04 14:29:27,694 [salt.utils.aws ][INFO ][11995] AssumeRole response: {"Error":{"Code":"SignatureDoesNotMatch","Message":"Credential should be scoped to a valid region, not 'eu-central-1'. ","Type":"Sender"},"RequestId":"b225b6f9-cb4b-11e5-8724-17b3d4e5f0fc"}
2016-02-04 14:29:27,695 [salt.state ][ERROR ][11995] Module function s3.get threw an exception. Exception: 403 Client Error: Forbidden

Reason for the problem:

  1. on salt/modules/s3.py:260 role_arn get replaced with empty string
  2. as result salt/utils/aws.py:219 will call the assumed_creds function instead of creds function and thus leads to the error above
@jfindlay jfindlay added the info-needed waiting for more info label Feb 4, 2016
@jfindlay jfindlay added this to the Blocked milestone Feb 4, 2016
@jfindlay
Copy link
Contributor

jfindlay commented Feb 4, 2016

@kluoto, thanks for the report. Will you also post the command line you are using that results in this error?

@patel66
Copy link

patel66 commented Feb 4, 2016

@jfindlay

root@ip:/home/admin# salt-call s3.get
[INFO    ] Starting new HTTPS connection (1): sts.amazonaws.com
[INFO    ] AssumeRole response: {"Error":{"Code":"SignatureDoesNotMatch","Message":"Credential should be scoped to a valid region, not 'us-west-2'. ","Type":"Sender"},"RequestId":"b6c98272-cb80-11e5-87aa-57f6b97b4226"}
[ERROR   ] An un-handled exception was caught by salt's global exception handler:
HTTPError: 403 Client Error: Forbidden
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    salt_call()
  File "/usr/lib/python2.7/dist-packages/salt/scripts.py", line 335, in salt_call
    client.run()
  File "/usr/lib/python2.7/dist-packages/salt/cli/call.py", line 53, in run
    caller.run()
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 133, in run
    ret = self.call()
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 196, in call
    ret['return'] = func(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/salt/modules/s3.py", line 169, in get
    role_arn=role_arn)
  File "/usr/lib/python2.7/dist-packages/salt/utils/s3.py", line 112, in query
    requesturl=requesturl,
  File "/usr/lib/python2.7/dist-packages/salt/utils/aws.py", line 219, in sig4
    access_key_id, secret_access_key, token = assumed_creds(prov_dict, role_arn, location=location)
  File "/usr/lib/python2.7/dist-packages/salt/utils/aws.py", line 194, in assumed_creds
    result.raise_for_status()
  File "/usr/lib/python2.7/dist-packages/requests/models.py", line 851, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
HTTPError: 403 Client Error: Forbidden
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    salt_call()
  File "/usr/lib/python2.7/dist-packages/salt/scripts.py", line 335, in salt_call
    client.run()
  File "/usr/lib/python2.7/dist-packages/salt/cli/call.py", line 53, in run
    caller.run()
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 133, in run
    ret = self.call()
  File "/usr/lib/python2.7/dist-packages/salt/cli/caller.py", line 196, in call
    ret['return'] = func(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/salt/modules/s3.py", line 169, in get
    role_arn=role_arn)
  File "/usr/lib/python2.7/dist-packages/salt/utils/s3.py", line 112, in query
    requesturl=requesturl,
  File "/usr/lib/python2.7/dist-packages/salt/utils/aws.py", line 219, in sig4
    access_key_id, secret_access_key, token = assumed_creds(prov_dict, role_arn, location=location)
  File "/usr/lib/python2.7/dist-packages/salt/utils/aws.py", line 194, in assumed_creds
    result.raise_for_status()
  File "/usr/lib/python2.7/dist-packages/requests/models.py", line 851, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 403 Client Error: Forbidden

@kluoto
Copy link
Author

kluoto commented Feb 5, 2016

@jfindlay This was part of my orchestration. State part that failed was:

download_the_pkg_file:
  module.run:
    - name: s3.get
    - bucket: {{ bucket }}
    - location: {{ aws_location }}
    - path: {{ pkg_file }}
    - local_file: {{ local_pkg_file }}

I made quick patch to my system by adding line:

 if not role_arn:
       role_arn = None

to end of the _get_key function in salt/modules/s3.py

@jfindlay jfindlay modified the milestones: Approved, Blocked Feb 5, 2016
@jfindlay jfindlay added Execution-Module Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around P2 Priority 2 RIoT Relates to integration with cloud providers, hypervisors, API-based services, etc. and removed info-needed waiting for more info labels Feb 5, 2016
@jfindlay
Copy link
Contributor

jfindlay commented Feb 5, 2016

@timcharper, do you have any ideas on this one?

@timcharper
Copy link
Contributor

I am so, so ashamed of myself. Sorry. Ugh. I suck. I tested the role_arn assumption because that is what my environment uses, but I didn't think to test that it worked with a role_arn.

I'll get a patch fixing this today, and a workaround. (maybe a sed script to patch salt).

Seriously sorry about this.

@lomeroe
Copy link
Contributor

lomeroe commented Feb 6, 2016

If the s3.role_arn isn't set, it looks like config.option just returns an empty string.

Changing

if role_arn is None and __salt__['config.option']('s3.role_arn') is not None:

to

if role_arn is None and __salt__['config.option']('s3.role_arn'):

does the trick for me (similar to fix from @kluoto above), but perhaps there's a better fix...

This sed does it...

sed "s/if role_arn is None and __salt__\['config\.option'\]('s3\.role_arn') is not None:/if role_arn is None and __salt__\['config\.option'\]\('s3\.role_arn'\):/g" -i /usr/lib/python2.7/site-packages/salt/modules/s3.py

Do the other checks in that function on the config.options need to be changed to?

@timcharper
Copy link
Contributor

Still looking at this. Present question, why is role_arn an empty string? Where is that coming from?

@timcharper
Copy link
Contributor

My mistake was assuming that the config.option returned None by default for undefined config. So terribly sorry about this mistake! I should've tested more throughly.

@timcharper
Copy link
Contributor

added a regression test

@timcharper
Copy link
Contributor

I had the assumption that the reason why config.option defaulted to empty string was because that's what PyYAML returned for keys lacking values, as in the following case:

s3.role_arn: # nothing here lol

But that turned out to be wrong. I'm not sure why config.option is the way it is. But, I assume there's a reason, so I coded it to match the same style as how key and keyid is being pulled from config.

This issue should be able to be worked around by simply adding this to your minion config (I believe this value can be propagate through pillar data, as well). You shouldn't need to edit the code.

s3.role_arn: 

That will cause role_arn to be read as None by the configuration.

Again, to the thousands of people I probably inconvenienced by this mistake: I'm sorry.

@lomeroe
Copy link
Contributor

lomeroe commented Feb 7, 2016

more oddities (at least I find it odd):

adding s3.role_arn on a masterless does not fix it:

masterful minion:

#cat /etc/salt/minion.d/20-s3fix.conf
s3:
  role_arn:

#salt-call config.option s3.role_arn
local:
    None

masterless minion

#cat /etc/salt/minion.d/20-s3fix.conf
s3:
  role_arn:

#salt-call config.option s3.role_arn
local:

even more odd, on a masterless if you config.option just s3, it does return None for role_arn

#salt-call config.option s3
local:
    ----------
    role_arn:
        None

@lomeroe
Copy link
Contributor

lomeroe commented Feb 7, 2016

Sorry, I am a bonehead. The masterful was returning None because I had created a pillar to assign s3.role_arn already (sigh).

A masterless with a pillar containing an empty s3.role_arn returns None as well. Adding it to the minion config only in either scenario returns an empty string.

Not sure if that is "as expected" or not...

@timcharper
Copy link
Contributor

@lomeroe I've confirmed, locally, that adding the line s3.role_arn: to the minion config /etc/salt/config indeed causes config.option('s3.role_arn') to return None.

@lomeroe
Copy link
Contributor

lomeroe commented Feb 7, 2016

@timcharper your right, using a single line as opposed to how I originally entered it in an included conf file above does indeed return None

@jfindlay
Copy link
Contributor

jfindlay commented Feb 8, 2016

@timcharper, thanks for your work on this.

@patel66
Copy link

patel66 commented Feb 11, 2016

@timcharper Defining a blank s3.role_arn fixes the issue when I run highstate directly on the minion but fails during s3.get when I execute a highstate from the master. I don't understand what would cause that

@cro cro closed this as completed in b4fe9aa Feb 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Execution-Module P2 Priority 2 RIoT Relates to integration with cloud providers, hypervisors, API-based services, etc. severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around
Projects
None yet
Development

No branches or pull requests

5 participants