Skip to content
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

Attempt to add 'standard' base64 bytes support #464

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

andrewbaxter
Copy link

Taking over #446

Byron#442 flags an issue where
some APIs respond with non-valid base64 bytes values for the "URL safe"
flavor of configuration.

This adds support for a "standard" wrapper adjacent to the URL safe one
with the intention of finding a way to flag which structures should use
which configuration.
@andrewbaxter
Copy link
Author

I did some testing (storage, kms) and no issues! So FWIW I think this is ready to go.

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! Looks good to me - and since this PR contains exactly the same commit as is in #446 one might wonder why that PR couldn't already have arrived at that conclusion.

Oh well 😅.

@Byron Byron merged commit 23aecc3 into Byron:main Jan 5, 2024
1 of 4 checks passed
@dlen
Copy link

dlen commented Mar 4, 2024

Hello guys I'm having similar issues as in #445 when encrypting using KMS. I see @andrewbaxter mentioning his tests were successful using KMS. Could you confirm?

A very simple code that triggers an error for me is this one:

use google_cloudkms1::{api::EncryptRequest, CloudKMS};
use yup_oauth2::{hyper, hyper_rustls, read_service_account_key};

#[tokio::main]
async fn main() {
    let auth = yup_oauth2::ServiceAccountAuthenticator::builder(
        read_service_account_key("/tmp/service_account.json")
            .await
            .unwrap(),
    )
    .build()
    .await
    .unwrap();
    let client = hyper::Client::builder().build(
        hyper_rustls::HttpsConnectorBuilder::new()
            .with_native_roots()
            .https_or_http()
            .enable_http1()
            .build(),
    );

    let req = EncryptRequest::default();
    let hub = CloudKMS::new(client, auth);

    hub
            .projects()
            .locations_key_rings_crypto_keys_encrypt(req, "CRYPTO_KEY_URL")
            .doit()
            .await
            .unwrap();
}

Sorry to comment on a closed PR but I have been battling on this quite a bit and I'm clueless. Thanks for any tip :)

@andrewbaxter
Copy link
Author

Correct, it did work for me. FWIW I was using the asymmetric signature functions: https://github.com/andrewbaxter/certipasta/blob/master/software/certifier/src/bin/certifier-rotate.rs and accessing signature which uses the standard b64 encoding.

The documentation for ciphertext in encrypt and signature in asymmetricSign are nearly identical, and they're part of the same API, so it seems unlikely to me that they'd use two different b64 encodings...

Actually just found this example which explicitly indicates that ciphertext is standard base64: googleapis/google-cloud-go#5966

Do you have the error message and line number? And can you confirm which library version you're using (is it using the urlsafe_base64 annotation or the standard_base64 annotation)?

@dlen
Copy link

dlen commented Mar 4, 2024

Thanks for the fast response!

About the versioning I'm using the latest one:

name = "google-cloudkms1"
version = "5.0.3+20230106"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "9452e6b25b5aff4268dfd2806b0caa5ea4abc84534895eb74220a9e59c1ddf1a"

name = "google-apis-common"
version = "6.0.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "34c72c9baded4d06742eaaa5def6158f9e28d20a679ad1d5f5deb2bae8358052"

From cloudkms1 src/api.rs:

#[serde_with::serde_as(crate = "::client::serde_with")]
#[derive(Default, Clone, Debug, Serialize, Deserialize)]
pub struct AsymmetricDecryptResponse {
    /// The decrypted data originally encrypted with the matching public key.
    
    #[serde_as(as = "Option<::client::serde::urlsafe_base64::Wrapper>")]
    pub plaintext: Option<Vec<u8>>,

The error I get with the code from my previous comment:

thread 'main' panicked at src/main.rs:30:14:
called `Result::unwrap()` on an `Err` value: JsonDecodeError("{\n  \"name\": \"REDACTED\",\n  \"ciphertext\": \"REDACTED\",\n  \"ciphertextCrc32c\": \"2122147469\",\n  \"protectionLevel\": \"SOFTWARE\"\n}\n", Error("Invalid byte 43, offset 32.", line: 3, column: 118))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@andrewbaxter
Copy link
Author

andrewbaxter commented Mar 4, 2024

Oh thanks! That helps a lot.

And actually... it looks like the last release of cloudkms was Aug 23 last year, so it seems like maybe this was never released? I just noticed I'm still using a git override locally.

@andrewbaxter
Copy link
Author

I see @Byron here but pinging in case he already dropped, in case maybe he can have a look.

@Byron
Copy link
Owner

Byron commented Mar 4, 2024

Sorry for the hassle, a new release is long overdue and I will tackle it soon(ish).

@andrewbaxter
Copy link
Author

Ah no, I'm just glad it's not new base64 issues 😄

@Byron
Copy link
Owner

Byron commented Mar 4, 2024

Oh, me too, even though I had a feeling that it's a problem with the version of the crate used.

@dlen
Copy link

dlen commented Mar 4, 2024

Thanks for the clarification guys!

I guess my options to workaround this would be to fork and rebuild myself, right?

@dlen
Copy link

dlen commented Mar 5, 2024

Small update just forked the repo and made a rebuild and indeed KMS is working. Thanks a lot for the fast responses and for the project!

@Byron
Copy link
Owner

Byron commented Mar 5, 2024

Thanks for the kind words! I would wish I didn't have to maintain these crates though and Google would just provide their own bindings. That can only be a matter of time, right… right ?! 😁

@Byron
Copy link
Owner

Byron commented Mar 5, 2024

Alright, in this PR I have fixed CI once again and managed to publish all the most recent APIs and CLIs. Please try them from crates.io, I hope they work as expected.

@dlen
Copy link

dlen commented Mar 6, 2024

I tried them. It's all working fine! Thanks a lot!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants