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 a comment to indicate ineffective macro #17484

Closed
wants to merge 1 commit into from
Closed

Add a comment to indicate ineffective macro #17484

wants to merge 1 commit into from

Conversation

sshedi
Copy link
Contributor

@sshedi sshedi commented Jan 12, 2022

Signed-off-by: Shreenidhi Shedi sshedi@vmware.com

Checklist
  • documentation is added or updated
  • tests are added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 12, 2022
@sshedi
Copy link
Contributor Author

sshedi commented Jan 12, 2022

Hi @DDvO / @mattcaswell / @t8m,

Is there any way I can sign the CLA online? I don't have a printer with me to scan and sign the pdf doc. Please assist.

@mattcaswell
Copy link
Member

if OSSL_LIB_CTX not given EVP_MD_fetch(...) always returns NULL for md5
when fips enabled.

The FIPS provider does not have an MD5 implementation - so it is entirely correct behaviour for EVP_MD_fetch() to fail to find MD5 if that is the only provider configured for use.

This does not look like a bug to me.

@sshedi
Copy link
Contributor Author

sshedi commented Jan 12, 2022

Then what is the use of "-fips" or "fips=no" parameter?
Refer: #10129

@sshedi
Copy link
Contributor Author

sshedi commented Jan 12, 2022

I'm a beginner with openssl code base but my question is how does md5 work when I pass the OSSL_LIB_CTX parameter? It works fine when fips is enabled.

@mattcaswell
Copy link
Member

Then what is the use of "-fips" or "fips=no" parameter?

This modifies the default property selection string to remove any "fips=yes" requirement. Note what I said:

it is entirely correct behaviour for EVP_MD_fetch() to fail to find MD5 if that is the only provider configured for use.

If the FIPS provider is the only one configured then "-fips" has no effect at all. We will still only consider algorithms from the FIPS provider and hence an attempt to fetch md5 is expected to fail.

Another perfectly valid configuration is to have both the FIPS and the default providers loaded at the same time. In that case you may well want to use the "fips=yes" default property to force most fetches to only consider the FIPS provider and ignore the default provider. In that case a fetch such as this one which uses the "-fips" property will allow the fetch to consider all loaded providers (including the default provider) and hence the fetch will work.

@mattcaswell
Copy link
Member

how does md5 work when I pass the OSSL_LIB_CTX parameter

In this case you are using a non-default libctx so any configuration you may have applied to make the default libctx only use the fips provider is not being used. The new libctx has no providers loaded at all when the fetch function is called. If a fetch is attempted and no providers have been loaded then OpenSSL will automatically load the default provider for you. So by making the change you have, you are simply avoiding the default configuration.

@mattcaswell
Copy link
Member

I recommend reading this man page:

https://www.openssl.org/docs/man3.0/man7/fips_module.html

@sshedi
Copy link
Contributor Author

sshedi commented Jan 12, 2022

Thanks for the info and pointers. I have one more query, how to use

# define EVP_MD_CTX_FLAG_NON_FIPS_ALLOW 0x0008/* Allow use of non FIPS
in openssl-3.x?

And why is this macro still kept even though it's unused.

@mattcaswell
Copy link
Member

And why is this macro still kept even though it's unused.

That macro does nothing in 3.0 so you cannot use it. It is a carry over from 1.0.x where it used to mean something. It probably should have been removed from 3.0 but got forgotten about.

@sshedi
Copy link
Contributor Author

sshedi commented Jan 12, 2022

Is it possible to achieve the same behaviour in 3.0? Is it possible?

I will at least modify this PR to remove that macro :)

@sshedi sshedi changed the title Fix: Without OSSL_LIB_CTX md5 does not work Remove EVP_MD_CTX_FLAG_NON_FIPS_ALLOW macro Jan 12, 2022
@mattcaswell
Copy link
Member

Is it possible to achieve the same behaviour in 3.0? Is it possible?

OpenSSL 3.0 is much more capable in this regards than the old module. With the old module you were either in FIPS mode or you weren't. The flag EVP_MD_CTX_FLAG_NON_FIPS_ALLOW allowed you to work around the restrictions of FIPS for some situations. In 3.0 it is perfectly possible to have an application use some algorithms from the FIPS provider and other algorithms from other providers and so the flag is not really relevant. Read the section "Loading the FIPS module at the same time as other providers" in the man page link I sent you for more information on this.

I will at least modify this PR to remove that macro :)

Unfortunately it is too late to remove it now. We don't remove macros (even unused ones) from headers in a stable branch as a matter of policy.

@mattcaswell
Copy link
Member

in a stable branch

Or actually even in the master branch where master is targeting a minor release. Minor releases must maintain source and binary compatibility with the previous release. Removal of a macro is not source compatible and only suitable when we are targeting a major release next.

@t8m
Copy link
Member

t8m commented Jan 12, 2022

Instead of removing the macro add a comment that the macro has no effect.

include/openssl/evp.h Outdated Show resolved Hide resolved
@sshedi
Copy link
Contributor Author

sshedi commented Jan 12, 2022

Instead of removing the macro add a comment that the macro has no effect.

Done. Can you help me to sign the CLA? or shall I mark it as trivial fix?

@t8m t8m added branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch triaged: documentation The issue/pr deals with documentation (errors) labels Jan 12, 2022
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

This would be acceptable under CLA: trivial - If you want please modify the commit to add CLA: trivial on a separate line in the commit message.

include/openssl/evp.h Outdated Show resolved Hide resolved
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 12, 2022
EVP_MD_CTX_FLAG_NON_FIPS_ALLOW macro is obsolete and unused from
openssl-3.0 onwards

CLA: trivial

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>
@sshedi sshedi changed the title Remove EVP_MD_CTX_FLAG_NON_FIPS_ALLOW macro Add a comment to indicate ineffective macro Jan 12, 2022
@sshedi
Copy link
Contributor Author

sshedi commented Jan 12, 2022

Thanks @t8m

@t8m t8m added approval: review pending This pull request needs review by a committer cla: trivial One of the commits is marked as 'CLA: trivial' labels Jan 12, 2022
@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 12, 2022
@mattcaswell
Copy link
Member

I agree this is trivial.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@mattcaswell
Copy link
Member

but as this PR has been updated in that time

No idea what the bot is on about. This looks ok to me - no updates were made.

@mattcaswell mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 14, 2022
@mattcaswell
Copy link
Member

Pushed to master and 3.0

openssl-machine pushed a commit that referenced this pull request Jan 14, 2022
EVP_MD_CTX_FLAG_NON_FIPS_ALLOW macro is obsolete and unused from
openssl-3.0 onwards

CLA: trivial

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #17484)
openssl-machine pushed a commit that referenced this pull request Jan 14, 2022
EVP_MD_CTX_FLAG_NON_FIPS_ALLOW macro is obsolete and unused from
openssl-3.0 onwards

CLA: trivial

Signed-off-by: Shreenidhi Shedi <sshedi@vmware.com>

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #17484)

(cherry picked from commit 79704a8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch cla: trivial One of the commits is marked as 'CLA: trivial' triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants