Add support for signed upload urls. #1661

erikcarlsson · 2024-12-19T09:19:26Z

No description provided.

encore-cla · 2024-12-19T09:19:41Z

All committers have signed the CLA.

eandre · 2024-12-20T10:58:06Z

runtimes/js/encore.dev/storage/objects/bucket.ts

+export interface UploadUrlOptions {
+  /** The expiration time of the url, in seconds from signing. The maximum
+   * value is seven days */
+  ttl: number;


Options should typically be optional:

Suggested change

ttl: number;

ttl?: number;

eandre · 2024-12-20T10:58:44Z

runtimes/js/src/objects.rs

+    #[napi]
+    pub async fn get_upload_url(
+        &self,
+        options: Option<UploadUrlOptions>, // TODO: can/should this be made non-optional, since ttl is required?


ttl shouldn't be required, right? The user-facing docs state the ttl defaults to 7 days if not provided

eandre · 2024-12-20T10:59:01Z

runtimes/js/src/objects.rs

+impl From<UploadUrlOptions> for core::UploadUrlOptions {
+    fn from(value: UploadUrlOptions) -> Self {
+        Self {
+            ttl: value.ttl as u64,


Convert this to the default of 7 days here

eandre · 2024-12-20T10:59:10Z

runtimes/js/src/objects.rs

+#[napi(object)]
+#[derive(Debug, Default)]
+pub struct UploadUrlOptions {
+    pub ttl: i64,


Suggested change

pub ttl: i64,

pub ttl: Option<i64>,

eandre · 2024-12-20T11:03:18Z

runtimes/go/storage/objects/options.go

+
+// WithTtl is used for signed URLs, to specify the lifetime of the generated
+// URL, in seconds. The max value is seven days.
+func WithTtl(ttl uint64) withTtlOption {


Go convention is TTL not Ttl. Also make it take a time.Duration instead of an uint64

Suggested change

func WithTtl(ttl uint64) withTtlOption {

func WithTTL(ttl time.Duration) withTTLOption {

eandre · 2024-12-20T11:09:06Z

runtimes/js/encore.dev/storage/objects/bucket.ts

+   * Anyone with possession of the URL can write to the given object name
+   * without any additional auth.
+   */
+  async getUploadUrl(name: string, options?: UploadUrlOptions): Promise<string> {


Let's make this signedUploadUrl, and have it return a SignedURL object with just a single field {url: string}

Suggested change

async getUploadUrl(name: string, options?: UploadUrlOptions): Promise<string> {

async signedUploadUrl(name: string, options?: SignedUploadUrlOptions): Promise<SignedURL> {

Done. Although used SignedUploadUrl instead of SignedUrl. I think we ended up agreeing on making upload URLs more flexible, but not so far as folding in signed GET and PUT URLs in one type. But let me know if I got that wrong.

eandre · 2024-12-20T11:12:10Z

tsparser/src/parser/resources/infra/objects.rs

@@ -120,6 +120,7 @@ pub fn resolve_bucket_usage(data: &ResolveUsageData, bucket: Lrc<Bucket>) -> Opt
                "list" => Operation::ListObjects,
                "exists" | "attrs" => Operation::GetObjectMetadata,
                "upload" => Operation::WriteObject,
+                "getUploadUrl" => Operation::WriteObject,


Rename to signedUploadUrl as per above. Also, let's introduce a new SignedUploadUrl operation (needs to be done in legacymeta.rs as well)

eandre · 2024-12-20T11:25:48Z

runtimes/go/storage/objects/internal/providers/s3/bucket.go

-		client: client,
-		cfg:    runtimeCfg,
+		client:         client,
+		presign_client: s3.NewPresignClient(client),


Instantiate in clientForProvider instead so we can cache it

Done, thanks

eandre · 2024-12-20T11:25:58Z

runtimes/go/storage/objects/internal/providers/s3/bucket.go

-	client *s3.Client
-	cfg    *config.Bucket
+	client         *s3.Client
+	presign_client *s3.PresignClient


Suggested change

presign_client *s3.PresignClient

presignClient *s3.PresignClient

eandre · 2024-12-20T11:26:09Z

runtimes/go/storage/objects/internal/providers/s3/bucket.go

+		Bucket: &b.cfg.CloudName,
+		Key:    &object,
+	}
+	sign_opts := func(opts *s3.PresignOptions) {


Suggested change

sign_opts := func(opts *s3.PresignOptions) {

signOpts := func(opts *s3.PresignOptions) {

eandre · 2024-12-20T11:27:02Z

runtimes/go/storage/objects/internal/providers/s3/bucket.go

+	sign_opts := func(opts *s3.PresignOptions) {
+		opts.Expires = time.Duration(data.Ttl) * time.Second
+	}
+	req, err := b.presign_client.PresignPutObject(data.Ctx, &params, sign_opts)


Do the error check here instead

Suggested change

req, err := b.presign_client.PresignPutObject(data.Ctx, &params, sign_opts)

req, err := b.presign_client.PresignPutObject(data.Ctx, &params, sign_opts)

if err != nil {

return "", mapErr(err)

}

return req.URL, nil

eandre · 2025-01-10T12:15:27Z

cli/daemon/run/runtime_config2.go

@@ -833,3 +838,32 @@ func gzipBytes(data []byte) []byte {
 	_ = w.Close()
 	return buf.Bytes()
 }
+
+const dummyPrivateKey = `-----BEGIN PRIVATE KEY-----


I think we should base64-encode this and decode it at runtime; there are a bunch of tools that automatically flag private keys in source code and they get upset otherwise

Agreed, better to avoid those. I reversed instead of base64 just because I think it's easier to inspect. Let me know if you can think of a problem with that.

eandre · 2025-01-10T12:17:11Z

proto/encore/runtime/v1/infra.proto

+    message LocalSignOptions {
+      // Base prefix to use for presigned URLs.
+	    string base_url = 1;


Indentation could use some work :)

Agh, vscode tabs/spaces, fixed.

eandre · 2025-01-10T12:33:31Z

runtimes/core/src/objects/gcs/bucket.rs

+    private_key: String,
+}
+
+fn local_sign_config_from_client(client: Arc<LazyGCSClient>) -> Option<LocalSignOptions> {


This can take &LazyGCSClient instead to avoid the clone below (this code path isn't performance sensitive but more idiomatic)

eandre · 2025-01-10T12:35:55Z

runtimes/core/src/objects/gcs/bucket.rs

        Self {
-            client,
+            client: client.clone(),
            encore_name: cfg.encore_name.clone().into(),
            cloud_name: cfg.cloud_name.clone().into(),
            public_base_url: cfg.public_base_url.clone(),
            key_prefix: cfg.key_prefix.clone(),
+            local_sign: local_sign_config_from_client(client.clone()),


Can instead do:

let local_sign = local_sign_config_from_client(&client); Self { client, encore_name: cfg.encore_name.clone().into(), cloud_name: cfg.cloud_name.clone().into(), public_base_url: cfg.public_base_url.clone(), key_prefix: cfg.key_prefix.clone(), local_sign, }

eandre · 2025-01-10T12:43:51Z

runtimes/core/src/objects/gcs/bucket.rs

+fn replace_url_prefix(orig_url: String, base: String) -> String {
+    match Url::parse(&orig_url) {
+        Ok(url) => {
+            let mut out = format!(
+                "{}/{}",
+                base.trim_end_matches('/'),
+                url.path().trim_start_matches("/")
+            );
+            if let Some(query) = url.query() {
+                out.push('?');
+                out.push_str(query);
+            }
+            out
+        }
+        Err(_) => {
+            // If the input URL fails parsing, just don't do the replace
+            orig_url
+        }
+    }
+}


To avoid unnecessary cloning of the input arguments we can do:

Suggested change

fn replace_url_prefix(orig_url: String, base: String) -> String {

match Url::parse(&orig_url) {

Ok(url) => {

let mut out = format!(

"{}/{}",

base.trim_end_matches('/'),

url.path().trim_start_matches("/")

);

if let Some(query) = url.query() {

out.push('?');

out.push_str(query);

}

out

}

Err(_) => {

// If the input URL fails parsing, just don't do the replace

orig_url

}

}

}

fn replace_url_prefix<'a>(orig_url: &'a str, base: &str) -> Cow<'a, str> {

match url::Url::parse(orig_url) {

Ok(url) => {

let mut out = format!(

"{}/{}",

base.trim_end_matches('/'),

url.path().trim_start_matches("/")

);

if let Some(query) = url.query() {

out.push('?');

out.push_str(query);

}

Cow::Owned(out)

}

Err(_) => {

// If the input URL fails parsing, just don't do the replace

Cow::Borrowed(orig_url)

}

}

}

eandre · 2025-01-10T12:58:25Z

runtimes/go/storage/objects/internal/providers/gcs/bucket.go

+	opts := &storage.SignedURLOptions{
+		Scheme:  storage.SigningSchemeV4,
+		Method:  "PUT",
+		Expires: time.Now().Add(time.Duration(data.Ttl)),


data.Ttl is already a time.Duration right? Can skip the extra conversion then

Suggested change

Expires: time.Now().Add(time.Duration(data.Ttl)),

Expires: time.Now().Add(data.Ttl),

eandre · 2025-01-10T12:58:44Z

runtimes/go/storage/objects/internal/providers/gcs/bucket.go

+	return url, nil
+}
+
+func replaceURLPrefix(orig_url string, base string) string {


Suggested change

func replaceURLPrefix(orig_url string, base string) string {

func replaceURLPrefix(origURL string, base string) string {

eandre · 2025-01-10T13:00:11Z

runtimes/go/storage/objects/internal/providers/gcs/bucket.go

+	if err != nil {
+		return orig_url // If the input URL is not valid, just return it as-is
+	}
+	out := strings.TrimRight(base, "/") + "/" + strings.TrimLeft(u.Path, "/")


This adds an extra trailing slash when u.Path is empty, right?

Suggested change

out := strings.TrimRight(base, "/") + "/" + strings.TrimLeft(u.Path, "/")

out := strings.TrimRight(base, "/")

if u.Path != "" {

out += "/" + strings.TrimLeft(u.Path, "/")

}

Correct, but IMO we don't need to handle that case. 1) we currently use path style URLs which means the bucket name is in the path. And 2) we only support object scoped signed URLs atm, which means we have a non-empty object key for all valid requests. Both would need to change and if we change (1) we would anyway need to make some other changes to make local URLs work. But let me know if I'm missing something or if you prefer not to have this potential bug lurking :).

I'd rather fix it now, since it's a trivial fix and the code is more general that way

eandre · 2025-01-10T13:01:41Z

runtimes/go/storage/objects/internal/providers/s3/bucket.go

+	clients := clientSet{
+		client:        client,
+		presignClient: s3.NewPresignClient(client),
+	}
+
+	mgr.clients[prov] = &clients
+	return &clients


Suggested change

clients := clientSet{

client: client,

presignClient: s3.NewPresignClient(client),

}

mgr.clients[prov] = &clients

return &clients

clients := &clientSet{

client: client,

presignClient: s3.NewPresignClient(client),

}

mgr.clients[prov] = clients

return clients

eandre · 2025-01-10T13:02:02Z

runtimes/go/storage/objects/internal/types/types.go

+	Ctx    context.Context
+	Object CloudObject
+
+	Ttl time.Duration


Suggested change

Ttl time.Duration

TTL time.Duration

eandre · 2025-01-13T13:32:14Z

proto/encore/parser/meta/v1/meta.pb.go

+// 	protoc-gen-go v1.35.2
+// 	protoc        v5.29.1


Would rather keep the protobuf versions the same so we don't end up with lots of spurious regenerations between different developers. Can you get those existing versions installed instead?

eandre · 2025-01-13T13:32:47Z

proto/encore/runtime/v1/infra.pb.go

@@ -1,7 +1,7 @@
 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
-// 	protoc-gen-go v1.31.0
-// 	protoc        v4.23.4
+// 	protoc-gen-go v1.35.2


erikcarlsson force-pushed the sign-url branch 3 times, most recently from 3d7696f to 6bf10f5 Compare December 19, 2024 10:31

eandre reviewed Dec 20, 2024

View reviewed changes

erikcarlsson force-pushed the sign-url branch 3 times, most recently from 3b6d7b6 to f165bfd Compare December 20, 2024 18:42

erikcarlsson force-pushed the sign-url branch from f165bfd to 0e507c6 Compare January 2, 2025 09:48

eandre reviewed Jan 10, 2025

View reviewed changes

Add a PUT handler for the local bucket server.

482d69f

erikcarlsson force-pushed the sign-url branch 6 times, most recently from 7e02e6c to 82bc1a0 Compare January 13, 2025 11:55

eandre reviewed Jan 13, 2025

View reviewed changes

erikcarlsson force-pushed the sign-url branch from 82bc1a0 to cabdb99 Compare January 13, 2025 16:04

Add support for signed upload urls.

b3c7614

erikcarlsson force-pushed the sign-url branch from cabdb99 to b3c7614 Compare January 13, 2025 16:11

eandre approved these changes Jan 13, 2025

View reviewed changes

	func WithTtl(ttl uint64) withTtlOption {
	func WithTTL(ttl time.Duration) withTTLOption {

	async getUploadUrl(name: string, options?: UploadUrlOptions): Promise<string> {
	async signedUploadUrl(name: string, options?: SignedUploadUrlOptions): Promise<SignedURL> {

	presign_client *s3.PresignClient
	presignClient *s3.PresignClient

	sign_opts := func(opts *s3.PresignOptions) {
	signOpts := func(opts *s3.PresignOptions) {

	Expires: time.Now().Add(time.Duration(data.Ttl)),
	Expires: time.Now().Add(data.Ttl),

	func replaceURLPrefix(orig_url string, base string) string {
	func replaceURLPrefix(origURL string, base string) string {

Add support for signed upload urls. #1661

Are you sure you want to change the base?

Add support for signed upload urls. #1661

Conversation

erikcarlsson commented Dec 19, 2024

encore-cla bot commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikcarlsson Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikcarlsson Dec 20, 2024 •

edited

Loading