Add X509 related libctx changes. by slontis · Pull Request #12153 · openssl/openssl

slontis · 2020-06-15T07:43:50Z

In order to not add many X509_XXXX_with_libctx() functions the libctx and propq may be stored in the X509 object via a call to X509_new_with_libctx().
Loading via PEM_read_bio_X509() or d2i_X509() should pass in a created cert using X509_new_with_libctx().
Renamed some XXXX_ex() to XXX_with_libctx() for X509 API's.
Removed the extra parameters in check_purpose..
X509_digest() has been modified so that it expects a const EVP_MD object() and then internally it does the fetch when it needs to (via ASN1_item_digest_with_libctx()).
Added API's that set the libctx when they load such as X509_STORE_new_with_libctx() so that the cert chains can be verified.

Checklist

documentation is added or updated
tests are added or updated

slontis · 2020-06-15T07:46:22Z

The size of PR #11884 is getting quite large - so I have tried to separate out some bits into smaller PR's to make it easier to review.

crypto/x509/x_all.c

doc/man3/X509_STORE_new.pod

crypto/asn1/a_digest.c

ssl/s3_lib.c

include/openssl/x509v3.h

ssl/statem/statem_clnt.c

ssl/statem/statem_srvr.c

doc/man3/X509_set0_libctx.pod

crypto/x509/x509_lu.c

doc/man3/X509_STORE_new.pod

ssl/s3_lib.c

slontis · 2020-06-26T09:53:47Z

I have temporarily merged in @levitte code for OPENSSL_CTX_set0_default().
The commit message needs changing and docs need to be added (once we are happy with the API's that have been added).

X509_set0_libctx() has been replaced by X509_new_with_libctx() (at least in this PR).
The new is important for some of the X509 read functions...
eg..

    cert = X509_new_with_libctx(libctx, NULL);

    PEM_read_bio_X509(in, &cert, ...);
       OR
    d2i_X509(&cert, in, len);

NOTE that d2i_X509() calls X509v3_cache_extensions if the cert object is passed in.

The ssl tests actually passed!!

slontis · 2020-06-29T01:05:09Z

Rebased and fixed up documentation.
ping

apps/storeutl.c

crypto/x509/by_file.c

crypto/x509/x509_local.h

include/openssl/x509v3.h

doc/man3/X509_sign.pod

test/sslapitest.c

slontis · 2020-07-22T05:22:33Z

@mattcaswell - Some X509 functionality has changed in master which caused an unpleasant rebase.
I have collapsed the reviewed code into the first commit.
The remaining fixup commits are new to address the comments and rebase to master.

include/openssl/store.h

mattcaswell · 2020-07-22T10:16:30Z

Travis failures are relevant.

apps/lib/apps.c

crypto/x509/x509_vfy.c

crypto/x509/x509_local.h

slontis · 2020-07-22T14:14:45Z

Fixed memory leak..
Addressed comments related to store open/attach/load.

include/openssl/store.h

levitte · 2020-07-22T15:31:00Z

include/openssl/store.h

Source code style wise, I believe this isn't OK, as parenthesised things should be aligned with the left parenthesis.

However, I see nothing wrong with this, and I believe it fits our coding style:

OSSL_STORE_CTX *OSSL_STORE_open_with_libctx (const char *uri, const UI_METHOD *ui_method, void *ui_data, OSSL_STORE_post_process_info_fn post_process, void *post_process_data, OPENSSL_CTX *libctx, const char *propq);

Err.. this style is done all over the place and is (arguably) more readable than the proposed alternative.

Just as this is more readable (also arguably)

if (....) { }

@levitte are you going to block on either of the above two comments?

I have no idea how you think a function declaration compares to an if statement. They are completely different in my mind (apart from using parentheses in their respective syntax)

I feel strongly about the < name, libctx, propq > tuple staying together. The code style issue, less so, but I am worried that we stray more and more from the style guide, and re-create the mess we had before.

(the coding style needs an update, and there's been several proposals, but not final decisions...)

I have no idea how you think a function declaration compares to an if statement.

As far as I am aware the reason for this format

if () { }

Is related to scan-ability of the line.
So I am not sure why you think this can't also apply to () when it needs to span multiple lines.

I have complied in this PR.. It doesn't mean I have to agree :)

slontis · 2020-07-22T22:40:18Z

Tests pass now - look like it is getting close to a final review.

- In order to not add many X509_XXXX_with_libctx() functions the libctx and propq may be stored in the X509 object via a call to X509_new_with_libctx(). - Loading via PEM_read_bio_X509() or d2i_X509() should pass in a created cert using X509_new_with_libctx(). - Renamed some XXXX_ex() to XXX_with_libctx() for X509 API's. - Removed the extra parameters in check_purpose.. - X509_digest() has been modified so that it expects a const EVP_MD object() and then internally it does the fetch when it needs to (via ASN1_item_digest_with_libctx()). - Added API's that set the libctx when they load such as X509_STORE_new_with_libctx() so that the cert chains can be verified.

slontis · 2020-07-23T08:20:08Z

Had to rebase and apply a small change now that #11948 has been merged (which added the env variable to set the app_lib_ctx.

mattcaswell

All my comments seem to have been addressed and CIs are green. Approving this subject to @levitte's outstanding issues being resolved.

slontis · 2020-07-23T11:04:13Z

Addressed @levitte's comments.

levitte

Not quite there yet...

crypto/store/loader_file.c

include/openssl/store.h

slontis · 2020-07-24T00:26:13Z

NOTE that

make update

does not like

void func
   (......
    .......)
{
    ...
    OSSL_STOREerr(0, ERR_R_MALLOC_FAILURE);
    ...
}

I assume it was looking for the '(' on the same line to indicate a function.. So there is still one in store_lib.c
that is doing

void func(
    ....
    ....)

slontis · 2020-07-24T00:28:22Z

Also removed the dead functions from include/crypto/store.h

slontis · 2020-07-24T06:58:32Z

@levitte function signatures changed now.. Will merge in a few hours in there are no more objections.

include/openssl/store.h

levitte

Ok!

mattcaswell

Reconfirm

- In order to not add many X509_XXXX_with_libctx() functions the libctx and propq may be stored in the X509 object via a call to X509_new_with_libctx(). - Loading via PEM_read_bio_X509() or d2i_X509() should pass in a created cert using X509_new_with_libctx(). - Renamed some XXXX_ex() to XXX_with_libctx() for X509 API's. - Removed the extra parameters in check_purpose.. - X509_digest() has been modified so that it expects a const EVP_MD object() and then internally it does the fetch when it needs to (via ASN1_item_digest_with_libctx()). - Added API's that set the libctx when they load such as X509_STORE_new_with_libctx() so that the cert chains can be verified. Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #12153)

slontis · 2020-07-24T12:59:45Z

Thanks for reviewing. Merged to master.

- In order to not add many X509_XXXX_with_libctx() functions the libctx and propq may be stored in the X509 object via a call to X509_new_with_libctx(). - Loading via PEM_read_bio_X509() or d2i_X509() should pass in a created cert using X509_new_with_libctx(). - Renamed some XXXX_ex() to XXX_with_libctx() for X509 API's. - Removed the extra parameters in check_purpose.. - X509_digest() has been modified so that it expects a const EVP_MD object() and then internally it does the fetch when it needs to (via ASN1_item_digest_with_libctx()). - Added API's that set the libctx when they load such as X509_STORE_new_with_libctx() so that the cert chains can be verified. Reviewed-by: Richard Levitte <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from openssl#12153)

slontis added approval: review pending This pull request needs review by a committer branch: master Applies to master branch labels Jun 15, 2020

slontis mentioned this pull request Jun 15, 2020

Add libctx support to CMS/PKCS7 #11884

Closed

2 tasks

mattcaswell requested changes Jun 15, 2020

View reviewed changes

mattcaswell requested changes Jun 16, 2020

View reviewed changes

doc/man3/X509_set0_libctx.pod Outdated Show resolved Hide resolved

crypto/x509/x509_lu.c Outdated Show resolved Hide resolved

doc/man3/X509_STORE_new.pod Outdated Show resolved Hide resolved

ssl/s3_lib.c Outdated Show resolved Hide resolved

slontis changed the title ~~X509 setlibctx~~ Add X509 setlibctx Jun 17, 2020

slontis force-pushed the x509_setlibctx branch from 06d0011 to 2a4f939 Compare June 26, 2020 09:39

slontis mentioned this pull request Jun 27, 2020

Add OPENSSL_CTX to CMP #11808

Closed

2 tasks

slontis force-pushed the x509_setlibctx branch from 2a4f939 to a1c4974 Compare June 29, 2020 01:01

slontis changed the title ~~Add X509 setlibctx~~ Add X509 related libctx changes. Jun 29, 2020

mattcaswell requested changes Jul 21, 2020

View reviewed changes

slontis force-pushed the x509_setlibctx branch from c4994ef to fb4bffa Compare July 22, 2020 05:11

levitte reviewed Jul 22, 2020

View reviewed changes

include/openssl/store.h Outdated Show resolved Hide resolved

levitte reviewed Jul 22, 2020

View reviewed changes

include/openssl/store.h Outdated Show resolved Hide resolved

mattcaswell requested changes Jul 22, 2020

View reviewed changes

apps/lib/apps.c Outdated Show resolved Hide resolved

crypto/x509/x509_vfy.c Outdated Show resolved Hide resolved

crypto/x509/x509_local.h Outdated Show resolved Hide resolved

levitte reviewed Jul 22, 2020

View reviewed changes

include/openssl/store.h Outdated Show resolved Hide resolved

levitte reviewed Jul 22, 2020

View reviewed changes

slontis added 6 commits July 23, 2020 17:57

fixup! Add X509 related libctx changes.

af125ae

fixup! Add X509 related libctx changes.

e45658a

fixup! Add generated files.

1cf63d2

fixup! Add X509 related libctx changes.

8bfb24d

fixup! Add X509 related libctx changes.

63c4337

mattcaswell approved these changes Jul 23, 2020

View reviewed changes

fixup! Add X509 related libctx changes.

e685383

levitte requested changes Jul 23, 2020

View reviewed changes

crypto/store/loader_file.c Outdated Show resolved Hide resolved

include/openssl/store.h Outdated Show resolved Hide resolved

fixup! Add X509 related libctx changes.

c287de4

slontis 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 Jul 24, 2020