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.

Sign up for GitHub

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

Jump to bottom

Fix fips compliance 272 #273

Closed
wants to merge 2 commits into from
Closed

Fix fips compliance 272 #273

wants to merge 2 commits into from

Conversation

gabe-l-hart
Copy link

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

This fixes #272

I have verified that when used in a FIPS enabled cluster, we no longer see the reported issue.

@gabe-l-hart gabe-l-hart mentioned this pull request Oct 7, 2020
Closed
eladb
eladb reviewed Oct 7, 2020
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

LGTM, @iliapolo what do you think?

eladb
eladb suggested changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

I take my "LGTM" back... we won't be able to change how uniqueId calculates hashes because it will break all CDK code that uses this property to create stable names.

As part of #250, I think we need to basically soft-deprecate uniqueId in favor of something more ubiquitous and then we can use a different hash algorithm.

@gabe-l-hart
Copy link
Author

@eladb Interesting, I was afraid that might be the case. Ultimately, I think this is an issue where what we want is the md5 hash algorithm but we want it for a non-crypto usecase so it should be acceptable for FIPS. I'll look a bit more to see if there is another non-crypto md5 implementation that we could be using instead.

@gabe-l-hart
Copy link
Author

gabe-l-hart commented Oct 7, 2020 •
edited
Loading

So, a couple of options come to mind for me:

  1. We could use a pure-js implementation of md5 like this one. That would keep the hashes consistent, but it would add a dependency to constructs which currently has none

  2. We could add an env-var check in uniqueId and use sha1 if it indicated FIPS-enablement

  3. My team could maintain a fork as needed until Node.of(this).uniqueId is bound to CloudFormation's logical ID rules #250 is handled and reassess at that point

Thoughts?

@gabe-l-hart gabe-l-hart mentioned this pull request Oct 7, 2020
Closed
@eladb
Copy link
Contributor

eladb commented Oct 7, 2020

@rix0rrr @RomainMuller @njlynch Interested on your take here

@gabe-l-hart
Copy link
Author

I noted it in the issue, but we've got a workaround script to unblock our team on this, so feel free to discuss as needed. Whatever fix feels right to the team should be fine with us once it's ready, and I'll be happy to verify it in our FIPS environment.

eladb pushed a commit that referenced this pull request Oct 25, 2020
feat: node.addr (deprecates node.uniqueId)
4c1310d 
The `uniqueId` property has a few issues:

1. It uses MD5 which is not FIPS-compliant (fixes #273)
2. They have 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. Their 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` (case insensitive) to allow refactoring of construct trees which preserve names within the tree.
@eladb eladb mentioned this pull request Oct 25, 2020
Merged
@eladb
Copy link
Contributor

eladb commented Oct 25, 2020

Superseded by #314

@eladb eladb closed this Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladb eladb eladb requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

crypto.createHash('md5') fails in a FIPS-enabled environment
2 participants
@gabe-l-hart @eladb