Project

General

Profile

Actions

Feature #4183

closed

[ext/openssl] Timestamp support

Added by MartinBosslet (Martin Bosslet) about 13 years ago. Updated over 2 years ago.

Status:
Closed
Target version:
-
[ruby-core:33801]

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
Actions #1

Updated by tenderlovemaking (Aaron Patterson) about 13 years ago

  • Assignee set to tenderlovemaking (Aaron Patterson)
  • Target version changed from 1.9.2 to 2.0.0
Actions #2

Updated by tenderlovemaking (Aaron Patterson) about 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/4183

Author: Martin Bosslet
Status: Open, Priority: Normal
Category: ext, Target version: 1.9.2

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.

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)

Actions #3

Updated by nobu (Nobuyoshi Nakada) about 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

Actions #4

Updated by nahi (Hiroshi Nakamura) about 13 years ago

=begin
Hi,

On Sat, Dec 25, 2010 at 06:32, Aaron Patterson
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

Actions #5

Updated by tenderlovemaking (Aaron Patterson) about 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
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

Actions #6

Updated by MartinBosslet (Martin Bosslet) about 13 years ago

=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

Actions #7

Updated by MartinBosslet (Martin Bosslet) about 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

Actions #8

Updated by MartinBosslet (Martin Bosslet) about 13 years ago

=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

Actions #9

Updated by shyouhei (Shyouhei Urabe) almost 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.

Actions #13

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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0