Skip to content

Use a mux #115

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.

Sign up for GitHub

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

Open
42wim wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −3
Open

Use a mux #115

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,12 @@ func (h *CLIHandler) Auth(c *api.Client, m map[string]string) (*api.Secret, erro
}

// Set up callback handler
doneCh := make(chan loginResp)
http.HandleFunc("/oidc/callback", callbackHandler(c, mount, clientNonce, doneCh))
doneCh := make(chan loginResp, 2)
mux := http.NewServeMux()
mux.HandleFunc("/oidc/callback", callbackHandler(c, mount, clientNonce, doneCh))
srv := &http.Server{Handler: mux}
srv.SetKeepAlivesEnabled(false)
defer srv.Close()

listener, err := net.Listen("tcp", listenAddress+":"+port)
Copy link
Contributor

@austingebauer austingebauer Jun 21, 2023

Choose a reason for hiding this comment

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

Trying to understand your use case a bit more -- Are you doing something to avoid port collision? I have to imagine so if you were hitting the "multiple registrations" issue.

Copy link
Author

@42wim 42wim Jun 24, 2023

Choose a reason for hiding this comment

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

This is used in a long running process, like mentioned above:

"I'm writing a windows ssh agent that automatically uses vault to sign ssh public keys and then loads those ssh certificates. The necessary vault token is gotten by the Auth function.

Users can then chose to renew those certificates in the interface, this again fires the same oidc login flow to get a new token to sign the new generated ssh pub/priv keys"

I've made a POC about the issue, see https://gist.github.com/42wim/e97d4dbf9d61ccfe436f535660d9c069
Running this with the current code, will give you panic: http: multiple registrations for /oidc/callback

Because it cannot re-register the same pattern in the default mux
https://github.com/golang/go/blob/a031f4ef83edc132d5f49382bfef491161de2476/src/net/http/server.go#L2529-L2531

This is not an issue in the CLI where you only run this once and it exits, but when running multiple times in a daemon/client app it is :)

Copy link

@danielskowronski danielskowronski Jan 19, 2024

Choose a reason for hiding this comment

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

This behaviour 100% matches what's seen in the official Terraform provider for Vault - hashicorp/terraform-provider-vault#2131

I think maintainers should definitely look at it, since it affects their own product :)

if err != nil {
Expand All @@ -146,7 +150,7 @@ func (h *CLIHandler) Auth(c *api.Client, m map[string]string) (*api.Secret, erro

// Start local server
go func() {
err := http.Serve(listener, nil)
err := srv.Serve(listener)
if err != nil && err != http.ErrServerClosed {
doneCh <- loginResp{nil, err}
}
Expand Down