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

Issue 19704 get default search path #19752

Closed

Conversation

pm-cfs
Copy link
Contributor

@pm-cfs pm-cfs commented Nov 23, 2022

Checklist
  • documentation is added or updated
  • tests are added or updated
  • CLA is signed
Description of change

Update core provider to add:

  • OSSL_PROVIDER_get1_default_search_path
  • Unit test to verify that set/get work as expected in library contexts

Fixes #19704

@@ -816,6 +816,23 @@ int OSSL_PROVIDER_set_default_search_path(OSSL_LIB_CTX *libctx,
return 0;
}

int OSSL_PROVIDER_get1_default_search_path(OSSL_LIB_CTX *libctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a random thought but would OSSL_PROVIDER_get0_default_search_path be more sensible? The caller gets to decide to strdup or not -- the path exists for the life of the libctx, so not duping it is reasonable.

const char *OSSL_PROVIDER_get0_default_search_path(OSSL_LIB_CTX *libctx)
{
    char *path = NULL;

    if ((store = get_provider_store(libctx)) != NULL
            && CRYPTO_THREAD_read_lock(store->default_path_lock)) {
        path = OPENSSL_strdup(store->default_path ? store->default_path : "");
        CRYPTO_THREAD_unlock(store->default_path_lock);
    }
    return path;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is always an option; in that case, I would just do:

path = store->default_path;

You do get less error info that way; if the lock failed for some reason or the library context was invalid which is why I went the other route.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, didn't look at the code properly :(

An invalid library context is likely to crash. The lock failing is a legitimate concern I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think an invalid library context should return NULL on get_provider_store (it calls ossl_lib_ctx_get_data which will also fail gracefully).

Copy link
Contributor

Choose a reason for hiding this comment

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

or it will crash or return rubbish. C has no way to distinguish a good pointer from a bad one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, switched to OSSL_PROVIDER_get0_default_search_path

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Nov 23, 2022
@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature tests: present The PR has suitable tests present labels Nov 24, 2022
@t8m
Copy link
Member

t8m commented Nov 24, 2022

I'd also prefer implementing get0 accessor instead. I would definitely be ok with adding it to 3.1. In that case the libcrypto.num and documentation HISTORY entry has to change.

Not sure about 3.0. I do not see it as bug fix.

@t8m t8m added the branch: 3.1 Merge to openssl-3.1 label Nov 24, 2022
@t8m
Copy link
Member

t8m commented Dec 1, 2022

Instead of adding merge commits please rebase the whole PR against fresh master branch.

@pm-cfs pm-cfs force-pushed the Issue-19704_get-default-search-path branch from b5eefaf to 6c7d6a9 Compare December 1, 2022 18:24
@pm-cfs
Copy link
Contributor Author

pm-cfs commented Dec 1, 2022

Ok, rebased to master and squashed to simplify commit history.

@t8m
Copy link
Member

t8m commented Dec 1, 2022

pyca-cryptography

The pyca-cryptography submodule change has to be reverted.

@pm-cfs
Copy link
Contributor Author

pm-cfs commented Dec 1, 2022

For the pyra-cryptography, do I just ignore those / Discard? Git keeps adding it in as a change from master.

@pm-cfs pm-cfs force-pushed the Issue-19704_get-default-search-path branch from 0e13313 to 8f1d650 Compare December 1, 2022 19:18
@t8m t8m added approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member labels Dec 2, 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.

The original commit message was lost. Could you please amend the first commit message and possibly squash the two commits into one? You can use git rebase -i for that.

doc/man3/OSSL_PROVIDER.pod Outdated Show resolved Hide resolved
@pm-cfs pm-cfs force-pushed the Issue-19704_get-default-search-path branch from 6224f28 to e12d6b0 Compare December 2, 2022 16:07
@pm-cfs pm-cfs requested a review from t8m December 3, 2022 07:44
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Dec 5, 2022
@t8m t8m requested review from paulidale, slontis and a team December 5, 2022 10:38
Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

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

LGTM

@tmshort tmshort 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 Dec 5, 2022
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Dec 6, 2022
@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Dec 6, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m
Copy link
Member

t8m commented Dec 6, 2022

As is this can be merged only to master branch. So I did that.

@pm-cfs if you want this in 3.1 would you please submit a PR against master branch that changes the version in the manpage and libcrypto.num and separate PR against openssl-3.1 branch that cherry-picks this merged commit?

@t8m t8m closed this Dec 6, 2022
openssl-machine pushed a commit that referenced this pull request Dec 6, 2022
Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #19752)
beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022
Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#19752)
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.1 Merge to openssl-3.1 severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to get default search path
6 participants