Re: [openssl-dev] [openssl.org #3887] PATCH: rsautl and intelligent retry for Public Key parse after Traditional/Subject Public Key Info parse fails
On 5/31/2015 2:46 AM, noloa...@gmail.com via RT wrote:
apps.c has a couple of parsing routines called load_pubkey and
load_key. rsautl uses those routines.
However, there's no option in rsautil to use anything other than a
ASN.1/DER or PEM encoded traditional key (or subject public key info).
The code paths are present, we just can't seem to get to them.
The load_pubkey already has code to support FORMAT_PEMRSA
and call PEM_read_bio_RSAPublicKey
Looking at OpenSSL 1.0.2, it looks like the problem is more in
apps.c in the str2fmt that does not have an option to set FORMAT_PEMRSA
or FORMAT_ASN1RSA.
Folks in the field have problem with it on occasion. See, for example,
Can't load programmatically generated public key,
http://stackoverflow.com/q/30547646.
This patch adds an intelligent fallback:
/* rsautl does not offer a way to specify just a public key. It
requires a */
/* subjectPublicKeyInfo, and there's no argument or option to
switch to */
/* the other type with either ASN.1/DER or PEM. This attempts to
make an */
/* intelligent retry. */
if(keyformat == FORMAT_PEM) {
next_format = FORMAT_PEMRSA;
} if(keyformat == FORMAT_ASN1) {
next_format = FORMAT_ASN1RSA;
} else {
next_format = -1;
}
switch (key_type) {
case KEY_PRIVKEY:
pkey = load_key(keyfile, keyformat, 0, passin, e, Private
Key, 0 /*last_try*/);
if(!pkey next_format != -1) { pkey = load_key(keyfile,
next_format, 0, passin, e, Private Key, 1 /*last_try*/); }
break;
case KEY_PUBKEY:
pkey = load_pubkey(keyfile, keyformat, 0, NULL, e, Public
Key, 0 /*last_try*/);
if(!pkey next_format != -1) { pkey = load_pubkey(keyfile,
next_format, 0, NULL, e, Public Key, 1 /*last_try*/); }
break;
Then, functions load_key and load_pubkey suppress the error messages
if last_try is 0:
if (pkey == NULL last_try) {
BIO_printf(bio_err, unable to load %s\n, key_descrip);
}
All in all, existing behavior and error messages are maintained. The
subcommand is just a little more robust.
Related, we might be able to make similar changes to rsa.c. But
there's some extra special sauce present, so it was not a clear
cut-in. I think the special sauce kid of attempts to do the same thing
as above. (Maybe this patch should act more like rsa.c?)
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
--
Douglas E. Engert deeng...@gmail.com
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3887] PATCH: rsautl and intelligent retry for Public Key parse after Traditional/Subject Public Key Info parse fails
On Sun, May 31, 2015 at 12:27 PM, Richard Levitte via RT r...@openssl.org
wrote:
Nice idea, I'm however thinking that much of the trying different formats
could
be moved to load_key / load_pubkey, all that would be needed is a keyformat
denoting try anything. -1, perhaps?
I like the idea, and I was entertaining it. The goal here ways to
improve rsautl, so I deferred.
There are some potential issues with moving the logic into load_key
and load_pubkey. The first issue is the change in behavior utility
wide. That is, it will affect most (all?) utilities. I think its a
good idea, but others may not.
The second issue (related to the first), is the rsa utility has tried
to address these issue already. So there's a few other
switches/options for the rsa utility. For example, RSAPublicKey_in and
RSAPublicKey_out. This may or may not be an issue.
The third issue (related to the first), is that other similar
functions, like load_certificate, may not get the change. This may or
may not matter, and may or may not be an issue.
The fourth issue is the cracker. Effectively, apps{c|h} only
recognizes PEM, PEMRSA, ASN1, and ASN1RSA (see str2fmt, which appears
to be removed in 1.1.0). So OpenSSL would need a way to identify other
encodings, types and algorithms. For example, a PEM encoded DSA
SubjectPublicKeyInfo or PrivateKeyInfo (traditional key) versus a
ANS.1/DER encoded DSA public key or private key.
**
I think there's little difference between load_key and load_pubkey.
Fold them together, and call the function that remains load_key and
return the EVP_PKEY.
load_key should take a encoding hint (PEM vs ASN.1, etc), a key type
hint (SPKI/Traditional versus Key-Only, etc), a key pair hint (public
versus private), and a key algorithm hint (RSA, DSA, etc).
I would envision it to be similar to:
int encoding_hint = -1, key_type_hint = -1, key_pair_hint = -1,
key_alg_hint = -1;
key_hints(…, encoding_hint, key_type_hint, key_pair_hint, key_alg_hint);
That's going to lead to an ugly message cracker, but I don't know how
to avoid it.
**
There is a key_file_format function in apps.c, but its not really
useful. I think this is the keystone to making things work here. It
should peek at the key, and return the tuple {key encoding, key type,
key pair, key algorithm} hints. Its similar to str2fmt, but it works
directly with the key specified by the user rather than command line
arguments.
One of the things I give the user credit for is that they know the
key-file they want to use. They may not know the encoding, format or
type - but they know the filename. So something to peek into the file
and then make intelligent decisions would probably be very helpful to
the user.
I understand key_file_format won't work with stdin. So maybe the
course of action is to peek from a BIO. If its a file BIO, then
everything just works. If its from stdin, then wrap it in a memory bio
so everything works as expected.
**
Jeff
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3887] PATCH: rsautl and intelligent retry for Public Key parse after Traditional/Subject Public Key Info parse fails
On Sun, May 31, 2015 at 12:27 PM, Richard Levitte via RT r...@openssl.org
wrote:
Nice idea, I'm however thinking that much of the trying different formats
could
be moved to load_key / load_pubkey, all that would be needed is a keyformat
denoting try anything. -1, perhaps?
I like the idea, and I was entertaining it. The goal here ways to
improve rsautl, so I deferred.
There are some potential issues with moving the logic into load_key
and load_pubkey. The first issue is the change in behavior utility
wide. That is, it will affect most (all?) utilities. I think its a
good idea, but others may not.
The second issue (related to the first), is the rsa utility has tried
to address these issue already. So there's a few other
switches/options for the rsa utility. For example, RSAPublicKey_in and
RSAPublicKey_out. This may or may not be an issue.
The third issue (related to the first), is that other similar
functions, like load_certificate, may not get the change. This may or
may not matter, and may or may not be an issue.
The fourth issue is the cracker. Effectively, apps{c|h} only
recognizes PEM, PEMRSA, ASN1, and ASN1RSA (see str2fmt, which appears
to be removed in 1.1.0). So OpenSSL would need a way to identify other
encodings, types and algorithms. For example, a PEM encoded DSA
SubjectPublicKeyInfo or PrivateKeyInfo (traditional key) versus a
ANS.1/DER encoded DSA public key or private key.
**
I think there's little difference between load_key and load_pubkey.
Fold them together, and call the function that remains load_key and
return the EVP_PKEY.
load_key should take a encoding hint (PEM vs ASN.1, etc), a key type
hint (SPKI/Traditional versus Key-Only, etc), a key pair hint (public
versus private), and a key algorithm hint (RSA, DSA, etc).
I would envision it to be similar to:
int encoding_hint = -1, key_type_hint = -1, key_pair_hint = -1,
key_alg_hint = -1;
key_hints(…, encoding_hint, key_type_hint, key_pair_hint, key_alg_hint);
That's going to lead to an ugly message cracker, but I don't know how
to avoid it.
**
There is a key_file_format function in apps.c, but its not really
useful. I think this is the keystone to making things work here. It
should peek at the key, and return the tuple {key encoding, key type,
key pair, key algorithm} hints. Its similar to str2fmt, but it works
directly with the key specified by the user rather than command line
arguments.
One of the things I give the user credit for is that they know the
key-file they want to use. They may not know the encoding, format or
type - but they know the filename. So something to peek into the file
and then make intelligent decisions would probably be very helpful to
the user.
I understand key_file_format won't work with stdin. So maybe the
course of action is to peek from a BIO. If its a file BIO, then
everything just works. If its from stdin, then wrap it in a memory bio
so everything works as expected.
**
Jeff
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3887] PATCH: rsautl and intelligent retry for Public Key parse after Traditional/Subject Public Key Info parse fails
I submitted this earlier, but I forgot to tweak the docs. The docs
were missing the -keyform option, and they needed the behavior change
documented.
I also fixed a typo in the patch. The following was missing an 'else if':
if(keyformat == FORMAT_PEM) {
next_format = FORMAT_PEMRSA;
} else if(keyformat == FORMAT_ASN1) {
next_format = FORMAT_ASN1RSA;
} else {
next_format = -1;
}
Attached is the updated patch.
If the patch is rejected, you might consider taking the documentation
updates (sans the behavioral changes).
On Sat, May 30, 2015 at 6:17 PM, Jeffrey Walton noloa...@gmail.com wrote:
apps.c has a couple of parsing routines called load_pubkey and
load_key. rsautl uses those routines.
However, there's no option in rsautil to use anything other than a
ASN.1/DER or PEM encoded traditional key (or subject public key info).
The code paths are present, we just can't seem to get to them.
...
diff --git a/apps/apps.c b/apps/apps.c
index 60f71c3..f80767a 100644
--- a/apps/apps.c
+++ b/apps/apps.c
@@ -763,7 +763,7 @@ X509_CRL *load_crl(const char *infile, int format)
}
EVP_PKEY *load_key(const char *file, int format, int maybe_stdin,
- const char *pass, ENGINE *e, const char *key_descrip)
+ const char *pass, ENGINE *e, const char *key_descrip, int
last_try)
{
BIO *key = NULL;
EVP_PKEY *pkey = NULL;
@@ -773,7 +773,7 @@ EVP_PKEY *load_key(const char *file, int format, int
maybe_stdin,
cb_data.prompt_info = file;
if (file == NULL (!maybe_stdin || format == FORMAT_ENGINE)) {
-BIO_printf(bio_err, no keyfile specified\n);
+if(last_try) { BIO_printf(bio_err, no keyfile specified\n); }
goto end;
}
#ifndef OPENSSL_NO_ENGINE
@@ -827,7 +827,7 @@ EVP_PKEY *load_key(const char *file, int format, int
maybe_stdin,
}
end:
BIO_free(key);
-if (pkey == NULL) {
+if (pkey == NULL last_try) {
BIO_printf(bio_err, unable to load %s\n, key_descrip);
ERR_print_errors(bio_err);
}
@@ -842,7 +842,7 @@ static const char *key_file_format(int format)
}
EVP_PKEY *load_pubkey(const char *file, int format, int maybe_stdin,
- const char *pass, ENGINE *e, const char *key_descrip)
+ const char *pass, ENGINE *e, const char *key_descrip,
int last_try)
{
BIO *key = NULL;
EVP_PKEY *pkey = NULL;
@@ -852,7 +852,7 @@ EVP_PKEY *load_pubkey(const char *file, int format, int
maybe_stdin,
cb_data.prompt_info = file;
if (file == NULL (!maybe_stdin || format == FORMAT_ENGINE)) {
-BIO_printf(bio_err, no keyfile specified\n);
+if(last_try) { BIO_printf(bio_err, no keyfile specified\n); }
goto end;
}
#ifndef OPENSSL_NO_ENGINE
@@ -914,8 +914,9 @@ EVP_PKEY *load_pubkey(const char *file, int format, int
maybe_stdin,
#endif
end:
BIO_free(key);
-if (pkey == NULL)
+if (pkey == NULL last_try) {
BIO_printf(bio_err, unable to load %s\n, key_descrip);
+}
return (pkey);
}
diff --git a/apps/apps.h b/apps/apps.h
index a8652a1..0cd563a 100644
--- a/apps/apps.h
+++ b/apps/apps.h
@@ -427,9 +427,9 @@ X509 *load_cert(const char *file, int format,
X509_CRL *load_crl(const char *infile, int format);
int load_cert_crl_http(const char *url, X509 **pcert, X509_CRL **pcrl);
EVP_PKEY *load_key(const char *file, int format, int maybe_stdin,
- const char *pass, ENGINE *e, const char *key_descrip);
+ const char *pass, ENGINE *e, const char *key_descrip, int
last_try);
EVP_PKEY *load_pubkey(const char *file, int format, int maybe_stdin,
- const char *pass, ENGINE *e, const char *key_descrip);
+ const char *pass, ENGINE *e, const char *key_descrip,
int last_try);
STACK_OF(X509) *load_certs(const char *file, int format,
const char *pass, ENGINE *e,
const char *cert_descrip);
diff --git a/apps/ca.c b/apps/ca.c
index 4dc9176..8c03efd 100644
--- a/apps/ca.c
+++ b/apps/ca.c
@@ -587,7 +587,7 @@ end_of_options:
goto end;
}
}
-pkey = load_key(keyfile, keyformat, 0, key, e, CA private key);
+pkey = load_key(keyfile, keyformat, 0, key, e, CA private key, 1
/*last_try*/);
if (key)
OPENSSL_cleanse(key, strlen(key));
if (pkey == NULL) {
diff --git a/apps/cms.c b/apps/cms.c
index 7ccca5b..5966873 100644
--- a/apps/cms.c
+++ b/apps/cms.c
@@ -762,7 +762,7 @@ int cms_main(int argc, char **argv)
keyfile = NULL;
if (keyfile) {
-key = load_key(keyfile, keyform, 0, passin, e, signing key file);
+key = load_key(keyfile, keyform, 0, passin, e, signing key file, 1
/*last_try*/);
if (!key)
goto end;
}
@@ -966,7 +966,7 @@ int cms_main(int argc, char **argv)
e, signer certificate);
if (!signer)
[openssl-dev] [openssl.org #3887] PATCH: rsautl and intelligent retry for Public Key parse after Traditional/Subject Public Key Info parse fails
Nice idea, I'm however thinking that much of the trying different formats could
be moved to load_key / load_pubkey, all that would be needed is a keyformat
denoting try anything. -1, perhaps?
On Sun May 31 09:46:28 2015, noloa...@gmail.com wrote:
apps.c has a couple of parsing routines called load_pubkey and
load_key. rsautl uses those routines.
However, there's no option in rsautil to use anything other than a
ASN.1/DER or PEM encoded traditional key (or subject public key info).
The code paths are present, we just can't seem to get to them.
Folks in the field have problem with it on occasion. See, for example,
Can't load programmatically generated public key,
http://stackoverflow.com/q/30547646.
This patch adds an intelligent fallback:
/* rsautl does not offer a way to specify just a public key. It
requires a */
/* subjectPublicKeyInfo, and there's no argument or option to
switch to */
/* the other type with either ASN.1/DER or PEM. This attempts to
make an */
/* intelligent retry. */
if(keyformat == FORMAT_PEM) {
next_format = FORMAT_PEMRSA;
} if(keyformat == FORMAT_ASN1) {
next_format = FORMAT_ASN1RSA;
} else {
next_format = -1;
}
switch (key_type) {
case KEY_PRIVKEY:
pkey = load_key(keyfile, keyformat, 0, passin, e, Private
Key, 0 /*last_try*/);
if(!pkey next_format != -1) { pkey = load_key(keyfile,
next_format, 0, passin, e, Private Key, 1 /*last_try*/); }
break;
case KEY_PUBKEY:
pkey = load_pubkey(keyfile, keyformat, 0, NULL, e, Public
Key, 0 /*last_try*/);
if(!pkey next_format != -1) { pkey = load_pubkey(keyfile,
next_format, 0, NULL, e, Public Key, 1 /*last_try*/); }
break;
Then, functions load_key and load_pubkey suppress the error messages
if last_try is 0:
if (pkey == NULL last_try) {
BIO_printf(bio_err, unable to load %s\n, key_descrip);
}
All in all, existing behavior and error messages are maintained. The
subcommand is just a little more robust.
Related, we might be able to make similar changes to rsa.c. But
there's some extra special sauce present, so it was not a clear
cut-in. I think the special sauce kid of attempts to do the same thing
as above. (Maybe this patch should act more like rsa.c?)
--
Richard Levitte
levi...@openssl.org
___
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3887] PATCH: rsautl and intelligent retry for Public Key parse after Traditional/Subject Public Key Info parse fails
apps.c has a couple of parsing routines called load_pubkey and
load_key. rsautl uses those routines.
However, there's no option in rsautil to use anything other than a
ASN.1/DER or PEM encoded traditional key (or subject public key info).
The code paths are present, we just can't seem to get to them.
Folks in the field have problem with it on occasion. See, for example,
Can't load programmatically generated public key,
http://stackoverflow.com/q/30547646.
This patch adds an intelligent fallback:
/* rsautl does not offer a way to specify just a public key. It
requires a */
/* subjectPublicKeyInfo, and there's no argument or option to
switch to */
/* the other type with either ASN.1/DER or PEM. This attempts to
make an */
/* intelligent retry. */
if(keyformat == FORMAT_PEM) {
next_format = FORMAT_PEMRSA;
} if(keyformat == FORMAT_ASN1) {
next_format = FORMAT_ASN1RSA;
} else {
next_format = -1;
}
switch (key_type) {
case KEY_PRIVKEY:
pkey = load_key(keyfile, keyformat, 0, passin, e, Private
Key, 0 /*last_try*/);
if(!pkey next_format != -1) { pkey = load_key(keyfile,
next_format, 0, passin, e, Private Key, 1 /*last_try*/); }
break;
case KEY_PUBKEY:
pkey = load_pubkey(keyfile, keyformat, 0, NULL, e, Public
Key, 0 /*last_try*/);
if(!pkey next_format != -1) { pkey = load_pubkey(keyfile,
next_format, 0, NULL, e, Public Key, 1 /*last_try*/); }
break;
Then, functions load_key and load_pubkey suppress the error messages
if last_try is 0:
if (pkey == NULL last_try) {
BIO_printf(bio_err, unable to load %s\n, key_descrip);
}
All in all, existing behavior and error messages are maintained. The
subcommand is just a little more robust.
Related, we might be able to make similar changes to rsa.c. But
there's some extra special sauce present, so it was not a clear
cut-in. I think the special sauce kid of attempts to do the same thing
as above. (Maybe this patch should act more like rsa.c?)
diff --git a/apps/apps.c b/apps/apps.c
index 60f71c3..f80767a 100644
--- a/apps/apps.c
+++ b/apps/apps.c
@@ -763,7 +763,7 @@ X509_CRL *load_crl(const char *infile, int format)
}
EVP_PKEY *load_key(const char *file, int format, int maybe_stdin,
- const char *pass, ENGINE *e, const char *key_descrip)
+ const char *pass, ENGINE *e, const char *key_descrip, int
last_try)
{
BIO *key = NULL;
EVP_PKEY *pkey = NULL;
@@ -773,7 +773,7 @@ EVP_PKEY *load_key(const char *file, int format, int
maybe_stdin,
cb_data.prompt_info = file;
if (file == NULL (!maybe_stdin || format == FORMAT_ENGINE)) {
-BIO_printf(bio_err, no keyfile specified\n);
+if(last_try) { BIO_printf(bio_err, no keyfile specified\n); }
goto end;
}
#ifndef OPENSSL_NO_ENGINE
@@ -827,7 +827,7 @@ EVP_PKEY *load_key(const char *file, int format, int
maybe_stdin,
}
end:
BIO_free(key);
-if (pkey == NULL) {
+if (pkey == NULL last_try) {
BIO_printf(bio_err, unable to load %s\n, key_descrip);
ERR_print_errors(bio_err);
}
@@ -842,7 +842,7 @@ static const char *key_file_format(int format)
}
EVP_PKEY *load_pubkey(const char *file, int format, int maybe_stdin,
- const char *pass, ENGINE *e, const char *key_descrip)
+ const char *pass, ENGINE *e, const char *key_descrip,
int last_try)
{
BIO *key = NULL;
EVP_PKEY *pkey = NULL;
@@ -852,7 +852,7 @@ EVP_PKEY *load_pubkey(const char *file, int format, int
maybe_stdin,
cb_data.prompt_info = file;
if (file == NULL (!maybe_stdin || format == FORMAT_ENGINE)) {
-BIO_printf(bio_err, no keyfile specified\n);
+if(last_try) { BIO_printf(bio_err, no keyfile specified\n); }
goto end;
}
#ifndef OPENSSL_NO_ENGINE
@@ -914,8 +914,9 @@ EVP_PKEY *load_pubkey(const char *file, int format, int
maybe_stdin,
#endif
end:
BIO_free(key);
-if (pkey == NULL)
+if (pkey == NULL last_try) {
BIO_printf(bio_err, unable to load %s\n, key_descrip);
+}
return (pkey);
}
diff --git a/apps/apps.h b/apps/apps.h
index a8652a1..0cd563a 100644
--- a/apps/apps.h
+++ b/apps/apps.h
@@ -427,9 +427,9 @@ X509 *load_cert(const char *file, int format,
X509_CRL *load_crl(const char *infile, int format);
int load_cert_crl_http(const char *url, X509 **pcert, X509_CRL **pcrl);
EVP_PKEY *load_key(const char *file, int format, int maybe_stdin,
- const char *pass, ENGINE *e, const char *key_descrip);
+ const char *pass, ENGINE *e, const char *key_descrip, int
last_try);
EVP_PKEY *load_pubkey(const char *file, int format, int maybe_stdin,
- const char *pass, ENGINE *e, const char *key_descrip);
+ const char *pass, ENGINE *e, const char *key_descrip,
int last_try);
STACK_OF(X509) *load_certs(const char
