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

add role-based credentials to COPY and UNLOAD command #88

Merged
merged 1 commit into from
May 18, 2016

Conversation

mtrbean
Copy link
Contributor

@mtrbean mtrbean commented May 8, 2016

Closes #87

Todos

  • MIT compatible
  • Tests
  • Documentation
  • Updated CHANGES.rst

@@ -51,6 +52,22 @@ def test_basic_copy_case():
assert clean(expected_result) == clean(compile_query(copy))


def test_iam_role():
"""Tests the use of iam role instead of access keys."""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably put iam_role as a variable here.

@graingert
Copy link
Member

@mtrbean can you clean up your commits, and tick all the boxes as appropriate

@mtrbean
Copy link
Contributor Author

mtrbean commented May 9, 2016

I have moved iam_role to the test functions and squashed the commits.

)
)
else:
return 'aws_iam_role=' + iam_role
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this is written, iam_role takes precedence over access_key_id/secret_access_key/session_token, but the documentation doesn't make this explicit. Providing both types of credentials seems like it indicates confusion on the part of the user, and I'd propose that we raise an error in that case indicating that the two credential styles are mutually exclusive and should not be mixed.

So, if within this if block, we'd check if any of access key ID, secret access key, or session token are not None, and then raise an error.

Copy link
Member

@graingert graingert May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably best to create two classes, so you can use the API like:

.copy(access=APIKeyCredential(access_key_id, secret_access_key, session_token), ...)
.copy(access=IAMRoleCredential(iam_role), ...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably best to create two classes

I'd be fine with that. Would we want to keep the existing access_key_id, secret_access_key, session_token options for backward-compatibility and mark them as deprecated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to stick with the code as is for now, maybe add a TypeError

I've added it to my personal v1-breaking-change-wishlist

Copy link
Member

@graingert graingert May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtrbean can you check split the iam_role into aws_account_id and iam_role_name validating both against regexes.

if iam_role_name is not None:
    if not AWS_ACCOUNT_ID_RE.match(aws_account_id):
        raise ValueError(...)
    if not IAM_ROLE_NAME_RE.match(iam_role_Name):
        raise ValueError(...)

    credentials = 'aws_iam_role=arn:aws:iam::{aws_acount_id}:role/{iam_role_name}'.format(...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to stick with the code as is for now, maybe add a TypeError

I'm fine with that.

Copy link
Member

@graingert graingert May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently the iam_role_name regex is .* allowing "; DROP TABLE foobar; --" which will match invalid roles.

Copy link
Member

@graingert graingert May 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iam role: "Maximum 64 characters. Use alphanumeric and '+=,.@-_' characters"
aws account id: 12-digit number

@mtrbean
Copy link
Contributor Author

mtrbean commented May 12, 2016

Changes made, let me know if anything further changes needed

@graingert
Copy link
Member

looks good, squash your commits though

@mtrbean
Copy link
Contributor Author

mtrbean commented May 16, 2016

Squashed. I think you can also make Github squash the commits for you when you merge.

@graingert
Copy link
Member

I can it's nicer not to though as it looses the merge commit
On 16 May 2016 7:29 pm, "Eric Wong" notifications@github.com wrote:

Squashed. I think you can also make Github squash the commits for you when
you merge.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#88 (comment)

@mtrbean
Copy link
Contributor Author

mtrbean commented May 18, 2016

@graingert can this be merged?

@graingert graingert merged commit cb93725 into sqlalchemy-redshift:master May 18, 2016
@graingert
Copy link
Member

@mtrbean merged! thanks :)

@mtrbean mtrbean deleted the rolebased-credentials branch January 5, 2017 21:58
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.

3 participants