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

Harden ssh client crypto policy #4681

Merged

Conversation

vojtapolasek
Copy link
Collaborator

Description:

Implementing rule and checks for hardening of SSH client Crypto Policies as required by Common Criteria.
Unfortunately overriding the Crypto Policy is not the way to go in this case, because of the way in which SSH client parses the configuration files.
Therefore a file in /etc/ssh/ssh_config.d/ will be created and chosen options of Crypto Policy should be overided.

Rationale:

Based on Common Criteria, this rule checks and offer a remediation for SSH client configuration. The CC restricts ciphers, accepted public key algorithms, GSSAjPI key exchange, HMAC algorithms, KEX algorithms.

@vojtapolasek
Copy link
Collaborator Author

This is work in progress! Currently it just builds, it is not tested at all.
I borrowed Jinja Oval macros and rewrote them so that they can create multiple criteria, tests, objects and states in one file. the problem was that the IDs of criteria, objects, tests and states were dependent only on rule ID, in my modified macros they depend also on the config parameter checked.

@dahaic dahaic changed the title Harden ssh client crypto policy [wip] Harden ssh client crypto policy Aug 6, 2019
@vojtapolasek vojtapolasek force-pushed the harden_ssh_client_crypto_policy branch from e3909b9 to c6014ad Compare August 16, 2019 14:10
@vojtapolasek vojtapolasek changed the title [wip] Harden ssh client crypto policy Harden ssh client crypto policy Aug 19, 2019
@vojtapolasek
Copy link
Collaborator Author

Work finished, need review please.

## TO DO: https://github.com/ComplianceAsCode/content/issues/4472
#sysctl enable rngd.service
## Enable the Hardware RNG Entropy Gatherer Service
- service_rngd_enabled
Copy link
Member

Choose a reason for hiding this comment

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

This rule is already selected in master branch. Do you know why it is showing as a new addition? Below is the link to the master file pointing to the modification:

- service_rngd_enabled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know. Maybe I messed up the ospp.profile file. What I did in the end was that I took this file from the actual master and copied it into my branch and only added my rule at the end, see the last commit. How to fix it?

Copy link
Member

Choose a reason for hiding this comment

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

No idea what happened here and I don't know how to fix. Maybe reverting this last commit and rebasing on top of master could help.

@vojtapolasek vojtapolasek force-pushed the harden_ssh_client_crypto_policy branch from 1491849 to 8d14f24 Compare August 19, 2019 11:59
@vojtapolasek
Copy link
Collaborator Author

should be fixed now

@yuumasato yuumasato self-assigned this Aug 20, 2019
@adelton
Copy link
Collaborator

adelton commented Aug 20, 2019

I did not dig into changes in detail but overall this is the approach we want to take to be able to use the FIPS crypto policy and override the options that need to be stricter.

Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

We discussed previously that Match finall all should be before the other settings.
So I suggest adding a test scenario where Match final all is not before those settings.

This will result in pass while not being the intended configuration.

Each setting checked that is not Match, should check that it is after Match.
A prefix_regex like used in harden_sshd_crypto_policy may work, something like:
^Match final all(?:.*\n)*?parameter (not thoroughly tested).

@vojtapolasek vojtapolasek force-pushed the harden_ssh_client_crypto_policy branch 2 times, most recently from ee5e2a9 to 731a973 Compare August 20, 2019 11:47
vojtapolasek and others added 16 commits August 20, 2019 17:37
copied and modified line-in-file Jinja macros to suit my needs
I need to check multiple param and values in one file and do it in one oval check
now the content can be built successfully
modified the suffix to include underscore
checking for PubkeyAcceptedKeyTypes
added third parameter to state macro so that it can create states with unique names based on this parameter
added checking for RekeyLimit and GSSAPIAuthentication
also checking for Match final all at the begining of the file
to incorporate support for parameters containing spaces
duplicate rule
…ning

added prefix_regex into oval check and included sample test
SSH client honors only the first occurence of config item and skips subsequent instances. One test, redefine_gssapi.fail.sh was failing because of this and I did not notice before.
added tests for supercompliant systems which so far fail
added tests which add additional ciphers/kexs etc
reacting to comments on PR
@vojtapolasek vojtapolasek force-pushed the harden_ssh_client_crypto_policy branch from 731a973 to 88e0761 Compare August 20, 2019 15:38
Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

Nice catch with the redefined_gssapi test.
Does it make sense to add a similar test where there are two definitions of GSSAPIAuthentication, where valueno is first?

@@ -5,11 +5,13 @@ title: 'Harden SSH client Crypto Policy'
description: |-
Crypto Policies are means of enforcing certain cryptographic settings for selected applications including OpenSSH server.
The SSH client is by default configured to modify its configuration based on currently configured Crypto-Policy. However, in certain cases it might be needed to override the Crypto Policy specific to OpenSSH Client and leave rest of the Crypto Policy intact.
This is unfortunately not possible through the Crypto Policy mechanism directly due to the way in which the SSH Client parses configuration files. It is needed to apply Crypto Policy modifications by dropping a file into the <tt>/etc/ssh/ssh_config.d/</tt> directory. The file needs to be named in a way such that it is parsed by SSH client before the actual Crypto Policy.
This is unfortunately not possible through the Crypto Policy mechanism directly due to the way in which the SSH Client parses configuration files. During the parsing process, as soon as the client parses some configuration option and its value, it remembers it and ignores any subsequent overides. The mechanism of crypto policies appends eventual customizations at the end of the system wide crypto policy. Therefore customizations will be ignored by the client.
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: not possible through the Crypto Policy mechanism directly due to ... to not directly possible through the Crypto Policy mechanism due to...
Typo: overrides.

Copy link
Member

Choose a reason for hiding this comment

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

The mechanism of crypto policies appends eventual customizations at the end of the system wide crypto policy. Therefore customizations will be ignored by the client.

Not sure what this means. crypto-policy mechanism append customizations at the end of crypto-policy, and because of this, they are ignored by the client?

Maybe you want to say that "due to how SSH Client reads the crypto-poicy, any setting that a "official" crypto-policy would configure, will be ignored by the client" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to say that if you go the /etc/crypto-policies/local.d way, in the case of Openssh client this approach will not work if you try to override a parameter which is already configured by the system wide policy.

Copy link
Member

Choose a reason for hiding this comment

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

Aha! I understand this as part of This file subsequently includes the actual system-wide crypto policy placed in <tt>/etc/crypto-policies/back-ends/openssh.config</tt>.
As any file in /etc/crypto-policies/local.d will become part of what is in back-ends/openssh.config.

To me this seems a bit too deep in details for the rule description.

@yuumasato
Copy link
Member

Good stuff, thank you @vojtapolasek!

@yuumasato yuumasato added this to the 0.1.46 milestone Aug 21, 2019
@yuumasato yuumasato merged commit 4d2d00c into ComplianceAsCode:master Aug 21, 2019
This pull request was closed.
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.

4 participants