-
Notifications
You must be signed in to change notification settings - Fork 824
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
Add GCS signed URL support #5300
Conversation
@tustvold Hi, I have completed the first version of GCP signed URL generation. Could you please take some time to review it? and then i can update the code gradually. next, i will add test for the sign blob and add code for using HAMC key to generating signed URL. |
Sorry for the delay in reviewing this, I wonder if instead of modifying
This could then have a separate |
I completely agree with you. However, a question arises: if I add a new credential type, will all implementations for TokenProvider need to be rewritten for GcpSigningCredential? This is because it involves additional operations to retrieve the client's email address. I am wondering if this would be acceptable. |
I would think you could use composition to avoid code duplication, with the GcpSigningCredential implementation calling through to the GcpCredential implementation |
@l1nxy how is this going? Let me know if there is anything I can do to help -- would love to use this! |
Apologies for the delayed update. I have been busy and unable to dedicate time to this. If you would like to proceed, feel free to do so, and I will close the pr. |
@s-i-l-k-e Hi, I have time now and will continue working on this. |
@tustvold Hi, sorry for the delayed update, i have a question and hope you can help me. my question is which part modification will cause a breaking change, can i add GcpSigningCredential provider to GoogleCloudStorageBuilder and GoogleStorageConfig? |
So long as you don't change an existing public API it won't be a breaking change. So you could add additional fields or new methods, but not change existing ones |
@tustvold Hi, I made changes to the sign URL implementation by adding a GcpSigningCredential Provider. There are some changes that I'm unsure about. The Thank you for your patience and help. |
@tustvold Hi, I tested it using my GCP account. The ServiceAccount approach yielded correct results, but testing the email from the meta server was not done. Perhaps adding more unit tests or addressing some code issues. maybe i can add sign by HMAC key. |
…into add-gcp-sign-url-support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really nice, I took the liberty of pushing some relatively minor API tweaks. I'll test this out tomorrow, and assuming that all works get it merged
|
||
/// A private RSA key for a service account | ||
#[derive(Debug)] | ||
pub struct ServiceAccountKey(RsaKeyPair); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prevents the ring types from leaking into the public API, whilst still avoiding needing to parse the RSA key for every sign request
@@ -295,21 +376,6 @@ fn seconds_since_epoch() -> u64 { | |||
.as_secs() | |||
} | |||
|
|||
fn decode_first_rsa_key(private_key_pem: String) -> Result<RsaKeyPair> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this into ServiceAccountKey avoids doing this multiple times
I've tested this and it appears to be working correctly 🎉 Thank you for this, epic work |
Which issue does this PR close?
Closes #5233.
Rationale for this change
Adding support for GCS Signed URL generation.
What changes are included in this PR?
Are there any user-facing changes?