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

TLS-RPT Support #107

Open
wants to merge 12 commits into
base: v2.0
Choose a base branch
from
Open

TLS-RPT Support #107

wants to merge 12 commits into from

Conversation

jnew-gh
Copy link
Contributor

@jnew-gh jnew-gh commented Jul 25, 2022

These commits add functionality to parse SMTP TLS Reports (TLS-RPT) and add the results into the database.

Two new tables, tls-report and tls-rptrecord, are created along with their corresponding columns.

jnew-gh added 6 commits July 25, 2022 12:53
Adds two new tables, tls_report and tls_rptrecord,  and their respective columns, to add TLS report parsing functionality.
Updates the README.md to reflect the new TLS Report parsing functionality.
Updates required packages to include perl-JSON.
Extends the parser's ability to processing JSON, in both compressed and uncompressed files
Includes documentation to support extension.
No change in functionality.
Copy link
Contributor

@ekalin ekalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed the code for creating tables in PostgreSQL and some minor changes are necessary.

I'd suggest using docker to run a PostgreSQL instance. Easy to setup, doesn't affect the rest of the system, and it's easy to delete everything and start again (often necessary when testing table creation).

I haven't tested the actual insertion of reports in the database yet.

dbx_Pg.pl Outdated Show resolved Hide resolved
dbx_Pg.pl Outdated Show resolved Hide resolved
dbx_Pg.pl Outdated Show resolved Hide resolved
jnew-gh and others added 3 commits July 26, 2022 14:51
Co-authored-by: Eduardo Kalinowski <[email protected]>
Co-authored-by: Eduardo Kalinowski <[email protected]>
Co-authored-by: Eduardo Kalinowski <[email protected]>
@ekalin
Copy link
Contributor

ekalin commented Jul 26, 2022

This really is up to @techsneeze to judge, my in my opinion changing the name of the config option isn't great. At least the code should check if the previous config option is set and print a warning that $maxsize_xml is deprecated and should be renamed to $raw_data_max_size.

@jnew-gh
Copy link
Contributor Author

jnew-gh commented Jul 26, 2022

Point taken.
What's the best way to handle it? If $raw_data_max_size isn't set, should execution stop with a warning that $maxsize_xml is deprecated in favour of $raw_data_max_size, or, should the program use the value set in $maxsize_xml for the new variable, warn the user and continue?

dbx_Pg.pl Outdated Show resolved Hide resolved
Co-authored-by: Eduardo Kalinowski <[email protected]>
@Maeglin73
Copy link

Not having a return statement at the end of the getDATAFrom* functions breaks the code that calls them if there is an error getting XML or JSON data, since it is expecting an array of return values in all cases and getting nothing. That also breaks the handling of the $delete_failed option and other things, at least when running the program with "-e", because $filecontent is non-empty going into processDATA.

Adding return("", ""); to the end of getDATAFromMessage fixed the problem for me, and will probably do the same for the other getDATAFrom* functions as well.

Now that that's sorted, I'm letting it parse incoming DMARC and TLS-RPT reports in debug mode over the next few days, to see how that goes.

@Maeglin73
Copy link

Maeglin73 commented Aug 13, 2022

Another problem found with getDATAFromMessage, in that the MIME::Parser cleanup at the end of the function is never done (leaving behind temporary files) if XML or JSON data is found. I fixed this by setting a variable @ret_arr to a default of ("", "") before the defined($location) condition, set that to the report data and type if it was found, then return @ret_arr; at the end of the function, after the parser cleanup is done.

@Maeglin73
Copy link

Of course, trying to do the return and cleanup properly results in another problem. I had to rewrite the part of the function that reads and parses the attachment. If I had more than zero experience with git, I could help more directly...

# Read data if possible (if open)
if ($unzip eq "") {
        my $report_data = "";
        my $raw_data = join("", <DATA>);
        close DATA;
        $report_data = getXMLFromXMLString($raw_data);
        if ($report_data) {
                @ret_arr = ($report_data, "xml");
        } else {
                if ($debug) {
                        warn "$scriptname: Subject: $subj\n:";
                        warn "$scriptname: The data found in ZIP file (temp. location: <$location>) does not seem to be valid XML! Let's try JSON...\n";
                }

                $report_data = getJSONFromJSONString($raw_data);
                if ($report_data) {
                        if ($debug) {
                                warn "$scriptname: The data found in ZIP file seems to be valid JSON!\n";
                        }
                        @ret_arr = ($report_data, "json");
                } else {
                        if ($debug) {
                                warn "$scriptname: Subject: $subj\n:";
                                warn "$scriptname: The data found in ZIP file (temp. location: <$location>) does not seem to be valid JSON either! \n";
                        }
                }
        }
} else {

It seems to work cleanly now, as far as reading MIME email messages, but I'll keep running it in debug mode over the next couple of days. I still haven't gotten any TLS-RPT reports to try with it, but I tend to get at least one per day from Google.

@Maeglin73
Copy link

A couple of days of processing both kinds of reports, and everything seems happy. I didn't have any negative TLS reports to exercise the tls_rptrecord code, but aside from that...

@Maeglin73
Copy link

After turning off debug mode, I'm still getting "No failure details in report" from the TLS-RPT processing. That seems more like a debug message to me.

@jnew-gh
Copy link
Contributor Author

jnew-gh commented Aug 20, 2022

@Maeglin73
Unfortunately, your changes do not work for me. It may be that I have transcribed your code incorrectly or have not followed your description of where to put variable declarations or possibly something else but this is the problem when code is "described" rather than submitted into git.

It would be more helpful if you could submit your suggestions through git (I didn't know how to use git either before I started helping with this project - and most of it is still a mystery to me).

@Maeglin73
Copy link

@jnew-gh Feels like trying to give myself a crash course would either take a while or do more harm than good, considering I'm not even sure where to start for something like this. If it works for you, what I could do is make my copy of the parser script available on a web server. You could run a diff, and go from there.

@jnew-gh
Copy link
Contributor Author

jnew-gh commented Aug 20, 2022

@Maeglin73
Just attach the file to a comment. Add ".txt" to the end of the filename to bypass githib's filters.

@Maeglin73
Copy link

Ah, ok. Here you go, then.
dmarcts-report-parser.pl.txt

Show TLS-RPT message "No failure details in report" only when $debug is set.
…ctions

Set up @ret_arr variable to handle errors getting XML or JSON data
Rearrange logic when parsing files for XML or JSON data
@Maeglin73
Copy link

Looks good.
Liked the error handling changes, did you? :-)

@jnew-gh
Copy link
Contributor Author

jnew-gh commented Aug 25, 2022

Sure, it works well. I eventually want to merge the three separate getDATAFrom* functions into one so I applied the same logic to all three.

@brknkfr
Copy link

brknkfr commented Oct 19, 2022

Just tested TLS-RPT support. It seems to fork fine.

@zell-mbc
Copy link

zell-mbc commented Feb 18, 2023

Any idea when this one will get merged? I have been trying to manually inject the modified files into my docker image. https://github.com/gutmensch/docker-dmarc-report
Unfortunately without success so far.

Edit: Got it working. The JSON module seems to be a new dependencies. I can see reports being added to the database which is really cool. Need to move my Grafana dashboard over to this db now and should be fully in business!
Still, would be nice for this merge request to become official soon.
Nevertheless, thanks for putting all of this together! If you need someone to test things or got interest in the Grafana dashboard (work in progress), I am around.

Edit2: @jnew-gh
A few comments if I may?

  • How about pulling out the policy mode from the policy_string field into it's own column? That way one can see if tls is enforced or not.

  • raw_json doesn't seem to get populated?
    image

  • How about pulling out the result type as a dedicated column? This is of interest for failed sessions

image

@anonsimba
Copy link

does this work with mysql? i am not able to execute the scripts to create the tls table/colums to the database

Erroor:
./dbx_Pgmysql.pl: line 1: syntax error near unexpected token (' ./dbx_Pgmysql.pl: line 1: %dbx = ('

@Maeglin73
Copy link

Erroor:
./dbx_Pgmysql.pl: line 1: syntax error near unexpected token (' ./dbx_Pgmysql.pl: line 1: %dbx = ('

Where did that file come from? It's not anywhere in the repository that I see.

To answer the question, though, I've had no trouble using it with MySQL or MariaDB.

@anonsimba
Copy link

@Maeglin73

I figured out the error, my database doesnt have the required table/columns for the tls records to be stored, any idea why its not getting created when i run the script, if you have the working script kindly share the same.

Error:

BD::mysql::st execute failed: Table 'dmarc.tls_report' doesn't exist at ./dmarcts-report-parser-tls.pl line 1310.

DBD::mysql::st fetchrow_array failed: fetch() without execute() at ./dmarcts-report-parser-tls.pl line 1311.

Use of uninitialized value $raw_data_max_size in numeric gt (>) at ./dmarcts-report-parser-tls.pl line 1372.

Skipping storage of large JSON (400 bytes) as defined in config file.

DBD::mysql::db do failed: Table 'dmarc.tls_report' doesn't exist at ./dmarcts-report-parser-tls.pl line 1376.

Cannot add report to database (Table 'dmarc.tls_report' doesn't exist). Skipped.

./dmarcts-report-parser-tls.pl: Skipping tlsreports/google.com!mydomain.com!1680220800!1680307199!001.json due to database errors.

@Maeglin73
Copy link

If you download what's in the main repository, then download the changed files from this pull request and overwrite the original files with those, you'll have what I'm running. Of course, then you'll have to create or update the config file based on the changed sample, since some of the value names have changed.

@anonsimba
Copy link

@Maeglin73

I was able to download the changed files, but still not able to read the json data.

└──╼ $./dmarcts-report-parser.pl -x tls* -d
Filename: tls, MimeType: inode/directory
This is not an archive file
./dmarcts-report-parser.pl: The data found in ZIP file does not seem to be valid XML! Let's try JSON...
./dmarcts-report-parser.pl: The data found in ZIP file does not seem to be valid JSON, either!


Processing file tls

Type: 2
FileContent:
MSG: tls

./dmarcts-report-parser.pl: The tls does not seem to contain a valid DMARC report. Skipped.
./dmarcts-report-parser.pl: Processed 1 messages(s).

@Maeglin73
Copy link

It's expecting data file names, and you're giving it a directory name.

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.

6 participants