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: Add IPv6 support for signing HTTP request #2701

Merged
merged 1 commit into from
Jan 17, 2022

Conversation

yuyin002
Copy link

@yuyin002 yuyin002 commented Jan 11, 2022

Closes: #2696

Description

Hostname that's an IPv6 address is expected to be surrounded by square brackets (like in URL), however govmomi uses url.Hostname() which strips those square brackets: https://github.com/vmware/govmomi/blob/master/sts/signer.go#L281

https://pkg.go.dev/net/url#URL.Hostname says:
Hostname returns u.Host, stripping any valid port number if present.
If the result is enclosed in square brackets, as literal IPv6 addresses are, the square brackets are removed from the result.

This may lead to authentication failure for services that use govmomi. This change added the check for IPv6 host and add the brackets.

Type of change

Please mark options that are relevant:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update
  • Build related change

How Has This Been Tested?

Previously, vCenter deployed with pure IPv6 failed to authenticate dcli request; after this change, the dcli request can be successfully authenticated.

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. If applicable, please also list any relevant
details for your test configuration.

  • Test Description 1
  • Test Description 2

Checklist:

  • My code follows the CONTRIBUTION guidelines of
    this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
    NA
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
ok  	github.com/vmware/govmomi/sts	0.291s	coverage: 47.1% of statements

Note: This test is failing before & after this change was made

guest_operations_manager_test.go:48: exit status 1
--- FAIL: TestFileInfo (0.02s)
  • Any dependent changes have been merged
    NA

zach96guan
zach96guan previously approved these changes Jan 11, 2022
Copy link
Member

@zach96guan zach96guan left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!!

sts/signer.go Outdated Show resolved Hide resolved
sts/signer.go Outdated Show resolved Hide resolved
zach96guan
zach96guan previously approved these changes Jan 12, 2022
Copy link
Member

@zach96guan zach96guan left a comment

Choose a reason for hiding this comment

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

lgtm!

@yuyin002 yuyin002 force-pushed the issue-2696 branch 5 times, most recently from 7946f56 to b87158a Compare January 12, 2022 01:32
@yuyin002 yuyin002 changed the base branch from master to release-0.20 January 12, 2022 04:50
@yuyin002 yuyin002 changed the base branch from release-0.20 to master January 12, 2022 04:50
@yuyin002 yuyin002 force-pushed the issue-2696 branch 2 times, most recently from ccf2ad2 to 53b2cad Compare January 12, 2022 05:11
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Thanks @yuyin002 , looks good. Can we just avoid adding the govalidator depenency?

sts/signer.go Outdated Show resolved Hide resolved
@yuyin002 yuyin002 force-pushed the issue-2696 branch 4 times, most recently from 6ed0ca1 to aa73299 Compare January 15, 2022 20:05
Add in a new function isIPv6() for IPv6 host check.
Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

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

Thanks @yuyin002 !

@dougm dougm merged commit 166fc01 into vmware:master Jan 17, 2022
dougm added a commit to dougm/govmomi that referenced this pull request Feb 4, 2022
PR vmware#2701 (ebeaa71) added IPv6 support for signing HTTP request,
but was treating all IPs as v6.
dougm added a commit to dougm/govmomi that referenced this pull request Feb 7, 2022
PR vmware#2701 (ebeaa71) added IPv6 support for signing HTTP request,
but was treating all IPs as v6.
dougm added a commit that referenced this pull request Feb 10, 2022
PR #2701 (ebeaa71) added IPv6 support for signing HTTP request,
but was treating all IPs as v6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Assuming IPv4 Address When Signing API Request
4 participants