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

Make use of hashlib.md5() FIPS compliant #6982

Merged
merged 1 commit into from
Feb 15, 2023
Merged

Conversation

nielspardon
Copy link
Contributor

resolves #6900

Description

  • replaces all direct usages of hashlib.md5() with calls to dbt.utils.md5()
  • sets usedforsecurity=False when calling hashlib.md5()

Previously when running make test on a FIPS enabled system there were many test failures. With the changes of this PR both make test and make integration run through without errors on a FIPS enabled system.

Checklist

@nielspardon nielspardon requested review from a team as code owners February 15, 2023 11:35
@cla-bot cla-bot bot added the cla:yes label Feb 15, 2023
@jtcohen6 jtcohen6 added Team:Language ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Feb 15, 2023
@gshank gshank self-requested a review February 15, 2023 14:37
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

We're getting a bunch of:

    def md5(string, charset="utf-8"):
>       return hashlib.md5(string.encode(charset), usedforsecurity=False).hexdigest()
E       TypeError: openssl_md5() takes no keyword arguments

@gshank
Copy link
Contributor

gshank commented Feb 15, 2023

It looks like that keyword parameter is only supported for Python 3.9 and later, so we'll have to check the python version when making those md5 calls.

@nielspardon
Copy link
Contributor Author

It looks like that keyword parameter is only supported for Python 3.9 and later, so we'll have to check the python version when making those md5 calls.

Since I'm not a big Python expert. Do you have a suggestion / pattern for such cases already that I can use? Like checking the Python version and then only supplying the parameter if we run on >3.9?

@nielspardon
Copy link
Contributor Author

I'm guessing something like this should do the trick?

def md5(string, charset="utf-8"):
    if sys.version_info >= (3, 9):
        return hashlib.md5(string.encode(charset), usedforsecurity=False).hexdigest()
    else:
        return hashlib.md5(string.encode(charset)).hexdigest()

@gshank
Copy link
Contributor

gshank commented Feb 15, 2023

Yes, that's the right way to check python versions.

@nielspardon
Copy link
Contributor Author

I updated the PR. Since the usedforsecurity parameter was only added in Python 3.9 one can not run dbt on Python <3.9 with FIPS mode enabled.

I verified on a test system that tests are running fine with Python 3.8 when FIPS is deactivated with the if statement.

@gshank gshank merged commit 3cee9d1 into dbt-labs:main Feb 15, 2023
@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 16, 2023

Backporting for inclusion in next v1.4 patch (probably v1.4.3), per reasonable request: #6900 (comment)

We shouldn't actually merge the backport PR until final release of v1.4.2 next week.

@nielspardon nielspardon deleted the par-fips branch February 19, 2023 09:22
sethugupta pushed a commit to sethugupta/dbt-core that referenced this pull request Feb 20, 2023
nielspardon added a commit to nielspardon/dbt-core that referenced this pull request Feb 27, 2023
jtcohen6 pushed a commit that referenced this pull request Feb 28, 2023
nielspardon added a commit to nielspardon/dbt-core that referenced this pull request Apr 4, 2023
jtcohen6 added a commit that referenced this pull request Apr 4, 2023
Signed-off-by: Niels Pardon <[email protected]>
Co-authored-by: Niels Pardon <[email protected]>
Co-authored-by: leahwicz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.4.latest cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-2062] [Bug] Dbt doesn't run when FIPS mode is enabled
3 participants