Mailing List Archive

[PATCH] ssh: update certificate support
Following up on gpg-agent certificate support:

* updated the patches to single patch and rebased atop 2.3 release
* updated per prior feedback
* considering this as useful functionality as it allows for smoother workflow

Looking forward to feedback and comments.

visual diff: https://github.com/gpg/gnupg/compare/master...okigan:feature/tp-5487-on-latest?expand=1
CI build results: https://github.com/okigan/gnupg-workspace/runs/2376308761




GnuPG-bug-id: https://dev.gnupg.org/T1756
Signed-off-by: Igor Okulist <okigan@gmail.com>
---
agent/agent.h | 3 +-
agent/command-ssh.c | 126 ++++++++++++++++++++++++++++++++++++++++----
agent/cvt-openpgp.c | 12 ++++-
agent/findkey.c | 47 ++++++++++++++---
4 files changed, 168 insertions(+), 20 deletions(-)

diff --git a/agent/agent.h b/agent/agent.h
index 064b7be74..19c8813d9 100644
--- a/agent/agent.h
+++ b/agent/agent.h
@@ -723,7 +723,8 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
const char **r_algoname, int *r_npkey, int *r_nskey,
const char **r_format,
gcry_mpi_t *mpi_array, int arraysize,
- gcry_sexp_t *r_curve, gcry_sexp_t *r_flags);
+ gcry_sexp_t *r_curve, gcry_sexp_t *r_flags,
+ gcry_sexp_t *key_type);

/*-- sexp-secret.c --*/
gpg_error_t fixup_when_ecc_private_key (unsigned char *buf, size_t *buflen_p);
diff --git a/agent/command-ssh.c b/agent/command-ssh.c
index 73f98e9cd..83b4f2fa7 100644
--- a/agent/command-ssh.c
+++ b/agent/command-ssh.c
@@ -59,7 +59,7 @@
#include "../common/ssh-utils.h"


-
+

/* Request types. */
#define SSH_REQUEST_REQUEST_IDENTITIES 11
@@ -1943,11 +1943,39 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
}
else
{
+ gcry_sexp_t list = gcry_sexp_find_token (sexp, "key-type", 0);
+ size_t len = 0;
+ const char *key_type = gcry_sexp_nth_data (list, 1, &len);
+
+ if (key_type)
+ {
+ gcry_sexp_t certificate_sexp = gcry_sexp_find_token (sexp, "certificate", 0);
+ size_t certificate_sexp_b64_len = 0;
+ char *certificate_sexp_b64 = gcry_sexp_nth_buffer(certificate_sexp, 1, &certificate_sexp_b64_len);
+
+ struct b64state b64s = {};
+ long int certificate_len = 0;
+
+ if ((err = b64dec_start (&b64s, NULL)) ||
+ (err = b64dec_proc (&b64s, certificate_sexp_b64, certificate_sexp_b64_len, &certificate_len)) ||
+ (err = b64dec_finish (&b64s)) ||
+ (err = stream_write_data (stream, certificate_sexp_b64, certificate_len)))
+ {
+ xfree (certificate_sexp_b64);
+ goto out;
+ }
+
+ xfree (certificate_sexp_b64);
+ goto done;
+ }
+ else
+ {
/* Note: This is also used for EdDSA. */
err = stream_write_cstring (stream, key_spec.ssh_identifier);
if (err)
goto out;
}
+ }

/* Write the parameters. */
for (p_elems = elems; *p_elems; p_elems++)
@@ -1995,6 +2023,7 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,
}
}

+done:
if (es_fclose_snatch (stream, &blob, &blob_size))
{
err = gpg_error_from_syserror ();
@@ -2014,7 +2043,6 @@ ssh_key_to_blob (gcry_sexp_t sexp, int with_secret,

return err;
}
-

/*

@@ -2078,19 +2106,19 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
if (err)
goto out;

+ unsigned char *cert_buffer = NULL;
+ u32 cert_buffer_len = 0;
+
if ((spec.flags & SPEC_FLAG_WITH_CERT))
{
/* This is an OpenSSH certificate+private key. The certificate
is an SSH string and which we store in an estream object. */
- unsigned char *buffer;
- u32 buflen;
char *cert_key_type;

- err = stream_read_string (stream, 0, &buffer, &buflen);
+ err = stream_read_string (stream, 0, &cert_buffer, &cert_buffer_len);
if (err)
goto out;
- cert = es_fopenmem_init (0, "rb", buffer, buflen);
- xfree (buffer);
+ cert = es_fopenmem_init (0, "rb", cert_buffer, cert_buffer_len);
if (!cert)
{
err = gpg_error_from_syserror ();
@@ -2238,8 +2266,61 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
goto out;
}

- err = sexp_key_construct (&key, spec, secret, curve_name, mpi_list,
- comment? comment:"");
+ if (0 == strcmp(spec.ssh_identifier, "ssh-rsa-cert-v01@openssh.com"))
+ {
+ struct b64state b64s = {};
+ estream_t stream = NULL;
+ long int b64_cert_buffer_len = 0;
+
+ stream = es_fopenmem(0, "wt");
+ if ((err = b64enc_start_es (&b64s, stream, "")) ||
+ (err = b64enc_write (&b64s, cert_buffer, cert_buffer_len)) ||
+ (err = b64enc_finish (&b64s)))
+ {
+ goto out;
+ }
+
+ b64_cert_buffer_len = es_ftell (stream);
+
+ char *b64_cert_buffer = xtrymalloc (b64_cert_buffer_len + 1);
+ size_t nread = 0;
+
+ if ((err = es_fseek(stream, 0, SEEK_SET)) ||
+ (err = es_read (stream, b64_cert_buffer, b64_cert_buffer_len, &nread)))
+ {
+ goto out;
+ }
+
+ b64_cert_buffer[b64_cert_buffer_len] = '\0';
+ es_fclose (stream);
+
+ err = gcry_sexp_build (&key, NULL,
+ "(private-key "
+ " (rsa (n %m) (e %m) (d %m) (p %m) (q %m) (u %m) )"
+ " (comment %s)"
+ " (key-type %s)"
+ " (certificate %s)"
+ " )",
+ // 1 and 0 required swapped!
+ mpi_list[1], mpi_list[0], mpi_list[2], mpi_list[3], mpi_list[4], mpi_list[5],
+ comment!=NULL?comment:"",
+ spec.ssh_identifier,
+ b64_cert_buffer);
+
+ xfree(b64_cert_buffer);
+ b64_cert_buffer = NULL;
+
+ if (err)
+ goto out;
+ }
+ else
+ {
+ err = sexp_key_construct (&key, spec, secret, curve_name, mpi_list,
+ comment? comment:"");
+ if (err)
+ goto out;
+ }
+
if (!err)
{
if (key_spec)
@@ -2253,6 +2334,8 @@ ssh_receive_key (estream_t stream, gcry_sexp_t *key_new, int secret,
xfree (key_type);
xfree (comment);

+ xfree (cert_buffer);
+
return err;
}

@@ -2345,7 +2428,7 @@ ssh_read_key_public_from_blob (unsigned char *blob, size_t blob_size,

/* This function calculates the key grip for the key contained in the
S-Expression KEY and writes it to BUFFER, which must be large
- enough to hold it. Returns usual error code. */
+ enough to hold 20 characters. Returns usual error code. */
static gpg_error_t
ssh_key_grip (gcry_sexp_t key, unsigned char *buffer)
{
@@ -2355,6 +2438,29 @@ ssh_key_grip (gcry_sexp_t key, unsigned char *buffer)
return err? err : gpg_error (GPG_ERR_INTERNAL);
}

+ // if the key contains "key-type" update the gcry_pk_get_keygrip computed
+ // keygrip by the hashing it with key-type value
+ gcry_sexp_t list = NULL;
+ const char *data = NULL;
+ size_t data_len = 0;
+
+ list = gcry_sexp_find_token (key, "key-type", 0);
+ data = gcry_sexp_nth_data(list, 1, &data_len);
+
+ if (data)
+ {
+ gcry_md_hd_t md = NULL;
+ gcry_md_open (&md, GCRY_MD_SHA1, 0);
+
+ gcry_md_write (md, buffer, 20);
+ gcry_md_write (md, data, data_len);
+
+ memcpy (buffer, gcry_md_read (md, GCRY_MD_SHA1), 20);
+ gcry_md_close (md);
+ }
+
+ gcry_sexp_release(list);
+
return 0;
}

diff --git a/agent/cvt-openpgp.c b/agent/cvt-openpgp.c
index 3da553f95..36d8b91d0 100644
--- a/agent/cvt-openpgp.c
+++ b/agent/cvt-openpgp.c
@@ -1259,7 +1259,8 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
const char **r_algoname, int *r_npkey, int *r_nskey,
const char **r_elems,
gcry_mpi_t *array, int arraysize,
- gcry_sexp_t *r_curve, gcry_sexp_t *r_flags)
+ gcry_sexp_t *r_curve, gcry_sexp_t *r_flags,
+ gcry_sexp_t *r_key_type)
{
gpg_error_t err;
gcry_sexp_t list, l2;
@@ -1268,6 +1269,7 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
int npkey, nskey;
gcry_sexp_t curve = NULL;
gcry_sexp_t flags = NULL;
+ gcry_sexp_t key_type = NULL;

*r_curve = NULL;
*r_flags = NULL;
@@ -1289,6 +1291,8 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
return gpg_error (GPG_ERR_BAD_SECKEY);
}

+ key_type = gcry_sexp_find_token (list, "key_type", 0);
+
l2 = gcry_sexp_cadr (list);
gcry_sexp_release (list);
list = l2;
@@ -1371,6 +1375,9 @@ extract_private_key (gcry_sexp_t s_key, int req_private_key_data,
*r_curve = curve;
*r_flags = flags;

+ if (r_key_type)
+ *r_key_type = key_type;
+
return 0;
}
}
@@ -1389,6 +1396,7 @@ convert_to_openpgp (ctrl_t ctrl, gcry_sexp_t s_key, const char *passphrase,
gcry_mpi_t array[10];
gcry_sexp_t curve = NULL;
gcry_sexp_t flags = NULL;
+ gcry_sexp_t key_name = NULL;
char protect_iv[16];
char salt[8];
unsigned long s2k_count;
@@ -1402,7 +1410,7 @@ convert_to_openpgp (ctrl_t ctrl, gcry_sexp_t s_key, const char *passphrase,
array[i] = NULL;

err = extract_private_key (s_key, 1, &algoname, &npkey, &nskey, NULL,
- array, DIM (array), &curve, &flags);
+ array, DIM (array), &curve, &flags, &key_name);
if (err)
return err;

diff --git a/agent/findkey.c b/agent/findkey.c
index 28ff61781..2b5830458 100644
--- a/agent/findkey.c
+++ b/agent/findkey.c
@@ -1192,14 +1192,14 @@ agent_public_key_from_file (ctrl_t ctrl,
gcry_mpi_t array[10];
gcry_sexp_t curve = NULL;
gcry_sexp_t flags = NULL;
- gcry_sexp_t uri_sexp, comment_sexp;
- const char *uri, *comment;
- size_t uri_length, comment_length;
- int uri_intlen, comment_intlen;
+ gcry_sexp_t uri_sexp, comment_sexp, key_type_sexp, certificate_sexp;
+ const char *uri, *comment, *key_type, *certificate;
+ size_t uri_length, comment_length, key_type_length, certificate_length;
+ int uri_intlen, comment_intlen, key_type_intlen, certificate_intlen;
membuf_t format_mb;
char *format;
- void *args[2+7+2+2+1]; /* Size is 2 + max. # of elements + 2 for uri + 2
- for comment + end-of-list. */
+ void *args[2+7+2+2+2+1]; /* Size is 2 + max. # of elements + 2 for uri + 2
+ for comment + key_type + certificate + end-of-list. */
int argidx;
gcry_sexp_t list = NULL;
const char *s;
@@ -1216,7 +1216,7 @@ agent_public_key_from_file (ctrl_t ctrl,
array[i] = NULL;

err = extract_private_key (s_skey, 0, &algoname, &npkey, NULL, &elems,
- array, DIM (array), &curve, &flags);
+ array, DIM (array), &curve, &flags, &key_type_sexp);
if (err)
{
gcry_sexp_release (s_skey);
@@ -1235,6 +1235,19 @@ agent_public_key_from_file (ctrl_t ctrl,
if (comment_sexp)
comment = gcry_sexp_nth_data (comment_sexp, 1, &comment_length);

+ key_type = NULL;
+ key_type_length = 0;
+ key_type_sexp = gcry_sexp_find_token (s_skey, "key-type", 0);
+ if (key_type_sexp)
+ key_type = gcry_sexp_nth_data (key_type_sexp, 1, &key_type_length);
+
+ certificate = NULL;
+ certificate_length = 0;
+ certificate_sexp = gcry_sexp_find_token (s_skey, "certificate", 0);
+ if (certificate_sexp)
+ certificate = gcry_sexp_nth_data (certificate_sexp, 1, &certificate_length);
+
+
gcry_sexp_release (s_skey);
s_skey = NULL;

@@ -1253,6 +1266,26 @@ agent_public_key_from_file (ctrl_t ctrl,
args[argidx++] = &array[idx];
}
put_membuf_str (&format_mb, ")");
+
+ if (key_type)
+ {
+ key_type_intlen = (int)key_type_length;
+ put_membuf_str (&format_mb, "(key-type %b)");
+ log_assert (argidx+1 < DIM (args));
+ args[argidx++] = (void *)&key_type_intlen;
+ log_assert (argidx+1 < DIM (args));
+ args[argidx++] = (void*)&key_type;
+ }
+ if (certificate)
+ {
+ certificate_intlen = (int)certificate_length;
+ put_membuf_str (&format_mb, "(certificate %b)");
+ log_assert (argidx+1 < DIM (args));
+ args[argidx++] = (void *)&certificate_intlen;
+ log_assert (argidx+1 < DIM (args));
+ args[argidx++] = (void*)&certificate;
+ }
+
if (uri)
{
put_membuf_str (&format_mb, "(uri %b)");
--
2.30.2


_______________________________________________
Gnupg-devel mailing list
Gnupg-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gnupg-devel
Re: [PATCH] ssh: update certificate support [ In reply to ]
On Sun, 18 Apr 2021 17:02, Igor Okulist said:
> + if (0 == strcmp(spec.ssh_identifier, "ssh-rsa-cert-v01@openssh.com"))

Don't do this. Use this pattern:

if (!strcmp(spec.ssh_identifier, "ssh-rsa-cert-v01@openssh.com"))

> + "(private-key "
> + " (rsa (n %m) (e %m) (d %m) (p %m) (q %m) (u %m) )"
> + " (comment %s)"
> + " (key-type %s)"
> + " (certificate %s)"

That is never going to fly. The "certificate" and other new items are
nothing we want as the part of a private key. See keyformat.txt on how
to add meta information to a key.


Shalom-Salam,

Werner


--
Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.
Re: [PATCH] ssh: update certificate support [ In reply to ]
Igor Okulist wrote:
> Following up on gpg-agent certificate support:
>
> * updated the patches to single patch and rebased atop 2.3 release
> * updated per prior feedback
> * considering this as useful functionality as it allows for smoother workflow
>
> Looking forward to feedback and comments.

Sorry for my miscommunication. Finally, I realized that OpenSSH newer
versions behave differently. (It were good if you had addressed that
directly.)

I tried to understand your shell script. The problem can be worked
around when we use -k option for ssh-add and -i option with certificate
for ssh. That recovers the old behaviour of ssh-add/ssh (of older
versions of OpenSSH); With the -k option, ssh-add does not send
certificates to ssh-agent. With -i option plus path to certificate, ssh
handles the certificate by itself (when asked by remote sshd) and only
asks ssh-agent for signing.

IIUC, the purpose of your patch is to make ssh-emulation of gpg-agent
behave just like original ssh-agent does. To support this feature (if
it's worth to support), we need to enhance the file format of the
private key. In the source code, gnupg/agent/keyformat.txt suggested
use of "OpenSSH-cert" field.

But, in my opinion, I'm not that positive to this approach.

I think that good points will be:

* ssh-agent emulation of gpg-agent will be more compatible.
* we will be able to remove the certificate file under .ssh.

And it would be also good if gpg frontend can support making SSH
certificate (the work ssh-keygen does) by signing SSH CA key.

I'm afraid that adding more feature (like handling certificate, public
part of data) to gpg-agent and adding more data to the private key file
are against the design philosophy... making gpg-agent as small as
possible, focusing on private key operations.
--

_______________________________________________
Gnupg-devel mailing list
Gnupg-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gnupg-devel
Re: [PATCH] ssh: update certificate support [ In reply to ]
On Mon, Apr 19, 2021 at 4:15 AM Werner Koch <wk@gnupg.org> wrote:
>
> On Sun, 18 Apr 2021 17:02, Igor Okulist said:
> > + if (0 == strcmp(spec.ssh_identifier, "ssh-rsa-cert-v01@openssh.com"))
>
> Don't do this. Use this pattern:
>
> if (!strcmp(spec.ssh_identifier, "ssh-rsa-cert-v01@openssh.com"))
>
IO: Noted, will change

> > + "(private-key "
> > + " (rsa (n %m) (e %m) (d %m) (p %m) (q %m) (u %m) )"
> > + " (comment %s)"
> > + " (key-type %s)"
> > + " (certificate %s)"
>
> That is never going to fly. The "certificate" and other new items are
> nothing we want as the part of a private key. See keyformat.txt on how
> to add meta information to a key.
IO: the reason for this approach is that the "meta information" needs
to be passed
IO: through multiple layers, so even if it would be saved in separate
field it seem it would
IO: be very extensive changes to pass such "meta information" through
the system.
IO: For example, if the comment was saved as such "meta information"
would we need to change
IO: all signatures of all functions passing the key to pass the
comment along? Is there or is there another way?

>
>
> Shalom-Salam,
>
> Werner
>
>
> --
> Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz.

_______________________________________________
Gnupg-devel mailing list
Gnupg-devel@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gnupg-devel