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

crypto.createHash('md5') fails in a FIPS-enabled environment #272

Closed
gabe-l-hart opened this issue Oct 6, 2020 · 1 comment · Fixed by #314
Closed

crypto.createHash('md5') fails in a FIPS-enabled environment #272

gabe-l-hart opened this issue Oct 6, 2020 · 1 comment · Fixed by #314

Comments

@gabe-l-hart
Copy link

gabe-l-hart commented Oct 6, 2020

Description

Our team is building an operator based on cdk8s and one of our deployment targets is a FIPS-enabled kubernetes cluster. When running cdk8s (and by proxy the constructs library), we encounter the following error when attempting to to run a synth:

Error: error:060800C8:digital envelope routines:EVP_DigestInit_ex:disabled for FIPS

Root Cause

The root-cause of this bug is this line where crypto.createHash('md5') is invoked. According to the internet the md5 crypto hash algorithm is considered insecure by FIPS standards and is thus disabled in the underlying openssl libraries when the operating system is running in FIPS mode.

Repro

(In a FIPS-enabled cluster. Not sure how to easily reproduce this)

const crypto = require('crypto');
crypto.createHash('md5').update(path.join('/')).digest("hex");

Proposed Solution

I believe that the point of the offending line is simply to compute a unique value for the given string path, so I think we can simply replace 'md5' with a FIPS-approved algorithm like sha1 or sha256. I've tested both and they both produce valid hash values.

Volunteering!

I'd be happy to try to fix this one and submit a PR

@gabe-l-hart
Copy link
Author

Per discussion in the fix PR, it looks like a simple swap from md5 to sha1 isn't immediately acceptable because it will break users who are depending on the current names of the files. In the interim, my team will be using the following script to work-around the issue in our python project:

#!/usr/bin/env bash

################################################################################
# This script is a workaround for a bug in the `constructs` library which
# causes the operator to fail to boot in a FIPS cluster. It is intended to be
# used during the build process inside the docker container to patch the
# version of the constructs javascript library that gets packaged along with
# the python package.
#
# https://github.com/aws/constructs/issues/272
################################################################################

# Find the tarball for constructs
cd /usr/local/lib/python3.6/site-packages
constructs_tar=$(find . -name "constructs*.tgz")

# Patch the constructs library tarball
tar_lib=$(dirname "$constructs_tar")
tar_name=$(basename "$constructs_tar")
cd "$tar_lib"
mkdir tmp
cd tmp
tar -xzvf "../$tar_name"
pushd package/lib/private/
sed -i'' 's/md5/sha1/g' uniqueid.js
popd
tar -czvf "$tar_name" package
mv "$tar_name" ..
cd ..
rm -rf tmp

@mergify mergify bot closed this as completed in #314 Oct 26, 2020
mergify bot pushed a commit that referenced this issue Oct 26, 2020
The `uniqueId` property has a few issues:

1. It uses MD5 which is not FIPS-compliant (fixes #272)
2. It has a variable length of up to 255 chars which may not be suitable in certain domains (e.g. k8s label names are limited to 63 characters).
3. The human-readable portion of `uniqueId` is sometimes confusing and in many cases does not offer sufficient value.
4. Its charset might be restrictive or too broad for certain domains.

To that end, we introduce `node.addr` as an alternative to `uniqueId`. It returns a 42 character hexadecimal, lowercase and opaque address for a construct which is unique within the tree (app). The address is calculated using SHA-1 which is FIPS-compliant.

For example:

    c83a2846e506bcc5f10682b564084bca2d275709ee

Similar to `uniqueId`, `addr` intentionally skips any path components called `Default` to allow refactoring of construct trees which preserve names within the tree.


*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant