Add option to disable presigned URL generation for public S3 buckets#660
Add option to disable presigned URL generation for public S3 buckets#660timbaev wants to merge 3 commits intojenkinsci:masterfrom
Conversation
- Add generatePresignedUrls configuration option (default: true) - Implement direct URL generation when presigned URLs are disabled - Update S3BlobStore to support both URL generation modes - Add UI checkbox for generatePresignedUrls setting - Include comprehensive test coverage for new functionality - Update validation method to include new parameter This change allows users to choose between presigned URLs (secure, time-limited) and direct URLs (permanent, bucket-policy dependent) for S3 artifact access.
src/main/java/io/jenkins/plugins/artifact_manager_jclouds/s3/S3BlobStore.java
Outdated
Show resolved
Hide resolved
src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/JCloudsArtifactManagerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/JCloudsArtifactManagerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/JCloudsArtifactManagerTest.java
Outdated
Show resolved
Hide resolved
|
Presumably your automated & manual tests involved a bucket with public read access but I am unclear on what your automated & manual tests consisted of beyond that. (The existence of https://docs.aws.amazon.com/config/latest/developerguide/s3-bucket-public-write-prohibited.html implies that public write access is technically possible, but that is certainly not something we would want to encourage.) |
…loads - Always use presigned URLs for PUT operations regardless of generatePresignedUrls setting - Respect generatePresignedUrls setting only for download operations (GET, etc.) - Remove direct URL generation for artifact uploads in artifactUrls method - Simplify testDirectUrlGeneration test and improve assertions with Hamcrest matchers
|
@jglick Thanks for the review! Updated the logic to use the new parameter only for downloading artifacts. Also checked the test - it passes now. |
|
@jglick Hello 👋 |
|
@jglick Hello 👋 |
|
On my list but so far I have not found the time to review this (or indeed maintain this plugin much at all). |
This PR adds a new configuration option
generatePresignedUrlsto the S3 artifact storage that allows users to disable the generation of presigned URLs and instead use direct S3 URLs for artifact access. This is useful when S3 buckets are configured with public read access and don't require authentication parameters in the URLs.Key changes:
generatePresignedUrlsboolean configuration field (defaults totruefor backward compatibility)S3BlobStore.toExternalURL()andS3BlobStore.artifactUrls()to return direct URLs when presigned URL generation is disabledhttps://bucket.s3.region.amazonaws.com/path/to/artifactwithout authentication parametersTesting done
Automated tests:
testDirectUrlGeneration()test inJCloudsArtifactManagerTestthat verifies:generatePresignedUrlsis set tofalse, the generated URLs do not contain presigned authentication parameters (X-Amz-Algorithm, X-Amz-Credential, X-Amz-Signature)checkGeneratePresignedUrlsDefaultAndSetterGetter()test inS3BlobStoreConfigTestthat verifies:true(maintaining backward compatibility)Manual testing:
UI Screenshots:
Configuration page before changes
Configuration page after changes
Submitter checklist