standards icon indicating copy to clipboard operation
standards copied to clipboard

Add Secure Connections Standard

Open markus-hentsch opened this issue 1 year ago • 10 comments

Closes #547

markus-hentsch avatar Apr 04 '24 10:04 markus-hentsch

@markus-hentsch I have general remark: Because TLS configuration and security is a moving target, have you considered to base the recommended configuration on one of the profiles offered by Mozilla SSL? For example, the "intermediate" profile, see https://ssl-config.mozilla.org/ and https://wiki.mozilla.org/Security/Server_Side_TLS. (AFAIK, these can be checked with sslyze.)

martinmo avatar Apr 10 '24 08:04 martinmo

@markus-hentsch I have general remark: Because TLS configuration and security is a moving target, have you considered to base the recommended configuration on one of the profiles offered by Mozilla SSL? For example, the "intermediate" profile, see https://ssl-config.mozilla.org/ and https://wiki.mozilla.org/Security/Server_Side_TLS. (AFAIK, these can be checked with sslyze.)

lol, I had the same idea and actually checked our haproxy TLS implementation, seems there is some opportunity to do some hardening there:

COMPLIANCE AGAINST MOZILLA TLS CONFIGURATION
 --------------------------------------------

    Checking results against Mozilla's "MozillaTlsConfigurationEnum.INTERMEDIATE" configuration. See https://ssl-config.mozilla.org/ for more details.

    a.regiocloud.tech:443: FAILED - Not compliant.
        * ciphers: Cipher suites {'TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA', 'TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384', 'TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA', 'TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256'} are supported, but should be rejected.

artificial-intelligence avatar Apr 10 '24 09:04 artificial-intelligence

working on a fix for upstream: https://bugs.launchpad.net/kolla-ansible/+bug/2060787

artificial-intelligence avatar Apr 10 '24 09:04 artificial-intelligence

Just some minor cosmetic changes:

* Is prefer to write Cloud Service Provider instead of CSP, as "CSP" is not an official abbreviation

* I do not like writing "SCS proposes..", "SCS decides...". AFAIK, SCS stands for Sovereign Cloud Stack, which is a Software Stack, which cannot decide something. I prefer to write "SCS project" or "SCS community"

But again. This is just cosmetics.

I adjusted the SCS references. I left "CSP" as-is and added a glossary instead, like I did with some other standards. We seem to use CSP a lot in other standards so I'd like to stay consistent. The glossary at the top should introduce the abbreviation sufficiently now.

markus-hentsch avatar Apr 19 '24 13:04 markus-hentsch

Updated standard and test script to use the Mozilla TLS "intermediate" preset now.

markus-hentsch avatar May 21 '24 15:05 markus-hentsch

Somewhat of a meta comment, but I find it weird that there are open discussions by reviewers but at the same time the PR is approved by the same people.

Please mark conversations as resolved if you approve the PR, or remove your approval if there are open questions which need to be addressed. I specifically did not yet approve the PR because there seem to be open questions from other reviewers. If these are already addressed I don't see that reflected in the current status, so I'm unsure if I should already approve the PR.

Thank you.

artificial-intelligence avatar Jun 19 '24 12:06 artificial-intelligence

Somewhat of a meta comment, but I find it weird that there are open discussions by reviewers but at the same time the PR is approved by the same people.

Please mark conversations as resolved if you approve the PR, or remove your approval if there are open questions which need to be addressed. I specifically did not yet approve the PR because there seem to be open questions from other reviewers. If these are already addressed I don't see that reflected in the current status, so I'm unsure if I should already approve the PR.

Thank you.

I searched for a revoke button for the approval, but only could re-request my own review.

josephineSei avatar Jun 19 '24 13:06 josephineSei

@josephineSei @artificial-intelligence

Based on your feedback I had another look at the libvirt security sections and in https://github.com/SovereignCloudStack/standards/commit/b60a38eaeb5b7c706dc44d6bc5a28fd8b9137126 I tried to come up with a more comprehensible phrasing for the open questions section and decision recommendations that are more in line with the multilateral nature of the problem.

Can you please check out the adjusted open questions and decision sections about the libvirt interface again?

markus-hentsch avatar Jun 20 '24 13:06 markus-hentsch

I updated the standard and removed the concrete config options from the RabbitMQ and Apache Kafka sections. Config snippets like these are hard to keep up-to-date in a standard. I placed links to documentation there instead.

markus-hentsch avatar Jul 26 '24 15:07 markus-hentsch

@artificial-intelligence this is still marked as "requested changes" by you. Was there anything left from your review that I didn't address yet?

markus-hentsch avatar Jul 26 '24 15:07 markus-hentsch

The markdown lint issues should be addressed. The link checker issues should solve themselves when this is merged.

mbuechse avatar Nov 06 '24 12:11 mbuechse

I removed the markdown linter errors. The only remaining errors coming from markdown link checker, who complain about the following two dead links:

  • https://github.com/SovereignCloudStack/standards/blob/main/Tests/iaas/secure-connections/tls-checker.py → Status: 404
  • https://github.com/SovereignCloudStack/standards/blob/main/Tests/iaas/secure-connections/README.md → Status: 404

The files are part if this PR and are not yet there.

@mbuechse Any idea how to fix this? Otherwise, I will merge PR anyway, as we have three approvals and all other checks were successfully.

anjastrunk avatar Nov 19 '24 14:11 anjastrunk

I removed the markdown linter errors. The only remaining errors coming from markdown link checker, who complain about the following two dead links:

* https://github.com/SovereignCloudStack/standards/blob/main/Tests/iaas/secure-connections/tls-checker.py → Status: 404

* https://github.com/SovereignCloudStack/standards/blob/main/Tests/iaas/secure-connections/README.md → Status: 404

The files are part if this PR and are not yet there.

@mbuechse Any idea how to fix this? Otherwise, I will merge PR anyway, as we have three approvals and all other checks were successfully.

I tried to replace absolute links with relative ones, to satisfy markdown link checker. This worked fine, but case markdown linter to fail, as relative links are not allowed. I reverted relative links and decided to merge this PR even markdown link checker fails. As dead links related to files added by this PR, merging will not break repo.

anjastrunk avatar Nov 25 '24 11:11 anjastrunk