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

add toggle to use fips #806

Closed
wants to merge 1 commit into from
Closed

Conversation

iamkrillin
Copy link

Pulled the code from @mcclouden on issue #190. Added a toggle to BaseClient to enable/disable fips mode

@sunilvijendra
Copy link

Any plan to merge the FIPS toggle changes?

@A9G-Data-Droid
Copy link

I have tested this and it works!

@daniellangemann
Copy link

I'll add my vote - any plans to merge the FIPS toggle changes? This seems like a safe and straightforward solution.

@drieseng
Copy link
Member

drieseng commented Mar 1, 2023

I'd prefer going for a builder pattern where the "user" explicitly adds whatever algorithms he/she wants.

@epatrick
Copy link

Any update on this PR? We've run into conflicts with Windows FIPS turned on as part of AWS' current STIG for Windows 2022 boxes.

@A9G-Data-Droid
Copy link

@epatrick My develop branch works on FIPS systems: https://github.com/A9G-Data-Droid/SSH.NET

This library hasn't seen an update since Jan 24, 2021 so I'm not waiting around for the next version to support it.

@mari3728
Copy link

Will this PR be included anytime soon?

@Rob-Hague
Copy link
Collaborator

It should not be necessary. AFAIK, on .NET Framework the Create methods already return a CryptoServiceProvider instance, and .NET Core forwards implementations to the OS through which FIPS is controlled (.NET Core Federal Information Processing Standard (FIPS) compliance). CryptoServiceProvider and other derived cryptographic types are obsolete (SYSLIB0021).

@WojciechNagorski
Copy link
Collaborator

@Rob-Hague So when we release a new version supporting .NET 7, could we close this PR?

@Rob-Hague
Copy link
Collaborator

Rob-Hague commented Sep 12, 2023

There is maybe still a need to disable MD5, but the rest of the changes (CryptoServiceProvider etc.) should not be needed.

Disclaimer: not an expert

@mari3728
Copy link

mari3728 commented Sep 12, 2023

on .NET Framework the Create methods already return a CryptoServiceProvider instance

I see, and agree for SHA implementations.
From my understanding, on .NET Framework MD5CryptoServiceProvider does not use an alternative implementation if the FIPS policy is enabled, and it uses clr.dll underneath (calling with QCall), which is not validated by NIST for FIPS compliance. Plus, the algorithm itself is not permitted.

So this:

There is still a need to disable MD5, but the rest of the changes (CryptoServiceProvider etc.) should not be needed.

is true from my understanding.

Should there be a PR to treat MD5 only then?

Disclaimer: also not an expert, just trying to understand FIPS enforcement for .NET (and going insane haha)

@Rob-Hague
Copy link
Collaborator

Ok I spoke too assertively and you may replace my comments with "I don't know" but what you have said makes sense, and my answer to

Should there be a PR to treat MD5 only then?

is "I guess", but I'm not sure how easy it will be to test

@A9G-Data-Droid
Copy link

In relation to this PR, you could eliminate both the bool UseFIPS and the MD5 methods entirely. That would make it work both in and outside of a FIPS mode OS. Some sort of PR is still required. You should be seeing those warnings about the MD5 methods being obsolete. We need to do something to remove them.

@A9G-Data-Droid
Copy link

This PR can be closed as the latest version is working on FIPS enabled computers. I think this was closed by #1293

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants