Skip to content

MFTF AWS Secrets Manager - CI Use #554

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jan 27, 2020
Merged

MFTF AWS Secrets Manager - CI Use #554

merged 12 commits into from
Jan 27, 2020

Conversation

jilu1
Copy link
Contributor

@jilu1 jilu1 commented Jan 21, 2020

Description

Fixed Issues (if relevant)

  1. magento/magento2-functional-testing-framework#<issue_number>: Issue title
  2. ...

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/verification tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)
  • Changes to Framework doesn't have backward incompatible changes for tests or have related Pull Request with fixes to tests

@coveralls
Copy link

coveralls commented Jan 21, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-1.5%) to 50.994% when pulling df5f4f6 on MQE-1919 into a84ee26 on develop.

soumyau
soumyau suggested changes Jan 23, 2020
View reviewed changes
Copy link
Contributor

@soumyau soumyau left a comment

Choose a reason for hiding this comment

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

Initial set of comments.

docs/credentials.md Outdated
#### Use AWS Secrets Manager from other AWS account

- AWS account ID where the AWS Secrets Manager service is hosted
- IAM User or Role with appropriate access permission
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe elaborate on what access permission is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

docs/credentials.md Outdated
```conf
# Secret name for carriers_usps_userid
mftf/magento/carriers_usps_userid
`Secrets Value` in plaintext format can be any content you want to secure. `Secrets Value` in key/value pairs format, however, the `key` must be same as the `Secret Name` with `mftf/<VENDOR>/` part removed.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe reword to -

Secret Value can be stored in two different formats: plaintext (AWS CLI) or key/value pairs (AWS Console).
For plaintext format, Secret Value can be any string you want to secure.

For key/value pairs format, Secret Value is a key/value pair with key same as <YOUR/SECRET/KEY> of Secret Name and value can be any string you want to secure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

docs/credentials.md Outdated

Full AWS KMS ([Key Management Service][]) key ARN ([Amazon Resource Name][]) is required when accessing secrets stored in other AWS account.
If this is the case, you will also need to set `CREDENTIAL_AWS_ACCOUNT_ID` environment variable so that MFTF can construct the full ARN.
This is also commonly used in CI system.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe remove this line since we have hyperlinks to KMS and ARN, if we need more information on them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reword it a bit. I actual think this is indeed most used for CI/CD.

docs/credentials.md Outdated
@@ -186,6 +208,16 @@ CREDENTIAL_AWS_SECRETS_MANAGER_REGION=us-east-1
CREDENTIAL_AWS_SECRETS_MANAGER_PROFILE=default
```

### Optionally set CREDENTIAL_AWS_ACCOUNT_ID environment variable

Full AWS KMS ([Key Management Service][]) key ARN ([Amazon Resource Name][]) is required when accessing secrets stored in other AWS account.
Copy link
Contributor

Choose a reason for hiding this comment

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

other AWS account seems a little vague. Needs to be reworded to specify the type of account configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded

etc/config/.env.example Outdated
@@ -37,8 +37,6 @@ BROWSER=chrome
#*** To use AWS Secrets Manager to manage _CREDS secrets, uncomment and set region, profile is optional, when omitted, AWS default credential provider chain will be used ***#
#CREDENTIAL_AWS_SECRETS_MANAGER_PROFILE=default
#CREDENTIAL_AWS_SECRETS_MANAGER_REGION=us-east-1
#*** If using non-default AWS account ***#
Copy link
Contributor

Choose a reason for hiding this comment

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

rename to #*** For cross AWS account use ***#

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer applied

soumyau
soumyau suggested changes Jan 24, 2020
View reviewed changes
Copy link
Contributor

@soumyau soumyau left a comment

Choose a reason for hiding this comment

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

Few comments added, may need more elaborate documentation on local use, focusing on setting up .aws credentials file with IAM user access key id, aws_access_key_id, aws_secret_access_key and region

docs/credentials.md Outdated
#### Use AWS Secrets Manager from your own AWS account

- AWS account with Secrets Manager service available
- IAM User or Role is created with appropriate AWS Secrets Manger access permission
Copy link
Contributor

Choose a reason for hiding this comment

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

spell check on Manger

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@jilu1 jilu1 requested a review from dobooth January 27, 2020 16:17
@jilu1
Copy link
Contributor Author

jilu1 commented Jan 27, 2020

Hi @dobooth, there are some dev doc change in this PR. Please take a look. Thanks!

KevinBKozan
KevinBKozan approved these changes Jan 27, 2020
View reviewed changes
Copy link
Contributor

@KevinBKozan KevinBKozan left a comment

Choose a reason for hiding this comment

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

Style looks on point, was a little concerned about some changes in return value around decrypting (value or false return) but you seem to have caught all uses of it.

@jilu1
Copy link
Contributor Author

jilu1 commented Jan 27, 2020

Code change in this ticket exposed an old problem in decryption. The fix is to show the calling function when things go wrong instead of hiding the underlying problem.

@KevinBKozan
Copy link
Contributor

@jilu1 I figured there was a good reason for changing the function 👍

@jilu1 jilu1 merged commit b552483 into develop Jan 27, 2020
@jilu1 jilu1 deleted the MQE-1919 branch February 6, 2020 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soumyau soumyau soumyau approved these changes

@KevinBKozan KevinBKozan KevinBKozan approved these changes

@dobooth dobooth Awaiting requested review from dobooth

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jilu1 @coveralls @KevinBKozan @soumyau