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

Jcbf/fix reply code #34

Merged
merged 3 commits into from
Nov 6, 2017
Merged

Jcbf/fix reply code #34

merged 3 commits into from
Nov 6, 2017

Conversation

jcbf
Copy link
Owner

@jcbf jcbf commented Nov 4, 2017

Fix issues identified in #33 by @gwharton.

  • Fix correct reply code on reject
  • Use SMFIS_TEMPFAIL for TempFail

@jcbf jcbf added the bug label Nov 4, 2017
@coveralls
Copy link

coveralls commented Nov 4, 2017

Coverage Status

Coverage decreased (-2.5%) to 78.672% when pulling ba30aa5 on jcbf/fix_reply_code into 40dd600 on master.

@codecov-io
Copy link

codecov-io commented Nov 4, 2017

Codecov Report

Merging #34 into master will decrease coverage by 2.56%.
The diff coverage is 24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
- Coverage   81.18%   78.61%   -2.57%     
==========================================
  Files           1        1              
  Lines         473      491      +18     
  Branches      161      166       +5     
==========================================
+ Hits          384      386       +2     
- Misses         89      105      +16
Impacted Files Coverage Δ
smf-spf.c 78.61% <24%> (-2.57%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40dd600...df9f6f1. Read the comment docs.

@gwharton
Copy link

gwharton commented Nov 4, 2017

Looking good, I understand what the SoftFail feature does now. Perhaps SoftFail is not the best name to choose in relation to this feature, as an SPF SoftFail is defined as Accept Message and Tag as domain is transitioning as indicated by the presence of the ~all tag in the domains SPF record.

I understand it is there now as it is, so difficult to change, so perhaps some words in the documentation or config file to explain that by setting SoftFail to on in the config will result in the SPF Failed mails being rejected with a temporary fail status, where the correct response to this from the sending MTA is to defer and try delivery again later.

I've just done a quick scan of the code for other such issues and I have a possible bug, so hang fire on the merge. I'll post more shortly.

@gwharton
Copy link

gwharton commented Nov 4, 2017

smf-spf/smf-spf.c

Lines 796 to 805 in ba30aa5

if (status == SPF_RESULT_TEMPERROR && !conf.accept_temperror) {
char reject[2 * MAXLINE];
snprintf(reject, sizeof(reject), "Found a problem processing SFP for %s. Error: %s", context->sender, SPF_strreason(spf_response->reason));
if (spf_response) SPF_response_free(spf_response);
if (spf_request) SPF_request_free(spf_request);
if (spf_server) SPF_server_free(spf_server);
smfi_setreply(ctx, "451" , "4.4.3", reject);
return SMFIS_REJECT;
}

Here's another one. In this case we are in a TEMPERROR situation, we set the correct 4.x.x/4xx temp failure codes, but then SMFIS_REJECT instead of TEMPFAILing.

That's the only one i could spot.

@jcbf jcbf self-assigned this Nov 4, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.5%) to 78.672% when pulling df9f6f1 on jcbf/fix_reply_code into 40dd600 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.5%) to 78.672% when pulling df9f6f1 on jcbf/fix_reply_code into 40dd600 on master.

@jcbf
Copy link
Owner Author

jcbf commented Nov 6, 2017

I'm pushing this changes to a new release.

@jcbf jcbf closed this Nov 6, 2017
@jcbf jcbf deleted the jcbf/fix_reply_code branch November 6, 2017 21:11
@jcbf jcbf restored the jcbf/fix_reply_code branch November 6, 2017 21:12
@jcbf jcbf reopened this Nov 6, 2017
@jcbf jcbf merged commit 8dfd517 into master Nov 6, 2017
@jcbf jcbf deleted the jcbf/fix_reply_code branch November 6, 2017 21:12
@gwharton
Copy link

gwharton commented Nov 6, 2017

Many thanks.

Graham

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.

4 participants