hashicorp / terraform-provider-google

Terraform Provider for Google Cloud Platform
https://registry.terraform.io/providers/hashicorp/google/latest/docs
Mozilla Public License 2.0
2.33k stars 1.74k forks source link

[docs] When `b64_url` of `random_id` is used as the Key for issuing signed URLs by CloudCDN, an error may occur in the gcloud command. #13584

Closed BIwashi closed 1 month ago

BIwashi commented 1 year ago

Background

Affected Resource(s)

When creating a signed URL for CloudCDN, I used random_id b64_url. google_compute_backend_bucket_signed_url_key | Resources | hashicorp/google | Terraform Registry

resource "random_id" "url_signature" {
  byte_length = 16
}

resource "google_compute_backend_bucket_signed_url_key" "backend_key" {
  name           = "test-key"
  key_value      = random_id.url_signature.b64_url
  backend_bucket = google_compute_backend_bucket.test_backend.name
}

I have confirmed that this works. However, when I created them from the Google Cloud console or gcloud command, the keys created were URL safe, base64 encoded, and padded with =.

Furthermore, the official Google Cloud sample code assumes padded keys, so I could not use the keys created with b64_url without modification. In addition, when I tried to create a signed URL using the gcloud compute sign-url command, an error occurred because it used a key without padding.

failed gcloud command

> gcloud compute sign-url \
  "https://example.com/test.png" \
  --key-name test-key \
  --key-file key-file \
  --expires-in 5m \
  --validate
ERROR: gcloud crashed (Error): Incorrect padding

If you would like to report this issue, please run the following command:
  gcloud feedback

To check gcloud for common problems, please run the following command:
  gcloud info --run-diagnostics

fixed google cloud sample code for golang Use signed URLs  |  Cloud CDN  |  Google Cloud

// readKeyFile reads the base64url-encoded key file and decodes it.
func readKeyFile(path string) ([]byte, error) {
        b, err := ioutil.ReadFile(path)
        if err != nil {
                return nil, fmt.Errorf("failed to read key file: %+v", err)
        }
-       d := make([]byte, base64.URLEncoding.DecodedLen(len(b)))
-       n, err := base64.URLEncoding.Decode(d, b)
+       d := make([]byte, base64.RawURLEncoding.DecodedLen(len(b)))
+       n, err := base64.RawURLEncoding.Decode(d, b)
        if err != nil {
                return nil, fmt.Errorf("failed to base64url decode: %+v", err)
        }
        return d[:n], nil
}

Therefore, for Cloud CDN key values, base64 url safe and padding with = is desirable, but currently random_id does not provide such output. One of the outputs of random_id, b64_std, is not url safe, but it is base64 encoded with = padding. So I modified the code to take advantage of this and use the replace function to convert it to url safe.

key_value       = replace(replace(random_id.url_signature.b64_std,"+", "-") ,"/", "_")

I have confirmed by looking at the implementation in the random_id repository (hashicorp/terraform-provider-random ) that b64_url and b64_std implemented in Go as follows

https://github.com/hashicorp/terraform-provider-random/blob/7b934142db2bb3569fa324df4409bb6c6dc69ec3/internal/provider/resource_id.go#L143-L161

b64Std := base64.StdEncoding.EncodeToString(bytes)
id := base64.RawURLEncoding.EncodeToString(bytes)
...
B64URL:     types.StringValue(prefix + id),

I think that base64.URLEncoding.EncodeToString(bytes) is suitable in this case, not base64.RawURLEncoding.EncodeToString(bytes). So I think that we need to add the output to random_id. (ActuallyI have issued a PR regarding this.)

What kind of contribution is this issue about?

better sample for the Google Cloud Platform

resource "google_compute_backend_bucket_signed_url_key" "backend_key_b64_std_url_safe" {
  name           = "test-key-b64-std-url-safe-key"
  key_value      = replace(replace(random_id.url_signature.b64_std, "+", "-"), "/", "_")
  backend_bucket = google_compute_backend_bucket.test_backend.name
}

Related PR(s), if any:

old issue: https://github.com/hashicorp/terraform-provider-google/issues/7202

Details

> terraform -version
Terraform v1.3.7
on darwin_arm64
+ provider registry.terraform.io/hashicorp/google v4.50.0
+ provider registry.terraform.io/hashicorp/random v3.4.3
terraform apply

https://gist.github.com/BIwashi/8a1c8a5f3c1be7f98566df24a9a32c47

gcloud command

success (b64_std + url safe)

> gcloud compute sign-url \
  "https://storage.cloud.google.com/test-storage-bucket-8c0dadbc-cea3-4819-9ded-3cf3990130ba/test.jpeg" \
  --key-name "test-key-b64-std-url-safe-key" \
  --key-file key-file-b64-std-url-safe \
  --expires-in 5m \
  --validate
signedUrl: https://storage.cloud.google.com/test-storage-bucket-8c0dadbc-cea3-4819-9ded-3cf3990130ba/test.jpeg?Expires=1674833243&KeyName=test-key-b64-std-url-safe-key&Signature=FBVJRNWPzyaQ9BzVlf8hkkwCqI0=
validationResponseCode: 200

fail (b64 url)

> gcloud compute sign-url \
  "https://storage.cloud.google.com/test-storage-bucket-8c0dadbc-cea3-4819-9ded-3cf3990130ba/test.jpeg" \
  --key-name "test-key-b64-url" \
  --key-file key-file-b64-url \
  --expires-in 5m \
  --validate
ERROR: gcloud crashed (Error): Incorrect padding

If you would like to report this issue, please run the following command:
  gcloud feedback

To check gcloud for common problems, please run the following command:
  gcloud info --run-diagnostics

success (b64std) <-- I dont'n know why.... [The Google Cloud Platform official documentation instructs to use base64 url.](https://cloud.google.com/cdn/docs/using-signed-urls#:~:text=The%20key%20file%20must%20be%20created%20by%20generating%20strongly%20random%20128%20bits%2C%20encoding%20them%20with%20base64%2C%20and%20then%20replacing%20the%20character%20%2B%20with%20%2D%20and%20replacing%20the%20character%20/%20with%20.%20For%20more%20information%2C%20see%20RFC%204648.)

> gcloud compute sign-url \
  "https://storage.cloud.google.com/test-storage-bucket-8c0dadbc-cea3-4819-9ded-3cf3990130ba/test.jpeg" \
  --key-name "test-key-b64-std" \
  --key-file key-file-b64-std \
  --expires-in 5m \
  --validate
signedUrl: https://storage.cloud.google.com/test-storage-bucket-8c0dadbc-cea3-4819-9ded-3cf3990130ba/test.jpeg?Expires=1674833437&KeyName=test-key-b64-std&Signature=rSU1Lln0Bi58Q8prkET-nSv_5Sc=
validationResponseCode: 200

b/318665331

roaks3 commented 1 year ago

This is how I'm understanding the current state:

I think the documentation change being suggested is an improvement, and it does match what our gcloud docs suggest, but it's also not ideal to be asking users to do this character replacement logic just to use the resource properly.

@BIwashi I see you've already created a PR to have the random_id resource include an option to use the explicit padding. If we're assuming that the padding is required, it might also be worth adding validation to the Terraform resource to match gcloud.

BIwashi commented 1 year ago

@roaks3 Thank you for your follow-up!

If we're assuming that the padding is required, it might also be worth adding validation to the Terraform resource to match gcloud.

I agree with your suggestion. I'll try this.

glasser commented 1 year ago

I suppose since the length is always 16, one could also just always add the same ==? :)

glasser commented 1 year ago

FWIW it seems like this is arguably a bug in the gcloud command, if Cloud CDN itself is happy with unpadded keys?

roaks3 commented 10 months ago

Forwarding to the service team to weigh in.

I think we've determined the gcloud surface does not accept a Base64 URL without = padding, while the Terraform surface does. Would we want to alter gcloud to accept either form? IMO this seems like the most reasonable option.

If we keep gcloud as-is, we would hypothetically want to add the same restriction to the Terraform resource to be consistent, but that would be a breaking change (so it could not be changed until the next major provider release). The change to the example in https://github.com/GoogleCloudPlatform/magic-modules/pull/7166 (or perhaps always adding ==) would probably be our best short-term option (https://github.com/hashicorp/terraform-provider-random/pull/352 would be better, but it seems unlikely to be merged soon).

pawelJas commented 2 months ago

@roaks3 Change in gcloud has been submitted. It will be published with next release on Tuesday 3rd Sep 2024. This issue can be resolved now.

roaks3 commented 2 months ago

Thanks @pawelJas ! I'm going to keep this issue open until that time just so we can confirm the full flow works as intended.

roaks3 commented 1 month ago

Confirmed that this is now resolved after updating gcloud.

github-actions[bot] commented 1 week ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.