Skip to content

dird: fix tls protocol shown and document TLS Protocol & ciphers restriction#1319

Merged
arogge merged 9 commits intobareos:masterfrom
bruno-at-bareos:dev/bruno/master/s5327-fix-tls-protocol-shown
Dec 7, 2022
Merged

dird: fix tls protocol shown and document TLS Protocol & ciphers restriction#1319
arogge merged 9 commits intobareos:masterfrom
bruno-at-bareos:dev/bruno/master/s5327-fix-tls-protocol-shown

Conversation

@bruno-at-bareos
Copy link
Contributor

@bruno-at-bareos bruno-at-bareos commented Nov 23, 2022 •
edited by arogge
Loading

This PR will fix

  • the version of the TLS protocol used in communication encryption between two daemons.
  • document how to restrict Protocol and ciphers usage
  • introduce systemtests to check those method on available platform (OpenSSL >= 1.1.1)
  • ISSUE #1382

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
  • Decision taken that a test is required (if not, then remove this paragraph)
  • The choice of the type of test (unit test or systemtest) is reasonable
  • Testname matches exactly what is being tested
  • On a fail, output of the test leads quickly to the origin of the fault

@bruno-at-bareos bruno-at-bareos marked this pull request as draft November 23, 2022 16:59
arogge
arogge reviewed Nov 23, 2022
View reviewed changes
@bruno-at-bareos bruno-at-bareos force-pushed the dev/bruno/master/s5327-fix-tls-protocol-shown branch from 5abb6ca to eb02b32 Compare November 28, 2022 15:01
@bruno-at-bareos bruno-at-bareos marked this pull request as ready for review November 28, 2022 15:12
@bruno-at-bareos bruno-at-bareos force-pushed the dev/bruno/master/s5327-fix-tls-protocol-shown branch from eb02b32 to 405d87c Compare December 1, 2022 11:23
alaaeddineelamri
alaaeddineelamri previously requested changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri left a comment

Choose a reason for hiding this comment

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

Mostly typos and wording in the documentation.

I would suggest changing the PR name to be usable in the changelog.

bareos-check-sources modifies one file.

@bruno-at-bareos
Copy link
Contributor Author

bruno-at-bareos commented Dec 6, 2022

Suggestions applied, thanks for the rewording and fixes.
I've also applied fixes from bareos-check-sources.
For the title, please apply any renamed that can fit (I've no other idea nor suggestion)

@bruno-at-bareos bruno-at-bareos changed the title b5327 fix tls protocol shown and document TLS Protocol & ciphers restriction dird: fix tls protocol shown and document TLS Protocol & ciphers restriction Dec 6, 2022
alaaeddineelamri
alaaeddineelamri reviewed Dec 6, 2022
View reviewed changes
Copy link
Contributor

@alaaeddineelamri alaaeddineelamri left a comment

Choose a reason for hiding this comment

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

ok for me!
Thanks!

@bruno-at-bareos
Copy link
Contributor Author

bruno-at-bareos commented Dec 6, 2022

@arogge maybe you want a final quick overview? Shall I do the squashing afterward, and create the changelog entry?

arogge
arogge reviewed Dec 7, 2022
View reviewed changes
Copy link
Member

@arogge arogge left a comment

Choose a reason for hiding this comment

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

I would really prefer not to do the openssl version-check at runtime.
Other than that it looks great.

systemtests/scripts/functions Outdated
Comment on lines +1113 to +1122
# Used by tlsrestricted
skip_if_openssl_too_old()
{
openssl_version=$(openssl version | cut -d " " -f2 | sed 's/\([0-9]\)\.\([0-9]\).*/\1\2/' || 0)
if [ ${openssl_version} -le 10 ]; then
echo "${TestName} test skipped: test cannot be run without openssl > 1.1x"
exit 77;
fi
}

Copy link
Member

@arogge arogge Dec 7, 2022

Choose a reason for hiding this comment

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

You can just check the OPENSSL_VERSION in CMake and disable the test. Should be a lot easier.

if(OPENSSL_VERSION VERSION_LESS "1.1.0")
  somehow_set_test_disabled()
endif()

@bruno-at-bareos bruno-at-bareos force-pushed the dev/bruno/master/s5327-fix-tls-protocol-shown branch 3 times, most recently from 83a29ef to 61a2913 Compare December 7, 2022 14:18
@bruno-at-bareos
Copy link
Contributor Author

bruno-at-bareos commented Dec 7, 2022

reordering and commit sentences have been reviewed to enter into the 50cols.
cmake now check openssl version and disable test if not recent enough.

@arogge arogge dismissed alaaeddineelamri’s stale review December 7, 2022 18:01

changes were addressed, but not approved by reviewer

@bruno-at-bareos @arogge
core: use SSL_get_version() for TLS protocol used.
9a070ff 
- replace SSL_get_cipher_version as this return the protocol version
  where the cipher was created.

OP 5327

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
Alaa Eddine Elamri and others added 8 commits December 7, 2022 19:03
@arogge
dird: show cipher when doing status storage
f9e95b8
@arogge
dird: update GetCipherMessageString() return val
6fffdca
@arogge
dird: add stored cipher string to JobLog
33c6980 
- filed: add cipher line to FD-SD connection joblog
- dird: sd add line return for joblog messages
@bruno-at-bareos @arogge
doc: Add Chapter How restrict TLS Protocol and Ciphers.
fce11bb 
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos @arogge
tests: add tlsrestricted systemtest
a134867 
- Add basic systemtest for restricted TLS Protocol and TLS Cipher List
  cmake check to disable if openssl < 1.1.1

Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@bruno-at-bareos @alaaeddineelamri @arogge
Fix typos and rewording text suggestions
b7e2e1b 
Co-authored-by: Alaa Eddine Elamri <46047750+alaaeddineelamri@users.noreply.github.com>
@bruno-at-bareos @arogge
Apply fixes from bareos-check-sources
4050b38 
Signed-off-by: Bruno Friedmann <bruno.friedmann@bareos.com>
@arogge
Update CHANGELOG.md
32cceb9
@arogge arogge force-pushed the dev/bruno/master/s5327-fix-tls-protocol-shown branch from f146ebb to 32cceb9 Compare December 7, 2022 18:03
@arogge arogge merged commit 449f6a7 into bareos:master Dec 7, 2022
@bruno-at-bareos bruno-at-bareos deleted the dev/bruno/master/s5327-fix-tls-protocol-shown branch March 1, 2023 12:58
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

@alaaeddineelamri alaaeddineelamri Awaiting requested review from alaaeddineelamri

Assignees

@alaaeddineelamri alaaeddineelamri

@bruno-at-bareos bruno-at-bareos

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bruno-at-bareos @arogge @alaaeddineelamri @pstorz