Allow PKCS12 export to set arbitrary bag attributes

[grahamwoodward]

Checklist

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

[grahamwoodward]

At this stage I want to make sure this is the right approach and I'm using the correct APIs

[grahamwoodward]

sigh a lot of failing checks, great start

[levicki]

@grahamwoodward Some of the errors seem to come from your code not adhering to C90 standard.

Make sure you read this if you haven't already:

https://www.openssl.org/policies/technical/coding-style.html

[grahamwoodward]

yeah just working through them

[grahamwoodward]

Can anyone tell me why the git diff is failing" I'm not seeing the issue

Also when I ran "make test" locally they passed but there still another memory leak

[mattcaswell]

So this adds the attribute by default for anyone that has a trust setting on the cert supplied in the param. I'm not sure we want that? There may be trust settings on a cert because it has OpenSSL trust markings. I'm not sure we necessarily want to automatically publish that as a jdk trust marking?

I was expecting that the pkcs12 app would be responsible for adding this attribute. Although looking at how that app creates a PKCS12 object its not immediately clear how to do that. I would have expected the pkcs12 app to build up the set of PKCS12_SAFEBAG objects (adding the attribute if appropriate) and then create the PKCS12 object from that. However, although there are functions to manipulate PKCS12_SAFEBAGs, there doesn't seem to be an easy way to create a PKCS12 object from them. Are there missing API functions here?

[levicki]

Very good question, I agree that it should not add by default unless there is an option set in the openssl.cnf.

[mattcaswell]

Very good question, I agree that it should not add by default unless there is an option set in the openssl.cnf.

Right - and this change is currently changing the behaviour of the public API function PKCS12_add_cert in libcrypto. So it would impact any 3rd party application using libcrypto that happens to call that function. I would not expect such a function itself to be affected by openssl.cnf. The pkcs12 app should be affected by it - and only add the jdk attribute if the pkcs12 app entry in the config file says so.

[levicki]

@mattcaswell I think the answer is simple -- is there other public API in libcrypto that is affected by openssl.cnf? If not, then this one should not be affected either.

But, we may consider introducing some public API that would allow adding bag attributes programmatically too.

[mattcaswell]

There are a handful. Mostly around provider and engine management. There are also some which affect libssl. But typically public API functions are not impacted by openssl.cnf.

Yes, we could add new API to enable adding the attributes.

[grahamwoodward]

also @mattcaswell what are you envisaging is being passed in the void *cbarg? As far as I can see I only need the bag, which we're passing as the first arg to the callback. In my cb I'm doing all the work to read the config and set the attr (although not quite working, see my comment below)

[mattcaswell]

The cbarg is application defined and is passed back to the callback. It can be useful for passing contextual information from the application back to its own callback. This is something we do for most of our callbacks. Your particular implementation might not need it, but I think it is still good to have it.

storing the callback within the p12_crt.c as a global

I don't think this is a good idea. Globals should be avoided. I think we either have to live with the excessively long param list to a new function PKCS12_create_ex2 - or we have to do the refactor and split the API up (adding a context variable). I don't see a middle ground at the moment.

[levicki]

@grahamwoodward

I'm simply (and maybe this is wrong?) storing the callback within the p12_crt.c as a global. Then the places within PKCS12_create_ex in p12_crt which call PKCS12_add_cert() then check whether this callback isn't NULL and call it. The function itself is defined within the app.

That doesn't sound thread-safe at all.

What guarantee do you have that someone on a different thread won't call SetCallback(NULL) after you test your global point, but before your thread gets the chance to dereference the pointer and execute the code it points to?

For that to work reliably you would have to introduce a dynamically allocated context in which, among other things, callback function pointer would be stored. Said context would be returned to the caller and they would have to pass it back to you on successive calls to your API so you know you are calling the right callback for the right tread / process.

As @mattcaswell said, doing this right would creep out of scope of this PR by a large margin. Regardless, I believe proper design should be made for this and reviewed by the OTC, because on projects as big and as widely used as OpenSSL a quick hack to add a relatively minor feature is a recipe for disaster.

[levicki]

@grahamwoodward

also what are you envisaging is being passed in the void *cbarg?

The easiest way to imagine what *cbarg could be used for would be if your callback is set to a C++ class member function you could pass this pointer as cbarg so the member function can access private members of the class it belongs to.

It is the same pattern used when you create thread -- void *param is what gets passed to newly created thread function.

[grahamwoodward]

yeah I get the idea as to why, we do the same for the code I work on when we have async call backs we'll allow a context to be passed so the callback can access stuff etc...I just meant in the context of this actually PR what he was envisaging but as matt said, there's probably nothing for my specific case but future cases might need something passed

[mattcaswell]

Can anyone tell me why the git diff is failing" I'm not seeing the issue

It's related to the "make update" issue I mentioned above. Looks like you have hand edited an auto generated file.

[grahamwoodward]

@mattcaswell thanks for the comments, I'm not sure about your last two, the ones around me adding the trust when in some cases I don't need to. I'm not sure how to work out whether I need to add them

[FireBurn]

Should this be commented out?

[levicki]

@FireBurn My initial idea was to introduce this new config option commented out like this:

# Uncomment this if you need Java compatible PKCS12 files
[cb_attr]
#jdkTrustedKeyUsage = anyExtendedKeyUsage

So people who need it would uncomment it, and updating config wouldn't affect other users.

[grahamwoodward]

yeah mistakenly left it in after previous change when testing. I'll update commit once I have a better handle on @mattcaswell 's comments

[FireBurn]

I've compiled your tree locally, which allows me to create a pkcs12 truststore from pem files that works with Tomcat

I appreciate that the final code might be different, but this has proven really useful at my work

[grahamwoodward]

@FireBurn excellent that's great to hear.

[grahamwoodward]

@mattcaswell @levicki hey I'm trying to write the attribute via the existing X509at_add1_attr_by_NID from within my call back but getting an error at runtime and not sure why...

In my cb I'm opening the config, finding the revenant section and checking that the val->name matches jdkTrustedKeyUsage and if it does, I'm retrieving the existing attrs using attrs = PKCS12_SAFEBAG_get0_attrs(bag) and then calling
X509at_add1_attr_by_NID(&attrs, NID_oracle_jdk_trustedkeyusage, V_ASN1_OBJECT, val->value, strlen(val->value))

The errors I'm getting are
40D7C65BF57F0000:error:068000DE:asn1 encoding routines:asn1_template_ex_i2d:illegal zero content:crypto/asn1/tasn_enc.c:312:

40D7C65BF57F0000:error:068C0100:asn1 encoding routines:ASN1_item_dup:malloc failure:crypto/asn1/a_dup.c:81:

I can see from my debug that it's finding the relevant jdkTrustedKeyUsage anyExtendedKeyUsage from within the config file.

`/* Let's get the config section we are using */                                                                                                                                 
if ((cb_attr = NCONF_get_string(conf, "pkcs12", "certBagAttr")) != NULL) {
    if ((cb_sk = NCONF_get_section(conf, cb_attr)) != NULL) {
        for (i = 0; i < sk_CONF_VALUE_num(cb_sk); i++) {
            val = sk_CONF_VALUE_value(cb_sk, i);
            if (strcmp(val->name, "jdkTrustedKeyUsage") == 0) {
                X509at_add1_attr_by_NID(&attrs, NID_oracle_jdk_trustedkeyusage, V_ASN1_OBJECT, (const unsigned char*)val->value, 10);
            } 
       }
} else {....`

Also not sure I really need the loop? Do I need to worry about how many "entries" there might be under the [cb_attr] section?

[mattcaswell]

The final val->value and 10 params don't look correct. I don't think it is expecting a string here.

I'd suggest you create the attribute using X509_ATTRIBUTE_create() as you do in the existing patch. The final parameter to that function is expected to be an ASN1_OBJECT which you might create via OBJ_txt2obj(val->value, 0).

[grahamwoodward]

I accidentally closed this

[grahamwoodward]

The final val->value and 10 params don't look correct. I don't think it is expecting a string here.

I'd suggest you create the attribute using X509_ATTRIBUTE_create() as you do in the existing patch. The final parameter to that function is expected to be an ASN1_OBJECT which you might create via OBJ_txt2obj(val->value, 0).

Yeah I had this initially but couldn't get it to compile... it's compiling now

@mattcaswell
I think my code is "right" but it's not adding the attribute. I can see from my fprintf's that the attribute count on the bag has increased to 1 after adding the one I create and the PKCS12 app exits cleanly but when I view the resulting PKCS12 file, it shows no bag attributes and I'm not sure why. I wonder whether something within the pkcs12_create_ex2() is undoing what I'm doing? can't see how

[mattcaswell]

PKCS12 app exits cleanly but when I view the resulting PKCS12 file, it shows no bag attributes and I'm not sure why

Hmm. I don't see an obvious problem just from eye balling the code

We will need to add documentation and tests for this (in this PR) - but lets get the code working first and gain agreement that this is the right API.

Some other comments below.

[grahamwoodward]

I'm having issues with git now...I tried to commit some changes to my 2nd commit and push them but I got into a world of pain with my local branch of allow_arbitrary_bag_attrs being out of sync and I think I might have messed up the merge(s). Now when I try and push from my local allow_arbitrary_bag_attrs branch it tells me I need to git pull first, which appears to be trying to pull down the remote allow_arbitrary_bag_attrs from my GitHub repo, when I fix up the changes in my local and commit, it's creating a merge commit

I kind of feel like closing this PR and starting from scratch if that's possible?

[mattcaswell]

Is your local branch correct? i.e. the code looks correct and has all the right commits in it? If so use the "-f" flag to git push to force the push without pulling down first. This will overwrite the contents of your remote branch with your local one.

[grahamwoodward]

Is your local branch correct? i.e. the code looks correct and has all the right commits in it? If so use the "-f" flag to git push to force the push without pulling down first. This will overwrite the contents of your remote branch with your local one.

seems to be good with that cheers...now just the issue it's not actually adding the attr to the bag :(

update:
I think I see the issue...I call PKCS12_SAFEBAG_get0_attrs but there's none to get so the first arg to X509at_add1_attr is NULL, this API creates the new stackof attrs but I'm ignoring it, although looking at that code the internally created one is assigned to the null one passed in...but if I call PKCS12_SAFEBAG_get0_attrs again, it's NULL again

update 2:
success! I might be using the wrong API but I couldn't find how to stash the attrs back on the bag so I added a PKCS12_SAFEBAG_set_attrs within p12_attr.c and it now works...

@mattcaswell is this set_attrs() needed or is there some existing API I've missed?

[mattcaswell]

Still will need documentation and tests adding at some point

[mattcaswell]

All of this does make me wonder whether the config file is strictly necessary. Could we do all of this just on the basis of a command line arg? Or possibly both command line arg and config file (users can use whichever they prefer).

[grahamwoodward]

this was the original idea I believe, a new CLI argument but the convo moved to it becoming a config option

[levicki]

@mattcaswell

All of this does make me wonder whether the config file is strictly necessary. Could we do all of this just on the basis of a command line arg? Or possibly both command line arg and config file (users can use whichever they prefer).

Config file was my idea because that avoids hard-coding single purpose, vendor specific, command line switches (which was the original idea).

Both config and command line would be nice, but command line options should then follow the existing syntax for adding extensions by either specifying extensions to add or config file sections that contain them, not be a single purpose command line switch (because I am sure you wouldn't want proliferation of vendor specific switches for everything).

[mattcaswell]

So we haven't implemented my suggestion of a 0 return meaning don't add the bag after all. I still think that might be useful. But, if we don't do that, then probably the return values should be 1 for success and 0 for error.

[grahamwoodward]

I'm unsure about the "don't add the bag". In the case of the cert we create/add a bag, then add the friendly name/localkeyid to it before calling any registered call backs, and if someone's implementation of the cb fails in such a way as to not want the bag added, then we're discarding that name/localkeyid?

[levicki]

@mattcaswell I am confused now. Didn't you mean to add the callback that adds optional attributes to the bag rather than the callback to add the bag to PKCS12?

Or do we now have two callbacks?

[grahamwoodward]

Yeah the call back is to add optional attrs to the safe bag

[levicki]

So if the friendlyName and localKeyId are added to the bag before the callback via some other mechanism, then we should not discard the whole bag or even the existing attributes if adding optional attributes fails?

I agree that the callback should return success / failure, but I am not sure whether the new API code calling the callback should do anything with the callback return value other than stopping calling the callback on failure and returning.

[mattcaswell]

I see there being one callback. Its purpose is to do optional application processing on the bag. One such use is to add attributes to the bag (which is what this particular PR is going to be doing). It could be used for anything, e.g. to add friendly names to all the "other" certs if the application so wishes (not sure why you would). This PR is only using the callback to add optional attributes, but the public API allows an application to do whatever it likes.

Another use could be to do filtering on the certs that get exported. So the PKCS12_create_ex2() call could supply a large number of certificates and the callback could be used to do some filtering and determine only a subset to be actually exported (perhaps based on some local policy rules).

My suggestion is that in the event that the return value from cb is 0, then remove bag from bags. This would also remove any friendly name/keyid added to the bag...but that's ok.

[levicki]

@mattcaswell I am not sure I like that "multi-purpose callback" idea.

Also, what happens if the passed in bag is say a shrouded keybag, unless that's impossible?

If it is possible, does the callback even have enough info to process it (i.e. can it see inside or can it just see the attributes and what can it do if there aren't any)?

[mattcaswell]

what happens if the passed in bag is say a shrouded keybag, unless that's impossible?

The callback is invoked immediately after bag creation and prior to any encryption being applied. So, not possible.

[mattcaswell]

I am not sure I like that "multi-purpose callback" idea.

Whether we like it or not, the callback as currently in this PR is multi purpose. We cannot restrict what an application might do in that callback. Nor do I think we want to. The idea behind the callback is to make pkcs12 creation more flexible.

[grahamwoodward]

is there an existing API to remove a bag from bags? or should I add one in pk12_crt?

Feels like PKCS12_add_cert could have been named better given it says it creates a bag with the given cert and adds to the set of bags...but feels like I need a PKCS12_remove_cert within p12_crt which can then pop the bag off

[mattcaswell]

Squashed and merged.

Thanks for your contribution @grahamwoodward!

[grahamwoodward]

Ooossh yeah I'm buzzing, thanks for the help and guidence @mattcaswell @levicki @t8m

[levicki]

@grahamwoodward

Ooossh yeah I'm buzzing, thanks for the help and guidence @mattcaswell @levicki @t8m

Congratulations!

@t8m

No, this is a new feature.

Can you please clarify what that means? When will it be released?

[DDvO]

@t8m

No, this is a new feature.

Can you please clarify what that means?

His point was that, according to OpenSSL policies, new features are not backported, but only bug and security fixes.
BTW, version 1.1.1 meanwhile has run out of full support; only security fixes are still to be applied until 2023-09-11.

When will it be released?

Unfortunately, so far it is unclear when version 3.1 with all the recently added features will be released - see also #18500.
By then, to make use of the new features you need to use a branch such as master that includes the respective commits.

[mattcaswell]

Unfortunately, so far it is unclear when version 3.1 with all the recently added features will be released

Note that new features added to master will actually go into 3.2 not 3.1. I added an update to #18500 to explain this.

[levicki]

@DDvO @mattcaswell Thanks for clarifying, too bad it wasn't able to squeak in to 3.1.

Also, I thought 1.1.x branch was the one going forward, now there is 3.x. I must admit I am slightly confused by OpenSSL versioning.

[mattcaswell]

Also, I thought 1.1.x branch was the one going forward, now there is 3.x. I must admit I am slightly confused by OpenSSL versioning.

1.1.1 is the previous LTS (Long Term Support) release. The latest version is 1.1.1q. The 1.1.1 branch is now in "security-fix only" mode - so you will see more "letter" releases (1.1.1r, 1.1.1s, etc), but you won't see 1.1.2.

3.0 is our new LTS release. The versioning scheme was changed in this release. See this old blog post from several years ago for some discussion on that (since that blog post 3.0 was released so its now a little out of date). Also see the release strategy for a slightly longer discussion on the topic. Under the new versioning scheme patch fixes change the final number, so the latest bug-fix release for 3.0 is 3.0.5. New features go into the next "minor" release numbers, so 3.1 and 3.2. The current master branch is targeting 3.2. The 3.1 branch has not yet been created but will be taken from the existing 3.0 branch. We are only going to be allowing a very limited set of features to be merged there.

[levicki]

@mattcaswell Thanks for the links. Now that I read the documents you linked and understood that the project has switched to semantic versioning I went and started reading OpenSSL 3.0 migration guide and quickly got overwhelmed by the amount of changes -- It seems I will have quite some brushing up to do when it comes to OpenSSL API use.

As to how I managed to stay ignorant of 3.0.x branch -- I used 1.0.2 (and 1.1.1 ever since its release) extensively over the past decade, but I was under impression that 3.0 was still in development (i.e. beta, not release). Turns out I wasn't paying attention, but now I know what I will be doing next weekend.

[michael-o]

Any ambitions to make this available (back port) to 3.0? Since 3.1 is non-LTS many won't even bother and this feature will be years away. E.g., FreeBSD base system or any other vendor.

[mattcaswell]

Any ambitions to make this available (back port) to 3.0?

As explained above this is a new feature and is not eligible to backport to stable releases. So this will not go into 3.0. It will become available in 3.2 when that is released later this year.

[mdavidsaver]

I have a project where interoperability of PKCS12 files with java will be useful. So I am pleased to find that this will be supported in 3.2.

One difference I notice between files produced by openssl pkcs12 vs. keytool is that jdk_trust() unconditionally adds the trust attribute to all bags (cert., crl, key, ...) while keytool only adds it to chain certificates. From what I can tell, trustedkeyusage and localKeyId are in effect mutually exclusive in files created by keytool. For my application, I will call PKCS12_create_ex2() directly, and only add trustedkeyusage according to the following condition.

        if(PKCS12_SAFEBAG_get_nid(bag)==NID_certBag
                && PKCS12_SAFEBAG_get_bag_nid(bag)==NID_x509Certificate
                && PKCS12_SAFEBAG_get0_attr(bag, NID_localKeyID)==NULL
                && PKCS12_SAFEBAG_get0_attr(bag, NID_oracle_jdk_trustedkeyusage)==NULL)
            /* add NID_oracle_jdk_trustedkeyusage */

[michael-o]

I have a project where interoperability of PKCS12 files with java will be useful. So I am pleased to find that this will be supported in 3.2.

One difference I notice between files produced by openssl pkcs12 vs. keytool is that jdk_trust() unconditionally adds the trust attribute to all bags (cert., crl, key, ...) while keytool only adds it to chain certificates. From what I can tell, trustedkeyusage and localKeyId are in effect mutually exclusive in files created by keytool. For my application, I will call PKCS12_create_ex2() directly, and only add trustedkeyusage according to the following condition.

        if(PKCS12_SAFEBAG_get_nid(bag)==NID_certBag
                && PKCS12_SAFEBAG_get_bag_nid(bag)==NID_x509Certificate
                && PKCS12_SAFEBAG_get0_attr(bag, NID_localKeyID)==NULL
                && PKCS12_SAFEBAG_get0_attr(bag, NID_oracle_jdk_trustedkeyusage)==NULL)
            /* add NID_oracle_jdk_trustedkeyusage */

Can you spawn a new issue for this? This is ineed interesting.

[Eng-Fouad]

I have a project where interoperability of PKCS12 files with java will be useful. So I am pleased to find that this will be supported in 3.2.

One difference I notice between files produced by openssl pkcs12 vs. keytool is that jdk_trust() unconditionally adds the trust attribute to all bags (cert., crl, key, ...) while keytool only adds it to chain certificates. From what I can tell, trustedkeyusage and localKeyId are in effect mutually exclusive in files created by keytool. For my application, I will call PKCS12_create_ex2() directly, and only add trustedkeyusage according to the following condition.

        if(PKCS12_SAFEBAG_get_nid(bag)==NID_certBag
                && PKCS12_SAFEBAG_get_bag_nid(bag)==NID_x509Certificate
                && PKCS12_SAFEBAG_get0_attr(bag, NID_localKeyID)==NULL
                && PKCS12_SAFEBAG_get0_attr(bag, NID_oracle_jdk_trustedkeyusage)==NULL)
            /* add NID_oracle_jdk_trustedkeyusage */

How to add oracle_jdk_trustedkeyusage using openssl pkcs12 command?