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

feat: add support for pattern file #144

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

Conversation

Ledorub
Copy link
Collaborator

@Ledorub Ledorub commented Aug 15, 2024

This PR adds support for pattern file. The file can be used to provide a custom list of regex patterns or domains (e.g. from GoodbyeDPI) that must be proxified.

  • The file path is specified via a new option -pattern-
  • The file should contain one pattern per
  • Patterns from the file are merged with patterns provided via -pattern option.

P.S. Had to re-create the branch, which automatically closed #127.

@xvzc
Copy link
Owner

xvzc commented Aug 15, 2024

There was a bit of refactoring on util package. parsing arguments should be done in util/args.go

@Ledorub
Copy link
Collaborator Author

Ledorub commented Aug 15, 2024

parsing arguments should be done in util/args.go

Updated the PR. Put file loading into config.go and updated config.Load signature to let errors bubble to main. Should file loader be moved to args.go?

@xvzc xvzc changed the title feature: adds support for pattern file feat: adds support for pattern file Aug 15, 2024
@xvzc
Copy link
Owner

xvzc commented Aug 15, 2024

I don't really think we need to add PatternFile field to the Config struct.
I refactored the config part little bit for you. Please refer to the pseudocode below.

util/args.go

type Args struct {
	Addr           *string
	Port           *int
	DnsAddr        *string
	DnsPort        *int
	EnableDoh      *bool
	Debug          *bool
	NoBanner       *bool
	SystemProxy    *bool
	Timeout        *int
	AllowedPattern *StringArray
        PatternFile    *string // Added here
	WindowSize     *int
	Version        *bool
}

func ParseArgs() *Args {
    // ...
    args.PatternFile = flag.String(...)
    // ...
}

util/file.go

func (c *Config) ReadFile(path *string) (string) {
    var content string
    // read the file content here
    return content
}

util/config.go

func (c *Config) Load(args *Args) {
	c.Addr = args.Addr
	c.Port = args.Port
	c.DnsAddr = args.DnsAddr
	c.DnsPort = args.DnsPort
	c.Debug = args.Debug
	c.EnableDoh = args.EnableDoh
	c.NoBanner = args.NoBanner
	c.SystemProxy = args.SystemProxy
	c.Timeout = args.Timeout
	c.AllowedPatterns = parseAllowedPattern(args.AllowedPattern, patternFile)
	c.WindowSize = args.WindowSize
}

func parseAllowedPattern(patterns *StringArray, patternFile *string) []*regexp.Regexp {
    var allowedPatterns []*regexp.Regexp

    for _, pattern := range *patterns {
        allowedPatterns = append(allowedPatterns, regexp.MustCompile(pattern))
    }

    for _, pattern := range strings.Split(ReadFile(patternFile), "\n") {
        allowedPatterns = append(allowedPatterns, regexp.MustCompile(pattern))
    }

    return allowedPatterns
}

Good luck with your contribution!

@xvzc xvzc changed the title feat: adds support for pattern file feat: add support for pattern file Aug 15, 2024
@xvzc xvzc added the feature label Aug 15, 2024
@Ledorub
Copy link
Collaborator Author

Ledorub commented Aug 18, 2024

I will update this PR in a couple of days.

@Ledorub Ledorub marked this pull request as draft August 20, 2024 20:05
@Ledorub
Copy link
Collaborator Author

Ledorub commented Aug 20, 2024

Updated the implementation. Pattern file is no longer present in config. I had to change signatures a bit, to let errors bubble to main.

@Ledorub Ledorub marked this pull request as ready for review August 20, 2024 22:49
@xvzc
Copy link
Owner

xvzc commented Aug 21, 2024

What about we utilize some kind of file which is like yaml or json? so that we can set the default value of the other arguments too. because of in my country there is some site that blocks my ip when i try to send payload with window-size 1, so there need to be some option that will not do the fragmentation for the given pattern. such as antipattern from #156. If agree with this, then let me do this.

@Ledorub
Copy link
Collaborator Author

Ledorub commented Aug 22, 2024

If agree with this, then let me do this.

I think this is a good idea. I tried to workaround the lack of antipattern with regex lookaround, but Go doesn't support it.

@Ledorub
Copy link
Collaborator Author

Ledorub commented Aug 22, 2024

I put some though into it, and I think that we should abstract file reading/parsing. My initial proposal was to accept a file with a list of patterns/domains separated by newlines. It is the most common and simple format there is, and it doesn't require any preprocessing, which makes me think that it should be supported. But structured format have it's benefits. The best of both worlds, I guess, would be supporting both a simple .txt and structured .yaml files that are parsed into some common data structure.

@xvzc
Copy link
Owner

xvzc commented Aug 22, 2024

Hmm, then i think the simple txt file support should be prioritized. because yaml, json or whatever formats are basically common things to software engineers, however not every users of this application is software engineer.

@Ledorub Ledorub marked this pull request as draft August 22, 2024 08:57
@xvzc
Copy link
Owner

xvzc commented Aug 22, 2024

What about we check if each given value of --pattern argument is a file? by using os.path.exists()?
it maybe slows down the startup time when there is too many patterns given, but we can decrease the number of options so that the usage goes more simple.

Also, i've been thinking about separate the --pattern option into two different options that are --allow and --deny or something else

@Ledorub
Copy link
Collaborator Author

Ledorub commented Aug 22, 2024

What about we check if each given value of --pattern argument is a file? by using os.path.exists()?
it maybe slows down the startup time when there is too many patterns given, but we can decrease the number of options so that the usage goes more simple.

I think that it would be too implicit. I don't see anything bad in adding options as long as their names are unambiguous and clearly reflect intentions, and they do not conflict with each other.

@uqus
Copy link

uqus commented Aug 28, 2024

Hey guys, it's a nice feature, it would be cool to finish it.
I would suggest implement the file with two sections: include and exclude for allowed and disallowed patterns

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

Successfully merging this pull request may close these issues.

3 participants