Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Changing the ssh key gen algorithm for FIPS machines #136

Merged
merged 2 commits into from
Jul 12, 2019

Conversation

junaid18183
Copy link

Changing the ssh key gen algorithm for FIPS machines
Fixes the - #135

Updated fips check

pkg/metadata/vmmd/fips.go
Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Hi @junaid18183 👋!
Thanks for this PR!

Funnily enough, I had never even heard about FIPS before, so I had to google it 😂
This PR is okay as it unblocks an use-case, but I'd like to be more explicit about this
in the future, so hence it makes sense to let the user specify in the configuration file what
key algorithm to use, instead of relying on autodetection, which might be brittle.

This PR is accepted, but please move the function to pkg/util and add the TODO in the code :)

// FIPSEnabled returns true if running in FIPS mode.
// currently it just checks the system wide /etc/system-fips file present or not.
// We can improve it later.
func FIPSEnabled() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this to pkg/util, that seems like a better place for it.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

// Use ED25519 instead of RSA for performance (it's equally secure, but a lot faster to generate/authenticate)
_, err := util.ExecuteCommand("ssh-keygen", "-q", "-t", "ed25519", "-N", "", "-f", privKeyPath)
ssh_key_algorithm := "ed25519"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a note here like this:

// TODO: In future versions, let the user specify what key algorithm to use through the API types

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@luxas luxas added this to the v0.4.1 milestone Jul 11, 2019
@luxas luxas self-assigned this Jul 11, 2019
@junaid18183
Copy link
Author

Thanks for the review. Have implemented the changes.

Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Very well 👍

LGTM

@luxas luxas merged commit ee09384 into weaveworks:master Jul 12, 2019
@junaid18183 junaid18183 deleted the fix-fips-key branch July 12, 2019 11:02
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.

2 participants