Re: [PATCH 2/4] add signing option -O hashalg=algorithm

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

 



On 22.12.2021 09:23, Linus Nordberg wrote:
---
[...]
diff --git a/ssh-keygen.c b/ssh-keygen.c
index d31dc503..ef99b9b6 100644
--- a/ssh-keygen.c
+++ b/ssh-keygen.c
@@ -142,6 +142,9 @@ struct cert_ext {
static struct cert_ext *cert_ext;
static size_t ncert_ext;

+/* Default hash algorithm for -Y sign and verify. */
+#define DEFAULT_SIGN_HASHALG_NAME "sha512"

I'm not a fan of redefining this. sshsig.c has HASHALG_DEFAULT we could resue when moved to sshsig.h. But I don't think we even need this default (see below).

[...]

static int
-sig_process_opts(char * const *opts, size_t nopts, uint64_t *verify_timep,
+sig_process_opts(char * const *opts, size_t nopts, char *hashalg, size_t hashalg_size, uint64_t *verify_timep,
    int *print_pubkey)
{
	size_t i;
	time_t now;

+	if (hashalg != NULL)
+		strlcpy(hashalg, DEFAULT_SIGN_HASHALG_NAME, hashalg_size);
	if (verify_timep != NULL)
		*verify_timep = 0;
	if (print_pubkey != NULL)
		*print_pubkey = 0;
	for (i = 0; i < nopts; i++) {
-		if (verify_timep &&
+		if (hashalg &&
+		    strncasecmp(opts[i], "hashalg=", 8) == 0) {
+			strlcpy(hashalg, opts[i] + 8, hashalg_size);

Why copy at all? If we simply return a pointer to the option (or NULL if not present) we can pass it on as is. This would use the default from sshsig.c and we would not need to care about one here. The size wouldn't be needed as well. Basically we could return hashalg = opts[i] + 8, no?

+		} else if (verify_timep &&
		    strncasecmp(opts[i], "verify-time=", 12) == 0) {
			if (parse_absolute_time(opts[i] + 12,
			    verify_timep) != 0 || *verify_timep == 0) {
@@ -2640,12 +2648,13 @@ sig_process_opts(char * const *opts, size_t nopts, uint64_t *verify_timep,
}

static int
-sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv)
+sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv, char * const *opts, size_t nopts)
{
	int i, fd = -1, r, ret = -1;
	int agent_fd = -1;
	struct sshkey *pubkey = NULL, *privkey = NULL, *signkey = NULL;
	sshsig_signer *signer = NULL;
+	char hashalg[7];	/* "shaXXX" */

If we use the pointer into opts directly we are also safe in case sha1024 (or some other hash with a longer name) comes along, without adjusting this one.


	/* Check file arguments. */
	for (i = 0; i < argc; i++) {
@@ -2655,6 +2664,9 @@ sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv)
			fatal("Cannot sign mix of paths and standard input");
	}

+	if (sig_process_opts(opts, nopts, hashalg, sizeof(hashalg), NULL, NULL) != 0)
+		goto done; /* error already logged */
+
	if ((r = sshkey_load_public(keypath, &pubkey, NULL)) != 0) {
		error_r(r, "Couldn't load public key %s", keypath);
		goto done;
@@ -2681,7 +2693,7 @@ sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv)

	if (argc == 0) {
		if ((r = sign_one(signkey, "(stdin)", STDIN_FILENO,
-		    sig_namespace, signer, &agent_fd)) != 0)
+		    sig_namespace, hashalg, signer, &agent_fd)) != 0)
			goto done;
	} else {
		for (i = 0; i < argc; i++) {
@@ -2693,7 +2705,7 @@ sig_sign(const char *keypath, const char *sig_namespace, int argc, char **argv)
				goto done;
			}
			if ((r = sign_one(signkey, argv[i], fd, sig_namespace,
-			    signer, &agent_fd)) != 0)
+			    hashalg, signer, &agent_fd)) != 0)
				goto done;
			if (fd != STDIN_FILENO)
				close(fd);
@@ -2723,7 +2735,7 @@ sig_verify(const char *signature, const char *sig_namespace,
	struct sshkey_sig_details *sig_details = NULL;
	uint64_t verify_time = 0;

-	if (sig_process_opts(opts, nopts, &verify_time, &print_pubkey) != 0)
+	if (sig_process_opts(opts, nopts, NULL, 0, &verify_time, &print_pubkey) != 0)
		goto done; /* error already logged */

	memset(&sig_details, 0, sizeof(sig_details));
@@ -2811,7 +2823,7 @@ sig_find_principals(const char *signature, const char *allowed_keys,
	char *principals = NULL, *cp, *tmp;
	uint64_t verify_time = 0;

-	if (sig_process_opts(opts, nopts, &verify_time, NULL) != 0)
+	if (sig_process_opts(opts, nopts, NULL, 0, &verify_time, NULL) != 0)
		goto done; /* error already logged */

	if ((r = sshbuf_load_file(signature, &abuf)) != 0) {
@@ -2857,7 +2869,7 @@ sig_match_principals(const char *allowed_keys, char *principal,
	char **principals = NULL;
	size_t i, nprincipals = 0;

-	if ((r = sig_process_opts(opts, nopts, NULL, NULL)) != 0)
+	if ((r = sig_process_opts(opts, nopts, NULL, 0, NULL, NULL)) != 0)
		return r; /* error already logged */

	if ((r = sshsig_match_principals(allowed_keys, principal,
@@ -3215,7 +3227,7 @@ usage(void)
	    "       ssh-keygen -Y find-principals -s signature_file -f allowed_signers_file\n"
	    "       ssh-keygen -Y match-principals -I signer_identity -f allowed_signers_file\n"
	    "       ssh-keygen -Y check-novalidate -n namespace -s signature_file\n"
-	    "       ssh-keygen -Y sign -f key_file -n namespace file ...\n"
+	    "       ssh-keygen -Y sign -f key_file -n namespace file [-O option] ...\n"
	    "       ssh-keygen -Y verify -f allowed_signers_file -I signer_identity\n"
	    "                  -n namespace -s signature_file [-r revocation_file]\n");
	exit(1);
@@ -3521,7 +3533,7 @@ main(int argc, char **argv)
				exit(1);
			}
			return sig_sign(identity_file, cert_principals,
-			    argc, argv);
+			    argc, argv, opts, nopts);
		} else if (strncmp(sign_op, "check-novalidate", 16) == 0) {
			if (ca_key_path == NULL) {
				error("Too few arguments for check-novalidate: "
--
2.30.2

_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev



[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux