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

Basic auth fixed #428

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

Basic auth fixed #428

wants to merge 1 commit into from

Conversation

clburlison
Copy link

Issue: When using AlwaysMitm ext/auth/basic.go would attempt to authenticate requests twice resulting in the 407 unauthorized message.

This fixes two outstanding issues with the basic auth behavior of this library.

  1. BasicConnect handler was preventing other handlers from running (AlwaysMitm)
  2. Basic was trying to authenticate connection that was already authenticated by BasicConnect

Resolves: #309, #416

Below is a simple test case to validate the behavior before/after the patch:

package main

import (
	"flag"
	"log"
	"net"
	"net/http"
	"regexp"

	"github.com/elazarl/goproxy"
	"github.com/elazarl/goproxy/ext/auth"
)

func handleRequest(req *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) {
	ip, _, err := net.SplitHostPort(req.RemoteAddr)
	if err != nil {
		log.Print(err)
	}

	log.Printf("[%d] %s --> %s %s", ctx.Session, ip, req.Method, req.URL)

	return req, nil
}

func main() {
	verbose := flag.Bool("v", false, "should every proxy request be logged to stdout")
	addr := flag.String("addr", ":8080", "proxy listen address")
	flag.Parse()
	username, password := "foo", "bar"
	hostmatch := "google.com|neverssl.com|apache.org"
	proxy := goproxy.NewProxyHttpServer()
	auth.ProxyBasic(proxy, "Auth", func(user, passwd string) bool {
		return user == username && password == passwd
	})
	proxy.OnRequest(goproxy.ReqHostMatches(regexp.MustCompile(hostmatch))).
		HandleConnect(goproxy.AlwaysMitm)
	proxy.OnRequest().DoFunc(handleRequest)
	proxy.Verbose = *verbose
	log.Fatal(http.ListenAndServe(*addr, proxy))
}

1. BasicConnect handler was preventing other
handlers from running
2. Basic was trying to authenticate connection
that was already authenticated by BasicConnect
@batuzyn
Copy link

batuzyn commented Sep 13, 2021

@elazarl Could you merge this PR?

@elazarl
Copy link
Owner

elazarl commented Sep 13, 2021

I'm very careful about adding yet another field to the context. Can we do it in some other way?

@batuzyn
Copy link

batuzyn commented Sep 13, 2021

Actually I did't think another way :) But this PR working correctly for me.

@batuzyn
Copy link

batuzyn commented Sep 15, 2021

Sorry, I had a problem after i implemented it.
'ctx.Authenticated' not returns 'true' for http connect methods.

I am getting 407 (proxy auth required) for this code. Is there something I did wrong?

`addr := flag.String("addr", ":8080", "proxy listen address")
hostmatch := flag.String("hostmatch", "^.*$", "hosts to trace (regexp pattern)")
verbose := flag.Bool("v", true, "verbose output")
flag.Parse()

log.SetFlags(log.Lmicroseconds)

proxy := goproxy.NewProxyHttpServer()

auth.ProxyBasic(proxy, "Auth", func(user, passwd string) bool {
	return true
})

proxy.OnRequest(goproxy.ReqHostMatches(regexp.MustCompile(*hostmatch))).
	HandleConnect(goproxy.AlwaysMitm)

proxy.OnRequest().DoFunc(handleRequest)

proxy.Verbose = *verbose
log.Fatal(http.ListenAndServe(*addr, proxy))`

@thalesfsp
Copy link

Any update on this?

@elazarl
Copy link
Owner

elazarl commented Oct 27, 2021

@thalesfsp as I said, I'm reluctant to add another field to the Ctx without a really good reason.

I reviewed the code and asked isn't using the User pointer possible, but didn't receive response yet.

@thalesfsp
Copy link

@elazarl Given the importance of the MR, could you do the change you are suggesting? Best regards!

@faf-xff
Copy link

faf-xff commented Nov 8, 2022

这个啥时候能修复呢

@thalesfsp
Copy link

thalesfsp commented Jan 5, 2023

@guanyufen123 just fork and merge the change. This is what happens when the community needs and demands things but authors are reluctant to listen.

@elazarl
Copy link
Owner

elazarl commented Jan 6, 2023

@thalesfsp can you please explain me again, why using the User pointer is not a good solution in this case?

I'm very open to listen, but was not able to spurr discussion.

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.

Using AlwaysMitm with Basic auth error EOF
5 participants