Skip to content
This repository has been archived by the owner on Jul 30, 2024. It is now read-only.

Add FIPS mode support to Faros #143

Merged
merged 9 commits into from
Jul 1, 2021
Merged

Conversation

hfenner
Copy link
Contributor

@hfenner hfenner commented Jun 23, 2021

We're adding a toggle for FIPS mode installation of OCP clusters. I believe we got all the places necessary to add the prompt to the CLI and ensure it results in a fips: true or fips: false within the install-config.yaml.

Please let us know if there is something that was missed or a test we should run. We will update the documentation after if this functionality is correct.

@hfenner
Copy link
Contributor Author

hfenner commented Jun 23, 2021

This pull request should close this issue: #62 (comment)

Copy link
Member

@rmkraus rmkraus left a comment

Choose a reason for hiding this comment

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

Looks pretty solid. I have a few requested changes to keep this from breaking older installs.

@@ -25,7 +25,8 @@ def __init__(self, path, footer, rtr_interfaces):
])
self.cluster = ParameterCollection('cluster', 'Cluster Configuration', [
PasswordParameter('ADMIN_PASSWORD', 'Adminstrator Password'),
PasswordParameter('PULL_SECRET', 'Pull Secret')
PasswordParameter('PULL_SECRET', 'Pull Secret'),
Parameter('FIPS_MODE', 'Fips Mode')
Copy link
Member

Choose a reason for hiding this comment

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

This parameter should be of BooleanParameter type. Also Fips should be FIPS as it is an acronym.

BooleanParameter('FIPS_MODE', 'FIPS Mode', 'False')

Note that False is of string type. This will set the default value to False.

app/inventory.py Outdated
@@ -210,6 +210,7 @@ def main(config, ipam, inv):
extra_nodes=extra_nodes,
ignored_macs=config['IGNORE_MACS'],
dns_forwarders=[item['server'] for item in json.loads(config.get('DNS_FORWARDERS', '[]'))],
fips_mode=config['FIPS_MODE'],
Copy link
Member

Choose a reason for hiding this comment

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

config.get('FIPS_MODE', 'False') == 'True'

This will account for legacy configs without a FIPS_MODE parameter.

@aarongreen85
Copy link
Contributor

@rmkraus The requested changes have been committed. Thanks.

@rmkraus rmkraus linked an issue Jun 29, 2021 that may be closed by this pull request
Closed
@rmkraus
Copy link
Member

rmkraus commented Jun 29, 2021

Do you think this should also enable FIPS mode on the bastion node? Obviously, that's tricky because a restart is required. Alternatively, this could just check if FIPS is enabled and warn if it is not. Or we just make a not in the docs that this switch does not change the FIPS status of the bastion.

I'm not really sure what is best. What do you think?

@aarongreen85
Copy link
Contributor

I'm still fairly new to this project. Is there other existing code that makes changes to the bastion node? If so, then we could open another issue to work on enabling FIPS there and add a note stating that the current code base does not configure FIPS on bastion node. If there is no existing code that makes changes to the bastion node, then in my mind it makes less sense to add an issue to work on that. Mainly because it expands the scope of this project. Alternatively, we could make an opinionated choice on some other hardening project and point folks that way if they desire a hardened or FIPS enabled bastion node?

@hfenner
Copy link
Contributor Author

hfenner commented Jun 29, 2021

There is code that makes changes to the bastion node (installs packages, sets up DNS, etc), but I also think we should keep the scope on this code change small. We need this specific change sooner rather than later, but we could follow up to add a FIPS on bastion node option later.

I vote for leaving the scope of this change as the cluster only and writing another issue for FIPS on bastion.

@rmkraus
Copy link
Member

rmkraus commented Jul 1, 2021

👍

We'll have to make sure (when we document) to mention that this option is for the cluster only and FIPS on the bastion should be done at OS install time. That should suffice.

@rmkraus rmkraus merged commit 8d580e6 into project-faros:master Jul 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fips
3 participants