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

On Azure DevOps, upload Windows crash dumps to S3 on release branches #19183

Merged
merged 1 commit into from Apr 19, 2019

Conversation

Projects
None yet
1 participant
@nathansobo
Copy link
Contributor

commented Apr 18, 2019

In #19171, I changed the build pipeline to publish Windows crash reports as artifacts on pull requests. As I stated in that PR, because these crash reports contain environment variables, publishing them as artifacts on our release builds would leak secrets such as our S3 credentials.

In this follow-up PR, we upload Windows crash reports produced during release builds to S3 with a private ACL. This will allow anyone with full S3 credentials to retrieve the reports but prevent the public from obtaining them.

After a test run in which I intentionally crashed the render process, I confirmed that the report was uploaded to S3 with the appropriate permissions:

Here's me fetching the access control list for the uploaded crash report:

> aws s3api get-object-acl --bucket github-atom-io --key vsts-artifacts/38193/940dc76a-8f0e-4f53-a51e-953a2733c8a1.dmp
{
    "Owner": {
        "DisplayName": "github",
        "ID": "a925e1762249f9122ac610ec6917c1465d18f063a083ee989c825cbaa9483def"
    },
    "Grants": [
        {
            "Grantee": {
                "DisplayName": "github",
                "ID": "a925e1762249f9122ac610ec6917c1465d18f063a083ee989c825cbaa9483def",
                "Type": "CanonicalUser"
            },
            "Permission": "FULL_CONTROL"
        }
    ]
}

Contrast the above ACL to one that makes the object publicly-readable, such as for our ordinary build artifacts. Notice that ACL includes an entry for AllUsers granting them READ access, whereas the entry above does not. This satisfies me that I've handled the permissions correctly and we will avoid leaking release build secrets crash reports.

> aws s3api get-object-acl --bucket github-atom-io --key vsts-artifacts/9824/atom-mac.zip
{
    "Owner": {
        "DisplayName": "github",
        "ID": "a925e1762249f9122ac610ec6917c1465d18f063a083ee989c825cbaa9483def"
    },
    "Grants": [
        {
            "Grantee": {
                "DisplayName": "github",
                "ID": "a925e1762249f9122ac610ec6917c1465d18f063a083ee989c825cbaa9483def",
                "Type": "CanonicalUser"
            },
            "Permission": "FULL_CONTROL"
        },
        {
            "Grantee": {
                "Type": "Group",
                "URI": "http://acs.amazonaws.com/groups/global/AllUsers"
            },
            "Permission": "READ"
        }
    ]
}

Tasks:

  • Confirm we upload Windows crash reports to S3 on release builds
  • Confirm that crash reports have the appropriate ACL on S3
  • Confirm we don't upload crash reports to S3 on PR builds
  • Confirm that uploading step doesn't interfere with green release builds
  • Confirm that uploading step doesn't interfere with green PR builds
On Azure DevOps, upload Windows crash dumps to S3 on release branches
On release branches, we can't upload crash dumps because they will leak
secret environment variables. So instead we will upload them to our S3
bucket with 'private' ACL. They can then be manually retrieved via the
AWS CLI with our private credentials.
@nathansobo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

Trying out a couple last scenarios before merging.

@nathansobo nathansobo requested a review from smashwilson Apr 19, 2019

@nathansobo nathansobo force-pushed the ns/upload-windows-crashes-on-release-branches branch 3 times, most recently from 171e105 to 4bc43eb Apr 19, 2019

@nathansobo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

The failure is timing related and on Mac OS. Has absolutely nothing to do with the changes in this PR, which are needed to fight intermittent failures. I'm going ahead and merging.

@nathansobo nathansobo merged commit 406e033 into master Apr 19, 2019

1 of 2 checks passed

Atom Pull Requests #20190419.11 failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nathansobo nathansobo deleted the ns/upload-windows-crashes-on-release-branches branch Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.