Details

    • Type: Wish
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.3.0
    • Component/s: SMTPServer
    • Labels:
      None

      Description

      IT would be nice to let /james-dev/src/java/org/apache/james/smtpserver/HeloCmdHandler.java to accept an init parameter for specify that the helo value must be an resolvable domainname. In RFC HELO names should always a valid domainname.

      1. ehlo.patch
        4 kB
        Norman Maurer
      2. EhloCmdHandler-ignoreRelay.patch
        2 kB
        Norman Maurer
      3. EhloCmdHandlerLog.patch
        0.8 kB
        Norman Maurer
      4. ehlo-junit.patch
        5 kB
        Norman Maurer
      5. helo.patch
        3 kB
        Norman Maurer
      6. HeloCmdHandler-ignoreRelay.patch
        3 kB
        Norman Maurer
      7. HeloCmdHandlerLog.patch
        0.8 kB
        Norman Maurer
      8. helo-v2.patch
        4 kB
        Norman Maurer
      9. helo-v2-junit.patch
        4 kB
        Norman Maurer
      10. james-config.xml-HELO-EHLO-ignoreRelay.patch
        2 kB
        Norman Maurer
      11. SMTPServerTest-HELO-EHLO-ignore-relayClient.patch
        4 kB
        Norman Maurer
      12. SMTPTestConfiguration-HELO-EHL-ignore-relayClient.patch
        2 kB
        Norman Maurer

        Activity

        Hide
        norman Norman Maurer added a comment -

        Here is a patch for the current trunk to resolv the wish of this report
        JAMES-451...

        It add support to HeloCmdHandler to only accept helo if its a resolvable
        domainname.

        It can be configured like:

        <handler command="HELO"
        class="org.apache.james.smtpserver.HeloCheckCmdHandler">
        <checkValidHelo> true </checkValidHelo>
        </handler>

        if no <checkValidHelo> parameter is passed it will act as it is set to
        false.. Like it work now.

        Would be great if someone can have a look.

        bye

        Show
        norman Norman Maurer added a comment - Here is a patch for the current trunk to resolv the wish of this report JAMES-451 ... It add support to HeloCmdHandler to only accept helo if its a resolvable domainname. It can be configured like: <handler command="HELO" class="org.apache.james.smtpserver.HeloCheckCmdHandler"> <checkValidHelo> true </checkValidHelo> </handler> if no <checkValidHelo> parameter is passed it will act as it is set to false.. Like it work now. Would be great if someone can have a look. bye
        Hide
        norman Norman Maurer added a comment -

        add the patch

        Show
        norman Norman Maurer added a comment - add the patch
        Hide
        norman Norman Maurer added a comment -

        any update on this feature ?

        Show
        norman Norman Maurer added a comment - any update on this feature ?
        Hide
        bago Stefano Bagnara added a comment -

        From a quick review I see the code has problems.
        E.g: Configuration.getChild NEVER returns null (read the javadocs), so the check to see if the child exists or not should be made differently.
        I have no time to fix it and check the correctness of the code now.

        As you can see this is assigned, don't ask for updates to issues: we have hundreds of issues, if everyone asked for updates I would pass my time reading comments .

        Don't take me wrong but it's not as easy as to apply and commit: I write tests for new features, we should add documentation and comments in the config.xml to let the user know he can use 2 different helo handlers. If we add this code as is, it will serve no one but you and me and it will be one more class in our repository to be mantained.

        Please, if you have time, check it works with no "checkValidHelo" child, add unittests to the SMTPServerTest to test both behaviours and add comment in the default config.xml. We can keep the HelpCmdHandler name and simply add the feature to that class (disabled by default).

        Show
        bago Stefano Bagnara added a comment - From a quick review I see the code has problems. E.g: Configuration.getChild NEVER returns null (read the javadocs), so the check to see if the child exists or not should be made differently. I have no time to fix it and check the correctness of the code now. As you can see this is assigned, don't ask for updates to issues: we have hundreds of issues, if everyone asked for updates I would pass my time reading comments . Don't take me wrong but it's not as easy as to apply and commit: I write tests for new features, we should add documentation and comments in the config.xml to let the user know he can use 2 different helo handlers. If we add this code as is, it will serve no one but you and me and it will be one more class in our repository to be mantained. Please, if you have time, check it works with no "checkValidHelo" child, add unittests to the SMTPServerTest to test both behaviours and add comment in the default config.xml. We can keep the HelpCmdHandler name and simply add the feature to that class (disabled by default).
        Hide
        norman Norman Maurer added a comment -

        Just start reading about junit. I will fix the problem and add junit test after i understand how it works..

        Show
        norman Norman Maurer added a comment - Just start reading about junit. I will fix the problem and add junit test after i understand how it works..
        Hide
        norman Norman Maurer added a comment -

        -fix the bug that stefano reported
        -update james-config.xml

        TODO: Add junit test.

        Show
        norman Norman Maurer added a comment - -fix the bug that stefano reported -update james-config.xml TODO: Add junit test.
        Hide
        norman Norman Maurer added a comment -

        Add junit tests. So it should be complete

        Show
        norman Norman Maurer added a comment - Add junit tests. So it should be complete
        Hide
        bago Stefano Bagnara added a comment -

        I applied the patch but I consider this issue not resolved because the same behaviour should be applied to EHLO too.
        Most mail agents use EHLO instead of HELO so it makes no sense to check host validity for HELO and not for EHLO.

        EhloCmdHandler and HeloCmdHandler should be refactored to reuse the common code (configuration, host validity check).

        Show
        bago Stefano Bagnara added a comment - I applied the patch but I consider this issue not resolved because the same behaviour should be applied to EHLO too. Most mail agents use EHLO instead of HELO so it makes no sense to check host validity for HELO and not for EHLO. EhloCmdHandler and HeloCmdHandler should be refactored to reuse the common code (configuration, host validity check).
        Hide
        norman Norman Maurer added a comment -

        I will port the code to ehlo and add a patch here..

        Show
        norman Norman Maurer added a comment - I will port the code to ehlo and add a patch here..
        Hide
        norman Norman Maurer added a comment -

        Add patch for EhloCmdHandler to support the same checks as HeloCmdHandler

        Show
        norman Norman Maurer added a comment - Add patch for EhloCmdHandler to support the same checks as HeloCmdHandler
        Hide
        norman Norman Maurer added a comment -

        Add junit tests for new ehlo feature. So this should now complete

        Show
        norman Norman Maurer added a comment - Add junit tests for new ehlo feature. So this should now complete
        Hide
        bago Stefano Bagnara added a comment -

        I committed the latest patches but now a test fail.
        I had to apply your patches manually because the format was not correctly handled by Eclipse: can you check I committed all the code and why the test doesn't pass?

        testEhloResolv(), line 332:

        junit.framework.AssertionFailedError: expected error: ehlo could not resolved expected:<501> but was:<250>
        at junit.framework.Assert.fail(Assert.java:47)
        at junit.framework.Assert.failNotEquals(Assert.java:282)
        at junit.framework.Assert.assertEquals(Assert.java:64)
        at junit.framework.Assert.assertEquals(Assert.java:201)
        at org.apache.james.smtpserver.SMTPServerTest.testEhloResolv(SMTPServerTest.java:332)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
        at java.lang.reflect.Method.invoke(Unknown Source)
        at junit.framework.TestCase.runTest(TestCase.java:154)
        at junit.framework.TestCase.runBare(TestCase.java:127)
        at junit.framework.TestResult$1.protect(TestResult.java:106)
        at junit.framework.TestResult.runProtected(TestResult.java:124)
        at junit.framework.TestResult.run(TestResult.java:109)
        at junit.framework.TestCase.run(TestCase.java:118)
        at junit.framework.TestSuite.runTest(TestSuite.java:208)
        at junit.framework.TestSuite.run(TestSuite.java:203)
        at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:478)
        at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:344)
        at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)

        Show
        bago Stefano Bagnara added a comment - I committed the latest patches but now a test fail. I had to apply your patches manually because the format was not correctly handled by Eclipse: can you check I committed all the code and why the test doesn't pass? testEhloResolv(), line 332: junit.framework.AssertionFailedError: expected error: ehlo could not resolved expected:<501> but was:<250> at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.failNotEquals(Assert.java:282) at junit.framework.Assert.assertEquals(Assert.java:64) at junit.framework.Assert.assertEquals(Assert.java:201) at org.apache.james.smtpserver.SMTPServerTest.testEhloResolv(SMTPServerTest.java:332) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) at java.lang.reflect.Method.invoke(Unknown Source) at junit.framework.TestCase.runTest(TestCase.java:154) at junit.framework.TestCase.runBare(TestCase.java:127) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:118) at junit.framework.TestSuite.runTest(TestSuite.java:208) at junit.framework.TestSuite.run(TestSuite.java:203) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:478) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:344) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)
        Hide
        bago Stefano Bagnara added a comment -

        PS: You don't need to create a new file and run the diff tool manually
        If you use eclipse you can simply change the file and use the "Team" => "Create patch..." command (this works if you use the subclipse plugin to synchronize your checkout with the apache svn repository).

        Show
        bago Stefano Bagnara added a comment - PS: You don't need to create a new file and run the diff tool manually If you use eclipse you can simply change the file and use the "Team" => "Create patch..." command (this works if you use the subclipse plugin to synchronize your checkout with the apache svn repository).
        Hide
        norman Norman Maurer added a comment -

        Thx for the tip

        At the meoment i don't know why it not work. I real time test it works:

        maurer@debbox:~/stuff/workspace/james-dev$ telnet localhost 25
        Trying 127.0.0.1...
        Connected to localhost.
        Escape character is '^]'.
        220 debbox SMTP Server (JAMES SMTP Server 2.3-dev) ready Thu, 30 Mar 2006 15:46:02 +0200 (CEST)
        ehlo test.de
        250-debbox Hello test.de (localhost [127.0.0.1])
        250-PIPELINING
        250-ENHANCEDSTATUSCODES
        250 8BITMIME
        quit
        221 2.0.0 debbox Service closing transmission channel
        Connection closed by foreign host.
        maurer@debbox:~/stuff/workspace/james-dev$ telnet localhost 25
        Trying 127.0.0.1...
        Connected to localhost.
        Escape character is '^]'.
        220 debbox SMTP Server (JAMES SMTP Server 2.3-dev) ready Thu, 30 Mar 2006 15:46:07 +0200 (CEST)
        ehlo greogjer.de
        501 Ehlo can not resolved
        quit
        221 2.0.0 debbox Service closing transmission channel

        So it must be a PRoblem in the junit tests. I think its a Problem in the SMTPTestConfiguration.java ..

        Show
        norman Norman Maurer added a comment - Thx for the tip At the meoment i don't know why it not work. I real time test it works: maurer@debbox:~/stuff/workspace/james-dev$ telnet localhost 25 Trying 127.0.0.1... Connected to localhost. Escape character is '^]'. 220 debbox SMTP Server (JAMES SMTP Server 2.3-dev) ready Thu, 30 Mar 2006 15:46:02 +0200 (CEST) ehlo test.de 250-debbox Hello test.de (localhost [127.0.0.1] ) 250-PIPELINING 250-ENHANCEDSTATUSCODES 250 8BITMIME quit 221 2.0.0 debbox Service closing transmission channel Connection closed by foreign host. maurer@debbox:~/stuff/workspace/james-dev$ telnet localhost 25 Trying 127.0.0.1... Connected to localhost. Escape character is '^]'. 220 debbox SMTP Server (JAMES SMTP Server 2.3-dev) ready Thu, 30 Mar 2006 15:46:07 +0200 (CEST) ehlo greogjer.de 501 Ehlo can not resolved quit 221 2.0.0 debbox Service closing transmission channel So it must be a PRoblem in the junit tests. I think its a Problem in the SMTPTestConfiguration.java ..
        Hide
        norman Norman Maurer added a comment -

        Find the problem after rereading the javdoc of avalon. Will submit a patch soon.

        Show
        norman Norman Maurer added a comment - Find the problem after rereading the javdoc of avalon. Will submit a patch soon.
        Hide
        bago Stefano Bagnara added a comment -

        I found the problem in the test and fixed it!
        Thank you for the new feature, let's move to something else

        Show
        bago Stefano Bagnara added a comment - I found the problem in the test and fixed it! Thank you for the new feature, let's move to something else
        Hide
        norman Norman Maurer added a comment -

        Add the provided helo to the log if it could not be resolved

        Show
        norman Norman Maurer added a comment - Add the provided helo to the log if it could not be resolved
        Hide
        norman Norman Maurer added a comment -

        Add the provided ehlo to the log if it could not be resolved

        Show
        norman Norman Maurer added a comment - Add the provided ehlo to the log if it could not be resolved
        Hide
        norman Norman Maurer added a comment -

        Add config option to allow to not use helochecks for relayclients

        Show
        norman Norman Maurer added a comment - Add config option to allow to not use helochecks for relayclients
        Hide
        norman Norman Maurer added a comment -

        Junit

        Show
        norman Norman Maurer added a comment - Junit
        Hide
        norman Norman Maurer added a comment -

        Junit

        Show
        norman Norman Maurer added a comment - Junit
        Hide
        bago Stefano Bagnara added a comment -

        Changed the option to "checkAuthNetworks", defaulting to false.
        Applied the code in the main Helo/Ehlo hanlders but I will work to refactor the whole fast-fail handler things, soon

        Show
        bago Stefano Bagnara added a comment - Changed the option to "checkAuthNetworks", defaulting to false. Applied the code in the main Helo/Ehlo hanlders but I will work to refactor the whole fast-fail handler things, soon
        Hide
        danny@apache.org Danny Angus added a comment -

        Closing issue fixed in released version.

        Show
        danny@apache.org Danny Angus added a comment - Closing issue fixed in released version.

          People

          • Assignee:
            bago Stefano Bagnara
            Reporter:
            norman Norman Maurer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development