headobject apifix

[nit-practo]

why - if file not found headobject api returns 403

[tarunjadhwani]

@justjkk Can you also merge this PR?

[anujith-singh]

Hey @nit-practo we are not using this repository.
This was just forked once but never maintained (I assumed I forked it, but I guess I didn't) even the repo from where this was forked is not maintained.

Across Practo we are using https://github.com/practo/base-images/tree/master/custom_services/db-archiver
Configuration example for above can be found here: https://github.com/practo/consult-api/blob/e357d2ae5583e323fa4111f2f75128aeb5ec25b2/config/kubernetes/core_crons.json#L354-L366

[anujith-singh]

The purpose of the method is to check if the file exists in s3, if so then the caller appends variable version to the fiel name.

If there is no access then the method should raise exception as it was earlier.

[nit-practo]

boto returns 403 when we check for the existence of file (if it does not exist)
botocore.exceptions.ClientError: An error occurred (403) when calling the HeadObject operation: Forbidden
cc : @justjkk

[justjkk]

According to this stackoverflow answer, s3 returns 403 instead of 404 if the ListBucket access is not present. In our case, the ListBucket access was present but restricted to a folder. I checked and found that the restriction was using StringEquals instead of StringLike which was preventing the ListBucket call from succeeding when applied on any path other than the top-level folder. I fixed the IAM permissions and it is now returning 404 as expected. However, it still makes sense to treat 403 also as a not found and not require the ListBucket permission.

[justjkk]

Final working IAM permissions for reference:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Allow",
            "Action": "s3:ListBucket",
            "Resource": "arn:aws:s3:::<bucket-name>",
            "Condition": {
                "StringLike": {
                    "s3:prefix": "<db-name>/*"
                }
            }
        },
        {
            "Sid": "VisualEditor1",
            "Effect": "Allow",
            "Action": [
                "s3:PutObject",
                "s3:GetObject",
                "s3:AbortMultipartUpload",
                "s3:ListMultipartUploadParts"
            ],
            "Resource": "arn:aws:s3:::<bucket-name>/<db-name>/*"
        },
        {
            "Sid": "VisualEditor2",
            "Effect": "Allow",
            "Action": "s3:ListBucketMultipartUploads",
            "Resource": "arn:aws:s3:::<bucket-name>"
        }
    ]
}

[anujith-singh]

@justjkk @nit-practo the problem with treating 403 as 404 is:

If the IAM user does not have list/read access but has write access (unlikely but techinically possible) then

• check_if_s3_file_exists() will return False
• and get_usable_s3_path() will end up returning the path it just examined
• and upload_to_s3() will end up overriding the file with a new version, which defeats the entire purpose of check_if_s3_file_exists()

Since as Kishore mentioned this is not a blocking problem any more(which we presumed earlier), we should update the ReadMe to have this document and call it done.

[justjkk]

@anujith-singh agreed with your reasoning.