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: normalize hostname for auth login #6999

Merged
merged 4 commits into from
Apr 17, 2023

Conversation

tuananhlai
Copy link
Contributor

@tuananhlai tuananhlai commented Feb 9, 2023

Fixes #6979

Normalize hostname before write to config file / login.

vscode ➜ /workspaces/cli (fix/normalize-hostname-login) $ go run cmd/gh/main.go auth login -h GitHub.com -p https
? Authenticate Git with your GitHub credentials? Yes
? How would you like to authenticate GitHub CLI? Login with a web browser

! First copy your one-time code: 4293-8C48
Press Enter to open github.com in your browser... 
✓ Authentication complete.
- gh config set -h github.com git_protocol https
✓ Configured git protocol
✓ Logged in as tuananhlai

vscode ➜ /workspaces/cli (fix/normalize-hostname-login) $ go run cmd/gh/main.go auth status
github.com
  ✓ Logged in to github.com as tuananhlai (/home/vscode/.config/gh/hosts.yml)
  ✓ Git operations for github.com configured to use https protocol.
  ✓ Token: gho_************************************
  ✓ Token scopes: gist, read:org, repo, workflow

vscode ➜ /workspaces/cli (fix/normalize-hostname-login) $ cat ~/.config/gh/hosts.yml 
github.com:
    user: tuananhlai
    oauth_token: <oauth_token>
    git_protocol: https

@tuananhlai tuananhlai requested a review from a team as a code owner February 9, 2023 05:23
@tuananhlai tuananhlai requested review from mislav and removed request for a team February 9, 2023 05:23
@tuananhlai tuananhlai marked this pull request as draft February 9, 2023 05:23
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Feb 9, 2023
@tuananhlai tuananhlai changed the title Normalize hostname for auth login fix: normalize hostname for auth login Feb 9, 2023
@tuananhlai tuananhlai marked this pull request as ready for review February 9, 2023 05:37
@@ -138,6 +138,7 @@ func loginRun(opts *LoginOptions) error {
return err
}
}
hostname = ghinstance.NormalizeHostname(hostname)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, thank you for attempting to fix this, but I would rather see that the original cause of the bug was addressed and discovered.

For example, it should be valid to do gh auth login -h GITHUB.COM and have the whole authentication flow complete and the token getting stored under GITHUB.COM. We should be matching hostnames case-insensitively throughout the codebase without having to normalize hostnames at the time of logging in and storing tokens.

I would appreciate if the bug fix was a change that was applied to the part of the codebase where the error that was originally reported comes from. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should the app handle if the user is already logged in to github.com, then reauthenticate with GitHub.com? Should we save both of them in hosts.yml? 🤔

Copy link
Contributor Author

@tuananhlai tuananhlai Feb 14, 2023

Choose a reason for hiding this comment

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

Furthermore, the cause of the bug in #6979 is because cfg.AuthToken(hostname) method normalized the hostname internally. If we want to store and validate the user-input hostname, we'd have to modify this method.

https://github.com/cli/cli/blob/trunk/pkg/cmd/auth/status/status.go#L94

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point about the cfg.AuthToken method. I have looked up how that works and saw that the fixes would have to be applied to our external library (go-gh). I've proposed some changes there: cli/go-gh#105

If we decide to go that route and the go-gh change gets merged, there will be no need for this PR, as the issue will be solved. However, I thank you for proposing to fix this in the first place. Let's first see what happens with the other PR; I will update this thread accordingly.

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Resurrecting this fix because the underlying fix in the go-gh library proved to be more trouble than it's worth.

@mislav mislav enabled auto-merge (squash) April 17, 2023 13:25
@mislav mislav merged commit 3d8057d into cli:trunk Apr 17, 2023
renovate bot added a commit to scottames/dots that referenced this pull request Apr 29, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [aquaproj/aqua-registry](https://togithub.com/aquaproj/aqua-registry)
| minor | `v3.158.0` -> `v3.159.0` |
| [cli/cli](https://togithub.com/cli/cli) | minor | `v2.27.0` ->
`v2.28.0` |
| [weaveworks/eksctl](https://togithub.com/weaveworks/eksctl) | minor |
`v0.138.0` -> `v0.139.0` |

---

### Release Notes

<details>
<summary>aquaproj/aqua-registry</summary>

###
[`v3.159.0`](https://togithub.com/aquaproj/aqua-registry/releases/tag/v3.159.0)

[Compare
Source](https://togithub.com/aquaproj/aqua-registry/compare/v3.158.1...v3.159.0)


[Issues](https://togithub.com/aquaproj/aqua-registry/issues?q=is%3Aissue+milestone%3Av3.159.0)
| [Pull
Requests](https://togithub.com/aquaproj/aqua-registry/pulls?q=is%3Apr+milestone%3Av3.159.0)
| aquaproj/aqua-registry@v3.158.0...v3.159.0

#### 🎉 New Packages


[#&#8203;11807](https://togithub.com/aquaproj/aqua-registry/issues/11807)
[kubecfg/kubecfg](https://togithub.com/kubecfg/kubecfg): A tool for
managing complex enterprise Kubernetes environments as code

[#&#8203;11808](https://togithub.com/aquaproj/aqua-registry/issues/11808)
[loov/goda](https://togithub.com/loov/goda): Go Dependency Analysis
toolkit

#### Fixes


[#&#8203;11806](https://togithub.com/aquaproj/aqua-registry/issues/11806)
solidiquis/erdtree: Follow up changes of erdtree v2.0.0

https://github.com/solidiquis/erdtree/releases/tag/v2.0.0

> Perhaps the most important change to note is that the compiled binary
has been renamed from et to erd in order to address the following issue
> regarding name collisions with other programs
>
> -
[solidiquis/erdtree#23

###
[`v3.158.1`](https://togithub.com/aquaproj/aqua-registry/releases/tag/v3.158.1)

[Compare
Source](https://togithub.com/aquaproj/aqua-registry/compare/v3.158.0...v3.158.1)


[Issues](https://togithub.com/aquaproj/aqua-registry/issues?q=is%3Aissue+milestone%3Av3.158.1)
| [Pull
Requests](https://togithub.com/aquaproj/aqua-registry/pulls?q=is%3Apr+milestone%3Av3.158.1)
| aquaproj/aqua-registry@v3.158.0...v3.158.1

#### Fixes


[#&#8203;11790](https://togithub.com/aquaproj/aqua-registry/issues/11790)
Follow up changes of cli/cli v2.28.0
[@&#8203;kyontan](https://togithub.com/kyontan)

GitHub's CLI (cli/cli) changed format for macOS to zip (from tar.gz)
since v2.28.0

See https://github.com/cli/cli/releases/tag/v2.28.0 for details.

</details>

<details>
<summary>cli/cli</summary>

### [`v2.28.0`](https://togithub.com/cli/cli/releases/tag/v2.28.0):
GitHub CLI 2.28.0

[Compare Source](https://togithub.com/cli/cli/compare/v2.27.0...v2.28.0)

#### What's New

-   macOS binaries are now signed and notarized
- ⚠️ macOS archives attached to our releases are no longer `.tar.gz`,
but `.zip` instead. This is because `.tar.gz` archives cannot be
notarized.
- The `checksums.txt` file attached to every release now includes the
checksum of the Windows MSI installer too.

- macOS and Windows binaries are now compiled from their respective
platforms and have `cgo` enabled. This might help resolve respecting
system proxy settings and avoid related networking issues.

- `issue edit`: edit multiple issues at the same time by
[@&#8203;heaths](https://togithub.com/heaths) in
[cli/cli#7259

- Add `gh org list` by
[@&#8203;joshkraft](https://togithub.com/joshkraft) in
[cli/cli#7257

- `ssh-key`: add ability to manage signing keys by
[@&#8203;kousikmitra](https://togithub.com/kousikmitra) in
[cli/cli#7270

- `search`: enable owner flag to take multiple values by
[@&#8203;kousikmitra](https://togithub.com/kousikmitra) in
[cli/cli#7305

- `codespace`: add `--web` flag for `list` & `create` commands by
[@&#8203;doaortu](https://togithub.com/doaortu) in
[cli/cli#7288

- Our Debian & RPM packages now ship with shell completion scripts by
[@&#8203;Xerkus](https://togithub.com/Xerkus) in
[cli/cli#7293

- `run list`: add `--event` and `--created` filters by
[@&#8203;cawfeecake](https://togithub.com/cawfeecake) in
[cli/cli#7363
[cli/cli#7352

- `repo`: add `visibility` JSON field by
[@&#8203;yeikel](https://togithub.com/yeikel) in
[cli/cli#7337

#### What's Changed

- Fix typo in `cs stop` command: `Stoppping` -> `Stopping` by
[@&#8203;FalseDev](https://togithub.com/FalseDev) in
[cli/cli#7318
- Update go-gh to v2 by [@&#8203;samcoe](https://togithub.com/samcoe) in
[cli/cli#7299
- `auth login`: normalize host name by
[@&#8203;tuananhlai](https://togithub.com/tuananhlai) in
[cli/cli#6999
- build(deps): bump github.com/cenkalti/backoff/v4 from 4.2.0 to 4.2.1
by [@&#8203;dependabot](https://togithub.com/dependabot) in
[cli/cli#7323
- Clarify how SSH keys are selected for `gh codespace ssh` by
[@&#8203;jkeech](https://togithub.com/jkeech) in
[cli/cli#7325
- Initialize deployment.yml workflow file by
[@&#8203;mislav](https://togithub.com/mislav) in
[cli/cli#7328
- Fix `gh cs ports` requiring `sudo` for privileged port numbers by
[@&#8203;cmbrose](https://togithub.com/cmbrose) in
[cli/cli#7326
- Correct some typos by [@&#8203;goggle](https://togithub.com/goggle) in
[cli/cli#7342
- Diacritics substitution in prompt by
[@&#8203;benjlevesque](https://togithub.com/benjlevesque) in
[cli/cli#7205
- gh: move `CODEOWNERS` inside the `.github/` dir by
[@&#8203;SauravMaheshkar](https://togithub.com/SauravMaheshkar) in
[cli/cli#7366
- Pretty-print gh api output when using --jq by
[@&#8203;mjpieters](https://togithub.com/mjpieters) in
[cli/cli#7236

#### New Contributors

- [@&#8203;joshkraft](https://togithub.com/joshkraft) made their first
contribution in
[cli/cli#7257
- [@&#8203;kousikmitra](https://togithub.com/kousikmitra) made their
first contribution in
[cli/cli#7270
- [@&#8203;doaortu](https://togithub.com/doaortu) made their first
contribution in
[cli/cli#7288
- [@&#8203;FalseDev](https://togithub.com/FalseDev) made their first
contribution in
[cli/cli#7318
- [@&#8203;tuananhlai](https://togithub.com/tuananhlai) made their first
contribution in
[cli/cli#6999
- [@&#8203;goggle](https://togithub.com/goggle) made their first
contribution in
[cli/cli#7342
- [@&#8203;Xerkus](https://togithub.com/Xerkus) made their first
contribution in
[cli/cli#7293
- [@&#8203;cawfeecake](https://togithub.com/cawfeecake) made their first
contribution in
[cli/cli#7363
- [@&#8203;yeikel](https://togithub.com/yeikel) made their first
contribution in
[cli/cli#7337
- [@&#8203;SauravMaheshkar](https://togithub.com/SauravMaheshkar) made
their first contribution in
[cli/cli#7366
- [@&#8203;mjpieters](https://togithub.com/mjpieters) made their first
contribution in
[cli/cli#7236

**Full Changelog**: cli/cli@v2.27.0...v2.28.0

</details>

<details>
<summary>weaveworks/eksctl</summary>

###
[`v0.139.0`](https://togithub.com/weaveworks/eksctl/releases/tag/v0.139.0):
eksctl 0.139.0 (permalink)

[Compare
Source](https://togithub.com/weaveworks/eksctl/compare/0.138.0...0.139.0)

### Release v0.139.0

#### 🚀 Features

- Security Policy for eksctl project
([#&#8203;6541](https://togithub.com/weaveworks/eksctl/issues/6541))

#### 🐛 Bug Fixes

- Fix flux version validation
([#&#8203;6530](https://togithub.com/weaveworks/eksctl/issues/6530))

#### 📝 Documentation

- Fix empty info block on Default Addon Upgrades page
([#&#8203;6524](https://togithub.com/weaveworks/eksctl/issues/6524))
- Update installation instructions
([#&#8203;6376](https://togithub.com/weaveworks/eksctl/issues/6376))
- AWS Private link support for fully private cluster
([#&#8203;6408](https://togithub.com/weaveworks/eksctl/issues/6408))

#### Acknowledgments

Weaveworks would like to sincerely thank:
[@&#8203;thezanke](https://togithub.com/thezanke), and
[@&#8203;yuxiang-zhang](https://togithub.com/yuxiang-zhang)

</details>

---

### Configuration

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

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

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

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/scottames/dots).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS41OC4yIiwidXBkYXRlZEluVmVyIjoiMzUuNjMuMSJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
jtpetty pushed a commit that referenced this pull request May 22, 2023
Co-authored-by: Mislav Marohnić <mislav@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external pull request originating outside of the CLI core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

login working, but "GitHub.com: authentication failed"
3 participants