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

add support for wildcards in allowed_hosts, add tests for allowsHost() #73

Closed

Conversation

peterkrejci
Copy link
Contributor

@peterkrejci peterkrejci commented Mar 10, 2017

In our project we need to allow wide range of hosts and wildcard seems to be pretty good for it, so I thought that someone else might appreciate this feature as well.

@flashmob
Copy link
Owner

Thanks for sharing! It's a good idea.

At present, there is a major refactoring work in progress, and going through final testing.

Once that's merged, it will open an interesting possibility of implementing this change as a component. See preliminary documentation for "processors" here https://github.com/flashmob/go-guerrilla/wiki/About-Backends:-introduction,-configuring-and-extending

If implemented as a "processor", you would add wildcard checking for the last item added to the e.RcptTo. Add your implementation inside the if task == backends.TaskValidateRcpt { block.

There's a MailDir processor which has a good example, it validates recipients based on a table that is set via the config. You can use that as a template. https://github.com/flashmob/maildir-processor

The other comment is that in our use case, there are a few hundred hosts... So doing a range server.hosts.table and applying a regex on each might incur some processing during busy times. There could be a few optimizations here: eg. check the table for wildcards when the config has been loaded, and place in a different regex table.

So If you're interested in implementing as a processor, that would be ideal. Only update of the docs is left to do, and the branch will be merged soon. #71

@peterkrejci
Copy link
Contributor Author

Thanks for your response! Yes, I'm interested in it, we will probably upgrade to the newest version as well, so I'll implement it then.

@mrcpvn
Copy link

mrcpvn commented Mar 14, 2017

I am interested in the same feature! I'll wait the newest version, the processors idea seems promising.

@flashmob
Copy link
Owner

flashmob commented Mar 15, 2017 via email

@mrcpvn
Copy link

mrcpvn commented Mar 21, 2017

Hello, I am trying to develop a processor that would validate the recipients in a custom way.
I added a processor that should validate all recipients as valid but the allowed_hosts validation is intercepting my emails. Is there a way to override the allowed_hosts validation?

@flashmob
Copy link
Owner

How about this:

add a feature for to allow disabling the default allowed hosts check, by configuring it with a single item containing a ".". Another way would be to do an empty list, however, an administrator may accidentally leave the list empty, so It's better to have something explicit.

Here is the PR #76

@mrcpvn
Copy link

mrcpvn commented Mar 22, 2017

sounds good to me thank you

@stuartskelton
Copy link

Afternoon,

I have been testing go-guerrilla, with the wildcard filtering as described in #80.

To use wildcard filtering as it is now, a user would have to

  1. open the SMTP server to all by allowed_hosts = ".",
  2. edit the server source code to add this as a backend,
  3. rely on the wildcard filtering to be the main filtering.

I understand the reason for (1), as it defers the filtering logic to that of the processor(s) because the host checking is upstream of the plugins, however, it now means I have would have to push any non-wildcard domains to the wildcard filter, which to me feels a little off.

This also means now, the error emitted to the sending server is now:

5.1.1 User unknown in local recipient table no such user

and not (the probably more correct)

454 4.1.1 Error: Relay access denied:

After playing around with this I think the better move would've been either:
leave the wildcard logic inside of the allowsHost function or,

add a hook inside 'allowsHost' so that the wildcard processor, can be processed in the right place removing the need for a '.' and allow for mix and matched domains. Adding the hook would mean another person could add a regex based domain check or a plugin that could check REDIS for valid domains addresses instead of having them soft-hardcoded in a config ... (add more examples here).

Apologies to the random 2c, I hope it all makes sense.

Ps, sorry for the PR hijack, and love the pluggable backends.

@flashmob
Copy link
Owner

flashmob commented Mar 12, 2018

Hi @stuartskelton sorry for the late reply to this. You make a good point, the . fix was really a band aid, and indeed cumbersome to configure and inaccurate error response. It seems like it would be better to support wildcards in the core, also regexes. If using regexes, prefix with a ~ character.

Todo:

  • Add wildcard and regexes support for host matching directly in the core.

  • if not matched, emit the following error
    454 4.1.1 Error: Relay access denied:

flashmob added a commit that referenced this pull request May 31, 2018
Wildcards will also allow to use subdomains, eliminating the need for PR #81
flashmob added a commit that referenced this pull request May 31, 2018
Wildcards will also allow to use subdomains, eliminating the need for PR #81
@flashmob
Copy link
Owner

Added wildcards in PR #111 which just got merged! Thanks for the code, some code from this PR was used in PR #111 - very helpful.

In the end, decided no need for regexes (unless someone requests them).

@flashmob flashmob closed this May 31, 2018
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.

4 participants