Feature #4183
closed[ext/openssl] Timestamp support
Added by MartinBosslet (Martin Bosslet) over 13 years ago. Updated over 2 years ago.
Description
=begin
Hi,
I'd like to propose the attached code to support timestamp creation and verification in ext/openssl under a new module OpenSSL::Timestamp. It comes also with various tests and rDoc documentation. Since OpenSSL only supports timestamps beginning with OpenSSL 1.0, the entire code is in #if brackets and the module is defined only for OpenSSL versions >= 1.0.
Best regards,
Martin
=end
Files
ts.tar.gz (15.1 KB) ts.tar.gz | MartinBosslet (Martin Bosslet), 12/22/2010 03:15 AM | ||
ts2.tar.gz (15.3 KB) ts2.tar.gz | MartinBosslet (Martin Bosslet), 12/27/2010 01:07 AM | ||
ts3.tar.gz (14.9 KB) ts3.tar.gz | MartinBosslet (Martin Bosslet), 12/29/2010 11:18 PM |
Updated by tenderlovemaking (Aaron Patterson) over 13 years ago
- Assignee set to tenderlovemaking (Aaron Patterson)
- Target version changed from 1.9.2 to 2.0.0
Updated by tenderlovemaking (Aaron Patterson) over 13 years ago
On Wed, Dec 22, 2010 at 03:19:12AM +0900, Martin Bosslet wrote:
Feature #4183: [ext/openssl] Timestamp support
http://redmine.ruby-lang.org/issues/show/4183Author: Martin Bosslet
Status: Open, Priority: Normal
Category: ext, Target version: 1.9.2Hi,
I'd like to propose the attached code to support timestamp creation and verification in ext/openssl under a new module OpenSSL::Timestamp. It comes also with various tests and rDoc documentation. Since OpenSSL only supports timestamps beginning with OpenSSL 1.0, the entire code is in #if brackets and the module is defined only for OpenSSL versions >= 1.0.
I think this is good functionality to add.
The patch seems good. I noticed some code that is not necessary though.
You can remove these lines:
/*
* Creates a Factory.
*
* call-seq:
* OpenSSL::Timestamp::Factory.new -> Factory
*/
static VALUE
ossl_tsfac_initialize(VALUE self)
{
return self;
}
rb_define_method(cTimestampFactory, "initialize", ossl_tsfac_initialize, 0);
and everything will Just Workâ˘.
This patch is really long, so if nobody objects after a week or two,
I'll apply.
--
Aaron Patterson
http://tenderlovemaking.com/
Attachment: (unnamed)
Updated by nobu (Nobuyoshi Nakada) over 13 years ago
Hi,
At Sat, 25 Dec 2010 06:32:14 +0900,
Aaron Patterson wrote in [ruby-core:33858]:
The patch seems good. I noticed some code that is not necessary though.
Just curious for my eyes,
In ossl_ts_verify()
:
1068 if (!(ctx->store = X509_STORE_new())) {
1069 ossl_raise(eTimestampError, NULL);
1070 goto end;
1071 }
Jump to clean-up after raise?
1109 end:
1110 TS_VERIFY_CTX_free(ctx);
1111 return ret;
In ossl_tsfac_create_ts()
:
1231 if (!(inter_certs = sk_X509_new_null())) goto end;
When inter_certs can't get created, all the rest are just skipped?
1232 if (tsa_cert)
1233 if (rb_obj_is_kind_of(additional_certs, rb_cArray)) {
1234 for (i = 0; i < RARRAY_LEN(additional_certs); i++) {
1235 cert = rb_ary_entry(additional_certs, i);
1236 sk_X509_push(inter_certs, GetX509CertPtr(cert));
1237 }
1238 }
1239 else {
1240 sk_X509_push(inter_certs, GetX509CertPtr(additional_certs));
1241 }
Just indentation of 1233..1241 is wrong, or 1232 is misplaced?
A patch to suppress some warnings.
diff --git i/ext/.document w/ext/.document
index 6f4f668..3c5ff49 100644
--- i/ext/.document
+++ w/ext/.document
@@ -37,6 +37,7 @@ openssl/ossl_pkey_rsa.c
openssl/ossl_rand.c
openssl/ossl_ssl.c
openssl/ossl_ssl_session.c
+openssl/ossl_ts.c
openssl/ossl_x509.c
openssl/ossl_x509attr.c
openssl/ossl_x509cert.c
diff --git i/ext/openssl/extconf.rb w/ext/openssl/extconf.rb
index b1f2d88..14e2745 100644
--- i/ext/openssl/extconf.rb
+++ w/ext/openssl/extconf.rb
@@ -127,6 +127,7 @@ end
have_struct_member("EVP_CIPHER_CTX", "flags", "openssl/evp.h")
have_struct_member("EVP_CIPHER_CTX", "engine", "openssl/evp.h")
have_struct_member("X509_ATTRIBUTE", "single", "openssl/x509.h")
+have_header("openssl/ts.h")
message "=== Checking done. ===\n"
diff --git i/ext/openssl/ossl.c w/ext/openssl/ossl.c
index 1e4f935..9a8d02e 100644
--- i/ext/openssl/ossl.c
+++ w/ext/openssl/ossl.c
@@ -865,6 +865,9 @@ Init_openssl()
Init_ossl_pkey();
Init_ossl_rand();
Init_ossl_ssl();
+#if HAVE_OPENSSL_TS_H
+ Init_ossl_ts();
+#endif
Init_ossl_x509();
Init_ossl_ocsp();
Init_ossl_engine();
diff --git i/ext/openssl/ossl.h w/ext/openssl/ossl.h
index 4bb18d5..6b2ddae 100644
--- i/ext/openssl/ossl.h
+++ w/ext/openssl/ossl.h
@@ -59,6 +59,9 @@ extern "C" {
#include <openssl/rand.h>
#include <openssl/conf.h>
#include <openssl/conf_api.h>
+#if HAVE_OPENSSL_TS_H
+# include <openssl/ts.h>
+#endif
#undef X509_NAME
#undef PKCS7_SIGNER_INFO
#if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ST_ENGINE)
@@ -215,6 +218,9 @@ void ossl_debug(const char *, ...);
#include "ossl_pkey.h"
#include "ossl_rand.h"
#include "ossl_ssl.h"
+#if HAVE_OPENSSL_TS_H
+# include "ossl_ts.h"
+#endif
#include "ossl_version.h"
#include "ossl_x509.h"
#include "ossl_engine.h"
diff --git i/ext/openssl/ossl_ts.c w/ext/openssl/ossl_ts.c
old mode 100755
new mode 100644
index a0b8ca2..6adc8d9
--- i/ext/openssl/ossl_ts.c
+++ w/ext/openssl/ossl_ts.c
@@ -110,19 +110,6 @@ obj_to_asn1obj(VALUE obj)
return a1obj;
}
-static ASN1_STRING*
-obj_to_asn1str(VALUE obj)
-{
- ASN1_STRING *str;
-
- StringValue(obj);
- if(!(str = ASN1_STRING_new()))
- ossl_raise(eASN1Error, NULL);
- ASN1_STRING_set(str, RSTRING_PTR(obj), RSTRING_LEN(obj));
-
- return str;
-}
-
static VALUE
get_asn1obj(ASN1_OBJECT *obj)
{
@@ -319,7 +306,7 @@ ossl_tsreq_set_msg_imprint(VALUE self, VALUE hash)
ossl_raise(eTimestampError, NULL);
return self;
}
- TS_MSG_IMPRINT_set_msg(mi, RSTRING_PTR(hash), RSTRING_LEN(hash));
+ TS_MSG_IMPRINT_set_msg(mi, (unsigned char *)RSTRING_PTR(hash), RSTRING_LEN(hash));
return hash;
}
@@ -401,6 +388,7 @@ ossl_tsreq_set_policy_id(VALUE self, VALUE oid)
ASN1_OBJECT_free(req->policy_id);
obj = obj_to_asn1obj(oid);
req->policy_id = obj;
+ return oid;
}
/*
@@ -536,7 +524,8 @@ ossl_tsresp_alloc(VALUE klass)
* OpenSSL::Timestamp::Response.new(string) -> response
*/
static VALUE
-ossl_ts_initialize(VALUE self, VALUE der) {
+ossl_ts_initialize(VALUE self, VALUE der)
+{
TS_RESP *ts_resp = DATA_PTR(self);
BIO *in;
@@ -666,8 +655,6 @@ ossl_ts_get_pkcs7(VALUE self)
{
TS_RESP *resp;
PKCS7 *p7;
- unsigned char *p;
- int len;
GetTS_RESP(self, resp);
p7 = resp->token;
@@ -920,7 +907,7 @@ ossl_ts_to_der(VALUE self)
}
static void
-int_ossl_handle_verify_errors()
+int_ossl_handle_verify_errors(void)
{
const char *msg = NULL;
int is_validation_err = 0;
@@ -1124,7 +1111,8 @@ ossl_tsfac_initialize(VALUE self)
}
static ASN1_INTEGER *
-ossl_tsfac_serial_cb(struct TS_resp_ctx *ctx, void *data) {
+ossl_tsfac_serial_cb(struct TS_resp_ctx *ctx, void *data)
+{
VALUE serial = *((VALUE *)data);
ASN1_INTEGER *num;
if (!(num = ASN1_INTEGER_new())) {
@@ -1280,7 +1268,7 @@ end:
* INIT
*/
void
-Init_ossl_ts()
+Init_ossl_ts(void)
{
#if 0
mOSSL = rb_define_module("OpenSSL"); /* let rdoc know about mOSSL */
@@ -1543,4 +1531,4 @@ Init_ossl_ts()
rb_define_method(cTimestampFactory, "create_timestamp", ossl_tsfac_create_ts, 3);
}
-#endif
\ No newline at end of file
+#endif
diff --git i/ext/openssl/ossl_ts.h w/ext/openssl/ossl_ts.h
index bbf3a0a..5034443 100755
--- i/ext/openssl/ossl_ts.h
+++ w/ext/openssl/ossl_ts.h
@@ -22,4 +22,4 @@ extern VALUE cTimestampFactory;
void Init_ossl_ts(void);
TS_RESP *GetTsRespPtr(VALUE obj);
-#endif
\ No newline at end of file
+#endif
~~~
--
Nobu Nakada
Updated by nahi (Hiroshi Nakamura) over 13 years ago
=begin
Hi,
On Sat, Dec 25, 2010 at 06:32, Aaron Patterson
aaron@tenderlovemaking.com wrote:
This patch is really long, so if nobody objects after a week or two,
I'll apply.
Isn't it good to ask him to be a ruby-ossl maintainer, instead of
applying the patch we don't understand?
Regards,
// NaHi
=end
Updated by tenderlovemaking (Aaron Patterson) over 13 years ago
=begin
On Sat, Dec 25, 2010 at 11:58:56PM +0900, Hiroshi Nakamura wrote:
Hi,
On Sat, Dec 25, 2010 at 06:32, Aaron Patterson
aaron@tenderlovemaking.com wrote:This patch is really long, so if nobody objects after a week or two,
I'll apply.Isn't it good to ask him to be a ruby-ossl maintainer, instead of
applying the patch we don't understand?
I don't mind spending time to maintain openssl, but a patch this long is
difficult for just one person to review. I take responsibility for
patches I apply, so if there are bugs, I will fix them.
That being said, we can always use help! :-D
--
Aaron Patterson
http://tenderlovemaking.com/
Attachment: (unnamed)
=end
Updated by MartinBosslet (Martin Bosslet) over 13 years ago
- File ts2.tar.gz ts2.tar.gz added
=begin
Thanks for the input! I updated the code with regard to Aaron's and Nobuyoshi's comments. Concerning Nobuyoshi's questions:
Just curious for my eyes,
In ossl_ts_verify():
1068 if (!(ctx->store = X509_STORE_new())) {
1069 ossl_raise(eTimestampError, NULL);
1070 goto end;
1071 }
Jump to clean-up after raise?
1109 end:
1110 TS_VERIFY_CTX_free(ctx);
1111 return ret;
I corrected this, cleanup is now performed before the raise.
In ossl_tsfac_create_ts():
1231 if (!(inter_certs = sk_X509_new_null())) goto end;
When inter_certs can't get created, all the rest are just skipped?
I forgot to set the error message there. Corrected this, now an error is raised when allocation fails.
1232 if (tsa_cert)
1233 if (rb_obj_is_kind_of(additional_certs, rb_cArray)) {
1234 for (i = 0; i < RARRAY_LEN(additional_certs); i++) {
1235 cert = rb_ary_entry(additional_certs, i);
1236 sk_X509_push(inter_certs, GetX509CertPtr(cert));
1237 }
1238 }
1239 else {
1240 sk_X509_push(inter_certs, GetX509CertPtr(additional_certs));
1241 }
Just indentation of 1233..1241 is wrong, or 1232 is misplaced?
No, you're right, it was simply misplaced, I removed it.
Regarding your patch, I already applied it in the update.
There is still some functionality that is not supported and some features I dislike about the way OpenSSL handles timestamp verification. Concerning the missing functionality - I'm planning to add this in case the feature will be accepted, I first wanted to get your reactions before I indulge too deeply in something nobody would want anyway :) Missing features right now are:
- Extensions are neither supported for Request nor for the Response
- Accuracy and TSAName are not supported for the Response.
- I'd like to add some factory methods, that create Responses indicating errors easily. E.g.
error_ts = OpenSSL::Timestamp::Response.create_fault_response(OpenSSL::Timestamp::Response::REJECTION,
:UNACCEPTED_POLICY,
"The policy you provided is not supported by this server")
This would simplify error creation for errors detected on the server side when manually analyzing the Request.
Things I would like different:
- I don't like the fact that for verification, you need to pass OpenSSL a BIO, even if you're still in possession of the TS_REQ*, as in our case (and I guess in most use cases). This forces encoding (and later decoding in OpenSSL) the TS_REQ*, which is unnecessary. I'll try to submit a patch to OpenSSL regarding this.
- The second thing I'm not really happy with is the fact that you cannot validate a timestamp solely based on the timestamp end entity certificate, verification always includes certificate validation of the entire chain. This is too tightly coupled in my opinion, certificate validation should only be an option (so that there is the possibility to perform it in a separate step). In addition, decoupling would also remove the necessity to provide intermediate certificates or root certificates. If the timestamp already contains the timestamp authority certificate(which it must if Request#cert_requested? is true), validation would be self-contained, no other external resources needed. I'll also try to submit a patch for this to OpenSSL.
@hiroshi (Hiroshi MORIYAMA): thanks for the confidence, I'm happy to help :)
Regards,
Martin
=end
Updated by MartinBosslet (Martin Bosslet) over 13 years ago
=begin
- I don't like the fact that for verification, you need to pass OpenSSL a BIO, even if you're still in possession of the TS_REQ*, as in >our case (and I guess in most use cases). This forces encoding (and later decoding in OpenSSL) the TS_REQ*, which is unnecessary. I'll >try to submit a patch to OpenSSL regarding this.
I meant for creation of a TS_RESP, not verification.
=end
Updated by MartinBosslet (Martin Bosslet) over 13 years ago
- File ts3.tar.gz ts3.tar.gz added
=begin
- Removed a redundant variable
- Corrected another code line with "raise without freeing before"
- Additional test
- replaced FileUtils by File method in the test
The attachment contains the two relevant files only, together with diff files showing the changes made to the versions in ts2.tar.gz (since changes are really minor).
Regards,
Martin
=end
Updated by shyouhei (Shyouhei Urabe) about 13 years ago
- Status changed from Open to Assigned
Updated by MartinBosslet (Martin Bosslet) almost 13 years ago
- Assignee changed from tenderlovemaking (Aaron Patterson) to MartinBosslet (Martin Bosslet)
Updated by ko1 (Koichi Sasada) over 11 years ago
ping. status?
Updated by MartinBosslet (Martin Bosslet) over 11 years ago
- Target version changed from 2.0.0 to 2.6
Given the short time until 2.0.0, I believe "next minor" is a more realistic target.
Updated by zzak (zzak _) over 8 years ago
- Assignee changed from MartinBosslet (Martin Bosslet) to 7150
Updated by mame (Yusuke Endoh) over 6 years ago
- Assignee changed from 7150 to rhenium (Kazuki Yamaguchi)
rhe-san, what do you think about this feature proposal?
Updated by jeremyevans0 (Jeremy Evans) over 2 years ago
- Status changed from Assigned to Closed
ruby-openssl 2.2.0 (included with Ruby 3.0) added OpenSSL::Timestamp, so this can be closed.