From: Theo Buehler <tb@theobuehler.org>
Subject: Re: PKCS#11 Ed25519 support
To: Damien Miller <djm@mindrot.org>
Cc: openssh@openssh.com, tech@openbsd.org
Date: Fri, 25 Jul 2025 15:37:26 +0200

On Fri, Jul 25, 2025 at 11:27:29PM +1000, Damien Miller wrote:
> On Fri, 25 Jul 2025, Theo Buehler wrote:
> 
> > On Fri, Jul 25, 2025 at 04:28:20PM +1000, Damien Miller wrote:
> > > Hi,
> > > 
> > > Now that the PKCS#11 refactoring has landed, it's now quite
> > > straightforward to teach OpenSSH how to use Ed25519 keys stored in
> > > PKCS#11 tokens.
> > > 
> > > The only wrinkle is that different tokens seems to use different
> > > ASN.1 encodings for the public key and its type identifier.
> > > This implementation handles the two possibilities I've observed so
> > > far: Yubikeys and SoftHSM2.
> > > 
> > > Note: this requires the pkcs11.h update diff I sent out earlier.
> > > 
> > > ok?
> > 
> > Generally looks good. A few comments inline.
> 
> Thanks - I think this whacks all of these:

Yes, that's all fine with me (still unsure about pkcs11_destroy_keypair(),
but seeing as it's unused...)

I'm ok with the combined diff.

> 
> diff --git a/ssh-pkcs11.c b/ssh-pkcs11.c
> index 26ee228..1b57a1a 100644
> --- a/ssh-pkcs11.c
> +++ b/ssh-pkcs11.c
> @@ -1092,11 +1092,12 @@ pkcs11_fetch_ed25519_pubkey(struct pkcs11_provider *p, CK_ULONG slotidx,
>  	size_t			len;
>  	char			*hex = NULL;
>  	int			 success = -1, i;
> -	const u_char		 oid1[14] = {
> +	/* https://docs.oasis-open.org/pkcs11/pkcs11-curr/v3.0/os/pkcs11-curr-v3.0-os.html#_Toc30061180 */
> +	const u_char		 id1[14] = {
>  		0x13, 0x0c, 0x65, 0x64, 0x77, 0x61, 0x72, 0x64,
>  		0x73, 0x32, 0x35, 0x35, 0x31, 0x39,
>  	}; /* PrintableString { "edwards25519" } */
> -	const u_char		 oid2[5] = {
> +	const u_char		 id1[5] = {
>  		0x06, 0x03, 0x2b, 0x65, 0x70,
>  	}; /* OBJECT_IDENTIFIER { 1.3.101.112 } */
>  
> @@ -1139,18 +1140,18 @@ pkcs11_fetch_ed25519_pubkey(struct pkcs11_provider *p, CK_ULONG slotidx,
>  		goto fail;
>  	}
>  
> -	/* Expect one of the supported oids in CKA_EC_PARAMS */
> +	/* Expect one of the supported identifiers in CKA_EC_PARAMS */
>  	d = (u_char *)key_attr[2].pValue;
>  	len = key_attr[2].ulValueLen;
> -	if ((len != sizeof(oid1) || memcmp(d, oid1, sizeof(oid1)) != 0) &&
> -	    (len != sizeof(oid2) || memcmp(d, oid2, sizeof(oid2)) != 0)) {
> +	if ((len != sizeof(id1) || memcmp(d, id1, sizeof(id1)) != 0) &&
> +	    (len != sizeof(id2) || memcmp(d, id2, sizeof(id2)) != 0)) {
>  		hex = tohex(d, len);
>  		logit_f("unsupported CKA_EC_PARAMS: %s (len %zu)", hex, len);
>  		goto fail;
>  	}
>  
>  	/*
> -	 * Expect an either a raw 32 byte pubkey or an OCTET_STRING with
> +	 * Expect either a raw 32 byte pubkey or an OCTET STRING with
>  	 * a 32 byte pubkey in CKA_VALUE
>  	 */
>  	d = (u_char *)key_attr[1].pValue;
> @@ -2233,7 +2234,6 @@ pkcs11_destroy_keypair(char *provider_id, char *pin, unsigned long slotidx,
>  			*err = rv;
>  			goto out;
>  		}
> -		sshkey_free(k);
>  	}
>  
>  out: