Apply jdub policy suggestions

[simonw]

https://github.com/simonw/s3-credentials/blob/main/s3_credentials/policies.py

My suggestions:

• specify individual actions explicitly (no wildcards)
• separate permissions by resource (Buckets vs. Objects)
• Sid is unnecessary

Your read/write policy is good, but instead of *Object, list GetObject and PutObject.

Your read-only policy would be better written like your read/write policy, one section for the bucket permission (ListBucket), one for the object permission (which should be GetObject, no wildcard).

Your write-only policy is great as is.

You may want to add additional permissions to let clients set ACLs. But if it's all simple object-by-object stuff, these very simple policies are great.

Originally posted by @jdub in #7 (comment)

[simonw]

I think this is the definitive list of S3 actions (if I'm going to stop using wildcards I want to refer to this): https://docs.aws.amazon.com/AmazonS3/latest/userguide/list_amazons3.html#amazons3-actions-as-permissions

[simonw]

Extracting the list of actions from that page table:

actions = Array.from(
  document.querySelector("table").querySelectorAll("tr td:first-of-type")
).map(el => el.innerText).filter(t => t).map(t => 's3:' + t)
console.log(actions.map(action => `- ${action}`).join("\n"))

• s3:AbortMultipartUpload
• s3:BypassGovernanceRetention
• s3:CreateAccessPoint
• s3:CreateAccessPointForObjectLambda
• s3:CreateBucket
• s3:CreateJob
• s3:DeleteAccessPoint
• s3:DeleteAccessPointForObjectLambda
• s3:DeleteAccessPointPolicy
• s3:DeleteAccessPointPolicyForObjectLambda
• s3:DeleteBucket
• s3:DeleteBucketOwnershipControls
• s3:DeleteBucketPolicy
• s3:DeleteBucketWebsite
• s3:DeleteJobTagging
• s3:DeleteObject
• s3:DeleteObjectTagging
• s3:DeleteObjectVersion
• s3:DeleteObjectVersionTagging
• s3:DeleteStorageLensConfiguration
• s3:DeleteStorageLensConfigurationTagging
• s3:DescribeJob
• s3:GetAccelerateConfiguration
• s3:GetAccessPoint
• s3:GetAccessPointConfigurationForObjectLambda
• s3:GetAccessPointForObjectLambda
• s3:GetAccessPointPolicy
• s3:GetAccessPointPolicyForObjectLambda
• s3:GetAccessPointPolicyStatus
• s3:GetAccessPointPolicyStatusForObjectLambda
• s3:GetAccountPublicAccessBlock
• s3:GetAnalyticsConfiguration
• s3:GetBucketAcl
• s3:GetBucketCORS
• s3:GetBucketLocation
• s3:GetBucketLogging
• s3:GetBucketNotification
• s3:GetBucketObjectLockConfiguration
• s3:GetBucketOwnershipControls
• s3:GetBucketPolicy
• s3:GetBucketPolicyStatus
• s3:GetBucketPublicAccessBlock
• s3:GetBucketRequestPayment
• s3:GetBucketTagging
• s3:GetBucketVersioning
• s3:GetBucketWebsite
• s3:GetEncryptionConfiguration
• s3:GetIntelligentTieringConfiguration
• s3:GetInventoryConfiguration
• s3:GetJobTagging
• s3:GetLifecycleConfiguration
• s3:GetMetricsConfiguration
• s3:GetObject
• s3:GetObjectAcl
• s3:GetObjectLegalHold
• s3:GetObjectRetention
• s3:GetObjectTagging
• s3:GetObjectVersion
• s3:GetObjectVersionAcl
• s3:GetObjectVersionForReplication
• s3:GetObjectVersionTagging
• s3:GetReplicationConfiguration
• s3:GetStorageLensConfiguration
• s3:GetStorageLensConfigurationTagging
• s3:GetStorageLensDashboard
• s3:ListAccessPoints
• s3:ListAccessPointsForObjectLambda
• s3:ListAllMyBuckets
• s3:ListBucket
• s3:ListBucketMultipartUploads
• s3:ListBucketVersions
• s3:ListJobs
• s3:ListMultipartUploadParts
• s3:ListStorageLensConfigurations
• s3:ObjectOwnerOverrideToBucketOwner
• s3:PutAccelerateConfiguration
• s3:PutAccessPointConfigurationForObjectLambda
• s3:PutAccessPointPolicy
• s3:PutAccessPointPolicyForObjectLambda
• s3:PutAccountPublicAccessBlock
• s3:PutAnalyticsConfiguration
• s3:PutBucketAcl
• s3:PutBucketCORS
• s3:PutBucketLogging
• s3:PutBucketNotification
• s3:PutBucketObjectLockConfiguration
• s3:PutBucketOwnershipControls
• s3:PutBucketPolicy
• s3:PutBucketPublicAccessBlock
• s3:PutBucketRequestPayment
• s3:PutBucketTagging
• s3:PutBucketVersioning
• s3:PutBucketWebsite
• s3:PutEncryptionConfiguration
• s3:PutIntelligentTieringConfiguration
• s3:PutInventoryConfiguration
• s3:PutJobTagging
• s3:PutLifecycleConfiguration
• s3:PutMetricsConfiguration
• s3:PutObject
• s3:PutObjectAcl
• s3:PutObjectLegalHold
• s3:PutObjectRetention
• s3:PutObjectTagging
• s3:PutObjectVersionAcl
• s3:PutObjectVersionTagging
• s3:PutReplicationConfiguration
• s3:PutStorageLensConfiguration
• s3:PutStorageLensConfigurationTagging
• s3:ReplicateDelete
• s3:ReplicateObject
• s3:ReplicateTags
• s3:RestoreObject
• s3:UpdateJobPriority
• s3:UpdateJobStatus

[simonw]

Here's a manual test after removing the Sid:

% s3-credentials create static.niche-museums.com --username read-write-niche-museums
Created user: read-write-niche-museums with permissions boundary: arn:aws:iam::aws:policy/AmazonS3FullAccess
Attached policy s3.read-write.static.niche-museums.com to user read-write-niche-museums
Created access key for user: read-write-niche-museums
{
    "UserName": "read-write-niche-museums",
    "AccessKeyId": "AKIAWXFXAIOZKRSNTSE3",
    "Status": "Active",
    "SecretAccessKey": "...",
    "CreateDate": "2021-11-03 19:05:07+00:00"
}
% s3-credentials list-user-policies read-write-niche-museums
User: read-write-niche-museums
PolicyName: s3.read-write.static.niche-museums.com
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": [
                "s3:ListBucket"
            ],
            "Resource": [
                "arn:aws:s3:::static.niche-museums.com"
            ]
        },
        {
            "Effect": "Allow",
            "Action": "s3:*Object",
            "Resource": [
                "arn:aws:s3:::static.niche-museums.com/*"
            ]
        }
    ]
}

[simonw]

You may want to add additional permissions to let clients set ACLs. But if it's all simple object-by-object stuff, these very simple policies are great.

I decided to handle that with a new --policy custom-policy.json option, see:

• Option to specify a custom JSON policy file #14

[simonw]

Your read/write policy is good, but instead of *Object, list GetObject and PutObject.

I'm really torn on this one.

For read-write access I think you need to be able to delete objects as well. And update their metadata/tags/CORS. As you can see from the list of permissions above, it's a lot - and I'm presuming AWS add even more *Object items as time goes on.

So I'm leaning towards sticking with the *Object wildcard as the default here. If people want to use a more restrictive policy (and good on them if they do) they can use the --policy mechanism from #14.

But... wildcards are so obviously gross! I think I'll open a separate issue to think through the implications of this one in even more detail.

[simonw]

Your read-only policy would be better written like your read/write policy, one section for the bucket permission (ListBucket), one for the object permission (which should be GetObject, no wildcard).

Applied that change, now the two policies are much more consistent with each other:

s3-credentials/s3_credentials/policies.py

Lines 1 to 35 in 86861a3

	def read_write(bucket):
	# https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_examples_s3_rw-bucket.html
	return {
	"Version": "2012-10-17",
	"Statement": [
	{
	"Effect": "Allow",
	"Action": ["s3:ListBucket"],
	"Resource": ["arn:aws:s3:::{}".format(bucket)],
	},
	{
	"Effect": "Allow",
	"Action": "s3:*Object",
	"Resource": ["arn:aws:s3:::{}/*".format(bucket)],
	},
	],
	}
	def read_only(bucket):
	return {
	"Version": "2012-10-17",
	"Statement": [
	{
	"Effect": "Allow",
	"Action": ["s3:ListBucket"],
	"Resource": ["arn:aws:s3:::{}".format(bucket)],
	},
	{
	"Effect": "Allow",
	"Action": "s3:GetObject*",
	"Resource": ["arn:aws:s3:::{}/*".format(bucket)],
	},
	],
	}

[simonw]

I've convinced myself in a comment here that wildcards as currently implemented really are bad: #15 (comment)

Closing this issue in favour of that one.