thx for the feedback,
all changes understood and will fix them shortly!

all change requests resolved and some more details to integrate better into the existing code base.
edit: some more indentation fixes...

yeah, sorry for this being so painful, would love to just throw clang-format on it,
but will give my best to resolve this by hand:

• function annotation indent is clearly a no-go
• good that you come up with the case/switch point, saw both in the code-base, will try to ensure a minimal diff, to avoid non-functional changes

next try follows shortly...

pushed the changes,

• cleaned up various indents to stay as they were before to reduce the diff size
• commit-diff looks much cleaner now
• the bigger block within libopensc/pkcs15-openpgp.c was mostly rewritten anyways

Yes, believe it should be backwards compatible:

• openpgp_store_data will check for the OpenPGP version and store only in id==3 for p15card->card->type < SC_CARD_TYPE_OPENPGP_V3
• thus no check is necessary during readout of the certs, uhm at least before, now the error check I just added will lead to problems, means I will do an explicit check inside the readout, too

Now also added explicit checks for the OpenPGP version inside the certificate readout and making sure the loop is only iterated once for OpenPGP < v3

Could it be that you forgot to push some commit? The error checking is not visible here https://github.com/OpenSC/OpenSC/pull/2103/files

sorry pushed into the wrong branch, now it's better, will be updated again in a minute because I again messed up the indentation 🙄
edit: better now

I realized the following:

When importing a key-cert pair (aka a .p12 file) with pkcs15-init --delete-objects privkey,pubkey --id 1 --store-private-key key-cert.pfx --format pkcs12 --auth-id 3 --verify-pin the store is iterated multiple times, depending on the slot used.

• openpgp_store_data (thus writing at 0, doesn't it?)
• select_data with 1 (!)
• openpgp_store_data
• select_data with 0 (!)
• openpgp_store_data

Instead it should do the following (imho):

• select_data with 2
• openpgp_store_data

When checking the stored certificates I get three instances which leaves me puzzled, because I would have guessed two instances, in AUT and in DEC...

But it doesn't stop here, if using --id 2 it iterates two times and does the following:

• openpgp_store_data
• select_data with 0 (!)
• openpgp_store_data
• select_data with 255 (!) -> fails, of course (overflow error)

Here again, I get two instances and I would have guessed only one (as both storing attempts should end up in the slot for AUT).

Consequently, it works if using --id 3 as it firstly use openpgp_store_data which stores in default AUT slot and then fails with a select_data attempt on 255 again.

I couldn't find the reason in the code yet, does anyone has any idea? Can you reproduce?

@daringer , could you please look at the comments from @alex-nitrokey ?

@frankmorgner yeah, of course, already in the queue, will update the PR accordingly

@alex-nitrokey looks like I cannot reproduce, let's automate a little.
Creating a certkey.pfx like this first:

openssl genrsa -out rsa_key.pem
openssl genrsa -out server.pem
openssl req -new -key rsa_key.pem -out cert.csr
openssl x509 -req -in cert.csr -signkey server.pem -out cert_signed.crt
openssl pkcs12 -export -out certkey.pfx -inkey server.pem -in cert_signed.crt

then run the following based on sources from this branch:

> openpgp-tool --erase
Using reader with a card: Nitrokey Nitrokey Pro (000000000000000000008161) 00 00
Erase card

> pkcs15-init --delete-objects privkey,pubkey --id 1 --store-private-key certkey.pfx --format pkcs12 --auth-id 3 --verify-pin --pin 12345678
Using reader with a card: Nitrokey Nitrokey Pro (000000000000000000008161) 00 00
NOTE: couldn't find privkey 01 to delete
NOTE: couldn't find pubkey 01 to delete
Deleted 0 objects
Importing 1 certificates:
  0: /C=AU/ST=Some-State/O=Internet Widgits Pty Ltd

> pkcs15-tool --list-certificates
Using reader with a card: Nitrokey Nitrokey Pro (000000000000000000008161) 00 00
X.509 Certificate [SIG certificate]
	Object Flags   : [0x02], modifiable
	Authority      : no
	Path           : 3f007f21
	ID             : 01
	Encoded serial : 02 14 695E59D1129E7CBC797905C28F31FE35E9D04312

this is pretty much the exact opposite of your result 😄 , can you pinpoint what's the difference?
here is the log for the pkcs15-init call: output log

I can see 2 calls to openpgp_store_data. The 1st is likely for the key and the 2nd one also correctly calls select data with: 2, looks correct for me, while not reproducing your 1st pkcs15-init testcase (and doing what you expect it to do, weird?) ...

What's the status of this topic, is there anything to do?

Revived this PR, rebased on top of master and resolved the comments accordingly:

• removed misleading comments
• ensure SELECT DATA is not used for older OpenPGP Cards (<v3)
• in any case always just import a single cert
• explained the conversion from --id to param in pkcs15init/pkcs15-openpgp.c

PR cleanups: squashed into one commit, various minor additional lines removed and some whitespaces in favor of tabs removed

I would love to get this merged for 0.23.0. The code looks ok and I believe it is handling all the use cases discussed above except the workaround for old Yubikey as mentioned by @ya-isakov (Yubico/yubikey-manager@0ba0ad0). Do we want to implement this?

The code is also not rebased on current master and there are no CI results (can you rebase?). As part of the CI we run few tests against some old yubico openpgp applet: https://github.com/OpenSC/OpenSC/blob/master/.github/test-openpgp.sh I think this one implements only much older versions of the applet so we can not test this case. Is somebody aware of some newer applet that could be used for testing given that yubico no longer releases the source of the yubico applets?

I would love to get this merged for 0.23.0.

affirmative, taking this as a motivation to keep the pace up...

The code is also not rebased on current master and there are no CI results (can you rebase?).

rebase on top of master + push: ✔️

Do we want to implement this?

Very good question, no personal preference on that.
Essentially, it looks like "SELECT DATA" needs to handle a "special case" for Yubikeys, technically no problem from a first glance - but I have no means to test such a change, no yubikey here. But I could easily realize that if someone could give me a hint for what (exactly) to check in order to branch into this "special case".

Just compiled tested on Fedora with a v3.3 card and could only see the AUT cert. Loading the extra certs appears to work as I can read them back out using gpg-card readcert x > file.pem. Does the card have to be initialised a certain way?

Happy to provide ADPU logs, a card for testing (I have a few lying round). Unfortunately not a C programmer :/

@amaccuish thx a lot for testing and finding this, shot my foot during the last update! should be fixed by now - as you correctly observed the certificates are in fact added, but not correctly listed nor readable using opensc, this should be fixed now.

For you and maybe others to test, I use the following:

# erase openpgp-card
openpgp-tool --erase 
# add certificate with id
pkcs15-init --store-certificate some-cert.pem --id 1
# list certificates
pkcs15-tool --list-certificates

for me this now lists the certificates properly, you can extract it like this:

pkcs15-tool --read-certificate 1

any permutation of ids works for me, too, looking for someone to verify ...

@daringer works great! Amazing 😍

I guess it's not possible to override the key usage? For example, slot 2 can never be used for TLS since the card refuses to perform the required operations? The key usage flags in the x509 certificate are ignored correct?

Otherwise I've tested slot 1 and 3 for S/MIME and TLS auth and they work 🎉❤️

@amaccuish good to hear, thanks for testing!

For example, slot 2 can never be used for TLS since the card refuses to perform the required operations? The key usage flags in the x509 certificate are ignored correct?

I have to admit that I cannot tell in full detail. The OpenPGPCard spec defines SIG, DEC and AUT and for which respective operations they shall be used (and can optionally be used), so I would assume that client applications simply obey the spec - and partly the smartcard implementation also enforces that (e.g., PSO: COMPUTE DIGITAL SIGNATURE may only use key 1). That's likely also the reason why the x509 flags are not taken into account.

uh nice, thanks for the thorough reviews! fixes are on the way

Thank you. Merging for next rc/final release of 0.23.0