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

fix: update ProductArn with account id #2782

Merged
merged 3 commits into from Sep 8, 2022

Conversation

AndrewCharlesHay
Copy link
Contributor

@AndrewCharlesHay AndrewCharlesHay commented Aug 25, 2022

Description

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've updated the documentation with the relevant information (if needed).
  • I've added tests that prove my fix is effective or that my feature works.
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@CLAassistant
Copy link

CLAassistant commented Aug 25, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@knqyf263 knqyf263 requested a review from afdesk August 30, 2022 12:58
@afdesk
Copy link
Contributor

afdesk commented Aug 30, 2022

@AndrewCharlesHay thanks a lot for your efforts and your PR. It's really cool.

contrib/asff.tpl Outdated Show resolved Hide resolved
@AndrewCharlesHay
Copy link
Contributor Author

@afdesk do you have approval permissions?

@afdesk
Copy link
Contributor

afdesk commented Sep 1, 2022

approved. thanks

@knqyf263
Copy link
Collaborator

knqyf263 commented Sep 1, 2022

@AndrewCharlesHay I'm sorry, but CLA assistant seems to have an issue and all the signs were reset. Do you mind if I ask you to sign it again?

@AndrewCharlesHay
Copy link
Contributor Author

AndrewCharlesHay commented Sep 1, 2022

@AndrewCharlesHay I'm sorry, but CLA assistant seems to have an issue and all the signs were reset. Do you mind if I ask you to sign it again?

@knqyf263 How do I sign it? I don't really know what that means

@AndrewCharlesHay
Copy link
Contributor Author

I made both of those commits using the Github web vscode. It looks like the are signed. I don't know how I would sign it again
image

@AndrewCharlesHay
Copy link
Contributor Author

AndrewCharlesHay commented Sep 1, 2022

Nevermind I think I got it. Should be good now I think

@AndrewCharlesHay
Copy link
Contributor Author

@afdesk Sorry I added documentation I guess that made your approval expire. Could you reapprove when you get a chance please

@AndrewCharlesHay
Copy link
Contributor Author

AndrewCharlesHay commented Sep 2, 2022

Thank you!! @afdesk

@AndrewCharlesHay
Copy link
Contributor Author

@knqyf263 Is the signing good?

@AndrewCharlesHay
Copy link
Contributor Author

@afdesk Are you able to merge this?

@afdesk
Copy link
Contributor

afdesk commented Sep 7, 2022

@afdesk Are you able to merge this?

no, I have no permissions to merge

@@ -82,7 +82,7 @@
{
"SchemaVersion": "2018-10-08",
"Id": "{{ $target }}/{{ .ID }}",
"ProductArn": "arn:aws:securityhub:{{ env "AWS_REGION" }}::product/aquasecurity/aquasecurity",
"ProductArn": "arn:aws:securityhub:{{ env "AWS_REGION" }}:{{ env "AWS_ACCOUNT_ID" }}:product/aquasecurity/trivy",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember there is permission importing findings to Security Hub. Isn't the ProductArn relevant?

Copy link
Contributor

Choose a reason for hiding this comment

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

@knqyf263 I took a loot at the documentation
https://docs.aws.amazon.com/service-authorization/latest/reference/list_awssecurityhub.html

and found next definition of resource type product:

arn:${Partition}:securityhub:${Region}:${Account}:product/${Company}/${ProductId}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AndrewCharlesHay After you changed ProductArn, did you confirm the ASFF file could be imported into Security Hub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I have local version of this file with just the arn changed and I am able to upload to Security Hub

@AndrewCharlesHay
Copy link
Contributor Author

It looks like that repo was abandoned in 2020 after it was made, but I made a PR ^ to be in line with these changes

@knqyf263 knqyf263 merged commit 6717665 into aquasecurity:main Sep 8, 2022
@AndrewCharlesHay AndrewCharlesHay deleted the hotfix/product-arn branch September 8, 2022 17:27
@AndrewCharlesHay
Copy link
Contributor Author

AndrewCharlesHay commented Sep 8, 2022

@knqyf263 and @afdesk Thank you! 🥳

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.

Incorrect Product ARN
4 participants