Skip to content

bsmtp: fix and update code, and change CLI parsing to CLI11#1316

Merged
arogge merged 2 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto
Feb 13, 2023
Merged

bsmtp: fix and update code, and change CLI parsing to CLI11#1316
arogge merged 2 commits intobareos:masterfrom
alaaeddineelamri:dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto

Conversation

@alaaeddineelamri
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri commented Nov 15, 2022 •
edited by arogge
Loading

Description

A few months ago, we upgraded the bareos daemons to use CLI11 as a CLI arguments parsing tool, but bsmtp and bscrypto were left behind. This PR brings them to the fold.

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General
  • PR name is meaningful
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
  • Check backport line
  • Is the PR title usable as CHANGELOG entry?
  • Separate commit for CHANGELOG.md ("update CHANGELOG.md"). The PR number is correct.
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR
  • bareos-check-sources --since-merge does not report any problems
Tests

bstmp is still manually tested.

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch 2 times, most recently from 937023d to 30e0cf4 Compare November 15, 2022 16:37
@bruno-at-bareos bruno-at-bareos self-requested a review November 17, 2022 11:00
@bruno-at-bareos bruno-at-bareos self-assigned this Nov 17, 2022
@bruno-at-bareos
Copy link
Contributor

bruno-at-bareos commented Nov 24, 2022

First build and local test reveal a problem with bsmtp
Now the -c --c is mandatory which was not the case previously
Parsing of -m --mailhost is failing if a port is added

Beware of -h as being replaced by -m which will imply a change in any existing configuration (doesn't sound like a good idea)

bsmtp -d200 --dt -f "hostmaster@domain.tld" -s "bsmtp CLI11 test" -m mail.domain.tld:25 --utf8 -6 -cc postmaster@domain.tld backups@domain.tld <<< "Hello bsmtp trying to send something."

24-Nov-2022 14:48:33.054685 bsmtp (20): tools/bsmtp.cc:275-0 host=mail.domain.tld:25
24-Nov-2022 14:48:33.055032 bsmtp (20): tools/bsmtp.cc:353-0 My hostname is: e441160ee859
24-Nov-2022 14:48:33.055043 bsmtp (20): tools/bsmtp.cc:375-0 From addr=hostmaster@domain.tld
bsmtp: tools/bsmtp.cc:401-0 Error unknown mail host "mail.domain.tld:25": ERR=Name or service not known
bsmtp: tools/bsmtp.cc:404-0 Retrying connection using "localhost".
bsmtp: tools/bsmtp.cc:425-0 Failed to connect to mailhost localhost

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch 6 times, most recently from 1a32d67 to 236a3e5 Compare November 29, 2022 12:45
bruno-at-bareos
bruno-at-bareos requested changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

bsmtp review

  • Tested with long options : ok
bsmtp \
  --debug-level 100 \
  --debug-timestamps \
  --from-address "hostmaster@localhost.localdomain" \
  --subject "bsmtp CLI11 test - final review - short options" \
  --mailhost remotesmtp@domain.tld \
  --utf8 \
  --ipv6-protocol \
  --reply-to "bareos@localhost.localdomain" \
  --copy-to "admins@localhost.localdomain" \
  hostmaster@localhost.localdomain \
<<< "bsmtp is the perfect tool to work with long options"

Tested with short options: ok

bsmtp \
  --d100 \
  --dt \
  -f "hostmaster@localhost.localdomain" \
  -s "bsmtp CLI11 test - final review - short options" \
  -h remotesmtp@domain.tld:25 \
  -8 \
  -6 \
  -r "bareos@localhost.localdomain" \
  -c "admins@localhost.localdomain
  hostmaster@localhost.localdomain \
<<< "bsmtp is the perfect tool to work with long options"

Failing cases:

If passing an ipv4 or ipv4:port is accepted and work, trying to pass an ipv6 is failing, the address being truncated by the first semi-column.

bsmtp \
  --debug-level 200 \
  --debug-timestamps \
  --from-address "hostmaster@localhost.localdomain" \
  --subject "bsmtp CLI11 test - final review - short options" \
  --mailhost 2001:DB8::baba \
  --utf8 \
  --ipv6-protocol \
  hostmaster@localhost.localdomain <<< "bsmtp is the perfect tool to work with long options availaible. ipv6 addr"

29-Nov-2022 13:15:27.673392 bsmtp (20): tools/bsmtp.cc:259-0 host=2001:DB8::baba
29-Nov-2022 13:15:27.673729 bsmtp (20): tools/bsmtp.cc:348-0 My hostname is: 1741065bd803
29-Nov-2022 13:15:27.673751 bsmtp (20): tools/bsmtp.cc:374-0 From addr=hostmaster@localhost.localdomain
bsmtp: tools/bsmtp.cc:401-0 Error unknown mail host "2001": ERR=Address family for hostname not supported

Also passing an ipv6+port separated by semi-column has to be supported.
The common syntax used look like [::1]:25

I've also let some other comments in code (flags named changes)

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch 2 times, most recently from 23c5b5f to 9593a87 Compare November 30, 2022 15:13
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch 2 times, most recently from 1a2bdce to 214582c Compare December 14, 2022 12:23
bruno-at-bareos
bruno-at-bareos reviewed Dec 21, 2022
View reviewed changes
@bruno-at-bareos bruno-at-bareos force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch from 1196e8f to 0b06fde Compare January 4, 2023 10:33
@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch from 424fe8a to 04adb8e Compare January 4, 2023 14:41
@alaaeddineelamri alaaeddineelamri changed the title bsmtp: bscrypto: change CLI parsing to CLI11 bsmtp: fix and update code, and change CLI parsing to CLI11 Jan 4, 2023
@bruno-at-bareos bruno-at-bareos self-requested a review January 5, 2023 08:57
bruno-at-bareos
bruno-at-bareos requested changes Jan 5, 2023
View reviewed changes
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos left a comment

Choose a reason for hiding this comment

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

Everything looks fine and have been tested manually are ok.
We should really check if we can add the ipv6 []:port parsing option to bring this tool on top notch level ;-)

bruno-at-bareos
bruno-at-bareos reviewed Jan 5, 2023
View reviewed changes
core/src/tools/bsmtp.cc Outdated
Dmsg1(10, "Reply-To: %s\r\n", reply_addr.c_str());
}

#if defined(HAVE_WIN32)
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos Jan 5, 2023

Choose a reason for hiding this comment

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

Note for the future: it is possible that soon or later having a sender not being equal to from will add spam weight to mail and as such make the mail undelivered.

Copy link
Contributor

@bruno-at-bareos bruno-at-bareos Jan 12, 2023

Choose a reason for hiding this comment

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

@arogge has something to add here or sender

Copy link
Member

@arogge arogge Jan 16, 2023

Choose a reason for hiding this comment

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

we should think about sending the e-mail with the null-sender, as we never want a bounce.

Copy link
Contributor

@bruno-at-bareos bruno-at-bareos Feb 9, 2023

Choose a reason for hiding this comment

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

@arogge can you have a review if 610150a is ok for that.

@alaaeddineelamri alaaeddineelamri force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch from 701a72e to cd86704 Compare January 9, 2023 14:50
@bruno-at-bareos bruno-at-bareos force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch from 610150a to 10043db Compare February 9, 2023 15:51
@bruno-at-bareos bruno-at-bareos mentioned this pull request Feb 13, 2023
Merged
10 tasks
@arogge arogge force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch 3 times, most recently from d05c16f to a64d0b3 Compare February 13, 2023 10:38
@arogge
bsmtp: CLI11: change cli parsing to CLI11
33de7c5 
- refactor: reorder variables where needed
- refactor: bsmtp: make global variables local and update functions
- refactor: bsmtp: change char* to std::string
- bsmtp: replace change from and cc long option names
- docs: update help documentation
- bsmtp: make CLI correctly parse IPv6 addresses
- fix --mailhost signature in bsmtp.txt
- remove unneed sender header

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@arogge arogge force-pushed the dev/alaaeddineelamri/master/cli11-bsmtp-bscrypto branch from a64d0b3 to 33de7c5 Compare February 13, 2023 10:39
@arogge arogge merged commit a579a83 into bareos:master Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arogge arogge arogge approved these changes

@bruno-at-bareos bruno-at-bareos bruno-at-bareos approved these changes

Assignees

@arogge arogge

@bruno-at-bareos bruno-at-bareos

Labels

requires backport to 22

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alaaeddineelamri @bruno-at-bareos @arogge