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

Use a mux #115

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use a mux #115

wants to merge 1 commit into from

Conversation

42wim
Copy link

@42wim 42wim commented May 16, 2020

Use a new mux instead of the default one so that the Auth method can be used in long-running programs.
This fixes the panic: http: multiple registrations for /oidc/callback when running Auth again.

@kalafut
Copy link
Contributor

kalafut commented May 16, 2020

Hi. This looks reasonable, though can you elaborate on the use case (which sounds interesting)?

@42wim
Copy link
Author

42wim commented May 16, 2020

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

@42wim
Copy link
Author

42wim commented May 17, 2020

After testing, some more changes were needed:

  • Disable keepalives so that the Serve function can exit ASAP
  • We need an extra buffer in doneCh because when everythings works like expected we got the secret on the doneCh in the select. But when the Serve functions exits it'll also send on the doneCh that now doesn't has a receiver anymore which results in a goroutine leak.

select {
case s := <-doneCh:
return s.secret, s.err

go func() {
err := http.Serve(listener, nil)
if err != nil && err != http.ErrServerClosed {
doneCh <- loginResp{nil, err}
}
}()

@42wim
Copy link
Author

42wim commented Jun 25, 2020

Can I do anything to get this merged? Any issues with this PR?

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@42wim
Copy link
Author

42wim commented Aug 19, 2022

Any maintainer that can take a look?

Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

Hi @42wim. I'm going to have a look and test this out. I understand that it's helpful for how you're planning to use this. I'd like to understand if there are any undesired changes for most using the regular CLI workflow. Thanks!

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
Member

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

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 :)

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants