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.

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

827 share verify and reapply fix #1062

Merged
merged 282 commits into from
Feb 29, 2024
Merged

827 share verify and reapply fix #1062

merged 282 commits into from
Feb 29, 2024

Conversation

noah-paige
Copy link
Contributor

@noah-paige noah-paige commented Feb 14, 2024 •
edited
Loading

Feature or Bugfix

  • Feature

Detail

  • Add Ability to Verify Health Status of Share Requests
  • Add Ability to Re-apply share request processing if in an unhealthy state

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

  • Does this PR introduce or modify any input fields or queries - this includes
    fetching data from storage outside the application (e.g. a database, an S3 bucket)?
    • Is the input sanitized?
    • What precautions are you taking before deserializing the data you consume?
    • Is injection prevented by parametrizing queries?
    • Have you ensured no eval or similar functions are used?
  • Does this PR introduce any functionality or component that requires authorization?
    • How have you ensured it respects the existing AuthN/AuthZ mechanisms?
    • Are you logging failed auth attempts?
  • Are you using or adding any cryptographic features?
    • Do you use a standard proven implementations?
    • Are the used keys controlled by the customer? Where are they stored?
  • Are you introducing any new policies/roles/users?
    • Have you used the least-privilege principle? How?

Pending TODO

  • ECS Scheduled Task to Verify Shares
  • Write Unit Tests
  • Add Verify On Dataset Level for All Shared Items in Associated Share Objects

Things to Consider

  • ECS Scheduled Task to be Weekly (default)

    • Open to discussion on if weekly is the best default schedule or if a different time range is better
    • Alternatively, can introducer a custom scheduler parameter in config.json

  • Currently shareitem health columns (healthStatus, healthMessage, lastVerifiedTimestamp) are a part of the same share_object_item table - makes it very easy to get the latest health of each share item

    • Take a look at the comment on Issue 827 for more information about the lifecycle of these health columns / how they are updated
    • Alternatively, could create a new share_object_item_health table that tracks every verify attempt and a column in share_object_item to point to the primary key of the record in share_object_item_health with the latest health information (updated for each verification run) - would persist each verification run for auditability / traceability

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

dlpzx and others added 30 commits October 31, 2023 09:19
@dlpzx
Revert overwrites 2.
1365e92
@dlpzx
Revert overwrites 3.
bbcfbd5
@dlpzx
Revert overwrites 4.
9e8cdf1
@dlpzx
Revert overwrites 4.
5d90797
@dlpzx
Revert overwrites 5.
94be491
@dlpzx
Revert overwrites 6.
cff577f
@dlpzx
Revert overwrites 7.
5ff80fb
@dlpzx
Revert overwrites 7.
3383166
@dlpzx
Revert overwrites 8.
7ed96af
@dlpzx
Revert overwrites 9.
c051896
@dlpzx
Revert overwrites 10.
f5d62d7
@dlpzx
Revert overwrites 11.
3783a95
@dlpzx
Revert overwrites 12.
dacba14
@dlpzx
Revert overwrites 13.
3b404cd
@dlpzx
Fix down revision for migration script
5d0fe68
@anushka-singh
feat: Enabling S3 bucket share
158925a
@anushka-singh
bugfix: Enabling S3 bucket share
d112a21
@anushka-singh
feat: Enabling S3 bucket share - Addressing comments on PR
06edb53
@anushka-singh
feat: Enabling S3 bucket share
f43003c
@anushka-singh
feat: Enabling S3 bucket share - Addressing comments on PR
4516f4d
@anushka-singh
feat: Enabling S3 bucket share - Addressing comments on PR
0f2faf7
@anushka-singh
Merge branch 'main' into bucket_share_anushka
9b0ab34 
# Conflicts:
#	backend/dataall/modules/datasets/api/table/resolvers.py
#	backend/migrations/versions/4f3c1d84a628_modify_notifications_column_types.py
#	deploy/stacks/backend_stack.py
#	deploy/stacks/ses_stack.py
#	frontend/src/modules/Environments/views/EnvironmentCreateForm.js
#	template_cdk.json
@anushka-singh
feat: Enabling S3 bucket share
7ab6427
@anushka-singh
feat: Enabling S3 bucket share - Addressing comments on PR
e251fcc
@anushka-singh
feat: Enabling S3 bucket share - Addressing comments on PR
e8bfb4b
@anushka-singh
Merge branch 'main' of https://github.com/anushka-singh/aws-dataall i…
2ff67bc 
…nto bucket_share_anushka
@anushka-singh
Update share.js
3254260
@anushka-singh
Update index.js
eb8bf3d
@mourya-33
Add User Directive
d3a2686
@mourya-33
Add User Directive
e961930
petrkalos
petrkalos reviewed Feb 26, 2024
View reviewed changes
Copy link
Contributor

@petrkalos petrkalos left a comment

Choose a reason for hiding this comment

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

There are a lot of formatting changes making it a bit hard to track what changed (#1076 )

backend/dataall/modules/dataset_sharing/db/share_object_repositories.py Outdated Show resolved Hide resolved
backend/dataall/modules/dataset_sharing/db/share_object_repositories.py Outdated Show resolved Hide resolved
backend/dataall/modules/dataset_sharing/services/data_sharing_service.py Outdated
revoked_tables,
source_environment,
target_environment,
env_group,
).process_revoked_shares()
log.info(f'revoking tables succeeded = {revoked_tables_succeed}')
log.info(f"revoking tables succeeded = {revoked_tables_succeed}")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Why switching those to double-quotes? I noticed that in other files you used single-quotes for strings.
Also generally speaking and especially in large PRs (like this) I'd refrain including unrelated changes in the same PR (i.e reformatting)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ran black formatter on this file to quickly fix the linting errors I was receiving from code changes I introduced - ended up re-formatting a lot of the file

To standardize formatting in the backend post this PR - for now I will leave the changes as such if okay with you

backend/dataall/modules/dataset_sharing/services/data_sharing_service.py Outdated Show resolved Hide resolved
...end/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py Outdated Show resolved Hide resolved
...end/dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py Show resolved Hide resolved
@dlpzx dlpzx mentioned this pull request Feb 26, 2024
Merged
dlpzx
dlpzx reviewed Feb 29, 2024
View reviewed changes
cdk.json Outdated Show resolved Hide resolved
dlpzx
dlpzx reviewed Feb 29, 2024
View reviewed changes
Copy link
Contributor

@dlpzx dlpzx left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Mostly minor things except for the default value of Healthy that I think it might be a tiny bug. I also reviewed the test cases, they look great. One curiosity, have you tested more than one error at the same time? From the code I think it will work fine

@noah-paige
Copy link
Contributor Author

Mostly minor things except for the default value of Healthy that I think it might be a tiny bug. I also reviewed the test cases, they look great. One curiosity, have you tested more than one error at the same time? From the code I think it will work fine

Yup have tested multiple at the same time as well! No errors seen so far with that either

dlpzx
dlpzx approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

lgtm, approving

@noah-paige noah-paige merged commit 45943a9 into main Feb 29, 2024
9 checks passed
@noah-paige noah-paige deleted the 827-share-verify-and-reapply-fix branch February 29, 2024 15:40
@dlpzx dlpzx mentioned this pull request Mar 8, 2024
Merged
noah-paige added a commit that referenced this pull request Mar 8, 2024
@dlpzx @noah-paige
Fix: Added check and reapply for attaching S3 IAM policy (#1096)
884c4c9 
### Feature or Bugfix
- Bugfix

### Detail
For S3 bucket shares and for S3 access point shares (folder sharing)
- In share-verify workflow add a check to verify the managed policy for
shares is attached to the target role. Log the error and raise an
unhealthy status
- In share-approve workflow add a check to verify the managed policy for
shares is attached to the target role and attach the policy if the
requester is a Group or a data.all Managed Consumption role

### Relates
- #1062 
- #1068 

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Co-authored-by: Noah Paige <noahpaig@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrkalos petrkalos petrkalos left review comments

@dlpzx dlpzx dlpzx approved these changes

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

Successfully merging this pull request may close these issues.

Add ability to re-apply KMS and bucket policy changes
9 participants
@noah-paige @petrkalos @dlpzx @anushka-singh @mourya-33 @TejasRGitHub @zsaltys @SofiaSazonova @dbalintx