Skip to content

Conversation

@rvagg
Copy link
Member

@rvagg rvagg commented Oct 30, 2016

This isn't strictly necessary but it's the remaining piece of FIPS code that is exposed in a build that hasn't been compiled with FIPS enabled. So, it's for consistency sake, mainly.

/R=@nodejs/crypto

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 30, 2016
@rvagg
Copy link
Member Author

rvagg commented Oct 30, 2016

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

FIPS_mode() is always available, whether FIPS is enabled or not, so this could be simplified to just args.GetReturnValue().Set(FIPS_mode()) - no compile- or run-time branches.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

There is no need in guarding FIPS_mode(), it works correctly. I don't mind adding a test, though.

@rvagg
Copy link
Member Author

rvagg commented Oct 30, 2016

I had an ulterior motive here regarding alt-ssl libraries but that's not an immediate concern so I'll close this for now and revisit later since it doesn't pass the sniff test as a standalone change.

@rvagg rvagg closed this Oct 30, 2016
@rvagg rvagg deleted the no-more-fips-for-you branch October 30, 2016 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants