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

fix: Avoid panic when s3 URL is invalid #501

Merged
merged 2 commits into from
Aug 14, 2024
Merged

Conversation

liamg
Copy link
Contributor

@liamg liamg commented Jul 24, 2024

Gracefully handle when S3 URLs have an unexpected number of path segments.

Currently we expect s3.amazonaws.com/bucket/path, but something like s3.amazonaws.com/bucket will cause a panic, e.g.

panic: runtime error: index out of range [2] with length 2

github.com/hashicorp/go-getter.(*S3Getter).parseUrl(,)
    /go/pkg/mod/github.com/hashicorp/go-getter@v1.7.5/get_s3.go:272
github.com/hashicorp/go-getter.(*S3Getter).Get(, {,},)
    /go/pkg/mod/git...

Gracefully handle when S3 URLs have an unexpected number of path segments.

Currently we expect `s3.amazonaws.com/bucket/path`, but something like `s3.amazonaws.com/bucket` will cause a panic, e.g.

```
panic: runtime error: index out of range [2] with length 2

github.com/hashicorp/go-getter.(*S3Getter).parseUrl(,)
    /go/pkg/mod/github.com/hashicorp/go-getter@v1.7.5/get_s3.go:272
github.com/hashicorp/go-getter.(*S3Getter).Get(, {,},)
    /go/pkg/mod/git...
```
Copy link

hashicorp-cla-app bot commented Jul 24, 2024

CLA assistant check
All committers have signed the CLA.

@liamg liamg changed the title Fix panic when s3 URL is invalid fix: Avoid panic when s3 URL is invalid Jul 25, 2024
@crw
Copy link
Contributor

crw commented Jul 30, 2024

Hi @liamg, out of curiosity, what is the context in which you discovered this issue? Is there an associated issue in go-getter or in one of the product repositories? Thanks!

@liamg
Copy link
Contributor Author

liamg commented Jul 30, 2024

Hey, we discovered this internally when using go-getter to pull down terraform modules by URL. Somebody had omitted the object path from the module URL and 💥 .

It's reproducible with the latest terraform by doing this:

module "uhoh" {
  source = "s3.amazonaws.com/bucket"
}

and then a terraform init:

Initializing the backend...
Initializing modules...
Downloading s3::https://s3.amazonaws.com/bucket for uhoh...

!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

Terraform crashed! This is always indicative of a bug within Terraform.
Please report the crash with Terraform[1] so that we can fix this.

When reporting bugs, please include your terraform version, the stack trace
shown below, and any additional information which may help replicate the issue.

[1]: https://github.com/hashicorp/terraform/issues

!!!!!!!!!!!!!!!!!!!!!!!!!!! TERRAFORM CRASH !!!!!!!!!!!!!!!!!!!!!!!!!!!!

runtime error: index out of range [2] with length 2
goroutine 1 [running]:
runtime/debug.Stack()
	/Users/runner/hostedtoolcache/go/1.21.1/x64/src/runtime/debug/stack.go:24 +0x64
runtime/debug.PrintStack()
	/Users/runner/hostedtoolcache/go/1.21.1/x64/src/runtime/debug/stack.go:16 +0x1c
github.com/hashicorp/terraform/internal/logging.PanicHandler()
	/Users/runner/work/terraform/terraform/internal/logging/panic.go:58 +0x164
panic({0x107e52f40?, 0x1400007a300?})
	/Users/runner/hostedtoolcache/go/1.21.1/x64/src/runtime/panic.go:920 +0x26c
github.com/hashicorp/go-getter.(*S3Getter).parseUrl(0x14000850bac?, 0x140004d6ea0)
	/Users/runner/go/pkg/mod/github.com/hashicorp/go-getter@v1.7.2/get_s3.go:272 +0x808

I'll raise an issue there too.

@crw
Copy link
Contributor

crw commented Jul 30, 2024

Related issue: hashicorp/terraform#35515

@crw crw linked an issue Jul 30, 2024 that may be closed by this pull request
@crw crw added the bug label Jul 30, 2024
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Hi @liamg
Thanks for the PR - this looks like a reasonable patch and something certainly worth fixing!

However, before we do so - can you add some tests that reproduce the panic without the patch and pass after the patch? This can be a new test under https://github.com/hashicorp/go-getter/blob/main/get_s3_test.go

@liamg
Copy link
Contributor Author

liamg commented Aug 1, 2024

@radeksimko Sure thing, I should have done that already 😅

I've added a few test cases now.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@radeksimko radeksimko merged commit 4f07d24 into hashicorp:main Aug 14, 2024
1 check passed
@liamg liamg deleted the patch-1 branch August 14, 2024 13:23
@liamg
Copy link
Contributor Author

liamg commented Aug 14, 2024

Thanks @radeksimko - do you know if we can expect a release any time soon so we can grab this fix?

@radeksimko
Copy link
Member

Took a while due to some confusion around versioning and releasing, but I just released v1.7.6: https://github.com/hashicorp/go-getter/releases/tag/v1.7.6

My understanding is that v1 is used by Terraform, so that should eventually address the bug there. We could cherry-pick the patch to v2 as well though.

renovate bot referenced this pull request in jippi/dottie Aug 17, 2024
…go.mod (#75)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/hashicorp/go-getter](https://github.com/hashicorp/go-getter)
| `v1.7.5` -> `v1.7.6` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fhashicorp%2fgo-getter/v1.7.6?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fhashicorp%2fgo-getter/v1.7.6?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fhashicorp%2fgo-getter/v1.7.5/v1.7.6?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fhashicorp%2fgo-getter/v1.7.5/v1.7.6?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>hashicorp/go-getter (github.com/hashicorp/go-getter)</summary>

###
[`v1.7.6`](https://github.com/hashicorp/go-getter/releases/tag/v1.7.6)

[Compare
Source](https://github.com/hashicorp/go-getter/compare/v1.7.5...v1.7.6)

#### What's Changed

- fix: Avoid panic when s3 URL is invalid by
[@&#8203;liamg](https://github.com/liamg) in
[https://github.com/hashicorp/go-getter/pull/501](https://github.com/hashicorp/go-getter/pull/501)

#### New Contributors

- [@&#8203;liamg](https://github.com/liamg) made their first
contribution in
[https://github.com/hashicorp/go-getter/pull/501](https://github.com/hashicorp/go-getter/pull/501)

**Full Changelog**:
hashicorp/go-getter@v1.7.5...v1.7.6

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "* */2 * * *" (UTC), Automerge - At
any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/jippi/dottie).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when module source is a malformed S3 address
3 participants